Re: [FFmpeg-devel] [PATCH v2] h264: Support multi-field closed captions by using AVBufferRef and not resetting per field
On 8/17/2018 7:41 AM, jos...@ob-encoder.com wrote: > From: Kieran Kunhya > > Signed-off-by: Josh de Kock > --- > libavcodec/h264_sei.c | 15 --- > libavcodec/h264_sei.h | 3 +-- > libavcodec/h264_slice.c | 26 ++ > libavcodec/h264dec.c| 5 +++-- > 4 files changed, 30 insertions(+), 19 deletions(-) > > diff --git a/libavcodec/h264_sei.c b/libavcodec/h264_sei.c > index 6499086210..43593d34d2 100644 > --- a/libavcodec/h264_sei.c > +++ b/libavcodec/h264_sei.c > @@ -51,8 +51,7 @@ void ff_h264_sei_uninit(H264SEIContext *h) > h->display_orientation.present = 0; > h->afd.present = 0; > > -h->a53_caption.a53_caption_size = 0; > -av_freep(>a53_caption.a53_caption); > +av_buffer_unref(>a53_caption.buf_ref); > } > > static int decode_picture_timing(H264SEIPictureTiming *h, GetBitContext *gb, > @@ -169,7 +168,8 @@ static int > decode_registered_user_data_closed_caption(H264SEIA53Caption *h, > size -= 2; > > if (cc_count && size >= cc_count * 3) { > -const uint64_t new_size = (h->a53_caption_size + cc_count > +int old_size = h->buf_ref ? h->buf_ref->size : 0; > +const uint64_t new_size = (old_size + cc_count > * UINT64_C(3)); > int i, ret; > > @@ -177,14 +177,15 @@ static int > decode_registered_user_data_closed_caption(H264SEIA53Caption *h, > return AVERROR(EINVAL); > > /* Allow merging of the cc data from two fields. */ > -ret = av_reallocp(>a53_caption, new_size); > +ret = av_buffer_realloc(>buf_ref, new_size); > if (ret < 0) > return ret; > > +/* Use of av_buffer_realloc assumes buffer is writeable */ > for (i = 0; i < cc_count; i++) { > -h->a53_caption[h->a53_caption_size++] = get_bits(gb, 8); > -h->a53_caption[h->a53_caption_size++] = get_bits(gb, 8); > -h->a53_caption[h->a53_caption_size++] = get_bits(gb, 8); > +h->buf_ref->data[old_size++] = get_bits(gb, 8); > +h->buf_ref->data[old_size++] = get_bits(gb, 8); > +h->buf_ref->data[old_size++] = get_bits(gb, 8); > } > > skip_bits(gb, 8); // marker_bits > diff --git a/libavcodec/h264_sei.h b/libavcodec/h264_sei.h > index a169a10e49..5b7c8ef9d8 100644 > --- a/libavcodec/h264_sei.h > +++ b/libavcodec/h264_sei.h > @@ -95,8 +95,7 @@ typedef struct H264SEIAFD { > } H264SEIAFD; > > typedef struct H264SEIA53Caption { > -int a53_caption_size; > -uint8_t *a53_caption; > +AVBufferRef *buf_ref; > } H264SEIA53Caption; > > typedef struct H264SEIUnregistered { > diff --git a/libavcodec/h264_slice.c b/libavcodec/h264_slice.c > index ede9a1a6ea..f31c9a2579 100644 > --- a/libavcodec/h264_slice.c > +++ b/libavcodec/h264_slice.c > @@ -430,6 +430,15 @@ int ff_h264_update_thread_context(AVCodecContext *dst, > > h->frame_recovered = h1->frame_recovered; > > +av_buffer_unref(>sei.a53_caption.buf_ref); > +h->sei.a53_caption.buf_ref = NULL; Unneeded, av_buffer_unref() already would have set the pointer to NULL. > + > +if (h1->sei.a53_caption.buf_ref) { > +h->sei.a53_caption.buf_ref = > av_buffer_ref(h1->sei.a53_caption.buf_ref); > +if (!h->sei.a53_caption.buf_ref) > +return AVERROR(ENOMEM); > +} > + > if (!h->cur_pic_ptr) > return 0; > > @@ -1269,15 +1278,16 @@ static int h264_export_frame_props(H264Context *h) > } > } > > -if (h->sei.a53_caption.a53_caption) { > +if (h->sei.a53_caption.buf_ref) { > H264SEIA53Caption *a53 = >sei.a53_caption; > -AVFrameSideData *sd = av_frame_new_side_data(cur->f, > - AV_FRAME_DATA_A53_CC, > - a53->a53_caption_size); > -if (sd) > -memcpy(sd->data, a53->a53_caption, a53->a53_caption_size); > -av_freep(>a53_caption); > -a53->a53_caption_size = 0; > + > +AVFrameSideData *sd = av_frame_new_side_data_from_buf(cur->f, > AV_FRAME_DATA_A53_CC, av_buffer_ref(a53->buf_ref)); > +if (sd) { > +av_buffer_unref(>buf_ref); The doxy for av_frame_new_side_data_from_buf() states that on success it takes ownership of the passed reference, but on failure said reference remains owned by the caller. So what you're doing here is creating and passing a reference that, if av_frame_new_side_data_from_buf() fails, will leak. What you need to do is directly pass a53->buf_ref to it, and then only unref it if !sd. I agree the semantics of av_frame_new_side_data_from_buf() are kinda ugly and unlike other AVBufferRef functions, but it's too late to change
[FFmpeg-devel] [PATCH v2] h264: Support multi-field closed captions by using AVBufferRef and not resetting per field
From: Kieran Kunhya Signed-off-by: Josh de Kock --- libavcodec/h264_sei.c | 15 --- libavcodec/h264_sei.h | 3 +-- libavcodec/h264_slice.c | 26 ++ libavcodec/h264dec.c| 5 +++-- 4 files changed, 30 insertions(+), 19 deletions(-) diff --git a/libavcodec/h264_sei.c b/libavcodec/h264_sei.c index 6499086210..43593d34d2 100644 --- a/libavcodec/h264_sei.c +++ b/libavcodec/h264_sei.c @@ -51,8 +51,7 @@ void ff_h264_sei_uninit(H264SEIContext *h) h->display_orientation.present = 0; h->afd.present = 0; -h->a53_caption.a53_caption_size = 0; -av_freep(>a53_caption.a53_caption); +av_buffer_unref(>a53_caption.buf_ref); } static int decode_picture_timing(H264SEIPictureTiming *h, GetBitContext *gb, @@ -169,7 +168,8 @@ static int decode_registered_user_data_closed_caption(H264SEIA53Caption *h, size -= 2; if (cc_count && size >= cc_count * 3) { -const uint64_t new_size = (h->a53_caption_size + cc_count +int old_size = h->buf_ref ? h->buf_ref->size : 0; +const uint64_t new_size = (old_size + cc_count * UINT64_C(3)); int i, ret; @@ -177,14 +177,15 @@ static int decode_registered_user_data_closed_caption(H264SEIA53Caption *h, return AVERROR(EINVAL); /* Allow merging of the cc data from two fields. */ -ret = av_reallocp(>a53_caption, new_size); +ret = av_buffer_realloc(>buf_ref, new_size); if (ret < 0) return ret; +/* Use of av_buffer_realloc assumes buffer is writeable */ for (i = 0; i < cc_count; i++) { -h->a53_caption[h->a53_caption_size++] = get_bits(gb, 8); -h->a53_caption[h->a53_caption_size++] = get_bits(gb, 8); -h->a53_caption[h->a53_caption_size++] = get_bits(gb, 8); +h->buf_ref->data[old_size++] = get_bits(gb, 8); +h->buf_ref->data[old_size++] = get_bits(gb, 8); +h->buf_ref->data[old_size++] = get_bits(gb, 8); } skip_bits(gb, 8); // marker_bits diff --git a/libavcodec/h264_sei.h b/libavcodec/h264_sei.h index a169a10e49..5b7c8ef9d8 100644 --- a/libavcodec/h264_sei.h +++ b/libavcodec/h264_sei.h @@ -95,8 +95,7 @@ typedef struct H264SEIAFD { } H264SEIAFD; typedef struct H264SEIA53Caption { -int a53_caption_size; -uint8_t *a53_caption; +AVBufferRef *buf_ref; } H264SEIA53Caption; typedef struct H264SEIUnregistered { diff --git a/libavcodec/h264_slice.c b/libavcodec/h264_slice.c index ede9a1a6ea..f31c9a2579 100644 --- a/libavcodec/h264_slice.c +++ b/libavcodec/h264_slice.c @@ -430,6 +430,15 @@ int ff_h264_update_thread_context(AVCodecContext *dst, h->frame_recovered = h1->frame_recovered; +av_buffer_unref(>sei.a53_caption.buf_ref); +h->sei.a53_caption.buf_ref = NULL; + +if (h1->sei.a53_caption.buf_ref) { +h->sei.a53_caption.buf_ref = av_buffer_ref(h1->sei.a53_caption.buf_ref); +if (!h->sei.a53_caption.buf_ref) +return AVERROR(ENOMEM); +} + if (!h->cur_pic_ptr) return 0; @@ -1269,15 +1278,16 @@ static int h264_export_frame_props(H264Context *h) } } -if (h->sei.a53_caption.a53_caption) { +if (h->sei.a53_caption.buf_ref) { H264SEIA53Caption *a53 = >sei.a53_caption; -AVFrameSideData *sd = av_frame_new_side_data(cur->f, - AV_FRAME_DATA_A53_CC, - a53->a53_caption_size); -if (sd) -memcpy(sd->data, a53->a53_caption, a53->a53_caption_size); -av_freep(>a53_caption); -a53->a53_caption_size = 0; + +AVFrameSideData *sd = av_frame_new_side_data_from_buf(cur->f, AV_FRAME_DATA_A53_CC, av_buffer_ref(a53->buf_ref)); +if (sd) { +av_buffer_unref(>buf_ref); +} else { +return AVERROR(ENOMEM); +} + h->avctx->properties |= FF_CODEC_PROPERTY_CLOSED_CAPTIONS; } diff --git a/libavcodec/h264dec.c b/libavcodec/h264dec.c index 7494c7a8f2..8d115fa040 100644 --- a/libavcodec/h264dec.c +++ b/libavcodec/h264dec.c @@ -609,9 +609,10 @@ static int decode_nal_units(H264Context *h, const uint8_t *buf, int buf_size) if (!(avctx->flags2 & AV_CODEC_FLAG2_CHUNKS)) { h->current_slice = 0; -if (!h->first_field) +if (!h->first_field) { h->cur_pic_ptr = NULL; -ff_h264_sei_uninit(>sei); +ff_h264_sei_uninit(>sei); +} } if (h->nal_length_size == 4) { -- 2.17.1 ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel