[FFmpeg-devel] [PATCH] Add support for parsing the Display Definition Segment in dvbsubdec.c

[Michael Niedermayer]

On Mon, May 24, 2010 at 11:51:20PM +0200, Janne Grunau wrote:
> On Mon, May 24, 2010 at 10:44:01PM +0200, Michael Niedermayer wrote:
> > On Fri, May 21, 2010 at 01:50:15AM +0200, Janne Grunau wrote:
> > > 
> > > The only (minor) problem I see is that recalculating the subtitle rect's
> > > x/y position makes scaling them up to the full display size harder.
> > > As far as I understand the only purpose of this part of the DVB subtitle
> > > standard is to center SD subtitles reused for HD streams.
> > > So it's valid concern but probably not important enough to clutter the
> > > API.
> > > 
> > > Attached patch doesn't change API and changes only x/y values.
> > 
> > > @@ -334,6 +346,9 @@ static void delete_state(DVBSubContext *ctx)
> > >          av_free(clut);
> > >      }
> > >  
> > > +    if (ctx->display_definition)
> > > +        av_free(ctx->display_definition);
> > 
> > the if() is superflous and av_freep is maybe safer
> 
> changed to av_freep
> 
> > > +
> > >      /* Should already be null */
> > >      if (ctx->object_list)
> > >          av_log(0, AV_LOG_ERROR, "Memory deallocation error!\n");
> > 
> > > @@ -1254,10 +1269,49 @@ static void save_display_set(DVBSubContext *ctx)
> > >  }
> > >  #endif
> > >  
> > > +static void dvbsub_parse_display_definition_segment(AVCodecContext *avctx,
> > > +                                                    const uint8_t *buf,
> > > +                                                    int buf_size)
> > > +{
> > > +    DVBSubContext *ctx = (DVBSubContext*) avctx->priv_data;
> > 
> > unneeded cast
> 
> removed
> 
> > > +    DVBSubDisplayDefinition *display_def = ctx->display_definition;
> > > +    int dds_version, info_byte;
> > > +
> > > +    if (buf_size < 5)
> > > +        return;
> > > +
> > > +    info_byte   = bytestream_get_byte(&buf);
> > > +    dds_version = (info_byte >> 4) & 0xF;
> > 
> > have you ever seen a byte that had its upper 4 bits > 0xf ?
> > ;)
> 
> no, fixed :)
> 
> > > +    if (display_def && (display_def->version == dds_version))
> > > +        return; // already have this display definition version
> > > +
> > > +    if (!display_def) {
> > > +        ctx->display_definition = av_mallocz(sizeof(DVBSubDisplayDefinition));
> > > +        display_def = ctx->display_definition;
> > > +    }
> > > +
> > > +    display_def->version = dds_version;
> > > +    display_def->x       = 0;
> > > +    display_def->y       = 0;
> > > +    display_def->width   = bytestream_get_be16(&buf) + 1;
> > > +    display_def->height  = bytestream_get_be16(&buf) + 1;
> > > +
> > > +    if (buf_size < 13)
> > > +        return;
> > > +
> > > +    if ((info_byte >> 3) & 1) { // display_window_flag
> > 
> > i would write & (1<<3) but i guess gcc can optimize that itself
> 
> changed, I think it is easier to read.
> 
> also removed a couple of superfluous parentheses and checks av_mallocz return
> values.
> 
> Janne

>  dvbsubdec.c |   67 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 65 insertions(+), 2 deletions(-)
> 36866d448453076c6c9d93a0d216adb438b0a77b  dvbsub_display_def_seg2.diff
> commit cb6c7ba08b84a7f571f1db3c144be04aecadedde

should be ok if tested

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Awnsering whenever a program halts or runs forever is
On a turing machine, in general impossible (turings halting problem).
On any real computer, always possible as a real computer has a finite number
of states N, and will either halt in less than N cycles or never halt.
-------------- 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/20100525/6366f8a7/attachment.pgp>