Re: [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

[Felipe C. <fel...@gm...>]

On Fri, Nov 6, 2009 at 4:58 AM, Rob Clark <ro...@ti...> 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.

> 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.

>> 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 ;)

Cheers.

-- 
Felipe Contreras