[FFmpeg-devel] [PATCH] Bluray Subtitle Support, v7 (Updated patch to v10)
Michael Niedermayer
michaelni
Sun Aug 23 22:56:17 CEST 2009
On Tue, Aug 11, 2009 at 08:43:39PM +1000, stev391 at exemail.com.au wrote:
> On Mon, 2009-08-10 at 14:08 +0200, Reimar D?ffinger wrote:
> > On Mon, Aug 10, 2009 at 09:32:40PM +1000, stev391 at exemail.com.au wrote:
> > > However the answer seems to be:
> > > Add a variable to the context to keep track of the size of buffer and
> > > initialise to 0:
> > >
> > > unsigned int bitmap_size;
> > >
> > > bitmap_size = 0;
> >
> > Yes, except that e.g. as part of the context or alloced via av_mallocz
> > it should already be initialized to 0, looking at the patch you already
> > noticed that yourself though.
> >
> > > Then when allocating just call:
> > > av_fast_malloc(&ctx->picture->bitmap, &ctx->picture->bitmap_size, width
> > > * height);
> > >
> > > and it will update the bitmap_size variable with the actual size of the
> > > buffer which is always equal or exceeding width * height, unless it
> > > fails:
> > >
> > > if (!ctx->picture->bitmap)
> > > return AVERROR(ENOMEM);
> > >
> > >
> > > Is this summary correct?
> >
> > Yes, seems all be correct, except that the size variable will be
> > correctly updated (to 0) if malloc fails, too.
> > While I see the *buf++ vs. bytestream_get_byte slightly different (IMHO
> > bytestream_get_byte is too "obfuscated" for what it does) I don't
> > object if you prefer it like this.
> I would like to keep it with bytestream_get_byte
> > > + PGSSubContext *ctx = avctx->priv_data;
> > > +
> > > + ctx->picture = av_mallocz(sizeof(PGSSubPicture));
> >
> > Why don't you just put the PGSSubPicture directly into the
> > PGSSubContext?
> > Doesn't really seem to have any advantage to malloc it separately?
> No reason anymore, initially I had a different idea on how to implement
> the code.
> Fixed.
> >
> > > + while (buf < rle_bitmap_end && line_count < height) {
> > > + block = bytestream_get_byte(&buf);
> > > + if (block == 0x00) {
> > > + flags = bytestream_get_byte(&buf);
> > > + run = flags & 0x3f;
> > > + if (flags & 0x40)
> > > + run = (run << 8) + bytestream_get_byte(&buf);
> > > + color = flags & 0x80 ? bytestream_get_byte(&buf) : 0;
> > > + } else {
> > > + run = 1;
> > > + color = block;
> > > + }
> >
> > Hm, run, color, block are not used outside this block, so I'd declare
> > them inside.
> Fixed. (Including flags)
> > Also I'll leave the "else" part to the compiler, i.e. initialize run to 1
> > and color to block instead of the else part (but that sure is a matter
> > of opinion and/or benchmarks).
> Agreed, also able to remove variable 'block' as it is initially the same
> as 'color'.
> >
> > > + while (buf < buf_end) {
> > > + color_id = bytestream_get_byte(&buf);
> > > + y = bytestream_get_byte(&buf);
> > > + cb = bytestream_get_byte(&buf);
> > > + cr = bytestream_get_byte(&buf);
> > > + alpha = bytestream_get_byte(&buf);
> > > +
> > > + YUV_TO_RGB1(cb, cr);
> > > + YUV_TO_RGB2(r, g, b, y);
> > > +
> > > + dprintf(avctx, "Color %d := (%d,%d,%d,%d)\n", color_id, r, g, b, alpha);
> > > +
> > > + /* Store color in palette */
> > > + if (color_id < 256)
> > > + ctx->clut[color_id] = RGBA(r,g,b,alpha);
> > > + else
> > > + av_log(avctx, AV_LOG_ERROR, "Invalid color defined with index %d\n", color_id);
> >
> > I can't see how color_id could ever be >= 256...
> Hmm, that was me being very silly. I'm just used to working with more
> then 8 bits...
> Fixed.
> >
> > > + block = bytestream_get_byte(&buf);;
> > > + if (block == 0x80) {
> > > + /**
> > > + * Skip 7 bytes of unknown:
> > > + * palette_update_flag (0x80),
> > > + * palette_id_to_use,
> > > + * Object Number (if > 0 determines if more data to process),
> > > + * object_id_ref (2 bytes),
> > > + * window_id_ref,
> > > + * composition_flag (0x80 - object cropped, 0x40 - object forced)
> > > + */
> >
> > doxy comments within functions tends to confuse doxygen I think, better
> > use a normal /* comment instead of /**
> Fixed
> >
> > > +static int display_end_segment(AVCodecContext *avctx, void *data,
> > > + const uint8_t *buf, int buf_size)
> > > +{
> > > + AVSubtitle *sub = data;
> > > + PGSSubContext *ctx = avctx->priv_data;
> > > +
> > > + /**
> > > + * TODO Fix start and end time.
> > > + * Currently the subtitle is displayed too late.
> > > + * The end display time is a timeout value and is only reached
> > > + * if the next subtitle is later then timeout or subtitle has
> > > + * not been cleared by a subsequent empty display command.
> > > + * Empty subtitle command is currently ignored as it clears
> > > + * the subtitle too early.
> > > + */
> >
> > Put that in the (anyway missing) doxy for the function.
> > All functions that are not like decode part of the standard API should
> > have a doxy comment.
> Created doxy comments, are these what you were expecting, or do I need
> more/less detail.
> > _______________________________________________
> > > + for (i = 0; i < buf_size; i++) {
> > > + av_log(avctx, AV_LOG_INFO, "%02x ", buf[i]);
> > > + if (i % 16 == 15)
> > > + av_log(avctx, AV_LOG_INFO, "\n");
> > > + }
> > > +
> > > + if (i % 16)
> > > + av_log(avctx, AV_LOG_INFO, "\n");
> >
> > I know it doesn't matter but I generally prefer to use & 15 instead of %
> > 16 always and in general (where possible, i.e. no negative values) to
> > avoid giving people bad ideas...
> Fixed.
> >
> > > static const StreamType HDMV_types[] = {
> > > - { 0x81, CODEC_TYPE_AUDIO, CODEC_ID_AC3 },
> > > - { 0x82, CODEC_TYPE_AUDIO, CODEC_ID_DTS },
> > > + { 0x81, CODEC_TYPE_AUDIO, CODEC_ID_AC3 },
> > > + { 0x82, CODEC_TYPE_AUDIO, CODEC_ID_DTS },
> > > { 0x90, CODEC_TYPE_SUBTITLE, CODEC_ID_HDMV_PGS_SUBTITLE },
> >
> > You should always align so that identical parts (here CODEC_ID_) are in
> > the same columns if possible I think.
> That is how I would do it, however to match the formatting of
> neighbouring sections in mpegts.c I did it this way. (The formatting
> and other code was only recently applied, IIRC sometime in the last few
> months.)
> Should I change it? And the rest of the occurrences in the file to how
> you suggest?
[...]
> +#define cm (ff_cropTbl + MAX_NEG_CROP)
is gcc able to simplify that? or is there a silly const + const addition
in the inner loops? (i know ive seen such things that gcc couldnt optimize
away long ago though dont remember if it was such a construct)
[...]
> +static av_cold int init_decoder(AVCodecContext *avctx)
> +{
> + avctx->pix_fmt = PIX_FMT_RGB32;
> + PGSSubContext *ctx = avctx->priv_data;
> +
> + ctx->picture.bitmap_size = 0;
> +
> + ctx->presentation.x = 0;
> + ctx->presentation.y = 0;
> + ctx->presentation.video_w = 0;
> + ctx->presentation.video_h = 0;
redundant unless i miss something they should be 0 already
> +
> + return 0;
> +}
> +
> +static av_cold int close_decoder(AVCodecContext *avctx)
> +{
> + PGSSubContext *ctx = avctx->priv_data;
> +
> + av_freep(&ctx->picture.bitmap);
id add a bitmap_size=0 for paranoia but i dont think its needed
> +
> + return 0;
> +}
> +
> +/**
> + * Parses the picture segment packet.
> + *
> + * The picture segment contains details on the sequence id,
> + * width, height and Run Length Encoded (RLE) bitmap data.
> + *
> + * @param avctx contains the current codec context
> + * @param buf pointer to the packet to process
> + * @param buf_size size of packet to process
> + * @todo TODO: Enable support for RLE data over multiple packets
> + */
> +static void parse_picture_segment(AVCodecContext *avctx,
> + const uint8_t *buf, int buf_size)
> +{
> + PGSSubContext *ctx = avctx->priv_data;
> +
> + uint8_t *rle_bitmap_end, sequence_desc;
> + int rle_bitmap_len, pixel_count, line_count, 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;
i think that should return something like -1
[...]
> +/**
> + * Parses the display segment packet.
> + *
> + * The display segment controls the updating of the display.
> + *
> + * @param avctx contains the current codec context
> + * @param data pointer to the data pertaining the subtitle to display
> + * @param buf pointer to the packet to process
> + * @param buf_size size of packet to process
> + * @todo TODO: Fix start time, relies on correct PTS, currently too late
> + *
> + * @todo TODO: Fix end time, normally cleared by a second display
> + * @todo segment, which is currently ignored as it clears
> + * @todo the subtitle too early.
> + */
> +static int display_end_segment(AVCodecContext *avctx, void *data,
> + const uint8_t *buf, int buf_size)
> +{
> + AVSubtitle *sub = data;
> + PGSSubContext *ctx = avctx->priv_data;
> +
> + /*
> + * The end display time is a timeout value and is only reached
> + * if the next subtitle is later then timeout or subtitle has
> + * not been cleared by a subsequent empty display command.
> + */
> +
> + sub->start_display_time = 0;
> + sub->end_display_time = 20000;
> + sub->format = 0;
> +
> + if (!sub->rects) {
> + sub->rects = av_mallocz(sizeof(*sub->rects));
> + sub->rects[0] = av_mallocz(sizeof(*sub->rects[0]));
> + sub->num_rects = 1;
> + }
> +
> + sub->rects[0]->x = ctx->presentation.x;
> + sub->rects[0]->y = ctx->presentation.y;
> + sub->rects[0]->w = ctx->picture.w;
> + sub->rects[0]->h = ctx->picture.h;
> + sub->rects[0]->type = SUBTITLE_BITMAP;
> +
> + /* Allocate memory for bitmap */
> + sub->rects[0]->pict.data[0] = av_malloc(ctx->picture.w * ctx->picture.h);
> + sub->rects[0]->pict.linesize[0] = ctx->picture.w;
> +
> + if (ctx->picture.bitmap)
> + memcpy(sub->rects[0]->pict.data[0], ctx->picture.bitmap, ctx->picture.w * ctx->picture.h);
cant the image be drawn into sub->rects[0]->pict.data[0] instad of bitmap to
avoid that memcpy ?
after fixing above iam also fine with the patch
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
No human being will ever know the Truth, for even if they happen to say it
by chance, they would not even known they had done so. -- Xenophanes
-------------- 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/20090823/208d818b/attachment.pgp>
More information about the ffmpeg-devel
mailing list