Re: [Gstreamer-openmax] [PATCH 1/5] Move component and library name fields to 'util'

[Clark, R. <ro...@ti...>]

On Mar 4, 2010, at 7:36 AM, Felipe Contreras wrote:

> On Thu, Mar 04, 2010 at 12:38:26AM +0100, Rob Clark wrote:
>> 
>> On Mar 3, 2010, at 4:51 PM, Felipe Contreras wrote:
>> 
>>> @@ -234,10 +231,10 @@ get_property (GObject *obj,
>>>    switch (prop_id)
>>>    {
>>>        case ARG_COMPONENT_NAME:
>>> -            g_value_set_string (value, self->omx_component);
>>> +            g_value_set_string (value, self->gomx->component_name);
>>>            break;
>>>        case ARG_LIBRARY_NAME:
>>> -            g_value_set_string (value, self->omx_library);
>>> +            g_value_set_string (value, self->gomx->library_name);
>>>            break;
>> 
>> one thought as I look at the patch (and something that I didn't think
>> of either when I made my original patch)..
>> 
>> but the get_property() for component-name and library-name (and when I
>> send a later patch,  component-role) are basically identical..
>> 
>> what about adding a g_omx_core_get_property_helper() which could be
>> shared by all the base classes?  This way, when I send the patch to
>> add component-role, it just changes GOmxCore and not the base classes
>> ;-)
>> 
>> I can send the updated patch if you like the idea
> 
> Yeah, I like the idea, but the g_omx_ is for util stuff that's not
> directly related to GStreamer (from the old GOmx library), it might make
> sense change this prefix in the future. Anyway, the rest of the stuff
> should have gstomx_.
> 
> Or, even better, add the properties to the interface:
> http://library.gnome.org/devel/gobject/unstable/howto-interface-properties.html
> 
> I'm not sure if that's what we want, but sounds like it.


It seems interface properties are limited to just that.. interface.  The implementation of the part in get_property() (and set_property() if applicable) must remain.

So I lean more towards the helper functions idea.  This makes it easier to add other common properties (like component-role) without touching each of the base classes.

Since gstomx.c already knows of the GOmxCore struct, I can add the helpers here for now.  But I think perhaps it gets cleaner if we just change g_omx_* to gstomx_*..

BR,
-R