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

[Steve Lhomme]

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