[vlc-devel] commit: Implement access_GetParentInput and demux_GetParentInput and use. ( Pierre d'Herbemont )
Rémi Denis-Courmont
remi at remlab.net
Thu Aug 27 12:49:30 CEST 2009
On Thu, 27 Aug 2009 11:24:35 +0200, Pierre d'Herbemont
<pdherbemont at gmail.com> wrote:
> On Aug 27, 2009, at 8:50 AM, Rémi Denis-Courmont wrote:
>> On Wed, 26 Aug 2009 22:27:10 +0200, Pierre d'Herbemont
>> <pdherbemont at gmail.com> wrote:
>>> My point is we don't do proper refcounting on some object that
>>> announce it.
>>
>> You still failed to give any concrete example of that.
>
> *All* object, it is a matter of naming. input_thread_t, demux_t,
> access_t are objects, They all have vlc_object_hold() and some of them
> may crash on public accessor.
No. It is irrelevant that vlc_object_hold() will automagically down-cast
*from* demux_t *to* vlc_object_t. There is only a problem if you up-cast,
which none of the object functions do. In particular, functions returning
objects from other threads, such as vlc_object_find, vlc_object_find_name
and vlc_list_children return vlc_object_t pointers.
> By object I generally refer to an entity that you create and destroy.
> This is simply a generic term that is being misused by vlc_object_t.
vlc_object_t can be created, referenced and dereferenced (and automatically
destroyed when they reference count drops to zero). You just invented a
problem.
> I say that demux_t is an object.
You do. I don't.
> and using vlc_object_hold() on it is misleading
> because it's only retaining some part of the structure.
The way to solve this is:
struct demux_t
{
- VLC_COMMON_MEMBERS
+ vlc_object_t *object;
But it would cause a huge disruptive churn of code, and add one more error
cases in every object allocation. That's quite expensive considering one
developer is confused by something that was discussed I don't know how many
times.
> Okay, that you say it's only retaining at least the COMMON_MEMBERS,
> but then, please change the name. If you have use some serious object
> oriented API this is completely misleading.
vlc_object_hole
vlc_object_t
Naming seems quite consistent to me.
> Come on. access is an object, demux is an object, whatever is an
> object. Don't use generic misleading term to describe something else.
> This is misleading.
No. access is a private structure made of an object, a module, private
module state and an interface between the owner and the module.
You can only get an access out of access_New() or similar functions
creating an access in the caller thread.
>>> We don't do proper refcounting for (all?) vlc_object_t, even if we
>>> pretend we do by having this vlc_object_hold. (This used to be even
>>> more true back in the time when we used to wait until object
>>> destruction).
>>
>> You still failed to give any such object. I am not aware of any
>> error in
>> our handling of access and demux objects, other than the way we
>> "kill" them
>
> I don't think there should be any error because the only vlc_object_t
> that are being used outside are
> aout_instance, input_thread_t, and playlist_t.
>
> Those are safe to use if you hold a reference that comes from
> vlc_object_hold().
Those three are normally coming with the right type from dedicated
functions.
You are welcome to get rid of the remaining faulty vlc_object_find() calls.
playlist_t is already completely fixed.
>> vlc_object_hold() is legal if:
>> - the caller already has a reference to the object, and/or
>
> Yes, but then the reference you keep is restrained compared to the
> reference that was passed. I would like to make that distinction at
> API level.
That does not make sense because there are no other type of references. You
cannot increase the reference count of an access_t as such. It always has
exactly one reference. Hence you don't need an API for it.
--
Rémi Denis-Courmont
More information about the vlc-devel
mailing list