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

[Cyril MATHE]

Le jeudi 25 juin 2009 à 00:08 -0700, Pierre d'Herbemont a écrit :
> 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.
You're welcome, thank you for the time to take care about my problem.
> 
> Else, on the implementation, I think jpd's comment is relevant, if you  
> want to factorize more. That's up to you.
I am not really sure to understand, do I have to make setter for each
marq option ?
ie:
void libvlc_video_set_marq_size(int n)
{
	libvlc_video_set_marq_option_as_int(libvlc_marq_Size, n);
}
with libvlc_video_set_marq_option_as_int static and furthemore being not
in the libvlc API, but libvlc_video_set_marq_size being in it ?

> 
> 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.
That's a good idea, I don't know how I couldn't think about making a
table before.
> 
> 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.
It has been implemented in case of we need it, but if you think that
really doesn't make any sense I remove it.
> 
> 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.
> 
It's a mistake of mine.

> > +    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.
Actually it's not possible to enable and set an option at the same time
(I think fenrir is fixing it, because it's due to vout-filter), so I
think that it's better to return an error to the user instead of
enabling marq video filter for him. In that case the user is aware of
his mistake and can enable it afterwards.

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

that will be changed.
> 
> > +    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?
return an error in case of NULL value.

Cyril