[vlc-devel] [PATCH 2/3] video_output: create pictures with the display format with interactive filters

[Rémi Denis-Courmont]

Le perjantaina 14. elokuuta 2020, 8.39.19 EEST Steve Lhomme a écrit :
> On 2020-08-13 16:47, Rémi Denis-Courmont wrote:
> > Le torstaina 13. elokuuta 2020, 15.13.06 EEST Steve Lhomme a écrit :
> >> Also used for whatever the last filter of the filter chains is.
> >> ---
> >> 
> >>   src/video_output/video_output.c | 12 +++++++-----
> >>   1 file changed, 7 insertions(+), 5 deletions(-)
> >> 
> >> diff --git a/src/video_output/video_output.c
> >> b/src/video_output/video_output.c index 6097b9df7f9..dc636d72928 100644
> >> --- a/src/video_output/video_output.c
> >> +++ b/src/video_output/video_output.c
> >> @@ -859,12 +859,14 @@ static void
> >> ThreadDelAllFilterCallbacks(vout_thread_sys_t *vout) static picture_t
> >> *VoutVideoFilterInteractiveNewPicture(filter_t *filter) {
> >> 
> >>       vout_thread_sys_t *sys = filter->owner.sys;
> >> 
> >> +    picture_t *picture;
> >> 
> >> -    picture_t *picture = picture_pool_Get(sys->private.private_pool);
> >> -    if (picture) {
> >> -        picture_Reset(picture);
> >> -        video_format_CopyCropAr(&picture->format,
> >> &filter->fmt_out.video);
> >> -    }
> >> +    vlc_mutex_lock(&sys->display_lock);
> >> +    if (sys->display)
> >> +        picture =  picture_NewFromFormat(&sys->display->fmt);
> >> +    else
> >> +        picture = NULL;
> >> +    vlc_mutex_unlock(&sys->display_lock);
> >> 
> >>       return picture;
> >>   
> >>   }
> > 
> > Are you sure that we can safely lock the display from that callback?
> 
> The filter chain(s) is only allocating pictures when calling
> filter_chain_VideoFilter(). This happens in
> ThreadDisplayPreparePicture() in the vout thread.
> 
> ThreadDisplayRenderPicture() called right after also holds the same lock
> for most of its duration. With a notable difference, the filters.lock is
> not held in this case. ThreadProcessMouseState() also holds them separately.
> 
> It doesn't seem that the code can mix them together as of now. But it
> may not be clean to allow this. It's probably cleaner to use
> filter_chain_GetFmtOut() instead to the get the proper video_format_t to
> use.

It would make sense that the filter lock is before the display lock. The 
opposite is unacceptable, as it would block display usage during filter 
processing. But if at all possible, I would much prefer they remain exclusive.

And I don't see how it helps here. If there is a possibility that the display 
format changes, then you would need to hold the lock from the picture 
allocation all the way until it is queued to the display - not just during the 
allocation.

-- 
Реми Дёни-Курмон
http://www.remlab.net/