[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 Contreras]

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.

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

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

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

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

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

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

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

Cheers.

-- 
Felipe Contreras