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

[Reimar Döffinger]

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.