[MPlayer-dev-eng] [PATCH] EOSD/ASS code factorization

[Diego Biurrun]

On Sat, Aug 21, 2010 at 09:18:24PM +0200, Nicolas George wrote:
> <snip>
> 
> --- /dev/null
> +++ b/eosd.c
> @@ -0,0 +1,67 @@
> +/*
> + * This file is part of MPlayer.
> + */

no author?

> --- /dev/null
> +++ b/eosd.h
> @@ -0,0 +1,30 @@
> +/*
> + * This file is part of MPlayer.

ditto

> --- a/eosd.c
> +++ b/eosd.c
> @@ -23,45 +23,113 @@
> +    for(src = sources; src; src = src->priv_next) {
> +        if(src->priv_images_tail)
> +            *src->priv_images_tail = NULL;
> +        if(src->update)
> +            src->update(src, &settings, ts);
> +        if(src->images) {
> +            *tail = src->images;
> +            for(img = src->images; img; img = img->next)
> +                tail = &img->next;

K&R style for new files please, i.e. place a space after if/for/while.

more below..

> +static void eosd_ass_update(mp_eosd_source_t *src,
> +    const mp_eosd_settings_t *res, double ts)

Indentation is off.

> +static void eosd_ass_uninit(mp_eosd_source_t *src)
> +{
> +    (void)src;
> +    ass_renderer_done(ass_renderer);

Maybe I'm talking nonsense because I dunno the surrounding code,
but why the cast?  And src looks unused..

> +void eosd_ass_init(ASS_Library *ass_library)
> +{
> +    ass_renderer = ass_renderer_init(ass_library);
> +    if (!ass_renderer) return;

Break this line please.

> --- a/eosd.h
> +++ b/eosd.h
> @@ -19,9 +19,37 @@
> +
> +typedef struct {
> +    int w, h;           // screen dimensions, including black borders
> +    int srcw, srch;     // unscaled source dimensions
> +    int mt, mb, ml, mr; // borders (top, bottom, left, right)
> +    int unscaled;       // EOSD objects are rendered at native resolution
> +    int changed;        // settings have changed since last update
> +} mp_eosd_settings_t;

The _t namespace is reserved for POSIX, please don't pollute it further.

> +typedef struct mp_eosd_source mp_eosd_source_t;

Same, what's so bad about writing 'struct'?

> --- a/command.c
> +++ b/command.c
> @@ -2493,6 +2494,94 @@ static void remove_subtitle_range(MPContext *mpctx, int start, int count)
>  
> +static void overlay_update(mp_eosd_source_t *src, const mp_eosd_settings_t *res,
> +    double ts);

ugly forward declaration

> +static void overlay_update(mp_eosd_source_t *src, const mp_eosd_settings_t *res,
> +    double ts)

Indentation is off.

> +{
> +    ASS_Image *img, **prev;
> +    (void)src;
> +    (void)res;
> +    (void)ts;

Why the casts?

Diego