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

[Reimar Döffinger]

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.

> +/* 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?

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

> +    init_put_bits(&pb, *buffer, bufsize);

put_bits does not ensure you do not write outside the buffer, you still
have to check that yourself.

> +    for (y = 0; y < h; y++) {
> +        x = 0;
> +        while (x < w2) {
> +            x1 = x;
> +            color = getcolor(bitmap, w, x1++);
> +            while (x1 < w2 && getcolor(bitmap, w, x1) == color)
> +                x1++;

You should try to handle the margin case somehow nicer.
E.g.
>    for (y = 0; y < h; y++) {
>        x = 0;
>        while (x < w) {
>            x1 = x;
>            color = bitmap[x1++] & 3;
>            while (x1 < w && (bitmap[x1] & 3) == color) x1++;
>            len = x1 - x;
>            if (x == 0) {
>                if (color == MARGIN_COLOR) len += MARGIN;
>                else put_xsub_rle(&pb, MARGIN, MARGIN_COLOR);
>            }
>            // Run can't be longer than 255, unless it is the rest of a row
>            if (x1 == w && color == MARGIN_COLOR) len += MARGIN;
>            else len = FFMIN(len, 255);
>            put_xsub_rle(&pb, len, color);
>            x += len;
>        }
>        if (color != MARGIN_COLOR)
>            put_xsub_rle(&pb, MARGIN + odd, MARGIN_COLOR);

You could add buffer size checks to put_xsub_rle. That is not the
fastest, but unfortunately there is no padding for output buffers,
and it should not be that bad.
Alternatively, subtract some constant from the buffer size right at the
start so you only need one check in the while loop.

> +        // byte align row
> +        if (put_bits_count(&pb) & 4)
> +            put_bits(&pb, 4, 0);

align_put_bits

> +    snprintf(hdr, 28,
> +        "[%02d:%02d:%02d.%03d-%02d:%02d:%02d.%03d]",
> +        start_tc[3], start_tc[2], start_tc[1], start_tc[0],
> +        end_tc[3],   end_tc[3],   end_tc[1],   end_tc[0]);
> +    hdr += 27;

Of course you should first check that this even fits into the output
buffer...
You should probably check all constant-size fields at once and subtract
their size from the buffer size.

Greetings,
Reimar D?ffinger