[FFmpeg-devel] [PATCH] Bluray Subtitle Support, v4
Michael Niedermayer
michaelni
Tue Jul 28 22:28:18 CEST 2009
On Tue, Jul 28, 2009 at 07:29:50PM +1000, stev391 at exemail.com.au wrote:
> On Sat, 2009-07-25 at 10:13 +1000, stev391 at exemail.com.au wrote:
> > On Fri, 2009-07-24 at 15:23 +0200, Diego Biurrun wrote:
> > > On Fri, Jul 24, 2009 at 05:57:24PM +1000, stev391 at exemail.com.au wrote:
> > > >
> > > > Updated patch including Diego's, Aurelien's & Baptiste's comments.
> > > > Thanks for the excellent and quick feedback.
> > >
> > Thanks again for the comments, I'm glad somebody is interested in my
> > patch.
> >
> > Updated patch (from v2) to include Diego's & Aurelien's comments.
>
> Updated patch to version 4.
>
> The previous patch kept allocating memory each time a subtitle was
> decoded and never freeing the bitmap memory. This originally was
> completed after the subtitle was displayed, however as the start time is
> so delayed it meant the subtitle were cleared to early. Now I free the
> memory if it is allocated before allocating the new bitmap size.
>
> Who needs to give the ok for this to be included into FFmpeg?
>
> And once it is Ok'ed can someone commit the patch as I do not have
> rights and do not want rights to do this. I would like to get this
> committed so I can then progress with the rare cases that this does not
> work on (currently it ignores the subtitle, no crashes/hangs etc after
> watching several movies with subtitles on) and try and work out why they
> are delayed.
>
> Thanks.
[...]
> +#define PGSSUB_PALETTE_SEGMENT 0x14
> +#define PGSSUB_PICTURE_SEGMENT 0x15
> +#define PGSSUB_PRESENTATION_SEGMENT 0x16
> +#define PGSSUB_WINDOW_SEGMENT 0x17
> +#define PGSSUB_DISPLAY_SEGMENT 0x80
id make these an enum
> +
> +#define cm (ff_cropTbl + MAX_NEG_CROP)
> +#define RGBA(r,g,b,a) (((a) << 24) | ((r) << 16) | ((g) << 8) | (b))
> +
> +typedef struct PGSSubPresentation {
> + int x;
> + int y;
> + int video_w;
> + int video_h;
> + int id_number;
> +} PGSSubPresentation;
> +
> +typedef struct PGSSubPicture {
> + int w;
> + int h;
> + uint8_t *bitmap;
> +} PGSSubPicture;
> +
> +typedef struct PGSSubContext {
> + PGSSubPresentation *presentation;
PGSSubPresentation presentation;
1 malloc & free less (and no chance for leaks and fewer ptr derefs)
[...]
> +static void pgssub_parse_picture_segment(AVCodecContext *avctx,
> + const uint8_t *buf, int buf_size)
> +{
> + PGSSubContext *ctx = (PGSSubContext*) avctx->priv_data;
useles cast?
> +
> + uint8_t block, flags, colour, *rle_bitmap_end, sequence_desc;
> + int rle_bitmap_len, pixel_count, line_count, run, width, height;
> +
> + /* skip 3 unknown bytes: Object ID (2 bytes), Version Number */
> + buf += 3;
> +
> + /* Read the Sequence Description to determine if start of RLE data or Appended to previous RLE */
> + sequence_desc = bytestream_get_byte(&buf);
> +
> + if (!(sequence_desc & 0x80)) {
> + av_log(avctx, AV_LOG_ERROR, "Decoder does not support object data over multiple packets.\n");
> + return;
> + }
> +
> + /* Decode rle bitmap length */
> + rle_bitmap_len = bytestream_get_be24(&buf);
> +
> + /* Check to ensure we have enough data for rle_bitmap_length if just a single packet*/
> + if (rle_bitmap_len > (buf_size - 7)) {
superflous ())
> + av_log(avctx, AV_LOG_ERROR, "Not enough RLE data for specified length of %d.\n", rle_bitmap_len);
> + return;
> + }
> +
> + rle_bitmap_end = buf + rle_bitmap_len;
> +
> + /* Get bitmap dimensions from data */
> + width = bytestream_get_be16(&buf);
> + height = bytestream_get_be16(&buf);
> +
> + /* Make sure the bitmap is not too large. */
> + if (ctx->presentation->video_w < width || ctx->presentation->video_h < height) {
> + av_log(avctx, AV_LOG_ERROR, "Bitmap dimensions larger then video.\n");
> + return;
> + }
> +
> + ctx->picture->w = width;
> + ctx->picture->h = height;
> +
> + if (ctx->picture->bitmap)
> + av_freep(&ctx->picture->bitmap);
the if() is unneeded
> +
> + ctx->picture->bitmap = av_malloc(width * height * sizeof(uint8_t));
> +
> + pixel_count = 0;
> + line_count = 0;
> +
> + while (buf < rle_bitmap_end && line_count < height) {
> + block = bytestream_get_byte(&buf);
> +
> + if (block == 0x00) {
> + block = bytestream_get_byte(&buf);
> + flags = block & 0xC0;
> +
> + 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;
> + default:
> + av_log(avctx, AV_LOG_ERROR, "Unknown RLE code.\n");
> + run = -1;
> + break;
unreachable
> + }
> + } else {
> + run = 1;
> + colour = block;
> + }
> +
> + if (run > 0) {
> + memset(ctx->picture->bitmap + pixel_count, colour, run);
is that able to overwrite the end of the buffer?
[...]
> +static void pgssub_parse_palette_segment(AVCodecContext *avctx,
> + const uint8_t *buf, int buf_size)
> +{
> + PGSSubContext *ctx = (PGSSubContext*) avctx->priv_data;
> +
> + const uint8_t *buf_end = buf + buf_size;
> + int colour_id, max_colour_id;
> + int y, cb, cr, alpha;
> + int r, g, b, r_add, g_add, b_add;
> +
> + max_colour_id = (buf_size - 2) % 5;
> +
> + /* Check to ensure that the buffer provided is not larger then the maximum palette */
> + if (max_colour_id > 255) {
> + av_log(avctx, AV_LOG_INFO, "Palette Packet too large, pontentially %d colours defined.\n",
> + max_colour_id);
> + return;
unreachable
[...]
> +static int pgssub_display_end_segment(AVCodecContext *avctx,
> + void *data,
> + const uint8_t *buf, int buf_size)
> +{
> + AVSubtitle *sub = data;
> + PGSSubContext *ctx = (PGSSubContext*) avctx->priv_data;
> +
> + sub->start_display_time = 0;
> + sub->end_display_time = 20000;
that really does not look correct
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
Many things microsoft did are stupid, but not doing something just because
microsoft did it is even more stupid. If everything ms did were stupid they
would be bankrupt already.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090728/ad538099/attachment.pgp>
More information about the ffmpeg-devel
mailing list