[vlc-devel] [PATCH 2/7] video_output: read the vd inside the display_lock
Steve Lhomme
robux4 at ycbcr.xyz
Thu Oct 10 10:51:07 CEST 2019
On 2019-10-10 10:38, Rémi Denis-Courmont wrote:
> 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.
It may be *me*, but it may also benefit 20 other newcomers that might
want to do stuff with VLC but are too confused by the bizarre code.
> 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é.
>
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel
>
More information about the vlc-devel
mailing list