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

[Filip Roséen]

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.

> +               [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.

> +           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.

However unlikely that such mismatching would take place in practice, I
am not a fan of the current implementation.

> +           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;
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20180815/116afb28/attachment.html>