[Gstreamer-openmax] [PATCH] a small bit of refactoring to gstomx_util, to allow for more flexibility of when the OMX component and port is instantiated

[Rob Clark]

On Nov 5, 2009, at 10:56 AM, Felipe Contreras wrote:

> On Wed, Nov 4, 2009 at 5:17 PM, Rob Clark <rob at ti.com> wrote:
>> On Nov 4, 2009, at 8:31 AM, Felipe Contreras wrote:
>>> This doesn't fit with $subject, seems to be a logically independent
>>> trivial rename that's also changing code-style at the same time.
>>
>> [RC] not really.. if you look a bit further down  
>> g_omx_core_get_port()
>> became a public (to the gstomx elements) API and most of the  
>> functionality
>> was split out into an internal get_port helper function which is  
>> called by
>> g_omx_core_get_port() and some other places.
>
> Ok, so it's a different function with essentially the same name.
>
> BTW. No need to prefix your replies with "[RC]". I'm not sure why TI
> people do that.


I guess just habit.. won't do that in future ;-)

>
>> Is your preference that additional parameters to a function go on  
>> separate
>> lines?  If so, my mistake.. I thought that was only for purposes of  
>> line
>> wrapping long lines.
>
> Well, yeah, that's how that function is right now. It probably doesn't
> matter since the coding style will change, but it introduces noise in
> the patch.
>
>>> Again, that's a logically separate change.
>>
>> [RC] hmm, yeah, that should have been one of the later patches..   
>> looks like
>> a couple small things slipped through in the process of trying to
>> re-organize the commits I had already made.  When a lot of  
>> different changes
>> touch the same file, grouping them into different commits isn't
>> straightforward.  (Hmm, need a way to 'git add' parts of a file)
>
> git add --patch $file
>
>>> I'm not sure about passing a GObjectClass to the _new function. A
>>> separate function would make more sense.
>>
>> [RC] yeah... but it is required to completely initialize the GOmxCore
>> object.  (Or could the klass be retrieved from the gobject?)
>
> It's not required right now, is it? The component_name and
> library_name are only needed in the _init function.

But the point of this patch was that _init() could be called upon  
demand the first time the element needs to access the handle.  So they  
might be needed earlier than when _init() was traditionally called.

>
>> The GOmxCore object isn't really complete w/o knowing the library and
>> component name.  And I was trying to keep things simpler for the  
>> gstomx
>> element classes.
>
> Currently GOmxCore is independent of the actual OpenMAX
> implementation. If you want to change that I think that should be a
> different patch.


I'm not sure I quite catch your point.. the GOmxCore object represents  
on instance of one OMX component.. and this requires both library name  
and component name (and later in a later commit, when I add component- 
role, the component role string as well)

but this could be split out into an earlier patch in the series


>
>>>> +    g_return_if_fail (component_name);
>>>> +    g_return_if_fail (library_name);
>>>
>>> If one component_name is there but not library_name there would be a
>>> memory leak, right?
>>
>> [RC] yup.. but you get a useful assert message about what was  
>> screwed up so
>> you can fix it.  Rather than just a segfault.
>
> g_return_if_fail is not fatal, it would just silently return.

yeah, agree.. should be g_assert()

(and if support to get these values from elsewhere, like a config  
file, were added.. then we'd have to make sure the config file  
actually contained these values at this point.  So I don't think that  
really changes anything.)

>
>> maybe those should be g_assert()'s, since it is pretty much a bad  
>> situation
>> of you don't know the component or library name.
>
> Maybe, unless we alternatively obtain those values from somewhere else
> (not happening right now).
>
>>>> +/**
>>>> + * Accessor for OMX component handle.  If the OMX component is not
>>>> constructed
>>>> + * yet, this will trigger it to be constructed  
>>>> (OMX_GetHandle()).  This
>>>> should
>>>> + * at least be used in places where g_omx_core_init() might not  
>>>> have
>>>> been
>>>> + * called yet (such as setting/getting properties)
>>>> + */
>>>> +OMX_HANDLETYPE
>>>> +g_omx_core_get_handle (GOmxCore *core)
>>>> +{
>>>> +  if (!core->omx_handle) g_omx_core_init (core);
>>>> +  return core->omx_handle;
>>>> +}
>>>
>>> Why do we want to do this?
>>
>> [RC] this way, you can do things like GetConfig/GetParameter  
>> earlier in
>> startup.  This was needed by some of the later patches, and is  
>> basically the
>> main purpose of the patch
>
> Perhaps, but I still don't see why another patch would *need* this. If
> anything, g_omx_core_init should be called in some other strategic
> place, not leaked in like this... it messes up the semantics of the
> API.
>


with the addition of call to g_omx_core_deinit() in  
g_omx_core_deinit(), the OMX component won't be leaked.. unless the  
GOmxCore itself was leaked, but that would be a problem in any case



>>> I'm not completely against this patch, but it's difficult to review
>>> with so many unrelated changes, and at the end of the day I still
>>> wonder; What's the point of this patch? I'm sure the purpose would  
>>> be
>>> revealed in latter patches, but each individual patch must make  
>>> sense
>>> on it's own: explain in detail on the commit message.
>>
>> [RC] yeah, sorry.. in process of making those changes I added a  
>> bunch of
>> traces (which is most of the unrelated changes).. but since they were
>> interleaved w/ actual changes, it isn't always easy to break them  
>> into a
>> separate commit after the fact.  If you *really* want to see those  
>> as a
>> separate commit, I'll break them out.
>
> It is easy with git :)
> http://gitcasts.com/posts/interactive-adding


ahh, that is the coolest thing since sliced bread

this will be very helpful, thx!


>
>> either way, I'll resend the patch with the component-role stuff  
>> split out..
>> that part really needs to go along w/ it's corresponding changes in  
>> some
>> other files
>
> Thanks.
>
>>> In general I try to answer these questions:
>>> * What does the patch do?
>>> * Why do we want it?
>>> * What is the impact?
>>
>> btw, I guess I should have put this in the commit msg, but main  
>> purpose of
>> the patch is to make it easier for the various element classes to  
>> have
>> properties which use OMX_{Get,Set}Parameter, for example.  This is  
>> used in
>> some of the later commits.  But was a big enough change by itself  
>> that I
>> wanted to break it into it's own commit.
>
> Yeah, exactly, this should go into the commit message. But it's still
> not clear to me; anybody can do OMX_{Get,Set}Parameter already, and
> the changes of component and library name seem completely independent
> of that.
>
>> Why do we want it?  I think it simplifies usage of the GOmxCore/ 
>> GOmxPort
>> utility classes by making it possible to initialize the core/port  
>> objects in
>> the element constructor while still deferring instantiation of the  
>> OMX
>> component (in the normal case) until where they were previously
>> instantiated. (For example, NULL_TO_READ state change for classes  
>> that
>> inherit from GstOmxBaseFilter)
>
> So you want to change the point at which OpenMAX is really
> initialized? There's some patches ready pending merging that serialize
> omx to gst state changes, although I'm not sure if they would be
> useful to you; in my testing those would require dynamic port
> configuration to work, and it doesn't seem to in TI's omx video
> components.


do you mind sending these patches to me?  Even if they are only half- 
baked and not ready to merge yet.  I can have a look and see if I need  
to change my patch to fit in better with these other patches

(btw, I have some later patches that I'm still working on for dynamic  
port enable/disable/reconfig.  Although these aren't for use w/ the  
omap3 omx components that you are familiar with.  And really has  
nothing to do with this patch.)


>
>> The impact will come later, when some properties that use Get/ 
>> SetParameter()
>> are introduced, that if the properties are set, the OMX component  
>> will be
>> instantiated earlier.  But this is mainly only in debug uses, such  
>> as if you
>> use gst-inspect.  In normal cases startup order stays the same.
>
> I see. In that case the impact is: non-functional/code-reorganization
> or something like that.
>
> Anyway, I see you only have one branch of development. It's generally
> not a good idea to do that because most probably one of the later
> patches might depend on the first ones, so if the merge/review process
> is slow for the initial patches, even if they are trivial, the
> important changes would be delayed. If you have multiple branches,
> then you can send multiple patch series for review at the same time.
> For internal development you can merge all your branches into a
> working branch. It sounds complicated but once you learn your git
> ropes then it's easy :)
>

well, several of these early commits are laying groundwork for some  
later commits.. so they aren't all completely independent.  Although  
some of them are, and different branches for the independent commits  
probably would have simplified life.. but I'm learning new things  
about git every day ;-)

BR,
-R