<html><head></head><body>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).<br><br>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.<br><br><div class="gmail_quote">Le 10 octobre 2019 09:39:07 GMT+03:00, Steve Lhomme <robux4@ycbcr.xyz> a écrit :<blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;">
<pre class="k9mail">On 2019-10-10 8:28, Rémi Denis-Courmont wrote:<br><blockquote class="gmail_quote" style="margin: 0pt 0pt 1ex 0.8ex; border-left: 1px solid #729fcf; padding-left: 1ex;">Hi,<br><br>There is no threading 101 here. It's common sense in threading that some <br>data is readable and some is not. Otherwise you couldn't even get a hold <br>of the lock objects.<br><br>Increasing lock scope for taste reasons is horrible. It just gives the <br>wrong impression about the requirements, and creates confusion.<br></blockquote><br>I am confused by reading this code, hence the existence of this patch. <br>So don't tell me there was less confusion before. As I explained I have <br>to read 2000 lines and probe all accesses to that variable to finally <br>conclude it's OK to read it without the lock and then lock all accesses <br>to read *right after*.<br><br>Actually it's more than 2000 lines, because of all the vout_XXX <br>semi-public functions I also have to know which are called from which <br>thread to conclude if there's not a case where the value of sys->display <br>can be modified during this function call.<br><br>The fact that there's no documentation on who owns the value and when it <br>can be set/not set doesn't help either.<br><br>So don't tell me about creating confusion. If there's one confusion it's <br>a variable read without a lock and locking it *right after*.<br><br><blockquote class="gmail_quote" style="margin: 0pt 0pt 1ex 0.8ex; border-left: 1px solid #729fcf; padding-left: 1ex;">Totally agree with Thomas against this change.<br><br>Le 10 octobre 2019 09:05:07 GMT+03:00, Steve Lhomme <robux4@ycbcr.xyz> a <br>écrit :<br><br> On 2019-10-09 18:15, Rémi Denis-Courmont wrote:<br><br> Le keskiviikkona 9. lokakuuta 2019, 18.50.23 EEST Steve Lhomme a<br> écrit :<br><br> On 2019-10-09 17:48, Thomas Guillem wrote:<br><br> On Wed, Oct 9, 2019, at 17:36, Steve Lhomme wrote:<hr> src/video_output/video_output.c | 3 +--<br> 1 file changed, 1 insertion(+), 2 deletions(-)<br><br> diff --git a/src/video_output/video_output.c<br> b/src/video_output/video_output.c<br> index a5046913867..df27a402b8b 100644<br> --- a/src/video_output/video_output.c<br> +++ b/src/video_output/video_output.c<br> @@ -1013,9 +1013,8 @@ static int<br> ThreadDisplayRenderPicture(vout_thread_t *vout, bool<br> is_forced)<br><br> if (filtered->date != sys->displayed.current->date)<br><br> msg_Warn(vout, "Unsupported timestamp modifications<br> done by<br><br> chain_interactive");<br><br> - vout_display_t *vd = sys->display;<br> -<br><br> vlc_mutex_lock(&sys->display_lock);<br><br> + vout_display_t *vd = sys->display;<br><br> It is not needed since this variable is set before the<br> thread is created.<br><br> At the very least it's easier to read than wonder why this<br> right exactly<br> before a lock. Why make things more complicated to read ?<br><br> On the complexity metric, it's exactly the same - not more or less<br> complicated.<br><br> And it gives the wrong impression that the value needs the lock<br> to be read. So<br> it's really just worse. I agree with Thomas.<br><br><br> When I read this I read: the coherence is not guaranteed. This is<br> multithreading 101. Forcing the reader to know the whole context of<br> where sys->display comes from and when it exists or is modified, just to<br> read this function, is just plain dumb. There is no technical reason.<br> The only reason given is that because of the conjunctions of all the<br> planets around this function it is actually possible to read the value<br> without the lock (and then lock any access to that value for everyone<br> else because it would have terrible consequences).<br><br> To me it's just keeping a complexity just for the sake of it.<hr> vlc-devel mailing list<br> To unsubscribe or modify your subscription options:<br> <a href="https://mailman.videolan.org/listinfo/vlc-devel">https://mailman.videolan.org/listinfo/vlc-devel</a><br><br><br>-- <br>Envoyé de mon appareil Android avec Courriel K-9 Mail. Veuillez excuser <br>ma brièveté.<hr>vlc-devel mailing list<br>To unsubscribe or modify your subscription options:<br><a href="https://mailman.videolan.org/listinfo/vlc-devel">https://mailman.videolan.org/listinfo/vlc-devel</a><br><br></blockquote><hr>vlc-devel mailing list<br>To unsubscribe or modify your subscription options:<br><a href="https://mailman.videolan.org/listinfo/vlc-devel">https://mailman.videolan.org/listinfo/vlc-devel</a></pre></blockquote></div><br>-- <br>Envoyé de mon appareil Android avec Courriel K-9 Mail. Veuillez excuser ma brièveté.</body></html>