[vlc-devel] [PATCH] lib: fix position enumeration values

[Mark Lee]

Sorry for top-posting, I'm not specifically replying to one person or
another here.

To summarise so far: the original patch that I submitted had the purpose of
making the libvlc_media_player_set_video_title_display() API method work
according to the API specification. As of right now, it would work
correctly for some values of libvlc_position_t, but not for others - this
would result in the video title sometimes appearing at the wrong position.

I suppose the original intent of libvlc_position_t was to be a generic
enumeration of "positions" that could be used throughout LibVLC whenever a
position value was needed.

So the follow-up issue arising from this thread I think distils down to
what do you want the public API to be? I see these options:

1. use the public libvlc_position_t enumeration as-is and change the
implementation of the API method;
2. change the public libvlc_position_t enumeration to match the constants
in include/vlc_subpicture.h;
3. change the public libvlc_position_t enumeration to use the constants in
include/vlc_subpicture.h;
4. something else.

For #1, I would envisage that the implementation of the
libvlc_media_player_set_video_title_display() method would change to set
the value of the "video-title-position" variable by translating from the
position enumeration to the corresponding constants defined in
"include/vlc_subpicture.h" e.g. in a switch statement. At least with this
approach the public LibVLC API values are decoupled from the private
subpicture implementation values.

For #2, with my initial patch at least the API method will work according
to its specification. The patch right now defines those constants directly
as integer values that happen to equate to the values of constants defined
in "include/vlc_subpicture.h". Whether these are defined directly as
integers, or bitmasks or whatever (#3) is a lesser issue since it would be
easy for me to change that to however you want.

Instinctively it seems to me like the public position enumeration values
should *not* be defined in terms of the private subpicture implementation,
since conceivably the position values could be used for something other
than subpictures - even though that is not the case right now. So I would
prefer #1 I suppose, which if agreed would make the discussion about how to
specify those constant values moot.

I am happy to prepare another patch if I could please get a bit more
guidance.

Regards,

-M.

On 20 May 2014 14:26, Rémi Denis-Courmont <remi at remlab.net> wrote:

> Le 2014-05-20 20:44, Francois Cartegnie a écrit :
>
>  Le 20/05/2014 14:27, Mark Lee a écrit :
>>
>>  /* Subpicture region position flags */
>>> #define SUBPICTURE_ALIGN_LEFT 0x1
>>> #define SUBPICTURE_ALIGN_RIGHT 0x2
>>> #define SUBPICTURE_ALIGN_TOP 0x4
>>> #define SUBPICTURE_ALIGN_BOTTOM 0x8
>>>
>>
>> Having defines & enums is bad for consistency.
>>
>
> How so? libc even mixes both, e.g. so that #ifdef works.
>
>
>  Maybe a best way is to split and compose enums:
>>
>> typedef enum libvlc_align_t {
>>     libvlc_align_left = 1 << 0,
>>     libvlc_align_right= 1 << 1,
>>     libvlc_align_bottom= 1 << 2,
>>     libvlc_align_top= 1 << 3,
>> }
>>
>> and then compose our second enum based on those values
>>
>>
>> typedef enum libvlc_position_t ...
>> libvlc_position_left         =  libvlc_align_left,
>> libvlc_position_top_left     = ( libvlc_align_left|libvlc_align_top ),
>>
>
> That's even more confusing than either a single enum or a set of #define's
> IMHO.
>
> --
> Rémi Denis-Courmont
>
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel
>



-- 
Mark Lee
mark.lee 'at' c <http://mark-lee.com>apricasoftware.co.uk
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20140520/f76bdc41/attachment.html>