[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