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

[Pierre d'Herbemont]

On Mon, Jun 22, 2009 at 7:48 AM, Cyril
MATHE<cmathe at actech-innovation.com> wrote:
>> This structure name does not follow the usual libvlc naming scheme.
>>
> maybe something like libvlc_video_marq_t ?
>
>> Plus, exposing a direct structure is a bit suspicious, especially in
>> this case where an option is being used to. And it is highly likely
>> that user will forget to free() the psz_text member of this structure.
>>
> Libvlc user have to be aware of that, doesn't he ?

Yes, that's a point against using that structure.

>> What about something like:
>>
>> libvlc_video_set_marq_text(const char * text);
>> char * libvlc_video_get_marq_text(const text); // Caller has to free()
>>
>> libvlc_video_set_marq_option(libvlc_video_marq_option_t option,
>> libvlc_video_marq_value_t value);
>> libvlc_video_get_marq_option(libvlc_video_marq_option_t option,
>> libvlc_video_marq_value_t value);
>>
>> libvlc_video_marq_value_t being a int.
>>
>> Pierre.
>
> I had done something like this before but fenrir and j-b prefered a
> solution with a unique structure and two functions. It was :
> libvlc_video_get_marq() //get marq options
> libvlc_video_set_marq(b_enable) //enable or disable marq
> libvlc_video_set_marq_option (libvlc_video_marq_option_t option, int
> value, char * psz_text) //set a marq option

I don't agree with J-B and Laurent, a structure seems not appropriate.
Especially because extension will be harder. But I do agree that first
prototype was not clear enough. Especially
libvlc_video_set_marq_option prototype.

> With your solution there is one function for int options and one for
> char * options, but in both we have to deal with marq enabling. I don't
> know if it's really a problem but I noticed it.


marq enabling would be an option:

libvlc_video_set_marq_option(libvlc_marq_Enabled, true);

You could add a new function for it though, if it seems clearer for you.

After more thinking we could even go further and have:

libvlc_video_set_marg_option_as_int(libvlc_marq_Blah, num);
libvlc_video_set_marg_option_as_string(libvlc_marq_Text, "string");
(And associated getters)

And to be even more explicit we would have libvlc_marq_string_option_t
and libvlc_marq_int_option_t.

It's up to you now.

Pierre.