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

[Steve Lhomme]

On 2020-12-18 9:47, Rémi Denis-Courmont wrote:
> 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?

I am fully aware of what I do. The code is not 100% equivalent, I never 
claimed otherwise. But the question is whether the logic of the code is 
preserved. And I think that's the case. The 'is_dead' means the control 
FIFO should not enqueue commands anymore. It's protected by a mutex just 
because in that code that's how code is protected. But if you move the 
enqueuing check outside it doesn't have to be that way.

It won't block other threads or wait for other threads using the control 
FIFO to be done, but it's not an issue for what this flag is for.

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

In fact I do accept outdated values (being pushed while the flag is 
being set, if that's what you meant). I would go as far as saying we 
might as well remove the flag entirely and enqueue the commands. When 
the flag is being set, the vout thread is dead. Anything that is 
enqueued will be discarded a few lines later. And anything enqueued 
before control_Dead() used to be called has exactly the same problem. 
Which is actually not a problem at all.

>> 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.

Yet you can found the time to give an actual explanation, which you 
could have done in the first reply, saving your time and mine.

> These types of issues have already been explained many times.

I don't think multithreading issues like these can be generalized 
easily. Basic concepts, sure. But particular cases it's tricky.