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

[Rémi Denis-Courmont]

I am confused by your confusing changes like protecting with locks stuff that does not need to be (thus confusing the purpose of the lock), or renaming function by which thread is supposed to call them instead of by what object they operate on (which is against the encapsulation principle), or by passing private pointers instead of public ones (which goes against the VLC coding style, typical C OOP style and, as noted by Thomas, causes useless churn).

All of that because *you* want to make the code *your* way, even if it makes it more confusing for everyone else, especially earlier devs, in this case (Laurent), me and Thomas.

Le 10 octobre 2019 09:39:07 GMT+03:00, Steve Lhomme <robux4 at ycbcr.xyz> a écrit :
>On 2019-10-10 8:28, Rémi Denis-Courmont wrote:
>> 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.
>
>I am confused by reading this code, hence the existence of this patch. 
>So don't tell me there was less confusion before. As I explained I have
>
>to read 2000 lines and probe all accesses to that variable to finally 
>conclude it's OK to read it without the lock and then lock all accesses
>
>to read *right after*.
>
>Actually it's more than 2000 lines, because of all the vout_XXX 
>semi-public functions I also have to know which are called from which 
>thread to conclude if there's not a case where the value of
>sys->display 
>can be modified during this function call.
>
>The fact that there's no documentation on who owns the value and when
>it 
>can be set/not set doesn't help either.
>
>So don't tell me about creating confusion. If there's one confusion
>it's 
>a variable read without a lock and locking it *right after*.
>
>> 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é.
>> 
>> _______________________________________________
>> 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

-- 
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/43fff388/attachment.html>