[vlc-devel] [vlc-commits] es_out: add multiview mode info in the ES description

[Rémi Denis-Courmont]

Adding a "dummy" maximum inside an enum causes warnings in switch statements. I don't recommend it...

Le 15 août 2018 10:48:00 GMT+03:00, Steve Lhomme <robux4 at ycbcr.xyz> a écrit :
>Hi Filip,
>
>
>On 15/08/2018 01:58, Filip Roséen wrote:
>>
>> Hi Steve,
>>
>> On 2018-08-14 15:26, Steve Lhomme wrote:
>>
>>     |vlc | branch: master | Steve Lhomme <robux4 at ycbcr.xyz> | Tue Aug
>>     14 12:49:51 2018 +0200|
>[25756f4b78a010599b541b6df80e534019f0d5ab]
>>     | committer: Steve Lhomme es_out: add multiview mode info in the
>>     ES description|
>>
>>
>>              
>http://git.videolan.org/gitweb.cgi/vlc.git/?a=commit;h=25756f4b78a010599b541b6df80e534019f0d5ab
>>
>>     |src/input/es_out.c | 19 +++++++++++++++++++ 1 file changed, 19
>>     insertions(+) diff --git a/src/input/es_out.c
>b/src/input/es_out.c
>>     index 37d4c10ab6..f1f13cad66 100644 --- a/src/input/es_out.c +++
>>     b/src/input/es_out.c @@ -3370,6 +3370,25 @@ static void
>>     EsOutUpdateInfo( es_out_t *out, es_out_id_t *es, const
>es_format_t
>>     * info_category_AddInfo( p_cat, _("Chroma location"), "%s",
>>     vlc_gettext(c_loc_names[fmt->video.chroma_location]) ); } + if(
>>     fmt->video.multiview_mode != MULTIVIEW_2D ) + { + static const
>>     char c_multiview_names[][16] = {|
>>
>> |sizeof( "Column Sequential" )| is |18|. I’d be extremely surprised
>if 
>> your compiler did not warn you about this, so please pay closer 
>> attention to diagnostics issued for paths you’ve touched.
>>
>
>Yes, I just noticed now. I recently switched to a newer Qt Creator
>which 
>uses Clang to produce a lot more useful warnings but I'm a bit 
>overwhelmed with all the stuff there is to clean. I made some changes
>to 
>see the most obvious one.
>
>>     |+ [MULTIVIEW_2D] = N_("2D"), + [MULTIVIEW_STEREO_SBS] =
>>     N_("Side-By-Side"), + [MULTIVIEW_STEREO_TB] = N_("Top-Bottom"), +
>>     [MULTIVIEW_STEREO_ROW] = N_("Row Sequential"), +
>>     [MULTIVIEW_STEREO_COL] = N_("Column Sequential"), +
>>     [MULTIVIEW_STEREO_FRAME] =N_("Frame Sequential"), +
>>     [MULTIVIEW_STEREO_CHECKERBOARD] = N_("Checkboard"), + }; +
>>     static_assert(ARRAY_SIZE(c_multiview_names) ==
>>     MULTIVIEW_STEREO_CHECKERBOARD+1, + "Multiview format table
>>     mismatch");|
>>
>> I would strongly advice against such a |static_assert| as it does not
>
>> really protect the read that follows.
>>
>
>Indeed.
>
>>     |+ info_category_AddInfo( p_cat, _("Stereo Mode"), "%s", +
>>     vlc_gettext(c_multiview_names[fmt->video.multiview_mode]) );|
>>
>> Imagine if someone, for whatever reason, adds an entry to |enum 
>> video_multiview_mode_t| after |MULTIVIEW_STEREO_CHECKERBOARD|, the 
>> |static_assert| will not fire, but your implementation would still 
>> result in an /out-of-bound/ read.
>>
>> There are numerous ways to solve this issue, one is using the same 
>> mechanism which is used in other places in the codebase; introduce 
>> something like |MULTIVIEW_STEREO_MAX| (or suitable name). See 
>> |CHROMA_LOCATION_MAX|, as an example.
>>
>
>Yes, the max approach is safer.
>
>> However unlikely that such mismatching would take place in practice,
>I 
>> am not a fan of the current implementation.
>>
>
>If you mean that way of static as done 4 times above this code ? If you
>
>have something cleaner feel free to submit a patch.
>
>>     |+ info_category_AddInfo( p_cat, _("First Stereo Eye"), +
>>     vlc_gettext(fmt->video.b_multiview_right_eye_first ? +
>N_("Right")
>>     : N_("Left")) ); + } if( fmt->video.projection_mode !=
>>     PROJECTION_MODE_RECTANGULAR ) { const char *psz_loc_name = NULL;|
>>
>>
>>
>> _______________________________________________
>> vlc-devel mailing list
>> To unsubscribe or modify your subscription options:
>> https://mailman.videolan.org/listinfo/vlc-devel
>
>_______________________________________________
>vlc-devel mailing list
>To unsubscribe or modify your subscription options:
>https://mailman.videolan.org/listinfo/vlc-devel

-- 
Envoyé de mon appareil Android avec Courriel K-9 Mail. Veuillez excuser ma brièveté.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20180815/2e84bc1c/attachment.html>