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

[Michael Niedermayer]

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
-------------- 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/d8c81097/attachment.asc>