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

stev391 at exemail.com.au stev391
Tue Aug 11 12:43:39 CEST 2009


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?

Regards,
Stephen.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: bluray_subtitles_ffmpeg_19625_v10.diff
Type: text/x-patch
Size: 17127 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090811/0420d2e9/attachment.bin>



More information about the ffmpeg-devel mailing list