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

stev391 at exemail.com.au stev391
Mon Aug 10 10:08:55 CEST 2009


On Mon, 2009-08-03 at 19:37 +1000, stev391 at exemail.com.au wrote:
> 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

(ping)

Slightly updated patch to apply cleanly against current Changelog and
also increment LIBAVCODEC_VERSION_MINOR. (Missed this previously).

Also attached is cosmetics patch for libavformat/mpegts.c after subtitle
patch has been applied.

Thanks,
Stephen.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: bluray_subtitles_ffmpeg_19613_v8.diff
Type: text/x-patch
Size: 15801 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090810/4efadc9f/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: bluray_subtitles_ffmpeg_19613_v8_cosmetics.diff
Type: text/x-patch
Size: 588 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090810/4efadc9f/attachment-0001.bin>



More information about the ffmpeg-devel mailing list