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

[Rémi Denis-Courmont]

Hi,

There is no threading 101 here. It's common sense in threading that some data is readable and some is not. Otherwise you couldn't even get a hold of the lock objects.

Increasing lock scope for taste reasons is horrible. It just gives the wrong impression about the requirements, and creates confusion.

Totally agree with Thomas against this change.

Le 10 octobre 2019 09:05:07 GMT+03:00, Steve Lhomme <robux4 at ycbcr.xyz> a écrit :
>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.
>_______________________________________________
>vlc-devel mailing list
>To unsubscribe or modify your subscription options:
>https://mailman.videolan.org/listinfo/vlc-devel

-- 
Envoyé de mon appareil Android avec Courriel K-9 Mail. Veuillez excuser ma brièveté.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20191010/60c6a839/attachment-0001.html>