[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