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

Michael Niedermayer michaelni at gmx.at
Wed Jan 8 17:33:59 CET 2014


On Wed, Jan 08, 2014 at 06:15:19PM +0200, Andriy Lysnevych wrote:
> Any way we must fix this bug at some point. Having two DVB subtitle formats
> in FFmpeg already caused a lot of related issues.
> 
> Applying only libavcodec change will break DVB subtitle encoder -> MPEG-TS
> muxer chain and as the result output MPEG-TS stream will contain malformed
> DVB subtitle payload.
> 
> To keep compatibility we can check version of libavformat in DVB subtitle
> encoder and add beginning 0x00 and trailing 0xFF bytes for older versions
> as it was before. To do so we must increase version of libavformat with
> this patch.
> 
> Questions are:

> 1) How to detect libavformat version from libavcodec? Is calling
> avformat_version() ok?

no
libavcodec has no way to detect the libavformat its used with or even
if its used with any libavformat or different code.

I think there are only 2 (reasonable) solutions
A. what ubitux suggested, that a option is added and the default stays
   the old format and the application would have to set it to have
   the new format used
B. just accept that the format was buggy&broken and that this is a bug
   fix

Did any application use / depend on this specific "buggy" format ?

I think ubitux and any application developers affected by this are
the best to decide of B. is acceptable


> 2) Is there any procedure for changing version? Should we increase major,
> minor or micro version of libavformat?

minor of libavformat/libavcodec should be changed by +1


> 
> 
> 
> On Wed, Jan 8, 2014 at 5:30 PM, Michael Niedermayer <michaelni at gmx.at>wrote:
> 
> > On Wed, Jan 08, 2014 at 03:55:36PM +0100, Clément Bœsch wrote:
> > > 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.
> >
> > As libavcodec is a dependancy of libavformat, you can have a newer
> > libavcodec with a older libavformat but not a newer libavformat with a
> > older libavcodec (that libavformat could even be using functions from
> > libavcodec that where not present in the older one)
> > (newer / older defined based on minor versions)
> >
> >
> > >
> > > 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:
> > >
> >
> > If its decided that this compatibility code is needed :
> > 0. split the patch between libavcodec and libavformat
> >    this makes it also easy to check if applying only the libavcodec
> >    change would break anything
> >
> > [...]
> >
> > --
> > Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
> >
> > No snowflake in an avalanche ever feels responsible. -- Voltaire
> >
> > _______________________________________________
> > ffmpeg-devel mailing list
> > ffmpeg-devel at ffmpeg.org
> > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> >
> >
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Awnsering whenever a program halts or runs forever is
On a turing machine, in general impossible (turings halting problem).
On any real computer, always possible as a real computer has a finite number
of states N, and will either halt in less than N cycles or never halt.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20140108/8b87a8e5/attachment.asc>


More information about the ffmpeg-devel mailing list