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

[stev391 at exemail.com.au]

On Sun, 2009-08-02 at 14:38 +0200, Reimar D?ffinger wrote:
> 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...
> 
This would then reduce readability and increase complexity using the mix
of commands, none changed.
> > +    av_freep(&ctx->picture->bitmap);
> > +
> > +    ctx->picture->bitmap = av_malloc(width * height * sizeof(uint8_t));
> 
> sizeof(uint8_t) is useless.
Agreed
> Also you should maybe use av_fast_malloc
Not sure if this is a good idea, as the subtitle size is constantly
fluctuating, what happens if I specify a smaller buffer then already
provided and then the next round increase it to larger then ever
provided? Will the the little bit that was not used be released properly
(i.e not leak) along with the specified buffer length (old width * old
height)?
> 
> > +    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
See above
> 
> > +            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.
Sorry I'm Australian, fixed.
> 
> flags = *buf;
> run = *buf++ & 0x3f;
> if (flags & 0x40)
>   run = run << 8 + *buf++;
> color = flags & 0x80 ? *buf++ : 0;
Hmm, very efficient, why didn't I think of that...
Only one minor detail is that the flags and the initial run value come
from the one byte.  Implemented using bytestream_get_byte to increase
readability.

Also fixed a check slightly further down in regards to ensuring the
buffer is not exceeded (previously I was not writing the last run of
colour, which just happened to be '0' colour anyway).
> 
> > +        /* Store colour in palette */
> > +        if (colour_id < 255)
> 
> Strange, 255 is not a valid palette index?
> IMO this warrants an extra comment
I was off by one.
> 
> > +    /* Skip 1 bytes of unknown, frame rate? */
> > +    buf += 1;
> 
> buf++;
Fixed
> 
> > +            x = 0; y =0;
> 
> Missing space after =, also could be
> x = y = 0;
Fixed
> 
> > +#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.
Removed (A similar function is in the DVD and DVB subtitles file, that
is why I left it there).
> 
> > +    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.
Fixed
> Well, except that pict.data[1] I think always needs to have the size for 256 palette entries,
> not just nb_colors...
nb_colors is 256 (set on previous lines directly above), in future I was
thinking of limiting the size, but as this will introduce slightly more
complicated checking in the palette parsing I thought I would commit the
working code first.  (I subscribe to the commit often ideology)
> 
> > +        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.
Fixed
> _______________________________________________

Michael, Sorry I had a brain fade, I thought it was '/' not '%' when I
replied to your comments.

Updated patch attached.


-------------- next part --------------
A non-text attachment was scrubbed...
Name: bluray_subtitles_ffmpeg_19571_v7.diff
Type: text/x-patch
Size: 15513 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090803/a47e6720/attachment.bin>