[FFmpeg-devel] [PATCH 2/3] lavf/srtenc: honor subtitle position side data.

[Clément Bœsch]

On Sat, Dec 01, 2012 at 08:22:16AM +0100, Reimar Döffinger wrote:
> On 1 Dec 2012, at 00:43, Clément Bœsch <ubitux at gmail.com> wrote:
> > ---
> > libavformat/srtenc.c | 18 +++++++++++++++++-
> > 1 file changed, 17 insertions(+), 1 deletion(-)
> > 
> > diff --git a/libavformat/srtenc.c b/libavformat/srtenc.c
> > index 97b297e..57be568 100644
> > --- a/libavformat/srtenc.c
> > +++ b/libavformat/srtenc.c
> > @@ -22,6 +22,7 @@
> > #include "avformat.h"
> > #include "internal.h"
> > #include "libavutil/log.h"
> > +#include "libavutil/intreadwrite.h"
> > 
> > /* TODO: add options for:
> >    - character encoding;
> > @@ -63,6 +64,17 @@ static int srt_write_packet(AVFormatContext *avf, AVPacket *pkt)
> > 
> >     if (write_ts) {
> >         int64_t s = pkt->pts, e, d = pkt->duration;
> > +        int size, x1 = -1, y1 = -1, x2 = -1, y2 = -1;
> > +        const uint8_t *p;
> > +
> > +        av_packet_split_side_data(pkt);
> > +        p = av_packet_get_side_data(pkt, AV_PKT_DATA_SUBTITLE_POSITION, &size);
> > +        if (p && size == 16) {
> > +            x1 = AV_RL32(p     );
> > +            y1 = AV_RL32(p +  4);
> > +            x2 = AV_RL32(p +  8);
> > +            y2 = AV_RL32(p + 12);
> > +        }
> > 
> >         if (d <= 0)
> >             /* For backward compatibility, fallback to convergence_duration. */
> > @@ -73,12 +85,16 @@ static int srt_write_packet(AVFormatContext *avf, AVPacket *pkt)
> >             return 0;
> >         }
> >         e = s + d;
> > -        avio_printf(avf->pb, "%d\n%02d:%02d:%02d,%03d --> %02d:%02d:%02d,%03d\n",
> > +        avio_printf(avf->pb, "%d\n%02d:%02d:%02d,%03d --> %02d:%02d:%02d,%03d",
> >                        srt->index,
> >                        (int)(s / 3600000),      (int)(s / 60000) % 60,
> >                        (int)(s /    1000) % 60, (int)(s %  1000),
> >                        (int)(e / 3600000),      (int)(e / 60000) % 60,
> >                        (int)(e /    1000) % 60, (int)(e %  1000));
> > +        if (p)
> > +            avio_printf(avf->pb, "  X1:%03d X2:%03d Y1:%03d Y2:%03d",
> > +                        x1, y1, x2, y2);
> > +        avio_printf(avf->pb, "\n");
> 
> Sorry for the stupid unrelated question, but why are we using side-data for this instead of having it in that format in the packet?
> I really dislike this kind of unnecessary use of side data, it was meant for cases that were really difficult to fix otherwise, not as the default solution to any case where we'd be "too lazy to do it properly".

That's actually the best solution we came up with: this information can
not be part of the payload (because you can't tell if it is the position
information or the text), unless you also put the timing data into it; but
then it's a pain for muxers and demuxers such as matroska, because it has
to split and re-create problematic packets (that's how it was done with
the SRT packets, which are now clean SubRip packets). In most containers,
matroska included, this extra side data information can not be muxed.

Note: there is the exact same problem with WebVTT; I think they
standardized a way of storing that side data information though… somewhere
in another related field of the block.

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