[FFmpeg-devel] BUG in use of extradata and extradata_size with dvb subtitles and teletext
Clément Bœsch
u at pkh.me
Wed Jan 8 15:55:36 CET 2014
On Wed, Jan 08, 2014 at 03:59:43PM +0200, Andriy Lysnevych wrote:
> Hi,
>
Hi,
(Please do not top post on this mailing-list)
> We decided to fix DVB subtitles without touching extradata. Please review
> it. The patch does:
>
> 1) Improved DVB subtitles encoder to generate AVPacket.data in the same
> format as generates MPEGTS demuxer + DVB subtitles parser. So now single
> format of DVB subtitles data is used across all the components of FFmpeg:
> only subtitles payload WITHOUT 0x20 0x00 bytes at the beginning and 0xFF
> trailing byte.
>
> 2) Improved MPEGTS muxer to support format of DVB subtitles in
> AVPacket.data described above: while muxing we add two bytes 0x20 0x00 to
> the beginning of and 0xFF to the end of DVB subtitles payload.
>
Can you include this two points in the commit description?
> Because of changes described above automatically were fixed few FFmpeg
> issues:
>
> 3) Because now MPEGTS muxer and demuxer work with the same format of DVB
> subtitles, problem described in ticket #2989 were fixed: copy of DVB
> subtitles from MPEGTS to MPEGTS stream.
>
> 4) Fixed similar DVB subtitles copy operations: Matroska -> MPEGTS, WTV ->
> MPEGTS and most likely NUT -> MPEGTS.
>
> 5) Fixed muxing of DVB subtitles generated by DVB subtitles encoder into
> Matroska (and probably NUT) containers. Muxing was incorrect because
> Matroska requires only DVB subtitles payload but DVB subtitles encoder
> generated extra 0x00 byte at the beginning and extra 0xFF byte at the end
> of the payload.
>
> Our next patch will fix of DVB teletext copy from MPEGTS to MPEGTS. This
> patch will touch extradata.
>
> P.S.
> We are still waiting for a correct sample with DVB subtitles in NUT
> container to ensure in correctness of our patch towards NUT as well.
>
> Regards,
> Andriy Lysnevych
>
[...]
> From 99c95ab0259cb021e472b7788a56552056e1b330 Mon Sep 17 00:00:00 2001
> From: Serhii Marchuk <serhii.marchuk at gmail.com>
> Date: Wed, 8 Jan 2014 12:59:18 +0200
> Subject: [PATCH] Fix dvb subtitle
>
> ---
> libavcodec/dvbsub.c | 4 ----
> libavformat/mpegtsenc.c | 34 ++++++++++++++++++++++++----------
> 2 files changed, 24 insertions(+), 14 deletions(-)
>
> diff --git a/libavcodec/dvbsub.c b/libavcodec/dvbsub.c
> index f30b767..f6b46e6 100644
> --- a/libavcodec/dvbsub.c
> +++ b/libavcodec/dvbsub.c
> @@ -261,8 +261,6 @@ static int encode_dvb_subtitles(DVBSubtitleContext *s,
> if (h->num_rects && h->rects == NULL)
> return -1;
>
> - *q++ = 0x00; /* subtitle_stream_id */
> -
> /* page composition segment */
>
> *q++ = 0x0f; /* sync_byte */
> @@ -437,8 +435,6 @@ static int encode_dvb_subtitles(DVBSubtitleContext *s,
>
> bytestream_put_be16(&pseg_len, q - pseg_len - 2);
>
> - *q++ = 0xff; /* end of PES data */
> -
> s->object_version = (s->object_version + 1) & 0xf;
> return q - outbuf;
> }
Since you are changing the format of the packet, this means it will break
compatibility between libraries with mismatching version (if someone
updates only libavcodec and not libavformat, or the other way around). It
will also break applications expecting the old packet format.
I don't know how to best deal with that, but if we really want
to do that without compatibility break, it would look like something like
that:
1. bump minor version in both libavformat and libavcodec (see the
version.h files in respective directory
2. add an heuristic in libavformat to check in which packet format you are
(so maybe check last and first byte of the packet) and adjust the
behaviour
3. add a codec option in libavcodec/dvbsub.c (you need to add the options
system like in some other codecs) to output the 2 bytes if it is set.
This option would not be set by default until next major bump (try to
git grep FF_API to see how this can be done).
4. to make sure the new behaviour works with our tools, make
ffmpeg/ffplay/... set the codec option to 1.
5. document the behaviour change in doc/APIChanges
Anyway, I know this sounds really overkill, and the other developers
may have some better ideas. I also don't know if we really want to be
pedantic about the compatibility in this area, but since the DVB subtitles
are here since the beginning, it's probable various apps are depending on
the layout.
> diff --git a/libavformat/mpegtsenc.c b/libavformat/mpegtsenc.c
> index 1d51b97..e44b63b 100644
> --- a/libavformat/mpegtsenc.c
> +++ b/libavformat/mpegtsenc.c
> @@ -859,7 +859,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, private_code, flags;
> + int val, is_start, len, header_len, write_pcr, is_dvb_subtitle, flags;
> int afc_len, stuffing_len;
> int64_t pcr = -1; /* avoid warning */
> int64_t delay = av_rescale(s->max_delay, 90000, AV_TIME_BASE);
> @@ -927,7 +927,7 @@ static void mpegts_write_pes(AVFormatContext *s, AVStream *st,
> *q++ = 0x00;
> *q++ = 0x00;
> *q++ = 0x01;
> - private_code = 0;
> + is_dvb_subtitle = 0;
> if (st->codec->codec_type == AVMEDIA_TYPE_VIDEO) {
> if (st->codec->codec_id == AV_CODEC_ID_DIRAC) {
> *q++ = 0xfd;
> @@ -944,9 +944,9 @@ static void mpegts_write_pes(AVFormatContext *s, AVStream *st,
> *q++ = 0xfd;
> } else {
> *q++ = 0xbd;
> - if (st->codec->codec_type == AVMEDIA_TYPE_SUBTITLE) {
> - private_code = 0x20;
> - }
> + if(st->codec->codec_type == AVMEDIA_TYPE_SUBTITLE &&
> + st->codec->codec_id == AV_CODEC_ID_DVB_SUBTITLE)
nitstyle:
if (st->codec->codec_type == AVMEDIA_TYPE_SUBTITLE &&
st->codec->codec_id == AV_CODEC_ID_DVB_SUBTITLE)
> + is_dvb_subtitle = 1;
> }
> header_len = 0;
> flags = 0;
> @@ -983,8 +983,13 @@ static void mpegts_write_pes(AVFormatContext *s, AVStream *st,
> header_len += 3;
> }
> len = payload_size + header_len + 3;
> - if (private_code != 0)
> - len++;
> + /* 3 extra bytes should be added to DVB subtitle payload:
> + * 0x20 0x00 at the beginning and trailing 0xFF.
> + */
nit++: move the */ on previous line
> + if (is_dvb_subtitle) {
> + len += 3;
> + payload_size++;
> + }
> if (len > 0xffff)
> len = 0;
> *q++ = len >> 8;
> @@ -1025,8 +1030,10 @@ static void mpegts_write_pes(AVFormatContext *s, AVStream *st,
> }
>
>
> - if (private_code != 0)
> - *q++ = private_code;
> + if (is_dvb_subtitle) {
> + *q++ = 0x20;
> + *q++ = 0x00;
A comment for each value would be welcome (also, maybe there are some
corresponding define in the header you could use)
> + }
> is_start = 0;
> }
> /* header size */
> @@ -1057,7 +1064,14 @@ static void mpegts_write_pes(AVFormatContext *s, AVStream *st,
> }
> }
> }
> - memcpy(buf + TS_PACKET_SIZE - len, payload, len);
> +
> + if (is_dvb_subtitle && payload_size == len) {
> + memcpy(buf + TS_PACKET_SIZE - len, payload, len - 1);
> + buf[TS_PACKET_SIZE - 1] = 0xff;
> + } else {
> + memcpy(buf + TS_PACKET_SIZE - len, payload, len);
nit style: add a space before memcpy
> + }
> +
> payload += len;
> payload_size -= len;
> mpegts_prefix_m2ts_header(s);
I don't have knowledge on MPEG-TS itself so I can't make a more meaningful
review, sorry.
--
Clément B.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 490 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20140108/5603ec60/attachment.asc>
More information about the ffmpeg-devel
mailing list