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

[Steve Lhomme]

On 28/09/2018 14:04, Filip Roséen wrote:
>
> 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.
>

IMO the picture_t.format is the one to use, precisely because it can 
change between each picture. Right now the changes have to be populated 
in the pool that creates/provides these pictures and it's a mess. Apart 
from pixels/sizes needed by the decoder everything else if probably fine 
being dynamic (in the picture). So yes, we need to clean that and come 
up with basic rules.

> Any opinions about this that might be worth sharing prior to any 
> extensive /workshop/ on the matter?
>
> Best Regards,
> Filip
>
>
>
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel