Message ID | 20211204074507.99950-1-fulinjie@zju.edu.cn |
---|---|
State | New |
Headers | show |
Series | [FFmpeg-devel] libavcodec/hevc_mp4toannexb_bsf: update the extradata in codec par if change detected | expand |
Context | Check | Description |
---|---|---|
andriy/make_x86 | success | Make finished |
andriy/make_fate_x86 | success | Make fate finished |
andriy/make_ppc | success | Make finished |
andriy/make_fate_ppc | success | Make fate finished |
Linjie Fu: > From: Linjie Fu <linjie.justin.fu@gmail.com> > > Container may support multiple sample descriptions in a single > bitstream, like multiple stsd in mov, which introduces different > sequence header(e.g.profile/bit_depth) in the middle of the bitstream. > > Update the extradata field in context parameter once packet with > different extradata is got. > > Signed-off-by: Linjie Fu <linjie.justin.fu@gmail.com> > --- > libavcodec/hevc_mp4toannexb_bsf.c | 23 +++++++++++++++++++++++ > 1 file changed, 23 insertions(+) > > diff --git a/libavcodec/hevc_mp4toannexb_bsf.c b/libavcodec/hevc_mp4toannexb_bsf.c > index 790dfb0394..36a83d0c95 100644 > --- a/libavcodec/hevc_mp4toannexb_bsf.c > +++ b/libavcodec/hevc_mp4toannexb_bsf.c > @@ -125,6 +125,9 @@ static int hevc_mp4toannexb_filter(AVBSFContext *ctx, AVPacket *out) > int got_irap = 0; > int i, ret = 0; > > + uint8_t *extradata; > + size_t extradata_size = 0; > + > ret = ff_bsf_get_packet(ctx, &in); > if (ret < 0) > return ret; > @@ -135,6 +138,26 @@ static int hevc_mp4toannexb_filter(AVBSFContext *ctx, AVPacket *out) > return 0; > } > > + extradata = av_packet_get_side_data(in, AV_PKT_DATA_NEW_EXTRADATA, &extradata_size); > + if (extradata && extradata_size && > + (ctx->par_in->extradata_size != extradata_size || > + memcmp(ctx->par_in->extradata, extradata, extradata_size))) { > + av_log(ctx, AV_LOG_VERBOSE, "Update extradata, size = %zu\n", extradata_size); We use SIZE_SPECIFIER instead of zu due to MSVC compatibility. I don't know if we actually support MSVC versions that old any more, but it is still used. > + /* Update extradata */ > + av_freep(&ctx->par_in->extradata); > + ctx->par_in->extradata_size = extradata_size; > + ctx->par_in->extradata = av_mallocz(extradata_size + AV_INPUT_BUFFER_PADDING_SIZE); > + if (!ctx->par_in->extradata) > + return AVERROR(ENOMEM); > + memcpy(ctx->par_in->extradata, extradata, extradata_size); > + /* Reinit */ > + ret = hevc_extradata_to_annexb(ctx); > + if (ret < 0) > + return ret; > + s->length_size = ret; > + s->extradata_parsed = 1; > + } > + > bytestream2_init(&gb, in->data, in->size); > > while (bytestream2_get_bytes_left(&gb)) { > 1. This is an API violation: par_in is set by the user before initializing the BSF, par_out is set by the BSF in init. After init, both are immutable. 2. Instead you should make hevc_extradata_to_annexb() accept a buffer with length field (or maybe even factor everything that hevc_mp4toannexb_init() does out of it and make that function accept a buffer and a length field; and another field to return the reformatted extradata and its size). 3. Notice that you need to actually modify the packet's AV_PKT_DATA_NEW_EXTRADATA side-data to make it Annex B. Otherwise the output will be incorrect. 4. And in case this BSF actually inserts the extradata in-band, it would need to retain an internal copy of this new extradata. (Andriy Gelman once sent a good patch that never got applied that added the correct extradata in-band even when there are in-band updates to the extradata. I suggest you look at this patch if you want to tackle this problem.) - Andreas
Andreas: On Sun, Dec 5, 2021 at 7:34 PM Andreas Rheinhardt < andreas.rheinhardt@outlook.com> wrote: > Linjie Fu: > > From: Linjie Fu <linjie.justin.fu@gmail.com> > > > > Container may support multiple sample descriptions in a single > > bitstream, like multiple stsd in mov, which introduces different > > sequence header(e.g.profile/bit_depth) in the middle of the bitstream. > > > > Update the extradata field in context parameter once packet with > > different extradata is got. > > > > Signed-off-by: Linjie Fu <linjie.justin.fu@gmail.com> > > --- > > libavcodec/hevc_mp4toannexb_bsf.c | 23 +++++++++++++++++++++++ > > 1 file changed, 23 insertions(+) > > > > diff --git a/libavcodec/hevc_mp4toannexb_bsf.c > b/libavcodec/hevc_mp4toannexb_bsf.c > > index 790dfb0394..36a83d0c95 100644 > > --- a/libavcodec/hevc_mp4toannexb_bsf.c > > +++ b/libavcodec/hevc_mp4toannexb_bsf.c > > @@ -125,6 +125,9 @@ static int hevc_mp4toannexb_filter(AVBSFContext > *ctx, AVPacket *out) > > int got_irap = 0; > > int i, ret = 0; > > > > + uint8_t *extradata; > > + size_t extradata_size = 0; > > + > > ret = ff_bsf_get_packet(ctx, &in); > > if (ret < 0) > > return ret; > > @@ -135,6 +138,26 @@ static int hevc_mp4toannexb_filter(AVBSFContext > *ctx, AVPacket *out) > > return 0; > > } > > > > + extradata = av_packet_get_side_data(in, AV_PKT_DATA_NEW_EXTRADATA, > &extradata_size); > > + if (extradata && extradata_size && > > + (ctx->par_in->extradata_size != extradata_size || > > + memcmp(ctx->par_in->extradata, extradata, extradata_size))) { > > + av_log(ctx, AV_LOG_VERBOSE, "Update extradata, size = %zu\n", > extradata_size); > > We use SIZE_SPECIFIER instead of zu due to MSVC compatibility. I don't > know if we actually support MSVC versions that old any more, but it is > still used. > > + /* Update extradata */ > > + av_freep(&ctx->par_in->extradata); > > + ctx->par_in->extradata_size = extradata_size; > > + ctx->par_in->extradata = av_mallocz(extradata_size + > AV_INPUT_BUFFER_PADDING_SIZE); > > + if (!ctx->par_in->extradata) > > + return AVERROR(ENOMEM); > > + memcpy(ctx->par_in->extradata, extradata, extradata_size); > > + /* Reinit */ > > + ret = hevc_extradata_to_annexb(ctx); > > + if (ret < 0) > > + return ret; > > + s->length_size = ret; > > + s->extradata_parsed = 1; > > + } > > + > > bytestream2_init(&gb, in->data, in->size); > > > > while (bytestream2_get_bytes_left(&gb)) { > > > > 1. This is an API violation: par_in is set by the user before > initializing the BSF, par_out is set by the BSF in init. After init, > both are immutable. > 2. Instead you should make hevc_extradata_to_annexb() accept a buffer > with length field (or maybe even factor everything that > hevc_mp4toannexb_init() does out of it and make that function accept a > buffer and a length field; and another field to return the reformatted > extradata and its size). > 3. Notice that you need to actually modify the packet's > AV_PKT_DATA_NEW_EXTRADATA side-data to make it Annex B. Otherwise the > output will be incorrect. > 4. And in case this BSF actually inserts the extradata in-band, it would > need to retain an internal copy of this new extradata. > (Andriy Gelman once sent a good patch that never got applied that added > the correct extradata in-band even when there are in-band updates to the > extradata. I suggest you look at this patch if you want to tackle this > problem.) > Will check Andriy‘s patch first, seems that it may handle this case properly, thanks for the review comments and pointing this out. - Linjie
diff --git a/libavcodec/hevc_mp4toannexb_bsf.c b/libavcodec/hevc_mp4toannexb_bsf.c index 790dfb0394..36a83d0c95 100644 --- a/libavcodec/hevc_mp4toannexb_bsf.c +++ b/libavcodec/hevc_mp4toannexb_bsf.c @@ -125,6 +125,9 @@ static int hevc_mp4toannexb_filter(AVBSFContext *ctx, AVPacket *out) int got_irap = 0; int i, ret = 0; + uint8_t *extradata; + size_t extradata_size = 0; + ret = ff_bsf_get_packet(ctx, &in); if (ret < 0) return ret; @@ -135,6 +138,26 @@ static int hevc_mp4toannexb_filter(AVBSFContext *ctx, AVPacket *out) return 0; } + extradata = av_packet_get_side_data(in, AV_PKT_DATA_NEW_EXTRADATA, &extradata_size); + if (extradata && extradata_size && + (ctx->par_in->extradata_size != extradata_size || + memcmp(ctx->par_in->extradata, extradata, extradata_size))) { + av_log(ctx, AV_LOG_VERBOSE, "Update extradata, size = %zu\n", extradata_size); + /* Update extradata */ + av_freep(&ctx->par_in->extradata); + ctx->par_in->extradata_size = extradata_size; + ctx->par_in->extradata = av_mallocz(extradata_size + AV_INPUT_BUFFER_PADDING_SIZE); + if (!ctx->par_in->extradata) + return AVERROR(ENOMEM); + memcpy(ctx->par_in->extradata, extradata, extradata_size); + /* Reinit */ + ret = hevc_extradata_to_annexb(ctx); + if (ret < 0) + return ret; + s->length_size = ret; + s->extradata_parsed = 1; + } + bytestream2_init(&gb, in->data, in->size); while (bytestream2_get_bytes_left(&gb)) {