[vlc-devel] [PATCH 3/7] video_output: asset the display existence inside the display_lock

[Thomas Guillem]

On Thu, Oct 10, 2019, at 08:10, Steve Lhomme wrote:
> On 2019-10-09 17:57, Thomas Guillem wrote:
> > Not needed, you shoud add a documentation that says that is API is not thread safe but reentrant like decoder_updatevideoformat()
> 
> Same remark, it requires to know the whole code around this function to 
> read 4 lines that would otherwise be totally coherent. It requires 
> reading a whole lot more code for the sake of it (and reading 
> multithread code in a 2000 lines file is anything but easy).
> 
> In other words it means: if you intend to touch this file, you have to 
> understand all of it. What may seem straightforward is not.
> 
> If a comment is needed that's why a variable with a dedicated lock is 
> read without that lock. What kind of multithreading good practice is that ?

Reducing the lock scope to the maximum allow a lot more flexibility from the caller of this API.
This API just lacks a proper documentation.
Anyway, this API is used from decoder.c that handle himself its locking/serialization.  Adding extra locks doesn't seem a good idea to me.

> 
> > On Wed, Oct 9, 2019, at 17:36, Steve Lhomme wrote:
> >> ---
> >>   src/video_output/video_output.c | 4 +---
> >>   1 file changed, 1 insertion(+), 3 deletions(-)
> >>
> >> diff --git a/src/video_output/video_output.c
> >> b/src/video_output/video_output.c
> >> index df27a402b8b..e8ee9572188 100644
> >> --- a/src/video_output/video_output.c
> >> +++ b/src/video_output/video_output.c
> >> @@ -1346,8 +1346,6 @@ void vout_ChangePause(vout_thread_t *vout, bool
> >> is_paused, vlc_tick_t date)
> >>   static void vout_FlushUnlocked(vout_thread_t *vout, bool below,
> >>                                  vlc_tick_t date)
> >>   {
> >> -    vout_thread_sys_t *sys = vout->p;
> >> -
> >>       vout->p->step.timestamp = VLC_TICK_INVALID;
> >>       vout->p->step.last      = VLC_TICK_INVALID;
> >>   
> >> @@ -1368,8 +1366,8 @@ static void vout_FlushUnlocked(vout_thread_t
> >> *vout, bool below,
> >>   
> >>       picture_fifo_Flush(vout->p->decoder_fifo, date, below);
> >>   
> >> -    assert(sys->display != NULL);
> >>       vlc_mutex_lock(&vout->p->display_lock);
> >> +    assert(vout->p->display != NULL);
> >>       vout_FilterFlush(vout->p->display);
> >>       vlc_mutex_unlock(&vout->p->display_lock);
> >>   
> >> -- 
> >> 2.17.1
> >>
> >> _______________________________________________
> >> vlc-devel mailing list
> >> To unsubscribe or modify your subscription options:
> >> https://mailman.videolan.org/listinfo/vlc-devel
> > _______________________________________________
> > vlc-devel mailing list
> > To unsubscribe or modify your subscription options:
> > https://mailman.videolan.org/listinfo/vlc-devel
> > 
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel