[FFmpeg-devel] BUG in use of extradata and extradata_size with dvb subtitles and teletext

[Timothy Gu]

On Jan 20, 2014 12:53 PM, "Marton Balint" <cus at passwd.hu> wrote:
>
>
>
> On Mon, 20 Jan 2014, Andriy Lysnevych wrote:
>
>> On Sat, Jan 18, 2014 at 2:19 PM, Marton Balint <cus at passwd.hu> wrote:
>>>
>>>
>>> On Mon, 13 Jan 2014, Andriy Lysnevych wrote:
>>>
>>>> Hi,
>>>>
>>>> Can you please also review next patch that fixes DVB teletext issues,
>>>> particularly ticket #2223.
>>>>
>>>> In the patch we use extradata for DVB teletext in the same way as it
>>>> was for DVB subtitles: copy PMT data from input stream and restore it
>>>> in output stream.
>>>>
>>>> After applying the patch DVB teletext and DVB teletext subtitles copy
>>>> from input TS stream to output works for us.
>>>>
>>>> Regards,
>>>> Andriy Lysnevych
>>>>
>
> [...]
>
>>
>> Hi,
>>
>> Updated patch with found issues fixed attached.
>>
>> Regards,
>> Andriy Lysnevych
>>
>> From d6b08494a8257a04ef7fe0e36921d2c5f5a88d2b Mon Sep 17 00:00:00 2001
>> From: Serhii Marchuk <serhii.marchuk at gmail.com>
>> Date: Mon, 20 Jan 2014 15:53:19 +0200
>>
>> Subject: mpegts muxer and demuxer: correct DVB teletext processing
>>
>> * Using extradata by TS demuxer to store values from PMT
>> * Using extradata by TS muxer to correctly restore PMT table
>> * PES_header_data_length should be always 0x24 for DVB teletext,
>>  according to DVB standard
>> * Support of multiple languages in one DVB teletext stream:
>>  comma separated language codes in metadata "language" field
>>
>> The patch fixes #2223 ticket.
>> ---
>> libavformat/mpegts.c    | 42 +++++++++++++++++++++---
>> libavformat/mpegtsenc.c | 87
++++++++++++++++++++++++++++++++++++++-----------
>> 2 files changed, 105 insertions(+), 24 deletions(-)
>>
>> diff --git a/libavformat/mpegts.c b/libavformat/mpegts.c
>> index db380ca..df76026 100644
>> --- a/libavformat/mpegts.c
>> +++ b/libavformat/mpegts.c
>> @@ -1470,11 +1470,43 @@ int ff_parse_mpeg2_descriptor(AVFormatContext
*fc, AVStream *st, int stream_type
>>
>>         }
>>         break;
>>     case 0x56: /* DVB teletext descriptor */
>> -        language[0] = get8(pp, desc_end);
>> -        language[1] = get8(pp, desc_end);
>> -        language[2] = get8(pp, desc_end);
>> -        language[3] = 0;
>> -        av_dict_set(&st->metadata, "language", language, 0);
>> +        {
>> +            uint8_t *extradata = NULL;
>> +            int language_count;
>> +
>> +            if (desc_len == 0 || desc_len % 5 != 0)

>> +                return -1
>
>
> Is dec_len == 0 forbidden in the spec? I admit it makes no sense, but
maybe we could just ignore it instead of rejecting it. (e.g. check for
desc_len > 0 as an if() condition to the whole code block).

Also it should return AVERROR_INVALIDDATA.

>
>> +
>>
>> +            language_count = desc_len / 5;
>> +
>> +            if (st->codec->extradata == NULL) {
>> +                if (ff_alloc_extradata(st->codec, language_count * 2)) {
>> +                    return AVERROR(ENOMEM);
>> +                }
>> +            }
>> +
>> +           if (st->codec->extradata_size < language_count * 2)

>> +               return -1;

AVERROR_INVALIDDATA

[...]

Timothy