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

[Reimar Döffinger]

On Tue, Feb 03, 2009 at 10:37:34AM +0100, Bj?rn Axelsson wrote:
> On Tue, 3 Feb 2009, Reimar D?ffinger wrote:
> > > +/* 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.

Ah, ok then. It might be because the subtitles are rendered via hardware
overlay, some players might have trouble rendering stuff at the borders
of the overlay.

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

Nah, but making it work with MARGIN == 0 would be nice IMO.

> > > +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).

Ok, as in my suggestion I would prefer it if you used a define (whether
MARGIN_COLOR or TRANSPARENT_COLOR I do not really care about).
I would like to point out that to my knowledge for DVD subs it is not
always index 0 that is transparent, and the XSUB encoder will horribly
fail for those then.
Some checking (and printing a warning or failing) or even reordering of
the palette may be a nice feature, I won't insist though (and it can be
added after the first version was applied)...

Greetings,
Reimar D?ffinger