[FFmpeg-devel] [PATCH] Bluray Subtitle Support, v5
stev391 at exemail.com.au
stev391
Sun Aug 2 04:15:35 CEST 2009
On Tue, 2009-07-28 at 22:28 +0200, Michael Niedermayer wrote:
[...]
> > Updated patch to version 4.
> >
[...]
>
> [...]
> > +#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
>
Fixed
>
> > +
> > +#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)
Fixed
>
> [...]
> > +static void pgssub_parse_picture_segment(AVCodecContext *avctx,
> > + const uint8_t *buf, int buf_size)
> > +{
> > + PGSSubContext *ctx = (PGSSubContext*) avctx->priv_data;
>
> useles cast?
>
Fixed (all other occurrences as well)
>
> > +
> > + 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 ())
>
Fixed, this was me being extra careful...
>
> > + 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
>
Fixed, thanks I was unaware of this functionality.
>
> > +
> > + 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
>
Fixed
>
> > + }
> > + } 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?
>
Fixed, Added additional check to ensure it will not exceed 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
This is reachable if it is a mangled packet, but as I check the colour
index further down, this is redundant. Removed and replaced with error
message if colour index is too large.
>
>
> [...]
> > +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
This is true.
I have now added a comment into the code noting this and why...
Bluray subtitles rely on the PTS to determine when to display them,
there is a display sequence and a clear sequence.
Currently the subtitles are being displayed too late (not too sure why
this is), as start_display_time is unsigned, '0' is the smallest value I
can assign and end_display_time is set to 20seconds as a timeout if the
clear sequence is not run. At the moment my code ignores the clear
sequence as it happens at approximately the right frame, which means
that the subtitle flashes up (due to delayed start) and disappears (as
correct end).
This is something I intend on investigating further once this patch is
applied. (As I suspect the possible solutions could effect other code).
>
> [...]
Also addressed Diego's comments and ran valgrind to check memory
allocations, (No errors from patch found).
Regards,
Stephen
-------------- next part --------------
A non-text attachment was scrubbed...
Name: bluray_subtitles_ffmpeg_19557_v5.diff
Type: text/x-patch
Size: 17093 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090802/06ae36f7/attachment.bin>
More information about the ffmpeg-devel
mailing list