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

[Steve Lhomme]

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.

 > 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
>