[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