On Mon, Oct 17, 2016 at 6:03 PM, Aman Gupta <ffm...@tmm1.net> wrote: > > > On Mon, Oct 17, 2016 at 5:51 PM, Richard Kern <ker...@gmail.com> wrote: > >> >> On Oct 17, 2016, at 8:47 PM, Aman Gupta <ffm...@tmm1.net> wrote: >> >> >> >> On Mon, Oct 17, 2016 at 6:35 AM, Richard Kern <ker...@gmail.com> wrote: >> >>> >>> On Sep 19, 2016, at 10:30 AM, Aman Gupta <ffm...@tmm1.net> wrote: >>> >>> >>> >>> On Monday, September 19, 2016, Richard Kern <ker...@gmail.com> wrote: >>> >>>> >>>> On Sep 10, 2016, at 10:33 PM, Aman Gupta <ffm...@tmm1.net> wrote: >>>> >>>> >>>> >>>> On Sunday, September 11, 2016, Richard Kern <ker...@gmail.com> wrote: >>>> >>>>> >>>>> > On Sep 8, 2016, at 4:19 AM, Aman Gupta <ffm...@tmm1.net> wrote: >>>>> > >>>>> > From: Aman Gupta <a...@tmm1.net> >>>>> > >>>>> > --- >>>>> > libavcodec/videotoolboxenc.c | 76 ++++++++++++++++++++++++++++++ >>>>> ++++++++------ >>>>> > 1 file changed, 67 insertions(+), 9 deletions(-) >>>>> > >>>>> > diff --git a/libavcodec/videotoolboxenc.c >>>>> b/libavcodec/videotoolboxenc.c >>>>> > index 4345ca3..859dde9 100644 >>>>> > --- a/libavcodec/videotoolboxenc.c >>>>> > +++ b/libavcodec/videotoolboxenc.c >>>>> > @@ -32,6 +32,7 @@ >>>>> > #include "libavutil/pixdesc.h" >>>>> > #include "internal.h" >>>>> > #include <pthread.h> >>>>> > +#include "h264.h" >>>>> > >>>>> > #if !CONFIG_VT_BT2020 >>>>> > # define kCVImageBufferColorPrimaries_ITU_R_2020 >>>>> CFSTR("ITU_R_2020") >>>>> > @@ -55,8 +56,14 @@ typedef enum VTH264Entropy{ >>>>> > >>>>> > static const uint8_t start_code[] = { 0, 0, 0, 1 }; >>>>> > >>>>> > +typedef struct ExtraSEI { >>>>> > + void *data; >>>>> > + size_t size; >>>>> > +} ExtraSEI; >>>>> > + >>>>> > typedef struct BufNode { >>>>> > CMSampleBufferRef cm_buffer; >>>>> > + ExtraSEI *sei; >>>>> > struct BufNode* next; >>>>> > int error; >>>>> > } BufNode; >>>>> > @@ -94,6 +101,7 @@ typedef struct VTEncContext { >>>>> > bool flushing; >>>>> > bool has_b_frames; >>>>> > bool warned_color_range; >>>>> > + bool a53_cc; >>>>> > } VTEncContext; >>>>> > >>>>> > static int vtenc_populate_extradata(AVCodecContext *avctx, >>>>> > @@ -136,7 +144,7 @@ static void set_async_error(VTEncContext *vtctx, >>>>> int err) >>>>> > pthread_mutex_unlock(&vtctx->lock); >>>>> > } >>>>> > >>>>> > -static int vtenc_q_pop(VTEncContext *vtctx, bool wait, >>>>> CMSampleBufferRef *buf) >>>>> > +static int vtenc_q_pop(VTEncContext *vtctx, bool wait, >>>>> CMSampleBufferRef *buf, ExtraSEI **sei) >>>>> > { >>>>> > BufNode *info; >>>>> > >>>>> > @@ -173,6 +181,12 @@ static int vtenc_q_pop(VTEncContext *vtctx, >>>>> bool wait, CMSampleBufferRef *buf) >>>>> > pthread_mutex_unlock(&vtctx->lock); >>>>> > >>>>> > *buf = info->cm_buffer; >>>>> > + if (sei && *buf) { >>>>> > + *sei = info->sei; >>>>> > + } else if (info->sei) { >>>>> > + if (info->sei->data) av_free(info->sei->data); >>>>> > + av_free(info->sei); >>>>> > + } >>>>> > av_free(info); >>>>> > >>>>> > vtctx->frame_ct_out++; >>>>> > @@ -180,7 +194,7 @@ static int vtenc_q_pop(VTEncContext *vtctx, bool >>>>> wait, CMSampleBufferRef *buf) >>>>> > return 0; >>>>> > } >>>>> > >>>>> > -static void vtenc_q_push(VTEncContext *vtctx, CMSampleBufferRef >>>>> buffer) >>>>> > +static void vtenc_q_push(VTEncContext *vtctx, CMSampleBufferRef >>>>> buffer, ExtraSEI *sei) >>>>> > { >>>>> > BufNode *info = av_malloc(sizeof(BufNode)); >>>>> > if (!info) { >>>>> > @@ -190,6 +204,7 @@ static void vtenc_q_push(VTEncContext *vtctx, >>>>> CMSampleBufferRef buffer) >>>>> > >>>>> > CFRetain(buffer); >>>>> > info->cm_buffer = buffer; >>>>> > + info->sei = sei; >>>>> > info->next = NULL; >>>>> > >>>>> > pthread_mutex_lock(&vtctx->lock); >>>>> > @@ -420,6 +435,7 @@ static void vtenc_output_callback( >>>>> > { >>>>> > AVCodecContext *avctx = ctx; >>>>> > VTEncContext *vtctx = avctx->priv_data; >>>>> > + ExtraSEI *sei = sourceFrameCtx; >>>>> > >>>>> > if (vtctx->async_error) { >>>>> > if(sample_buffer) CFRelease(sample_buffer); >>>>> > @@ -440,7 +456,7 @@ static void vtenc_output_callback( >>>>> > } >>>>> > } >>>>> > >>>>> > - vtenc_q_push(vtctx, sample_buffer); >>>>> > + vtenc_q_push(vtctx, sample_buffer, sei); >>>>> > } >>>>> > >>>>> > static int get_length_code_size( >>>>> > @@ -1258,7 +1274,8 @@ static int copy_replace_length_codes( >>>>> > static int vtenc_cm_to_avpacket( >>>>> > AVCodecContext *avctx, >>>>> > CMSampleBufferRef sample_buffer, >>>>> > - AVPacket *pkt) >>>>> > + AVPacket *pkt, >>>>> > + ExtraSEI *sei) >>>>> > { >>>>> > VTEncContext *vtctx = avctx->priv_data; >>>>> > >>>>> > @@ -1269,6 +1286,7 @@ static int vtenc_cm_to_avpacket( >>>>> > size_t header_size = 0; >>>>> > size_t in_buf_size; >>>>> > size_t out_buf_size; >>>>> > + size_t sei_nalu_size = 0; >>>>> > int64_t dts_delta; >>>>> > int64_t time_base_num; >>>>> > int nalu_count; >>>>> > @@ -1298,9 +1316,14 @@ static int vtenc_cm_to_avpacket( >>>>> > if(status) >>>>> > return status; >>>>> > >>>>> > + if (sei) { >>>>> > + sei_nalu_size = sizeof(start_code) + 3 + sei->size + 1; >>>>> > + } >>>>> > + >>>>> > in_buf_size = CMSampleBufferGetTotalSampleSize(sample_buffer); >>>>> > out_buf_size = header_size + >>>>> > in_buf_size + >>>>> > + sei_nalu_size + >>>>> > nalu_count * ((int)sizeof(start_code) - >>>>> (int)length_code_size); >>>>> > >>>>> > status = ff_alloc_packet2(avctx, pkt, out_buf_size, >>>>> out_buf_size); >>>>> > @@ -1317,7 +1340,7 @@ static int vtenc_cm_to_avpacket( >>>>> > length_code_size, >>>>> > sample_buffer, >>>>> > pkt->data + header_size, >>>>> > - pkt->size - header_size >>>>> > + pkt->size - header_size - sei_nalu_size >>>>> > ); >>>>> > >>>>> > if (status) { >>>>> > @@ -1325,6 +1348,19 @@ static int vtenc_cm_to_avpacket( >>>>> > return status; >>>>> > } >>>>> > >>>>> > + if (sei_nalu_size > 0) { >>>>> > + uint8_t *sei_nalu = pkt->data + pkt->size - sei_nalu_size; >>>>> > + memcpy(sei_nalu, start_code, sizeof(start_code)); >>>>> > + sei_nalu += sizeof(start_code); >>>>> > + sei_nalu[0] = NAL_SEI; >>>>> > + sei_nalu[1] = SEI_TYPE_USER_DATA_REGISTERED; >>>>> > + sei_nalu[2] = sei->size; >>>>> > + sei_nalu += 3; >>>>> > + memcpy(sei_nalu, sei->data, sei->size); >>>>> > + sei_nalu += sei->size; >>>>> > + sei_nalu[0] = 1; // RBSP >>>>> > + } >>>>> > + >>>>> >>>>> SEI data should come after the parameter sets and before the other NAL >>>>> units. >>>> >>>> >>>> Thanks. I have no prior experience with the h264 bitstream format so >>>> the help is much appreciated. Any advice on how to get the SEI inserted at >>>> the right spot? >>>> >>>> I also am unsure if I need to worry about start code emulation >>>> prevention within the SEI data. >>>> >>>> The SEI data comes after the parameter sets and AUD NAL units (when >>>> they’re present). If I understand correctly, only one SEI NAL unit can be >>>> present per access unit (but it can contain multiple SEI messages). When >>>> the SEI NAL contains a buffering period SEI message it comes first in the >>>> list. The H.264 spec is available from the ITU website and contains the >>>> syntax for NAL units, SEI data, etc. >>>> >>>> Sorry I’ve been slow getting back to you. Hope this is helpful. If >>>> you’d prefer, I can reorder the SEI data later this week. >>>> >>> >>> No problem. If you get a chance to work up a patch, I would very much >>> appreciate it. >>> >>> >>> Pushed, thanks. >>> >> >> Thanks! >> >> I tried a few samples which worked, but on others I'm getting "Error >> copying packet data: -1397118274" errors (looks like >> AVERROR_BUFFER_TOO_SMALL). >> >> Here's a sample that reproduces the error: https://s3.amazonaws.co >> m/tmm1/ccaptions/nightlynews2.ts >> >> Thanks, looking into it. >> > > Actually, it looks like it's only happening when I use a specific video > filter that I'm writing. Other filters seem fine, so the bug must be in my > code somewhere. >
Still not sure why my filter was triggering the issue, since it's not doing anything that interesting.. Nevertheless, here's a sample which reproduces the overflow with just a simple use of -c:v h264_videotoolbox: https://s3.amazonaws.com/tmm1/ccaptions/nightlynews3.ts It looks like an off-by-one somewhere, because if I increase out_buf_size by 1 the error goes away. Aman > > Aman > > >> >> >> >>> >>> >>> >>>> >>>> >>>> >>>>> >>>>> > if (is_key_frame) { >>>>> > pkt->flags |= AV_PKT_FLAG_KEY; >>>>> > } >>>>> > @@ -1707,6 +1743,7 @@ static int vtenc_send_frame(AVCodecContext >>>>> *avctx, >>>>> > CMTime time; >>>>> > CFDictionaryRef frame_dict; >>>>> > CVPixelBufferRef cv_img = NULL; >>>>> > + ExtraSEI *sei = NULL; >>>>> > int status = create_cv_pixel_buffer(avctx, frame, &cv_img); >>>>> > >>>>> > if (status) return status; >>>>> > @@ -1717,6 +1754,20 @@ static int vtenc_send_frame(AVCodecContext >>>>> *avctx, >>>>> > return status; >>>>> > } >>>>> > >>>>> > + if (vtctx->a53_cc) { >>>>> > + sei = av_mallocz(sizeof(*sei)); >>>>> > + if (!sei) { >>>>> > + av_log(avctx, AV_LOG_ERROR, "Not enough memory for >>>>> closed captions, skipping\n"); >>>>> > + } else { >>>>> > + int ret = ff_alloc_a53_sei(frame, 0, &sei->data, >>>>> &sei->size); >>>>> > + if (ret < 0) { >>>>> > + av_log(avctx, AV_LOG_ERROR, "Not enough memory for >>>>> closed captions, skipping\n"); >>>>> > + av_free(sei); >>>>> > + sei = NULL; >>>>> > + } >>>>> > + } >>>>> > + } >>>>> > + >>>>> > time = CMTimeMake(frame->pts * avctx->time_base.num, >>>>> avctx->time_base.den); >>>>> > status = VTCompressionSessionEncodeFrame( >>>>> > vtctx->session, >>>>> > @@ -1724,7 +1775,7 @@ static int vtenc_send_frame(AVCodecContext >>>>> *avctx, >>>>> > time, >>>>> > kCMTimeInvalid, >>>>> > frame_dict, >>>>> > - NULL, >>>>> > + sei, >>>>> > NULL >>>>> > ); >>>>> > >>>>> > @@ -1749,6 +1800,7 @@ static av_cold int vtenc_frame( >>>>> > bool get_frame; >>>>> > int status; >>>>> > CMSampleBufferRef buf = NULL; >>>>> > + ExtraSEI *sei = NULL; >>>>> > >>>>> > if (frame) { >>>>> > status = vtenc_send_frame(avctx, vtctx, frame); >>>>> > @@ -1785,11 +1837,15 @@ static av_cold int vtenc_frame( >>>>> > goto end_nopkt; >>>>> > } >>>>> > >>>>> > - status = vtenc_q_pop(vtctx, !frame, &buf); >>>>> > + status = vtenc_q_pop(vtctx, !frame, &buf, &sei); >>>>> > if (status) goto end_nopkt; >>>>> > if (!buf) goto end_nopkt; >>>>> > >>>>> > - status = vtenc_cm_to_avpacket(avctx, buf, pkt); >>>>> > + status = vtenc_cm_to_avpacket(avctx, buf, pkt, sei); >>>>> > + if (sei) { >>>>> > + if (sei->data) av_free(sei->data); >>>>> > + av_free(sei); >>>>> > + } >>>>> > CFRelease(buf); >>>>> > if (status) goto end_nopkt; >>>>> > >>>>> > @@ -1878,7 +1934,7 @@ static int vtenc_populate_extradata(AVCodecContext >>>>> *avctx, >>>>> > if (status) >>>>> > goto pe_cleanup; >>>>> > >>>>> > - status = vtenc_q_pop(vtctx, 0, &buf); >>>>> > + status = vtenc_q_pop(vtctx, 0, &buf, NULL); >>>>> > if (status) { >>>>> > av_log(avctx, AV_LOG_ERROR, "popping: %d\n", status); >>>>> > goto pe_cleanup; >>>>> > @@ -1976,6 +2032,8 @@ static const AVOption options[] = { >>>>> > { "frames_after", "Other frames will come after the frames in >>>>> this session. This helps smooth concatenation issues.", >>>>> > OFFSET(frames_after), AV_OPT_TYPE_BOOL, { .i64 = 0 }, 0, 1, >>>>> VE }, >>>>> > >>>>> > + { "a53cc", "Use A53 Closed Captions (if available)", >>>>> OFFSET(a53_cc), AV_OPT_TYPE_BOOL, {.i64 = 0}, 0, 1, VE }, >>>>> > + >>>>> > { NULL }, >>>>> > }; >>>>> > >>>>> > -- >>>>> > 2.8.1 >>>>> >>>>> Patches should be made against the latest master branch. It doesn’t >>>>> compile as-is. >>>> >>>> >>>> Oops, rebased locally for next patch. >>>> >>>> >>>>> >>>>> Thanks for your work. I look forward to an updated patch. >>>>> >>>>> > >>>>> > _______________________________________________ >>>>> > ffmpeg-devel mailing list >>>>> > ffmpeg-devel@ffmpeg.org >>>>> > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel >>>> >>>> >>> >> >> > _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel