[FFmpeg-devel] [PATCH] Add SUP (raw BluRay PGS subtitle) muxer

[Petri Hintukainen]

On to, 2014-09-04 at 10:19 +0200, Hendrik Leppkes wrote:
> On Fri, Aug 29, 2014 at 12:31 PM, Petri Hintukainen <phintuka at gmail.com> wrote:
> > From: Petri Hintukainen <phintuka at users.sourceforge.net>
> >
> > Fixes ticket #2208
> > ---
> >  libavformat/Makefile     |  1 +
> >  libavformat/allformats.c |  1 +
> >  libavformat/supenc.c     | 61 ++++++++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 63 insertions(+)
> >  create mode 100644 libavformat/supenc.c
> >
> > diff --git a/libavformat/Makefile b/libavformat/Makefile
> > index 3d124fb..e3cc653 100644
> > --- a/libavformat/Makefile
> > +++ b/libavformat/Makefile
> > @@ -405,6 +405,7 @@ OBJS-$(CONFIG_SRT_MUXER)                 += srtenc.o
> >  OBJS-$(CONFIG_STR_DEMUXER)               += psxstr.o
> >  OBJS-$(CONFIG_SUBVIEWER1_DEMUXER)        += subviewer1dec.o subtitles.o
> >  OBJS-$(CONFIG_SUBVIEWER_DEMUXER)         += subviewerdec.o subtitles.o
> > +OBJS-$(CONFIG_SUP_MUXER)                 += supenc.o
> >  OBJS-$(CONFIG_SWF_DEMUXER)               += swfdec.o swf.o
> >  OBJS-$(CONFIG_SWF_MUXER)                 += swfenc.o swf.o
> >  OBJS-$(CONFIG_TAK_DEMUXER)               += takdec.o apetag.o img2.o rawdec.o
> > diff --git a/libavformat/allformats.c b/libavformat/allformats.c
> > index 8f70c4b..a1a55f7 100644
> > --- a/libavformat/allformats.c
> > +++ b/libavformat/allformats.c
> > @@ -280,6 +280,7 @@ void av_register_all(void)
> >      REGISTER_DEMUXER (STR,              str);
> >      REGISTER_DEMUXER (SUBVIEWER1,       subviewer1);
> >      REGISTER_DEMUXER (SUBVIEWER,        subviewer);
> > +    REGISTER_MUXER   (SUP,              sup);
> >      REGISTER_MUXDEMUX(SWF,              swf);
> >      REGISTER_DEMUXER (TAK,              tak);
> >      REGISTER_MUXER   (TEE,              tee);
> > diff --git a/libavformat/supenc.c b/libavformat/supenc.c
> > new file mode 100644
> > index 0000000..3447f76
> > --- /dev/null
> > +++ b/libavformat/supenc.c
> > @@ -0,0 +1,61 @@
> > +/*
> > + * SUP muxer
> > + * Copyright (c) 2014 Petri Hintukainen <phintuka at users.sourceforge.net>
> > + *
> > + * This file is part of FFmpeg.
> > + *
> > + * FFmpeg is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU Lesser General Public
> > + * License as published by the Free Software Foundation; either
> > + * version 2.1 of the License, or (at your option) any later version.
> > + *
> > + * FFmpeg is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> > + * Lesser General Public License for more details.
> > + *
> > + * You should have received a copy of the GNU Lesser General Public
> > + * License along with FFmpeg; if not, write to the Free Software
> > + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
> > + */
> > +
> > +#include "avformat.h"
> > +#include "internal.h"
> > +
> > +
> > +#define SUP_PGS_MAGIC 0x5047 /* "PG", big endian */
> > +
> > +static int sup_write_packet(AVFormatContext *s, AVPacket *pkt)
> > +{
> > +    /* header */
> > +    avio_wb16(s->pb, SUP_PGS_MAGIC);
> > +    avio_wb32(s->pb, (uint64_t)pkt->pts);
> > +    avio_wb32(s->pb, (uint64_t)pkt->dts);
> > +
> 
> All the timestamp talk in the demuxer thread made me think about this part.
> What if no dts or pts is set (ie. its AV_NOPTS_VALUE), I doubt writing
> that as-is is going to fly over well with other tools (especially
> because its value is > 32-bit anyway)
>
>Should you write 0?
 
It writes only 32 lowest bits, so AV_NOPTS_VALUE is written as 0.

Also looks like cast to uint64_t is not necessary ?

If timestamp does not fit to 32 bits, highest bits are dropped when
converting to 32 bits. I don't see any better way to handle this, the
format can only store 32 bits.

I'm more worried with the possibility that ffmpeg may modify the
timestamps (like, to start from 0). I noticed that if I demux only
single PGS stream from .m2ts, timestamps have different starting point
than if I demux also the video track. This requires extra work when
syncing subtitles with the video (sync is lost).

Now, I don't know if the timestamps in .sup should be the original
timestamps from .m2ts, or re-positioned so that start of the movie is at
0. Probably other tools expect it to start from 0, automatic conversions
between formats would be difficult otherwise (PGS streams are sparse,
and start time of the stream is not stored in .sup file). Also those
eac3to samples seem to confirm this.
If this is true, I'm not very worried about the timestamp wrap after 32
bits (that would require ~ 13 hours movie).

>Should you write dts=pts if only pts is set, and dts is unset?

I'd leave DTS to 0 if it is unset. Setting it to PTS would be wrong (no
time allocated for decoding, also PTS is not monotonic => DTS wouldn't
be either). I also think incorrect DTS is harder to detect than missing
DTS.

It wouldn't be impossible to re-generate valid DTS based on PTS, but I
think muxer is not correct place to do it. (it would require several
segments, and analyzing the contents of the segments ... also the code
would need to be duplicated to every muxer that can mux PGS).

Another question is if the muxer should split AVPacket to PGS segments ?
Or should there be a parser ?
If PGS from mkvmerge-generated matroska file is muxed to .sup, there
will be multiple segments in one AVPacket. This will create broken /
unreadable .sup file.


- Petri