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

[Thomas Guillem]

On Thu, Oct 10, 2019, at 09:38, Steve Lhomme wrote:
> On 2019-10-10 9:11, Thomas Guillem wrote:
> > 
> > 
> > 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.
> 
> As a general principal yes. But is there a case where the assert will 
> fail where it wouldn't fail when outside of the lock ? If yes, then the 
> assert should be in the lock. If not it doesn't hurt to put it in the lock.
> 
> > This API just lacks a proper documentation.
> > Anyway, this API is used from decoder.c that handle himself its locking/serialization.
> 
> Assuming the code is safe because the caller is safe doesn't seem good 
> multithreading practice.

For me, it is very fine for an internal API (it's not even exposed by VLC core)

> 
>  > Adding extra locks doesn't seem a good idea to me.
> 
> I agree and there is no lock added.
> 
> >>
> >>> 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
> > _______________________________________________
> > 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