[vlc-devel] [PATCH] Add marq filter to libvlc

[Pierre d'Herbemont]

Cyril,

Thanks for all the update, it's pretty much over on the API side (only  
1- and 2-)

Thanks for your effort, API needs to be discussed.

Else, on the implementation, I think jpd's comment is relevant, if you  
want to factorize more. That's up to you.

If you want to factorize even more, I would suggest to use a table to  
convert between enum and the char * name provides that you do proper  
value checking. That could be shared between setters and getters.

More comment below.

On Jun 24, 2009, at 2:17 AM, Cyril MATHE wrote:

> Hello,

> +typedef enum libvlc_video_marq_option_t {
> +    libvlc_marq_Error       = -1,

1- Why do we have a Error option? I don't think it makes sens.

2- Either we have two differents, option_t enum for option that takes  
strings and other that takes int, or we add at least a comment about  
what option takes what in the header.

> +    if( !b_enable )
> +    {
> +        /* if marq is disabled don't get options values */
> +        libvlc_exception_raise( p_e, "Marq video filter is  
> currently disabled" );
> +        return VLC_EGENERIC;
> +    }
>

3- (optional but would be great in a second patch): This would be  
great if those option were working even when the marq filter is not  
enabled.

> +        case libvlc_marq_Error:
> +        {
> +            /* Error */
> +            libvlc_exception_raise( p_e, "Unable to get a marq  
> option" );
> +            i_return = VLC_EGENERIC;
> +            break;
> +        }
> +        default: break;

I think you meant "default" instead of "case libvlc_marq_Error".

> +    if ( option != libvlc_marq_Enabled )
> +        vlc_object_release( p_marq_object );

Is it a leak if option == libvlc_marq_Enabled?

> +            if ( psz_text != NULL )
> +                var_SetString(p_marq_object, "marq-marquee",  
> psz_text);

Do we really want to do that here? What about returning an error on  
NULL value, or simply pass it along to the core if it is supported?

Pierre.