[FFmpeg-devel] [PATCH 4/6] Support encoding of Active Format Description (AFD) in libx264

Devin Heitmueller dheitmueller at ltnglobal.com
Fri Nov 17 02:38:41 EET 2017


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 at 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


More information about the ffmpeg-devel mailing list