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

[Steve Lhomme]

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
>