[FFmpeg-devel] [PATCH] Bluray Subtitle Support, v5

[Reimar Döffinger]

On Sun, Aug 02, 2009 at 09:13:33PM +1000, stev391 at exemail.com.au wrote:
> +    /* Read the Sequence Description to determine if start of RLE data or Appended to previous RLE */
> +    sequence_desc = bytestream_get_byte(&buf);

I didn't even know this function existed, but since buf is
uint8_t * I think just using the usual "*buf++" would be preferable...

> +    av_freep(&ctx->picture->bitmap);
> +
> +    ctx->picture->bitmap = av_malloc(width * height * sizeof(uint8_t));

sizeof(uint8_t) is useless.
Also you should maybe use av_fast_malloc

> +    while (buf < rle_bitmap_end && line_count < height) {
> +        block = bytestream_get_byte(&buf);
> +
> +        if (block == 0x00) {
> +            block = bytestream_get_byte(&buf);
> +            flags = block & 0xC0;

I'd consider using *buf, *buf++ or similarly directly

> +            switch (flags) {
> +            case 0x00:
> +                run = block & 0x3F;
> +                if (run > 0)
> +                    colour = 0;
> +                break;
> +            case 0xC0:
> +                run = (block & 0x3F) << 8 | bytestream_get_byte(&buf);
> +                colour = bytestream_get_byte(&buf);
> +                break;
> +            case 0x80:
> +                run = block & 0x3F;
> +                colour = bytestream_get_byte(&buf);
> +                break;
> +            case 0x40:
> +                run = (block & 0x3F) << 8 | bytestream_get_byte(&buf);
> +                colour = 0;
> +                break;
> +            }

Note: we use US English, so it should be color for consistency with other code.

flags = *buf;
run = *buf++ & 0x3f;
if (flags & 0x40)
  run = run << 8 + *buf++;
color = flags & 0x80 ? *buf++ : 0;

> +        /* Store colour in palette */
> +        if (colour_id < 255)

Strange, 255 is not a valid palette index?
IMO this warrants an extra comment

> +    /* Skip 1 bytes of unknown, frame rate? */
> +    buf += 1;

buf++;

> +            x = 0; y =0;

Missing space after =, also could be
x = y = 0;

> +#ifdef DEBUG_SAVE_IMAGES
> +#undef fprintf
> +#undef perror
> +#undef exit
> +static void ppm_save(const char *filename, uint8_t *bitmap, int w, int h,
> +                     uint32_t *rgba_palette)
> +{
> +    int x, y, v;
> +    FILE *f;
> +
> +    f = fopen(filename, "w");
> +    if (!f) {
> +        perror(filename);
> +        exit(1);
> +    }
> +    fprintf(f, "P6\n"
> +            "%d %d\n"
> +            "%d\n",
> +            w, h, 255);
> +    for (y = 0; y < h; y++) {
> +        for (x = 0; x < w; x++) {
> +            v = rgba_palette[bitmap[y * w + x]];
> +            putc((v >> 16) & 0xff, f);
> +            putc((v >> 8) & 0xff, f);
> +            putc((v >> 0) & 0xff, f);
> +        }
> +    }
> +    fclose(f);
> +}
> +#endif

That obviouslyy should go for the final version, if it is not already possible ffmpeg
might be extended to allow for this.

> +    sub->rects[0]->pict.data[1] = av_mallocz(sub->rects[0]->nb_colors * sizeof(uint32_t));
> +
> +    memcpy(sub->rects[0]->pict.data[1], ctx->clut, sub->rects[0]->nb_colors * sizeof(uint32_t));

Initializing to 0 and then fully memcpying it over seems like overkill, av_malloc instead
of av_mallocz should do it I think.
Well, except that pict.data[1] I think always needs to have the size for 256 palette entries,
not just nb_colors...

> +        if (!(segment_type == DISPLAY_SEGMENT) && (buf + segment_length > buf_end))

Uh, !(a == b) should be a != b... also some useless ().
Also the check is wrong,
buf + segment_length can overflow,
segment_length > buf_end - buf
is the correct way that avoids an overflow.