[vlc-devel] [PATCH 2/5] video_output: move the dead (don't push) control handling in video_output

[Rémi Denis-Courmont]

Le perjantaina 18. joulukuuta 2020, 10.10.23 EET Steve Lhomme a écrit :
> On 2020-12-18 9:05, Rémi Denis-Courmont wrote:
> > Le perjantaina 18. joulukuuta 2020, 9.59.13 EET Steve Lhomme a écrit :
> >> On 2020-12-18 8:54, Rémi Denis-Courmont wrote:
> >>> Le perjantaina 18. joulukuuta 2020, 9.49.59 EET Steve Lhomme a écrit :
> >>>> On 2020-12-18 8:47, Rémi Denis-Courmont wrote:
> >>>>> Le perjantaina 18. joulukuuta 2020, 7.48.13 EET Steve Lhomme a écrit :
> >>>>>> On 2020-12-17 21:37, Rémi Denis-Courmont wrote:
> >>>>>>> Le torstaina 17. joulukuuta 2020, 17.39.16 EET Steve Lhomme a écrit 
:
> >>>>>>>> Only the code that pushes code need to know it's not supposed to
> >>>>>>>> push
> >>>>>>>> anything anymore. So far it was under a lock of the control thread.
> >>>>>>>> We
> >>>>>>>> use
> >>>>>>>> an atomic boolean instead.
> >>>>>>> 
> >>>>>>> This is not equivalent and leads to races.
> >>>>>> 
> >>>>>> Please explain.
> >>>>> 
> >>>>> This breaks atomicity in a number of places.
> >>>> 
> >>>> Where ?
> >>> 
> >>> atomic_load() duh, where else?
> >> 
> >> Maybe it's funny to you. But I would like explanation on what the real
> >> issues are.
> > 
> > The point of code review is not to teach basic coding. I pointed out what
> > the problem was and where it was. If you can't be bothered to question
> > your own code, that's on you.
> 
> You're just leaving me guessing what you have in your head which I
> obviously didn't see (if it actually exists).

Guessing what? You've replaced a mutex-protected variable with an atomic no 
longer protected by the same mutex. This, _obviously_, breaks atomicity w.r.t. 
anything else that is/was protected by that mutex. You should be aware of the 
essence of your own patch, no?

Not only that, but using only load and store is already a red flag. That 
intrinsically implies that you accept "outdated" values on load.

> If you're not capable in explaining the issue clearly it's probably that
> it's not there at all.

That's rich of a paid developer to tell a volunteer. You're just abusing other 
people's time here.

These types of issues have already been explained many times.


-- 
雷米‧德尼-库尔蒙
http://www.remlab.net/