[vlc-devel] commit: Implement access_GetParentInput and demux_GetParentInput and use. ( Pierre d'Herbemont )

[Rémi Denis-Courmont]

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