[vlc-devel] [PATCH 2/7] video_output: read the vd inside the display_lock
Thomas Guillem
thomas at gllm.fr
Thu Oct 10 10:54:05 CEST 2019
On Thu, Oct 10, 2019, at 10:51, Steve Lhomme wrote:
> 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.
Maybe it's just a question of taste. I have no problem at all with this file.
>
> > 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
> >
> _______________________________________________
> 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