[vlc-devel] [PATCH 2/4] input: add a new variable storing which ES type was last modified

[Felix Abecassis]

2014-07-21 14:44 GMT+02:00 Rémi Denis-Courmont <remi at remlab.net>:
> Le 2014-07-21 15:23, Felix Abecassis a écrit :
>
>> 2014-07-19 8:30 GMT+02:00 Rémi Denis-Courmont <remi at remlab.net>:
>>>
>>> Le vendredi 18 juillet 2014, 19:40:50 Felix Abecassis a écrit :
>>>>
>>>> ---
>>>>  src/input/event.c | 9 +++++++++
>>>>  src/input/var.c   | 2 ++
>>>>  2 files changed, 11 insertions(+)
>>>
>>>
>>>
>>> I still do not see how that ensures serialization. As far as I can tell,
>>> the
>>> ES lock is *not* mandatory at that point.
>>>
>>
>> I am probably missing something here, I do not see what the problem is.
>> For instance, in input_SendEventPosition (src/input/event.c), we set
>> the variables "position" and "time" and set "intf-event" with
>> INPUT_EVENT_POSITION.
>
>
> Again, existing bad code is not an excuse for new bad code. Also again, an
> input has only one position, so concurrent updates are much less of a
> potential issue, than for ES.
>

Apologies for having you repeat yourself. I wanted to be sure I
understood you correctly on this one.
If this variable/callback mechanism is flawed, we need to find a
better way for handling all events, not just this one. Maybe we could
add a vlc_event_manager_t object to the input thread and safely pass
events this way. This is what we do currently for input_item events.
What do you think?

>
>> Do you mean that functions input_SendEventEs* can be called
>> concurrently?
>
>
> I don't see anything preventing it.
>
>
>> Calls to these functions seems to be protected by a lock
>> in the EsOut functions (src/input/es_out.c).
>
>
> Again, it does not seem that "the" ES lock is taken on all code paths.
>

I think it is, so we should be fine. However the call graph of this
module is a bit complex and indeed it is not specified explicitly that
the lock must be held to use the callback.

> The ES output is supposed to be (mostly) reentrant, and indeed the decoders have
> one thread each.
>

I don't follow you here, why are you mentioning the decoder threads?
There is one es_out object and it contains the several decoder modules, right?

>
>>> And even if that worked, eventually people would break it, as it's too
>>> easy to
>>> access the variable and make incorrect assumptions.
>
>
>> The variable name suggested by Francois sounds better, there should be
>> less confusion about the usage of this variable.
>
>
> I'm not concerned about the variable name. I'm concerned about people doing
> var_Get/var_Inherit; as things stand, I think the variables are only usable
> with var_AddCallback.
>
> --
> Rémi Denis-Courmont
>
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel

Thank you,

-- 
Félix Abecassis
http://felix.abecassis.me