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

[Clément Bœsch]

On Sat, Dec 01, 2012 at 04:43:10PM +0100, Reimar Döffinger wrote:
> On Sat, Dec 01, 2012 at 10:31:47AM +0100, Clément Bœsch wrote:
> > 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
> 
> Well, you don't necessarily need timing information, you would just need
> _something_ to recognize it by (for example the first byte could be the
> start offset of the text), but I see that it is quite messy, too.

Adding a fake timing information or even a simple binary marker to notify
the presence of the side isn't reliable, but it's also pain for demuxers
(which need to construct such packets) and muxers (which need to split the
data) all the time. See for instance the various SRT strings hacks in
matroska… We wanted to avoid that, that's the reason this information is
put in the side data of the packets for the muxers supporting it (like SRT
where they are coded as a text string along the timing line). Now the
payload just contains the text event.

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