Hello Derek,
Thanks for taking the time to review these patches. Comments below.
> On Nov 16, 2017, at 7:20 PM, Derek Buitenhuis <derek.buitenh...@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
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
