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

Marton Balint cus at passwd.hu
Mon Jan 20 21:53:15 CET 2014



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).

>+
>+            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;
>+
>+           extradata = st->codec->extradata;
>+
>+            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) {

I beleive the if() is no longer necessary.

>+                    memcpy(extradata, *pp, 2);
>+                    extradata += 2;
>+                }
>+
>+                *pp += 2;
>+            }
>+
>+            language[i * 4 - 1] = 0;
>+            av_dict_set(&st->metadata, "language", language, 0);
>+        }
>         break;
>     case 0x59: /* subtitling descriptor */
>         language[0] = get8(pp, desc_end);
>diff --git a/libavformat/mpegtsenc.c b/libavformat/mpegtsenc.c
>index 1e2d5cc..fcaa032 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[] = "und";
>+                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 (strlen(language) >= 3) {
>+                        *q++ = *language++;
>+                        *q++ = *language++;
>+                        *q++ = *language++;
>+                        /* Skip comma */
>+                        if (*language != '\0')
>+                            language++;
>+
>+                        if (st->codec->extradata_size - 1 > extradata_copied) {
>+                            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,11 @@ 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++) {

Inline declaration is still here. BTW you may use memset instead.

>+                    *q++ = 0xff;
>+                }
>+            }
>             is_start = 0;
>         }
>         /* header size */
>-- 
>1.8.3.2
>

Regards,
Marton


More information about the ffmpeg-devel mailing list