[vlc-devel] [PATCH] picture: fix missing video_format_t attributes from the picture

[Filip Roséen]

Hi,

On 2018-09-28 12:26, Steve Lhomme wrote:

> regression introduced by 2054d46ba29d7986d8403656170ff9445ab18efc
> ---
>  src/misc/picture.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/src/misc/picture.c b/src/misc/picture.c
> index f8059a603e3..f61e0a3d0ef 100644
> --- a/src/misc/picture.c
> +++ b/src/misc/picture.c
> @@ -189,6 +189,7 @@ static picture_priv_t *picture_NewPrivate(const video_format_t *restrict p_fmt)
>  
>      memset( p_picture, 0, sizeof( *p_picture ) );
>  
> +    p_picture->format = *p_fmt;

LGTM, as long as we are sure that we will not (once again) step into
life-time issues related to the ownership of
`video_format_t.p_palette` (but as it worked before, well.. it at
least wouldn't get worse than what we had previously).

>      /* Make sure the real dimensions are a multiple of 16 */
>      if( picture_Setup( p_picture, p_fmt ) )
>      {

This regression was noticed as part of a conversation between me and
`haasn` on `#videolan` (as `haasn` was confused about what caused his
previously working code to all of a sudden behave weird).

I started digging in the commit-log, and found that `picture_t.format`
a long time in the past had a different type than `video_format_t`,
but later a `typedef` was introduced to make `video_frame_format_t` an
alias for `video_format_t`.

 - [1a67448183a: ... include/vlc_video.h: extended video_frame_format_t
   ](http://git.videolan.org/?p=vlc.git;a=commit;h=1a67448183a9c5ab7b5b427fb80d2d1a2e34ff8d)

`haasn` is working on `video_output/vulcan`. As the information he is
after is available in `vout_display_t.fmt` he simply modified his
patch to use that instead of the format associated with a single
`picture_t`.

So, my question is (ignoring the obvious breakage of current modules
relying on `picture_t.format` having all its fields populated and not
only those from `src/misc/picture.c:picture_Setup`): should we go back
to the previous behavior where a smaller distinct type,
`video_frame_format_t`, is used for each `picture_t`?

We of course have the issue where a *format-change* mid-stream will
result in weird behavior due to the async nature of changing the
associated format, and the `picture_t`-processing not matching the
update (applying the format to too many or too few frames). After a
very quick conversation with `tguillem` this is something we can and
will address at the *vout-workshop*, though I would like to plant this
seed (regarding change of type and things that might need changing)
here on `vlc-devel` to get as many opinions as possible prior to said
workshop.

Any opinions about this that might be worth sharing prior to any
extensive *workshop* on the matter?

Best Regards,\
Filip
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20180928/0541317a/attachment.html>