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

[Andriy Lysnevych]

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?
2) Is there any procedure for changing version? Should we increase major,
minor or micro version of libavformat?



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