[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 14, 2009, at 6:33 AM, Felipe Contreras wrote:

> On Fri, Nov 6, 2009 at 4:58 AM, Rob Clark <rob at ti.com> wrote:
>> On Nov 5, 2009, at 10:56 AM, Felipe Contreras wrote:
>>> 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.
>
> I sent a reply to your personal mail explaining why this is a
> fundamental change, and we should probably wait until we have an omx
> registry implemented.


yeah, I'm starting to get your point about needing a registry..  it  
might be another month or so before I have much time to work on it,  
but I'd be happy to implement some sort of registry / config-file when  
I do have a bit more time.



>
>> 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
>
> I'm saying GOmxCore is independent of the omx implementation when you
> call _new(), it's tied to it *only* when you call _init(). In theory
> you can call _deinit() and _init() again, and re-use it for a
> different implementation. I'm not saying this is good, or desirable,
> just the way it is, and that if you want to change that, that should
> be a different patch.


Is this a desirable feature to anyone?  I'm not entirely sure how this  
fits in w/ a registry..

And in particular, I'm interested in being able to expose more  
settings of the OMX component thru properties (since that is nice for  
debug-ability in a lot of cases, and at least one of the new  
components I'm implementing will have a lot of configurability to  
expose to the application..

but I admit to be biased here because for the most part I'm only  
caring about a single OMX implementation ;-)


>
>>> 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.)
>
> Unless the config file format makes that impossible.
>
>>> 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 didn't mean "leak" as in memory leak. I meant as in silently tagged
> along to something completely different. If you need the _init() to
> happen sooner, then do a separate call to _init() sooner.
>
>>> 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.)
>
> Sure. I'm in the process of cleaning all the repos I've been working
> on for Maemo 5, among different machines and servers. I'm going to
> push my pending gst-openmax branches to github (personal repo) soon.
>
>> 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 ;-)
>
> Yeap, this is usually referred to as "feature branches", and it's not
> specific to git, but git makes to easy to work with them :)
>
> For example, in linux I have 'fc-cleanup', and 'fc-iommu-reorg' and
> these are completely independent, so I send the patch series for
> review independently too, and can be reviewed and merged in the same
> way. If 'fc-iommu-reorg' requires a try v5 that doesn't affect
> 'fc-cleanup' at all. Similarly, I start new branches based on the
> 'master' branch, so that they are independent of all the other patches
> I've done.
>
> I had a similar problem than you do when the 'maemo' branch in
> gst-openmax was huge, and it was not clear which patches should go
> into 'master', which into 'omap', which just dropped, and so on. But
> with a little bit of training on git commands such as chery-pick,
> rebase --interactive, and mergetool, it's even fun ;)

well, at least I'm learning a lot about git in the process ;-)


BR,
-R