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

[Michael Niedermayer]

On Fri, May 21, 2010 at 01:50:15AM +0200, Janne Grunau wrote:
> On Thu, May 20, 2010 at 10:20:22PM +0200, Michael Niedermayer wrote:
> > 
> > i see no case in which a hierachical system with an additional rectangle
> > that contanins the other rectangles would make sense.
> > thats besides that whatever is done the API must be unambigously and clearly
> > be described. that is the interaction/relation of all the values and their
> > scale must be documented.
> > anyway, please elaborate on what problem you see
> 
> 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.
> 
> Janne

[...]

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


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


> +    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 ?
;)


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


[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Those who are too smart to engage in politics are punished by being
governed by those who are dumber. -- Plato 
-------------- 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/20100524/acec49d8/attachment.pgp>