[FFmpeg-devel] [PATCH]Basic XSUB encoder (take 2)

[Björn Axelsson]

On Tue, 3 Feb 2009, Reimar D?ffinger wrote:

> On Mon, Feb 02, 2009 at 10:55:05PM +0100, Bj?rn Axelsson wrote:
> > > * An extra field for the absolute pts is needed in AVSubtitle for the
> > > encoder. Any ideas for a better solution are welcome.
> >
> > No one mentioned this in the first review, but it still is an ugly hack. I
> > did clean it up and document it in this version, though.
>
> Sorry, Michael is more responsible for the "big picture", and there was
> a lot of discussion about subtitle timestamps I did not follow.

Ok. I'll have a look in the archives...

> > +/* For unknown reasons we pad the subtitles with two pixels on either side */
> > +#define MARGIN 2
>
> Huh? I sure hope you know why you are doing this? Maybe not why you have
> to do that, but it has to have some bad effect if you don't do it?

Unfortunately I don't know why I'm doing it. The original patch from DivX
said only "// 2 pixels required on either side of subtitle", and my naive
assumption is that they should have a good reason.

Do you suggest that I remove the padding unless I can see any
problems with the few players I have?

> > +static inline int getcolor(const uint8_t *row, int w, int x)
> > +{
> > +    if (x >= MARGIN && x < w+MARGIN)
> > +        return row[x-MARGIN] & 3;
> > +    else
> > +        return 0;
>
> Hm. That's not exactly fast. While I don't know a solution, this also
> assumes that index 0 is a transparent colour...

According to the multimediawiki and the XSUB decoder, index 0 is always
transparent (XSUB doesn't support alpha).

[...]

(The other comments will be addressed in the next patch)

-- 
Bj?rn Axelsson