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

Stephen Backway stev391
Mon Aug 24 12:08:18 CEST 2009


On Sun, 2009-08-23 at 22:56 +0200, Michael Niedermayer wrote:
> 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:
[...]
> > +#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)

This is a case of monkey see, monkey do.

This is out of my level of experience and is how it is in dvbsubdec.c

Would you like me to change it to:
uint8_t *cm = ff_cropTbl + MAX_NEG_CROP;
like in vp3dsp.c, svq3.c, rv40dsp.c, rv40.c etc

Or

const uint8_t *cm = ff_cropTbl + MAX_NEG_CROP;
like in vc1dsp.c, simple_idct.c etc


> 
> [...]
> > +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
Removed
> 
> 
> > +
> > +    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
Added
> 
> 
> > +
> > +    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
Fixed, however there is no opportunity to check the return value, as
when processing this packet we never return from decode() stating a
subtitle is ready.
> 
> 
> [...]
> > +/**
> > + * 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 ?
As the packet containing the RLE image data and the packet that issues
the display command are in seperate instances I need at least one memcpy
to store either the RLE data or the decoded bitmap in the context.

After review of the changes required to support RLE over multiple
packets (still not supported, but getting closer) I have decided to
store the RLE data (as it is smaller) and then decode the RLE on the
display command directly into the sub->rects[0]->pict.data[0].

To achieve this a new function was created to process the data (this
keeps the code neater then inserting it into the display_end_segment
function). I hope this is acceptable.
> 
> after fixing above iam also fine with the patch
> 
> [...]

Also attached is the post-commit cosmetics patch, so it is not
forgotten.
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at mplayerhq.hu
> https://lists.mplayerhq.hu/mailman/listinfo/ffmpeg-devel
-------------- next part --------------
A non-text attachment was scrubbed...
Name: bluray_subtitles_ffmpeg_19689_v11.diff
Type: text/x-patch
Size: 17663 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090824/072ca07a/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: bluray_subtitles_ffmpeg_19613_v8_cosmetics.diff
Type: text/x-patch
Size: 588 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090824/072ca07a/attachment-0001.bin>



More information about the ffmpeg-devel mailing list