| Message ID | 20171116183417.93073-5-dheitmueller@ltnglobal.com |
|---|---|
| State | Superseded |
| Headers | show |
On 11/16/2017 6:34 PM, Devin Heitmueller wrote: > + /* Active Format Description */ > + if (x4->afd) { > + void *sei_data; > + size_t sei_size; > + > + ret = ff_alloc_afd_sei(frame, 0, &sei_data, &sei_size); > + if (ret < 0) { > + av_log(ctx, AV_LOG_ERROR, "Not enough memory for AFD, skipping\n"); > + } else if (sei_data) { In an OOM situation, we always fail hard. > + x4->pic.extra_sei.payloads = av_realloc(x4->pic.extra_sei.payloads, > + sizeof(x4->pic.extra_sei.payloads[0]) * (num_payloads + 1)); > + if (x4->pic.extra_sei.payloads == NULL) { > + av_log(ctx, AV_LOG_ERROR, "Not enough memory for AFD, skipping\n"); This will leak the original x4->pic.extra_sei.payloads on failure, won't it? Also, as above, we should fail hard here. > + /* country code (SCTE 128-1 Sec 8.1.1) */ > + sei_data[0] = 181; > + sei_data[1] = 0; > + sei_data[2] = 49; > + > + /* country code (SCTE 128-1 Sec 8.1.2) */ > + AV_WL32(sei_data + 3, MKTAG('D', 'T', 'G', '1')); > + > + /* country code (SCTE 128-1 Sec 8.2.5) */ > + sei_data[7] = 0x41; > + sei_data[8] = 0xf0 | side_data->data[0]; I assume these values are supposed to always be the same? Excuse my unfamiliarity with SCTE-128 - country codes sounds like something you wouldn't want to hardcode. - Derek
Hello Derek, Thanks for taking the time to review these patches. Comments below. > On Nov 16, 2017, at 7:20 PM, Derek Buitenhuis <derek.buitenhuis@gmail.com> wrote: > > On 11/16/2017 6:34 PM, Devin Heitmueller wrote: > >> + /* Active Format Description */ >> + if (x4->afd) { >> + void *sei_data; >> + size_t sei_size; >> + >> + ret = ff_alloc_afd_sei(frame, 0, &sei_data, &sei_size); >> + if (ret < 0) { >> + av_log(ctx, AV_LOG_ERROR, "Not enough memory for AFD, skipping\n"); >> + } else if (sei_data) { > > In an OOM situation, we always fail hard. Ok. > >> + x4->pic.extra_sei.payloads = av_realloc(x4->pic.extra_sei.payloads, >> + sizeof(x4->pic.extra_sei.payloads[0]) * (num_payloads + 1)); >> + if (x4->pic.extra_sei.payloads == NULL) { >> + av_log(ctx, AV_LOG_ERROR, "Not enough memory for AFD, skipping\n"); > > This will leak the original x4->pic.extra_sei.payloads on failure, won't it? > > Also, as above, we should fail hard here. Ok. > >> + /* country code (SCTE 128-1 Sec 8.1.1) */ >> + sei_data[0] = 181; >> + sei_data[1] = 0; >> + sei_data[2] = 49; >> + >> + /* country code (SCTE 128-1 Sec 8.1.2) */ >> + AV_WL32(sei_data + 3, MKTAG('D', 'T', 'G', '1')); >> + >> + /* country code (SCTE 128-1 Sec 8.2.5) */ >> + sei_data[7] = 0x41; >> + sei_data[8] = 0xf0 | side_data->data[0]; > > I assume these values are supposed to always be the same? Excuse my unfamiliarity > with SCTE-128 - country codes sounds like something you wouldn't want to hardcode. For whatever reason, the spec explicitly calls for the country code to be set to these values. Here’s the specific language from the spec: itu_t_t35_country_code – A fixed 8-bit field, the value of which shall be 0xB5. itu_t_35_provider_code – A fixed 16-bit field registered by the ATSC. The value shall be 0x0031. (Note, the code in question was actually copied from the function directly above it which creates the SEI for A53 captions). All that said, it looks like I did screw up the comments. The Spec section references are correct but for some reason all three say “country code”, which is a typo. I’ll clean up the OOM handling as you requested, as well as fix the comments in a V2 patch. Just an FYI, the spec is freely available here in case you want to know more: https://www.scte.org/documents/pdf/Standards/ANSI_SCTE%20128-1%202013.pdf <https://www.scte.org/documents/pdf/Standards/ANSI_SCTE%20128-1%202013.pdf> Devin
On 11/17/2017 12:38 AM, Devin Heitmueller wrote: > For whatever reason, the spec explicitly calls for the country code to be set to these values. Here’s the specific language from the spec: > > itu_t_t35_country_code – A fixed 8-bit field, the value of which shall be 0xB5. > itu_t_35_provider_code – A fixed 16-bit field registered by the ATSC. The value shall be 0x0031. "Spec says so" is a good enough answer for me. - Derek
diff --git a/libavcodec/internal.h b/libavcodec/internal.h index d47ce0e..a2c7be4 100644 --- a/libavcodec/internal.h +++ b/libavcodec/internal.h @@ -408,6 +408,9 @@ int ff_side_data_set_encoder_stats(AVPacket *pkt, int quality, int64_t *error, i int ff_alloc_a53_sei(const AVFrame *frame, size_t prefix_len, void **data, size_t *sei_size); +int ff_alloc_afd_sei(const AVFrame *frame, size_t prefix_len, + void **data, size_t *sei_size); + /** * Get an estimated video bitrate based on frame size, frame rate and coded * bits per pixel. diff --git a/libavcodec/libx264.c b/libavcodec/libx264.c index 9c67c91..f0f3260 100644 --- a/libavcodec/libx264.c +++ b/libavcodec/libx264.c @@ -86,6 +86,7 @@ typedef struct X264Context { int forced_idr; int coder; int a53_cc; + int afd; int b_frame_strategy; int chroma_offset; int scenechange_threshold; @@ -275,6 +276,7 @@ static int X264_frame(AVCodecContext *ctx, AVPacket *pkt, const AVFrame *frame, x264_nal_t *nal; int nnal, i, ret; x264_picture_t pic_out = {0}; + int num_payloads = 0; int pict_type; x264_picture_init( &x4->pic ); @@ -323,10 +325,37 @@ static int X264_frame(AVCodecContext *ctx, AVPacket *pkt, const AVFrame *frame, } else { x4->pic.extra_sei.sei_free = av_free; - x4->pic.extra_sei.payloads[0].payload_size = sei_size; - x4->pic.extra_sei.payloads[0].payload = sei_data; - x4->pic.extra_sei.num_payloads = 1; - x4->pic.extra_sei.payloads[0].payload_type = 4; + x4->pic.extra_sei.payloads[num_payloads].payload_size = sei_size; + x4->pic.extra_sei.payloads[num_payloads].payload = sei_data; + x4->pic.extra_sei.payloads[num_payloads].payload_type = 4; + x4->pic.extra_sei.num_payloads++; + num_payloads++; + } + } + } + + /* Active Format Description */ + if (x4->afd) { + void *sei_data; + size_t sei_size; + + ret = ff_alloc_afd_sei(frame, 0, &sei_data, &sei_size); + if (ret < 0) { + av_log(ctx, AV_LOG_ERROR, "Not enough memory for AFD, skipping\n"); + } else if (sei_data) { + x4->pic.extra_sei.payloads = av_realloc(x4->pic.extra_sei.payloads, + sizeof(x4->pic.extra_sei.payloads[0]) * (num_payloads + 1)); + if (x4->pic.extra_sei.payloads == NULL) { + av_log(ctx, AV_LOG_ERROR, "Not enough memory for AFD, skipping\n"); + av_free(sei_data); + } else { + x4->pic.extra_sei.sei_free = av_free; + + x4->pic.extra_sei.payloads[num_payloads].payload_size = sei_size; + x4->pic.extra_sei.payloads[num_payloads].payload = sei_data; + x4->pic.extra_sei.payloads[num_payloads].payload_type = 4; + x4->pic.extra_sei.num_payloads++; + num_payloads++; } } } @@ -892,6 +921,7 @@ static const AVOption options[] = { {"passlogfile", "Filename for 2 pass stats", OFFSET(stats), AV_OPT_TYPE_STRING, {.str=NULL}, 0, 0, VE}, {"wpredp", "Weighted prediction for P-frames", OFFSET(wpredp), AV_OPT_TYPE_STRING, {.str=NULL}, 0, 0, VE}, {"a53cc", "Use A53 Closed Captions (if available)", OFFSET(a53_cc), AV_OPT_TYPE_BOOL, {.i64 = 1}, 0, 1, VE}, + {"afd", "Use Active Format Description (AFD) (if available)",OFFSET(afd), AV_OPT_TYPE_BOOL, {.i64 = 1}, 0, 1, VE}, {"x264opts", "x264 options", OFFSET(x264opts), AV_OPT_TYPE_STRING, {.str=NULL}, 0, 0, VE}, { "crf", "Select the quality for constant quality mode", OFFSET(crf), AV_OPT_TYPE_FLOAT, {.dbl = -1 }, -1, FLT_MAX, VE }, { "crf_max", "In CRF mode, prevents VBV from lowering quality beyond this point.",OFFSET(crf_max), AV_OPT_TYPE_FLOAT, {.dbl = -1 }, -1, FLT_MAX, VE }, diff --git a/libavcodec/utils.c b/libavcodec/utils.c index e50de6e..550b030 100644 --- a/libavcodec/utils.c +++ b/libavcodec/utils.c @@ -2396,6 +2396,42 @@ int ff_alloc_a53_sei(const AVFrame *frame, size_t prefix_len, return 0; } +/* Defined in SCTE 128-1 2013 Sec 8.1 */ +int ff_alloc_afd_sei(const AVFrame *frame, size_t prefix_len, + void **data, size_t *sei_size) +{ + AVFrameSideData *side_data = NULL; + uint8_t *sei_data; + + if (frame) + side_data = av_frame_get_side_data(frame, AV_FRAME_DATA_AFD); + + if (!side_data) { + *data = NULL; + return 0; + } + + *sei_size = 9; + *data = av_mallocz(*sei_size + prefix_len); + if (!*data) + return AVERROR(ENOMEM); + sei_data = (uint8_t*)*data + prefix_len; + + /* country code (SCTE 128-1 Sec 8.1.1) */ + sei_data[0] = 181; + sei_data[1] = 0; + sei_data[2] = 49; + + /* country code (SCTE 128-1 Sec 8.1.2) */ + AV_WL32(sei_data + 3, MKTAG('D', 'T', 'G', '1')); + + /* country code (SCTE 128-1 Sec 8.2.5) */ + sei_data[7] = 0x41; + sei_data[8] = 0xf0 | side_data->data[0]; + + return 0; +} + int64_t ff_guess_coded_bitrate(AVCodecContext *avctx) { AVRational framerate = avctx->framerate;
If AFD side data is present, include it in an H.264 SEI payload when encoding with libx264. This is done in the same manner that we currently handle A53 closed captions (where the business logic for constructing the SEI is in libavcodec/utils.c), so it should be portable to the other encoder types (i.e. videotoolbox, etc). Signed-off-by: Devin Heitmueller <dheitmueller@ltnglobal.com> --- libavcodec/internal.h | 3 +++ libavcodec/libx264.c | 38 ++++++++++++++++++++++++++++++++++---- libavcodec/utils.c | 36 ++++++++++++++++++++++++++++++++++++ 3 files changed, 73 insertions(+), 4 deletions(-)