[vlc-devel] [PATCH 2/7] video_output: read the vd inside the display_lock

[Steve Lhomme]

On 2019-10-09 18:15, Rémi Denis-Courmont wrote:
> Le keskiviikkona 9. lokakuuta 2019, 18.50.23 EEST Steve Lhomme a écrit :
>> On 2019-10-09 17:48, Thomas Guillem wrote:
>>> On Wed, Oct 9, 2019, at 17:36, Steve Lhomme wrote:
>>>> ---
>>>>
>>>>    src/video_output/video_output.c | 3 +--
>>>>    1 file changed, 1 insertion(+), 2 deletions(-)
>>>>
>>>> diff --git a/src/video_output/video_output.c
>>>> b/src/video_output/video_output.c
>>>> index a5046913867..df27a402b8b 100644
>>>> --- a/src/video_output/video_output.c
>>>> +++ b/src/video_output/video_output.c
>>>> @@ -1013,9 +1013,8 @@ static int
>>>> ThreadDisplayRenderPicture(vout_thread_t *vout, bool is_forced)
>>>>
>>>>        if (filtered->date != sys->displayed.current->date)
>>>>        
>>>>            msg_Warn(vout, "Unsupported timestamp modifications done by
>>>>
>>>> chain_interactive");
>>>>
>>>> -    vout_display_t *vd = sys->display;
>>>> -
>>>>
>>>>        vlc_mutex_lock(&sys->display_lock);
>>>>
>>>> +    vout_display_t *vd = sys->display;
>>>
>>> It is not needed since this variable is set before the thread is created.
>>
>> At the very least it's easier to read than wonder why this right exactly
>> before a lock. Why make things more complicated to read ?
> 
> On the complexity metric, it's exactly the same - not more or less
> complicated.
> 
> And it gives the wrong impression that the value needs the lock to be read. So
> it's really just worse. I agree with Thomas.

When I read this I read: the coherence is not guaranteed. This is 
multithreading 101. Forcing the reader to know the whole context of 
where sys->display comes from and when it exists or is modified, just to 
read this function, is just plain dumb. There is no technical reason. 
The only reason given is that because of the conjunctions of all the 
planets around this function it is actually possible to read the value 
without the lock (and then lock any access to that value for everyone 
else because it would have terrible consequences).

To me it's just keeping a complexity just for the sake of it.