| Message ID | 20190325142253.5276-1-jamrial@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
On 3/25/2019 11:22 AM, James Almer wrote: > Based on itut_t35 Matadata OBU parsing code. > > Signed-off-by: James Almer <jamrial@gmail.com> > --- > libavcodec/cbs_av1.c | 20 +++++++++++++++++ > libavcodec/cbs_av1.h | 7 ++++++ > libavcodec/cbs_av1_syntax_template.c | 32 ++++++++++++++++++++++++++++ > 3 files changed, 59 insertions(+) Ping for set.
On 25/03/2019 14:22, James Almer wrote: > Based on itut_t35 Matadata OBU parsing code. > > Signed-off-by: James Almer <jamrial@gmail.com> > --- > libavcodec/cbs_av1.c | 20 +++++++++++++++++ > libavcodec/cbs_av1.h | 7 ++++++ > libavcodec/cbs_av1_syntax_template.c | 32 ++++++++++++++++++++++++++++ > 3 files changed, 59 insertions(+) > > diff --git a/libavcodec/cbs_av1.c b/libavcodec/cbs_av1.c > index 02f168b58d..22330eabf3 100644 > --- a/libavcodec/cbs_av1.c > +++ b/libavcodec/cbs_av1.c > @@ -857,6 +857,11 @@ static void cbs_av1_free_tile_data(AV1RawTileData *td) > av_buffer_unref(&td->data_ref); > } > > +static void cbs_av1_free_padding(AV1RawPadding *pd) > +{ > + av_buffer_unref(&pd->payload_ref); > +} > + > static void cbs_av1_free_metadata(AV1RawMetadata *md) > { > switch (md->metadata_type) { > @@ -883,6 +888,9 @@ static void cbs_av1_free_obu(void *unit, uint8_t *content) > case AV1_OBU_METADATA: > cbs_av1_free_metadata(&obu->obu.metadata); > break; > + case AV1_OBU_PADDING: > + cbs_av1_free_padding(&obu->obu.padding); > + break; > } > > av_freep(&obu); > @@ -1057,6 +1065,12 @@ static int cbs_av1_read_unit(CodedBitstreamContext *ctx, > } > break; > case AV1_OBU_PADDING: > + { > + err = cbs_av1_read_padding(ctx, &gbc, &obu->obu.padding); > + if (err < 0) > + return err; > + } > + break; > default: > return AVERROR(ENOSYS); > } > @@ -1182,6 +1196,12 @@ static int cbs_av1_write_obu(CodedBitstreamContext *ctx, > } > break; > case AV1_OBU_PADDING: > + { > + err = cbs_av1_write_padding(ctx, pbc, &obu->obu.padding); > + if (err < 0) > + return err; > + } > + break; > default: > return AVERROR(ENOSYS); > } > diff --git a/libavcodec/cbs_av1.h b/libavcodec/cbs_av1.h > index 71ceff9427..e799964b72 100644 > --- a/libavcodec/cbs_av1.h > +++ b/libavcodec/cbs_av1.h > @@ -364,6 +364,12 @@ typedef struct AV1RawMetadata { > } metadata; > } AV1RawMetadata; > > +typedef struct AV1RawPadding { > + uint8_t *payload; > + size_t payload_size; > + AVBufferRef *payload_ref; > +} AV1RawPadding; > + > > typedef struct AV1RawOBU { > AV1RawOBUHeader header; > @@ -377,6 +383,7 @@ typedef struct AV1RawOBU { > AV1RawTileGroup tile_group; > AV1RawTileList tile_list; > AV1RawMetadata metadata; > + AV1RawPadding padding; > } obu; > } AV1RawOBU; > > diff --git a/libavcodec/cbs_av1_syntax_template.c b/libavcodec/cbs_av1_syntax_template.c > index 76eb90b279..a6cafdd99f 100644 > --- a/libavcodec/cbs_av1_syntax_template.c > +++ b/libavcodec/cbs_av1_syntax_template.c > @@ -1763,3 +1763,35 @@ static int FUNC(metadata_obu)(CodedBitstreamContext *ctx, RWContext *rw, > > return 0; > } > + > +static int FUNC(padding)(CodedBitstreamContext *ctx, RWContext *rw, > + AV1RawPadding *current) > +{ > + int i, err; > + > + HEADER("Padding"); > + > +#ifdef READ > + // The payload runs up to the start of the trailing bits, but there might > + // be arbitrarily many trailing zeroes so we need to read through twice. > + { > + GetBitContext tmp = *rw; > + current->payload_size = 0; > + for (i = 0; get_bits_left(rw) >= 8; i++) { > + if (get_bits(rw, 8)) > + current->payload_size = i; > + } > + *rw = tmp; > + > + current->payload_ref = av_buffer_alloc(current->payload_size); > + if (!current->payload_ref) > + return AVERROR(ENOMEM); > + current->payload = current->payload_ref->data; > + } > +#endif That looks factorisable. Can we make a "measure length of payload and allocate buffer for it" function and use it in metadata_itu_t35 as well? > + > + for (i = 0; i < current->payload_size; i++) > + xf(8, obu_padding_byte[i], current->payload[i], 0x00, 0xff, 1, i); > + > + return 0; > +} > Code looks sensible, but could you explain a bit more about why this is helpful. Is there a use-case for preserving the actual padding bytes? If that's some sort of CBR application, is it actually going to need to preserve the trailing zeroes as well? Thanks, - Mark
On 4/1/2019 7:47 PM, Mark Thompson wrote: > On 25/03/2019 14:22, James Almer wrote: >> Based on itut_t35 Matadata OBU parsing code. >> >> Signed-off-by: James Almer <jamrial@gmail.com> >> --- >> libavcodec/cbs_av1.c | 20 +++++++++++++++++ >> libavcodec/cbs_av1.h | 7 ++++++ >> libavcodec/cbs_av1_syntax_template.c | 32 ++++++++++++++++++++++++++++ >> 3 files changed, 59 insertions(+) >> >> diff --git a/libavcodec/cbs_av1.c b/libavcodec/cbs_av1.c >> index 02f168b58d..22330eabf3 100644 >> --- a/libavcodec/cbs_av1.c >> +++ b/libavcodec/cbs_av1.c >> @@ -857,6 +857,11 @@ static void cbs_av1_free_tile_data(AV1RawTileData *td) >> av_buffer_unref(&td->data_ref); >> } >> >> +static void cbs_av1_free_padding(AV1RawPadding *pd) >> +{ >> + av_buffer_unref(&pd->payload_ref); >> +} >> + >> static void cbs_av1_free_metadata(AV1RawMetadata *md) >> { >> switch (md->metadata_type) { >> @@ -883,6 +888,9 @@ static void cbs_av1_free_obu(void *unit, uint8_t *content) >> case AV1_OBU_METADATA: >> cbs_av1_free_metadata(&obu->obu.metadata); >> break; >> + case AV1_OBU_PADDING: >> + cbs_av1_free_padding(&obu->obu.padding); >> + break; >> } >> >> av_freep(&obu); >> @@ -1057,6 +1065,12 @@ static int cbs_av1_read_unit(CodedBitstreamContext *ctx, >> } >> break; >> case AV1_OBU_PADDING: >> + { >> + err = cbs_av1_read_padding(ctx, &gbc, &obu->obu.padding); >> + if (err < 0) >> + return err; >> + } >> + break; >> default: >> return AVERROR(ENOSYS); >> } >> @@ -1182,6 +1196,12 @@ static int cbs_av1_write_obu(CodedBitstreamContext *ctx, >> } >> break; >> case AV1_OBU_PADDING: >> + { >> + err = cbs_av1_write_padding(ctx, pbc, &obu->obu.padding); >> + if (err < 0) >> + return err; >> + } >> + break; >> default: >> return AVERROR(ENOSYS); >> } >> diff --git a/libavcodec/cbs_av1.h b/libavcodec/cbs_av1.h >> index 71ceff9427..e799964b72 100644 >> --- a/libavcodec/cbs_av1.h >> +++ b/libavcodec/cbs_av1.h >> @@ -364,6 +364,12 @@ typedef struct AV1RawMetadata { >> } metadata; >> } AV1RawMetadata; >> >> +typedef struct AV1RawPadding { >> + uint8_t *payload; >> + size_t payload_size; >> + AVBufferRef *payload_ref; >> +} AV1RawPadding; >> + >> >> typedef struct AV1RawOBU { >> AV1RawOBUHeader header; >> @@ -377,6 +383,7 @@ typedef struct AV1RawOBU { >> AV1RawTileGroup tile_group; >> AV1RawTileList tile_list; >> AV1RawMetadata metadata; >> + AV1RawPadding padding; >> } obu; >> } AV1RawOBU; >> >> diff --git a/libavcodec/cbs_av1_syntax_template.c b/libavcodec/cbs_av1_syntax_template.c >> index 76eb90b279..a6cafdd99f 100644 >> --- a/libavcodec/cbs_av1_syntax_template.c >> +++ b/libavcodec/cbs_av1_syntax_template.c >> @@ -1763,3 +1763,35 @@ static int FUNC(metadata_obu)(CodedBitstreamContext *ctx, RWContext *rw, >> >> return 0; >> } >> + >> +static int FUNC(padding)(CodedBitstreamContext *ctx, RWContext *rw, >> + AV1RawPadding *current) >> +{ >> + int i, err; >> + >> + HEADER("Padding"); >> + >> +#ifdef READ >> + // The payload runs up to the start of the trailing bits, but there might >> + // be arbitrarily many trailing zeroes so we need to read through twice. >> + { >> + GetBitContext tmp = *rw; >> + current->payload_size = 0; >> + for (i = 0; get_bits_left(rw) >= 8; i++) { >> + if (get_bits(rw, 8)) >> + current->payload_size = i; >> + } >> + *rw = tmp; >> + >> + current->payload_ref = av_buffer_alloc(current->payload_size); >> + if (!current->payload_ref) >> + return AVERROR(ENOMEM); >> + current->payload = current->payload_ref->data; >> + } >> +#endif > > That looks factorisable. Can we make a "measure length of payload and allocate buffer for it" function and use it in metadata_itu_t35 as well? Will do. > >> + >> + for (i = 0; i < current->payload_size; i++) >> + xf(8, obu_padding_byte[i], current->payload[i], 0x00, 0xff, 1, i); >> + >> + return 0; >> +} >> > > Code looks sensible, but could you explain a bit more about why this is helpful. Is there a use-case for preserving the actual padding bytes? The padding could be anything, and samples could exploit it to insert arbitrary data for whatever reason into the bitstream without damaging it (ASCII art signature? :P). I figured it was better to let the CBS caller decide if they want to keep them, replace them with something else, or even inserting a brand new one. Hardcoding it to 0xff or whatever didn't seem proper. > If that's some sort of CBR application, is it actually going to need to preserve the trailing zeroes as well? I don't think we're keeping them for any OBU type whatsoever when for example when doing an av1_metadata packet passthrough. In cbs_av1_write_obu() the obu_size is calculated based on actual defined bits in the OBU syntax, plus tile data if any. The trailing bits it writes are essentially just byte boundary alignment at the end of the OBU. In any case, I personally don't think it's worth keeping extra trailing zeroes, since they are fixed and can't have arbitrary data. For that kind of extra padding, you could just add them to the obu_padding_byte array. > > Thanks, > > - Mark > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe". >
diff --git a/libavcodec/cbs_av1.c b/libavcodec/cbs_av1.c index 02f168b58d..22330eabf3 100644 --- a/libavcodec/cbs_av1.c +++ b/libavcodec/cbs_av1.c @@ -857,6 +857,11 @@ static void cbs_av1_free_tile_data(AV1RawTileData *td) av_buffer_unref(&td->data_ref); } +static void cbs_av1_free_padding(AV1RawPadding *pd) +{ + av_buffer_unref(&pd->payload_ref); +} + static void cbs_av1_free_metadata(AV1RawMetadata *md) { switch (md->metadata_type) { @@ -883,6 +888,9 @@ static void cbs_av1_free_obu(void *unit, uint8_t *content) case AV1_OBU_METADATA: cbs_av1_free_metadata(&obu->obu.metadata); break; + case AV1_OBU_PADDING: + cbs_av1_free_padding(&obu->obu.padding); + break; } av_freep(&obu); @@ -1057,6 +1065,12 @@ static int cbs_av1_read_unit(CodedBitstreamContext *ctx, } break; case AV1_OBU_PADDING: + { + err = cbs_av1_read_padding(ctx, &gbc, &obu->obu.padding); + if (err < 0) + return err; + } + break; default: return AVERROR(ENOSYS); } @@ -1182,6 +1196,12 @@ static int cbs_av1_write_obu(CodedBitstreamContext *ctx, } break; case AV1_OBU_PADDING: + { + err = cbs_av1_write_padding(ctx, pbc, &obu->obu.padding); + if (err < 0) + return err; + } + break; default: return AVERROR(ENOSYS); } diff --git a/libavcodec/cbs_av1.h b/libavcodec/cbs_av1.h index 71ceff9427..e799964b72 100644 --- a/libavcodec/cbs_av1.h +++ b/libavcodec/cbs_av1.h @@ -364,6 +364,12 @@ typedef struct AV1RawMetadata { } metadata; } AV1RawMetadata; +typedef struct AV1RawPadding { + uint8_t *payload; + size_t payload_size; + AVBufferRef *payload_ref; +} AV1RawPadding; + typedef struct AV1RawOBU { AV1RawOBUHeader header; @@ -377,6 +383,7 @@ typedef struct AV1RawOBU { AV1RawTileGroup tile_group; AV1RawTileList tile_list; AV1RawMetadata metadata; + AV1RawPadding padding; } obu; } AV1RawOBU; diff --git a/libavcodec/cbs_av1_syntax_template.c b/libavcodec/cbs_av1_syntax_template.c index 76eb90b279..a6cafdd99f 100644 --- a/libavcodec/cbs_av1_syntax_template.c +++ b/libavcodec/cbs_av1_syntax_template.c @@ -1763,3 +1763,35 @@ static int FUNC(metadata_obu)(CodedBitstreamContext *ctx, RWContext *rw, return 0; } + +static int FUNC(padding)(CodedBitstreamContext *ctx, RWContext *rw, + AV1RawPadding *current) +{ + int i, err; + + HEADER("Padding"); + +#ifdef READ + // The payload runs up to the start of the trailing bits, but there might + // be arbitrarily many trailing zeroes so we need to read through twice. + { + GetBitContext tmp = *rw; + current->payload_size = 0; + for (i = 0; get_bits_left(rw) >= 8; i++) { + if (get_bits(rw, 8)) + current->payload_size = i; + } + *rw = tmp; + + current->payload_ref = av_buffer_alloc(current->payload_size); + if (!current->payload_ref) + return AVERROR(ENOMEM); + current->payload = current->payload_ref->data; + } +#endif + + for (i = 0; i < current->payload_size; i++) + xf(8, obu_padding_byte[i], current->payload[i], 0x00, 0xff, 1, i); + + return 0; +}
Based on itut_t35 Matadata OBU parsing code. Signed-off-by: James Almer <jamrial@gmail.com> --- libavcodec/cbs_av1.c | 20 +++++++++++++++++ libavcodec/cbs_av1.h | 7 ++++++ libavcodec/cbs_av1_syntax_template.c | 32 ++++++++++++++++++++++++++++ 3 files changed, 59 insertions(+)