[vlc-devel] [PATCH] lib: map position enum to the correct subpicture bitwise mask values

[Mark Lee]

Hello,

On 23 May 2014 03:02, Rémi Denis-Courmont <remi at remlab.net> wrote:

> Le 2014-05-23 01:23, Mark Lee a écrit :
>
>  libvlc_media_player_set_video_title_display() was wrongly using the
>> enum value directly, leading to the video title appearing in the wrong
>> position for some values
>> ---
>>  lib/media_player.c | 36 +++++++++++++++++++++++++++++++++++-
>>  1 file changed, 35 insertions(+), 1 deletion(-)
>>
>> diff --git a/lib/media_player.c b/lib/media_player.c
>> index 67bbde7..fb7d758 100644
>> --- a/lib/media_player.c
>> +++ b/lib/media_player.c
>> @@ -1449,8 +1449,42 @@ void
>> libvlc_media_player_set_video_title_display( libvlc_media_player_t
>> *p_mi, l
>>  {
>>      if ( position != libvlc_position_disable )
>>      {
>> +        int64_t pos;
>> +        switch (position) {
>> +            case libvlc_position_center:
>> +                pos = 0;
>> +                break;
>> +            case libvlc_position_left:
>> +                pos = SUBPICTURE_ALIGN_LEFT;
>> +                break;
>> +            case libvlc_position_right:
>> +                pos = SUBPICTURE_ALIGN_RIGHT;
>> +                break;
>> +            case libvlc_position_top:
>> +                pos = SUBPICTURE_ALIGN_TOP;
>> +                break;
>> +            case libvlc_position_top_left:
>> +                pos = SUBPICTURE_ALIGN_TOP | SUBPICTURE_ALIGN_LEFT;
>> +                break;
>> +            case libvlc_position_top_right:
>> +                pos = SUBPICTURE_ALIGN_TOP | SUBPICTURE_ALIGN_RIGHT;
>> +                break;
>> +            case libvlc_position_bottom:
>> +                pos = SUBPICTURE_ALIGN_BOTTOM;
>> +                break;
>> +            case libvlc_position_bottom_left:
>> +                pos = SUBPICTURE_ALIGN_BOTTOM | SUBPICTURE_ALIGN_LEFT;
>> +                break;
>> +            case libvlc_position_bottom_right:
>> +                pos = SUBPICTURE_ALIGN_BOTTOM | SUBPICTURE_ALIGN_RIGHT;
>> +                break;
>> +            default:
>> +                pos = 0;
>> +                break;
>>
>
> I think a lookup table (preceded by a boundary check) makes more sense
> than a switch here. Also there should be no need for int64. Otherwise OK.


If the boundary check fails should an error code be returned? Currently the
method has a void return type - is it OK to change the return type void ->
int in the public API in this case?

Or should I implement it so if the boundary check fails it is effectively
the same as disabling the video title, and keep the return type as void?


>
>  +        }
>> +
>>          var_SetBool( p_mi, "video-title-show", true );
>> -        var_SetInteger( p_mi, "video-title-position", position );
>> +        var_SetInteger( p_mi, "video-title-position", pos );
>>          var_SetInteger( p_mi, "video-title-timeout", timeout );
>>      }
>>      else
>>
>
> --
> Rémi Denis-Courmont
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel
>


<http://apricasoftware.co.uk>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20140523/b7f1d0fb/attachment.html>