[FFmpeg-devel] [PATCH] Bluray Subtitle Support, v7 (Updated patch to v9)

[Reimar Döffinger]

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.
> +    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?

> +    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.
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).

> +    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...

> +    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 /**

> +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.

> +    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...

>  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.