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

Marton Balint cus at passwd.hu
Sat Jan 18 13:19:12 CET 2014


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


More information about the ffmpeg-devel mailing list