| Message ID | 20180816105236.6113-1-joshdk@obe.tv |
|---|---|
| State | Superseded |
| Headers | show |
Hello, > On Aug 16, 2018, at 6:52 AM, joshdk@ob-encoder.com wrote: > > From: Kieran Kunhya <kierank@obe.tv> > > --- > This is also useful when CC is fully contained in the first field. FWIW: I’ve been running a variant of this this patch for a while now. It’s needed for PAFF formatted H.264 streams (i.e. slices which are sent as individual fields rather than frames), and without it those streams will not render EIA-708 captions properly. It typically doesn’t happen with H.264 streams created by ffmpeg because libx264 only supports MBAFF encoding, but it does occur with a number of commercial hardware encoders I’ve worked with. We can debate the mechanics of how the buffers are referenced (which is why I believe it wasn’t merged last year), but I can say that this is a very real problem and at least this patch results in those streams playing properly. Cheers, Devin --- Devin Heitmueller - LTN Global Communications dheitmueller@ltnglobal.com
On 8/16/2018 7:52 AM, joshdk@ob-encoder.com wrote: > From: Kieran Kunhya <kierank@obe.tv> > > --- > This is also useful when CC is fully contained in the first field. > > libavcodec/h264_sei.c | 15 ++++++++------- > libavcodec/h264_sei.h | 3 +-- > libavcodec/h264_slice.c | 15 ++++++++++----- > libavcodec/h264dec.c | 5 +++-- > 4 files changed, 22 insertions(+), 16 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(&h->a53_caption.a53_caption); > + av_buffer_unref(&h->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(&h->a53_caption, new_size); > + ret = av_buffer_realloc(&h->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; Nit: buf is shorter. > } H264SEIA53Caption; > > typedef struct H264SEIUnregistered { > diff --git a/libavcodec/h264_slice.c b/libavcodec/h264_slice.c > index ede9a1a6ea..7bbe65878f 100644 > --- a/libavcodec/h264_slice.c > +++ b/libavcodec/h264_slice.c > @@ -429,6 +429,12 @@ int ff_h264_update_thread_context(AVCodecContext *dst, > MAX_DELAYED_PIC_COUNT + 2, h, h1); > > h->frame_recovered = h1->frame_recovered; > + if (h1->sei.a53_caption.buf_ref) { > + h->sei.a53_caption.buf_ref = av_buffer_ref(h1->sei.a53_caption.buf_ref); Missing check for failure. > + av_buffer_unref(&h1->sei.a53_caption.buf_ref); Does the reference in h1 need to be freed? h1 does not seem to be modified in this function anywhere else. > + } > + else > + h->sei.a53_caption.buf_ref = NULL; Is it not NULL already in this case? > > if (!h->cur_pic_ptr) > return 0; > @@ -1269,15 +1275,14 @@ static int h264_export_frame_props(H264Context *h) > } > } > > - if (h->sei.a53_caption.a53_caption) { > + if (h->sei.a53_caption.buf_ref) { > H264SEIA53Caption *a53 = &h->sei.a53_caption; > AVFrameSideData *sd = av_frame_new_side_data(cur->f, > AV_FRAME_DATA_A53_CC, > - a53->a53_caption_size); > + a53->buf_ref->size); > if (sd) > - memcpy(sd->data, a53->a53_caption, a53->a53_caption_size); > - av_freep(&a53->a53_caption); > - a53->a53_caption_size = 0; > + memcpy(sd->data, a53->buf_ref->data, a53->buf_ref->size); > + av_buffer_unref(&a53->buf_ref); Use av_frame_new_side_data_from_buf() instead. It takes ownership of the provided AVBufferRef instead of creating a new one for itself, so it's ideal for this scenario. Saves you one memcpy. > 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(&h->sei); > + ff_h264_sei_uninit(&h->sei); > + } > } > > if (h->nal_length_size == 4) { >
On 8/16/2018 7:52 AM, joshdk@ob-encoder.com wrote: > diff --git a/libavcodec/h264_slice.c b/libavcodec/h264_slice.c > index ede9a1a6ea..7bbe65878f 100644 > --- a/libavcodec/h264_slice.c > +++ b/libavcodec/h264_slice.c > @@ -429,6 +429,12 @@ int ff_h264_update_thread_context(AVCodecContext *dst, > MAX_DELAYED_PIC_COUNT + 2, h, h1); > > h->frame_recovered = h1->frame_recovered; > + if (h1->sei.a53_caption.buf_ref) { > + h->sei.a53_caption.buf_ref = av_buffer_ref(h1->sei.a53_caption.buf_ref); > + av_buffer_unref(&h1->sei.a53_caption.buf_ref); > + } > + else > + h->sei.a53_caption.buf_ref = NULL; Going back to this, and judging by similar buffers above, you probably need to first unref h->sei.a53_caption.buf_ref unconditionally before checking if you need to make it a reference to h1->sei.a53_caption.buf_ref, the latter which should itself not be unreffed. Could you corroborate this by running your samples through gcc-tsan or clang-tsan?
2018-08-16 12:52 GMT+02:00, joshdk@ob-encoder.com <joshdk@ob-encoder.com>: > @@ -429,6 +429,12 @@ int ff_h264_update_thread_context(AVCodecContext *dst, > MAX_DELAYED_PIC_COUNT + 2, h, h1); > > h->frame_recovered = h1->frame_recovered; > + if (h1->sei.a53_caption.buf_ref) { > + h->sei.a53_caption.buf_ref = > av_buffer_ref(h1->sei.a53_caption.buf_ref); > + av_buffer_unref(&h1->sei.a53_caption.buf_ref); > + } > + else > + h->sei.a53_caption.buf_ref = NULL; Since you have to change the patch, please fix the code style here. (Some people prefer to always use two braces around "else", it makes debugging much easier.) Carl Eugen
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(&h->a53_caption.a53_caption); + av_buffer_unref(&h->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(&h->a53_caption, new_size); + ret = av_buffer_realloc(&h->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..7bbe65878f 100644 --- a/libavcodec/h264_slice.c +++ b/libavcodec/h264_slice.c @@ -429,6 +429,12 @@ int ff_h264_update_thread_context(AVCodecContext *dst, MAX_DELAYED_PIC_COUNT + 2, h, h1); h->frame_recovered = h1->frame_recovered; + if (h1->sei.a53_caption.buf_ref) { + h->sei.a53_caption.buf_ref = av_buffer_ref(h1->sei.a53_caption.buf_ref); + av_buffer_unref(&h1->sei.a53_caption.buf_ref); + } + else + h->sei.a53_caption.buf_ref = NULL; if (!h->cur_pic_ptr) return 0; @@ -1269,15 +1275,14 @@ static int h264_export_frame_props(H264Context *h) } } - if (h->sei.a53_caption.a53_caption) { + if (h->sei.a53_caption.buf_ref) { H264SEIA53Caption *a53 = &h->sei.a53_caption; AVFrameSideData *sd = av_frame_new_side_data(cur->f, AV_FRAME_DATA_A53_CC, - a53->a53_caption_size); + a53->buf_ref->size); if (sd) - memcpy(sd->data, a53->a53_caption, a53->a53_caption_size); - av_freep(&a53->a53_caption); - a53->a53_caption_size = 0; + memcpy(sd->data, a53->buf_ref->data, a53->buf_ref->size); + av_buffer_unref(&a53->buf_ref); 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(&h->sei); + ff_h264_sei_uninit(&h->sei); + } } if (h->nal_length_size == 4) {
From: Kieran Kunhya <kierank@obe.tv> --- This is also useful when CC is fully contained in the first field. libavcodec/h264_sei.c | 15 ++++++++------- libavcodec/h264_sei.h | 3 +-- libavcodec/h264_slice.c | 15 ++++++++++----- libavcodec/h264dec.c | 5 +++-- 4 files changed, 22 insertions(+), 16 deletions(-)