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

Andriy Lysnevych andriy.lysnevych at gmail.com
Mon Jan 20 15:14:57 CET 2014


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
>>
>> From 5877de1bb5ccbc6d151401aab205004495ad3283 Mon Sep 17 00:00:00 2001
>> From: Serhii Marchuk <serhii.marchuk at gmail.com>
>> Date: Mon, 13 Jan 2014 11:45:29 +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    | 30 ++++++++++++++---
>>  libavformat/mpegtsenc.c | 85
>> ++++++++++++++++++++++++++++++++++++++-----------
>>  2 files changed, 91 insertions(+), 24 deletions(-)
>>
>> diff --git a/libavformat/mpegts.c b/libavformat/mpegts.c
>> index db380ca..91279b1 100644
>> --- a/libavformat/mpegts.c
>> +++ b/libavformat/mpegts.c
>> @@ -1470,11 +1470,31 @@ 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);
>> +        {
>> +            int language_count = desc_len / 5;
>> +            uint8_t *extradata = 0;
>
> nit: NULL
>
>> +
>> +            if (!st->codec->extradata)
>> +                if (!ff_alloc_extradata(st->codec, language_count * 2))
>> +                        extradata = st->codec->extradata;
>
> else return AVERROR(ENOMEM)?
>
>> +
>> +            for (i = 0; i < language_count; i++) {
>> +                language[i * 4 + 0] = get8(pp, desc_end);
>> +                language[i * 4 + 1] = get8(pp, desc_end);
>> +                language[i * 4 + 2] = get8(pp, desc_end);
>> +                language[i * 4 + 3] = ',';
>> +
>> +                if (extradata) {
>> +                    memcpy(extradata, *pp, 2);
>> +                    extradata += 2;
>> +                }
>> +
>> +                *pp += 2;
>> +            }
>> +
>> +            language[i * 4 - 1] = 0;
>
> That seems broken if language_count == 0.
>
>> +            av_dict_set(&st->metadata, "language", language, 0);
>
> Only set this is language_count > 0
>
>> +        }
>
> You may consider using a similar logic that is in case 0x0a for constructing
> the language metadata, but this here can also have its merits and can be
> made right.
>
>>          break;
>>      case 0x59: /* subtitling descriptor */
>>          language[0] = get8(pp, desc_end);
>> diff --git a/libavformat/mpegtsenc.c b/libavformat/mpegtsenc.c
>> index 1e2d5cc..243654f 100644
>> --- a/libavformat/mpegtsenc.c
>> +++ b/libavformat/mpegtsenc.c
>> @@ -373,21 +373,56 @@ static void mpegts_write_pmt(AVFormatContext *s,
>> MpegTSService *service)
>>              break;
>>          case AVMEDIA_TYPE_SUBTITLE:
>>              {
>> -                const char *language;
>> -                language = lang && strlen(lang->value)==3 ? lang->value :
>> "eng";
>> -                *q++ = 0x59;
>> -                *q++ = 8;
>> -                *q++ = language[0];
>> -                *q++ = language[1];
>> -                *q++ = language[2];
>> -                *q++ = 0x10; /* normal subtitles (0x20 = if hearing pb)
>> */
>> -                if(st->codec->extradata_size == 4) {
>> -                    memcpy(q, st->codec->extradata, 4);
>> -                    q += 4;
>> -                } else {
>> -                    put16(&q, 1); /* page id */
>> -                    put16(&q, 1); /* ancillary page id */
>> -                }
>> +                char default_language[] ="eng";
>
> nit: =" missing space
> Default should be "und" for undetermined.
>
>> +                char *language = lang && strlen(lang->value) >= 3 ?
>> lang->value : default_language;
>> +
>> +                if (st->codec->codec_id == AV_CODEC_ID_DVB_SUBTITLE) {
>> +                    /* The descriptor tag. subtitling_descriptor */
>> +                    *q++ = 0x59;
>> +                    *q++ = 8;
>> +                    *q++ = language[0];
>> +                    *q++ = language[1];
>> +                    *q++ = language[2];
>> +                    *q++ = 0x10; /* normal subtitles (0x20 = if hearing
>> pb) */
>> +                    if(st->codec->extradata_size == 4) {
>> +                        memcpy(q, st->codec->extradata, 4);
>> +                        q += 4;
>> +                    } else {
>> +                        put16(&q, 1); /* page id */
>> +                        put16(&q, 1); /* ancillary page id */
>> +                    }
>> +                } else if (st->codec->codec_id ==
>> AV_CODEC_ID_DVB_TELETEXT) {
>> +                    uint8_t *len_ptr = NULL;
>> +                    int extradata_copied = 0;
>> +
>> +                    /* The descriptor tag. teletext_descriptor */
>> +                    *q++ = 0x56;
>> +                    len_ptr = q++;
>> +
>> +                    while (*language != '\0') {
>
> strlen(language) >= 3
>
>> +                        *q++ = *language++;
>> +                        *q++ = *language++;
>> +                        *q++ = *language++;
>> +                        /* Skip comma */
>> +                        if (*language != '\0')
>> +                            language++;
>> +
>> +                        if (st->codec->extradata_size > extradata_copied)
>> {
>
> I'd rather check for extradata_size - 1 > extradata_copied because you
> access two bytes...
>
>> +                            memcpy(q, st->codec->extradata +
>> extradata_copied, 2);
>> +                            extradata_copied += 2;
>> +                            q += 2;
>> +                        } else {
>> +                            /* The Teletext descriptor:
>> +                             * teletext_type: This 5-bit field indicates
>> the type of Teletext page indicated. (0x01 Initial Teletext page)
>> +                             * teletext_magazine_number: This is a 3-bit
>> field which identifies the magazine number.
>> +                             * teletext_page_number: This is an 8-bit
>> field giving two 4-bit hex digits identifying the page number. */
>> +                            *q++ = 0x08;
>> +                            *q++ = 0x00;
>> +                        }
>> +                    }
>> +
>> +                    *len_ptr = q - len_ptr - 1;
>> +                 }
>>              }
>>              break;
>>          case AVMEDIA_TYPE_VIDEO:
>> @@ -859,7 +894,7 @@ static void mpegts_write_pes(AVFormatContext *s,
>> AVStream *st,
>>
>>      MpegTSWrite *ts = s->priv_data;
>>      uint8_t buf[TS_PACKET_SIZE];
>>      uint8_t *q;
>> -    int val, is_start, len, header_len, write_pcr, is_dvb_subtitle,
>> flags;
>> +    int val, is_start, len, header_len, write_pcr, is_dvb_subtitle,
>> is_dvb_teletext, flags;
>>
>>      int afc_len, stuffing_len;
>>      int64_t pcr = -1; /* avoid warning */
>>      int64_t delay = av_rescale(s->max_delay, 90000, AV_TIME_BASE);
>> @@ -923,11 +958,13 @@ static void mpegts_write_pes(AVFormatContext *s,
>> AVStream *st,
>>          }
>>          if (is_start) {
>>              int pes_extension = 0;
>> +            int pes_header_stuffing_bytes = 0;
>>              /* write PES header */
>>
>>              *q++ = 0x00;
>>              *q++ = 0x00;
>>              *q++ = 0x01;
>>              is_dvb_subtitle = 0;
>> +            is_dvb_teletext = 0;
>>
>>              if (st->codec->codec_type == AVMEDIA_TYPE_VIDEO) {
>>                  if (st->codec->codec_id == AV_CODEC_ID_DIRAC) {
>>                      *q++ = 0xfd;
>> @@ -944,9 +981,12 @@ static void mpegts_write_pes(AVFormatContext *s,
>> AVStream *st,
>>
>>                  *q++ = 0xfd;
>>              } else {
>>                  *q++ = 0xbd;
>> -                if (st->codec->codec_type == AVMEDIA_TYPE_SUBTITLE &&
>> -                    st->codec->codec_id == AV_CODEC_ID_DVB_SUBTITLE) {
>> -                    is_dvb_subtitle = 1;
>> +                if(st->codec->codec_type == AVMEDIA_TYPE_SUBTITLE) {
>> +                    if (st->codec->codec_id == AV_CODEC_ID_DVB_SUBTITLE)
>> {
>> +                        is_dvb_subtitle = 1;
>> +                    } else if (st->codec->codec_id ==
>> AV_CODEC_ID_DVB_TELETEXT) {
>> +                        is_dvb_teletext = 1;
>> +                    }
>>                  }
>>              }
>>              header_len = 0;
>> @@ -983,6 +1023,10 @@ static void mpegts_write_pes(AVFormatContext *s,
>> AVStream *st,
>>                          flags |= 0x01;
>>                          header_len += 3;
>>              }
>> +            if (is_dvb_teletext) {
>> +                pes_header_stuffing_bytes = 0x24 - header_len;
>> +                header_len = 0x24;
>> +            }
>>
>>              len = payload_size + header_len + 3;
>>              /* 3 extra bytes should be added to DVB subtitle payload:
>> 0x20 0x00 at the beginning and trailing 0xff */
>>              if (is_dvb_subtitle) {
>> @@ -1036,6 +1080,9 @@ static void mpegts_write_pes(AVFormatContext *s,
>> AVStream *st,
>>                  *q++ = 0x20;
>>                  *q++ = 0x00;
>>              }
>> +            if (is_dvb_teletext)
>> +                for (int i = 0; i < pes_header_stuffing_bytes; i++)
>
> I think inline declarations should not be used in ffmpeg.
>
>> +                    *q++ = 0xff;
>>
>>              is_start = 0;
>>          }
>>          /* header size */
>> --
>> 1.8.3.2
>
>
> Regards,
> Marton
>

Hi,

Updated patch with found issues fixed attached.

Regards,
Andriy Lysnevych
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-correct-DVB-teletext-processing.patch
Type: text/x-patch
Size: 8808 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20140120/20160d0b/attachment.bin>


More information about the ffmpeg-devel mailing list