[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
rob at ti.com
Wed Nov 4 07:17:15 PST 2009
Hi Felipe,
On Nov 4, 2009, at 8:31 AM, Felipe Contreras wrote:
> Hi,
>
> The subject is too long. See the 'discussion' section in 'man git-
> commit'[1].
>
> On Wed, Nov 4, 2009 at 4:25 AM, Rob Clark <rob at ti.com> wrote:
>>
>> Signed-off-by: Rob Clark <rob at ti.com>
>> ---
>> omx/gstomx_base_filter.c | 24 ++------
>> omx/gstomx_base_sink.c | 21 ++------
>> omx/gstomx_base_src.c | 22 ++------
>> omx/gstomx_util.c | 133 ++++++++++++++++++++++++++++++++++
>> +-----------
>> omx/gstomx_util.h | 12 +++--
>> 5 files changed, 123 insertions(+), 89 deletions(-)
>
> [...]
>
>> diff --git a/omx/gstomx_util.c b/omx/gstomx_util.c
>> index 39d900b..1cd224f 100644
>> --- a/omx/gstomx_util.c
>> +++ b/omx/gstomx_util.c
>> @@ -22,6 +22,7 @@
>>
>> #include "gstomx_util.h"
>> #include <dlfcn.h>
>> +#include <string.h>
>>
>> #include "gstomx.h"
>>
>> @@ -76,9 +77,7 @@ omx_state_to_str (OMX_STATETYPE omx_state);
>> static inline const char *
>> omx_error_to_str (OMX_ERRORTYPE omx_error);
>>
>> -static inline GOmxPort *
>> -g_omx_core_get_port (GOmxCore *core,
>> - guint index);
>> +static inline GOmxPort *get_port (GOmxCore *core, guint index);
>
> 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.
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.
>
>> static inline void
>> port_free_buffers (GOmxPort *port);
>> @@ -133,7 +132,7 @@ core_for_each_port (GOmxCore *core,
>> {
>> GOmxPort *port;
>>
>> - port = g_omx_core_get_port (core, index);
>> + port = get_port (core, index);
>
> ditto.
>
>> if (port)
>> func (port);
>> @@ -159,6 +158,7 @@ imp_new (const gchar *name)
>> void *handle;
>>
>> imp->dl_handle = handle = dlopen (name, RTLD_LAZY);
>> + GST_DEBUG ("dlopen(%s) -> %p", name, handle);
>
> 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)
>
>> if (!handle)
>> {
>> g_warning ("%s\n", dlerror ());
>> @@ -264,13 +264,22 @@ g_omx_deinit (void)
>> * Core
>> */
>>
>> +/**
>> + * Construct new core
>> + *
>> + * @object: the GstOmx object (ie. GstOmxBaseFilter,
>> GstOmxBaseSrc, or
>> + * GstOmxBaseSink). The GstOmx object should have "component-
>> name"
>> + * and "library-name" properties.
>> + */
>
> Also doing more than advertised.
>
>> GOmxCore *
>> -g_omx_core_new (void)
>> +g_omx_core_new (gpointer object, gpointer klass)
>
> 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?)
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.
>
>> {
>> GOmxCore *core;
>>
>> core = g_new0 (GOmxCore, 1);
>>
>> + core->object = object;
>> +
>> core->ports = g_ptr_array_new ();
>>
>> core->omx_state_condition = g_cond_new ();
>> @@ -282,12 +291,33 @@ g_omx_core_new (void)
>>
>> core->omx_state = OMX_StateInvalid;
>>
>> + {
>> + gchar *library_name, *component_name, *component_role;
>> +
>> + library_name = g_type_get_qdata (G_OBJECT_CLASS_TYPE
>> (klass),
>> + g_quark_from_static_string ("library-name"));
>> +
>> + component_name = g_type_get_qdata (G_OBJECT_CLASS_TYPE
>> (klass),
>> + g_quark_from_static_string ("component-name"));
>> +
>> + component_role = g_type_get_qdata (G_OBJECT_CLASS_TYPE
>> (klass),
>> + g_quark_from_static_string ("component-role"));
>
> "component-role"? Again, that's a completely independent change.
[RC] yeah, that is another one that was supposed to be part of a later
commit but slipped through.. my bad
>
>> + g_object_set (core->object,
>> + "component-role", component_role,
>> + "component-name", component_name,
>> + "library-name", library_name,
>> + NULL);
>> + }
>> +
>> return core;
>> }
>>
>> void
>> g_omx_core_free (GOmxCore *core)
>> {
>> + g_omx_core_deinit (core); /* just in case we didn't have a
>> READY->NULL.. mainly for gst-inspect */
>
> That comment seems unnecessary; if that's the way it is, then that's
> the way it is. But I'm worried about what would happen with multiple
> calls to core_deinit.
[RC] yeah, I was running into a problem w/ gst-inspect otherwise. My
solution was to just make it ok to call g_omx_core_deinit() multiple
times.
>
>> g_sem_free (core->port_sem);
>> g_sem_free (core->flush_sem);
>> g_sem_free (core->done_sem);
>> @@ -301,10 +331,23 @@ g_omx_core_free (GOmxCore *core)
>> }
>>
>> void
>> -g_omx_core_init (GOmxCore *core,
>> - const gchar *library_name,
>> - const gchar *component_name)
>> +g_omx_core_init (GOmxCore *core)
>> {
>> + gchar *library_name=NULL, *component_name=NULL;
>> +
>> + if (core->omx_handle)
>> + return;
>> +
>> + GST_DEBUG_OBJECT (core->object, "loading: %s (%s)",
>> component_name, library_name);
>
> Unrelated change.
>
>> + g_object_get (core->object,
>> + "component-name", &component_name,
>> + "library-name", &library_name,
>> + NULL);
>> +
>> + 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.
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.
>
>> core->imp = request_imp (library_name);
>>
>> if (!core->imp)
>> @@ -314,8 +357,15 @@ g_omx_core_init (GOmxCore *core,
>> (char *)
>> component_name,
>> core,
>> &callbacks);
>> +
>> + GST_DEBUG_OBJECT (core->object, "OMX_GetHandle(&%p) -> %d",
>> + core->omx_handle, core->omx_error);
>
> Unrelated.
>
>> if (!core->omx_error)
>> core->omx_state = OMX_StateLoaded;
>> +
>> + g_free (component_name);
>> + g_free (library_name);
>> }
>>
>> void
>> @@ -328,7 +378,12 @@ g_omx_core_deinit (GOmxCore *core)
>> core->omx_state == OMX_StateInvalid)
>> {
>> if (core->omx_handle)
>> + {
>> core->omx_error = core->imp->sym_table.free_handle (core-
>> >omx_handle);
>> + GST_DEBUG_OBJECT (core->object, "OMX_FreeHandle(%p) ->
>> %d",
>> + core->omx_handle, core->omx_error);
>
> Unrelated.
>
>> + core->omx_handle = NULL;
>
> This one seems to be a fix... still unrelated.
>
>> + }
>> }
>>
>> release_imp (core->imp);
>> @@ -394,37 +449,29 @@ g_omx_core_unload (GOmxCore *core)
>> g_ptr_array_clear (core->ports);
>> }
>>
>> -GOmxPort *
>> -g_omx_core_setup_port (GOmxCore *core,
>> - OMX_PARAM_PORTDEFINITIONTYPE *omx_port)
>> +static inline GOmxPort *
>> +get_port (GOmxCore *core, guint index)
>> {
>> - GOmxPort *port;
>> - guint index;
>> -
>> - index = omx_port->nPortIndex;
>> - port = g_omx_core_get_port (core, index);
>> -
>> - if (!port)
>> + if (G_LIKELY (index < core->ports->len))
>> {
>> - port = g_omx_port_new (core);
>> - g_ptr_array_insert (core->ports, index, port);
>> + return g_ptr_array_index (core->ports, index);
>> }
>>
>> - g_omx_port_setup (port, omx_port);
>> -
>> - return port;
>> + return NULL;
>> }
>>
>> -static inline GOmxPort *
>> -g_omx_core_get_port (GOmxCore *core,
>> - guint index)
>> +GOmxPort *
>> +g_omx_core_get_port (GOmxCore *core, guint index)
>> {
>> - if (G_LIKELY (index < core->ports->len))
>> + GOmxPort *port = get_port (core, index);
>> +
>> + if (!port)
>> {
>> - return g_ptr_array_index (core->ports, index);
>> + port = g_omx_port_new (core, index);
>> + g_ptr_array_insert (core->ports, index, port);
>> }
>>
>> - return NULL;
>> + return port;
>> }
>>
>> void
>> @@ -452,17 +499,31 @@ g_omx_core_flush_stop (GOmxCore *core)
>> core_for_each_port (core, g_omx_port_resume);
>> }
>>
>> +/**
>> + * 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
>
>> /*
>> * Port
>> */
>>
>> GOmxPort *
>> -g_omx_port_new (GOmxCore *core)
>> +g_omx_port_new (GOmxCore *core, guint index)
>> {
>> GOmxPort *port;
>> port = g_new0 (GOmxPort, 1);
>>
>> port->core = core;
>> + port->port_index = index;
>> port->num_buffers = 0;
>> port->buffer_size = 0;
>> port->buffers = NULL;
>> @@ -508,6 +569,10 @@ g_omx_port_setup (GOmxPort *port,
>> port->buffer_size = omx_port->nBufferSize;
>> port->port_index = omx_port->nPortIndex;
>>
>> + GST_DEBUG_OBJECT (port->core->object,
>> + "type=%d, num_buffers=%d, buffer_size=%d, port_index=%d",
>> + port->type, port->num_buffers, port->buffer_size, port-
>> >port_index);
>
> Unrelated.
>
>> g_free (port->buffers);
>> port->buffers = g_new0 (OMX_BUFFERHEADERTYPE *, port-
>> >num_buffers);
>> }
>> @@ -840,6 +905,8 @@ EventHandler (OMX_HANDLETYPE omx_handle,
>>
>> cmd = (OMX_COMMANDTYPE) data_1;
>>
>> + GST_DEBUG_OBJECT (core->object,
>> "OMX_EventCmdComplete: %d", cmd);
>
> Unrelated.
>
>> switch (cmd)
>> {
>> case OMX_CommandStateSet:
>> @@ -858,6 +925,7 @@ EventHandler (OMX_HANDLETYPE omx_handle,
>> }
>> case OMX_EventBufferFlag:
>> {
>> + GST_DEBUG_OBJECT (core->object,
>> "OMX_EventBufferFlag");
>
> I think you got the point.
>
>> if (data_2 & OMX_BUFFERFLAG_EOS)
>> {
>> g_omx_core_set_done (core);
>> @@ -866,6 +934,7 @@ EventHandler (OMX_HANDLETYPE omx_handle,
>> }
>> case OMX_EventPortSettingsChanged:
>> {
>> + GST_DEBUG_OBJECT (core->object,
>> "OMX_EventPortSettingsChanged");
>> /** @todo only on the relevant port. */
>> if (core->settings_changed_cb)
>> {
>> @@ -902,7 +971,7 @@ EmptyBufferDone (OMX_HANDLETYPE omx_handle,
>> GOmxPort *port;
>>
>> core = (GOmxCore*) app_data;
>> - port = g_omx_core_get_port (core, omx_buffer->nInputPortIndex);
>> + port = get_port (core, omx_buffer->nInputPortIndex);
>>
>> GST_CAT_LOG_OBJECT (gstomx_util_debug, core->object, "omx_buffer=
>> %p", omx_buffer);
>> got_buffer (core, port, omx_buffer);
>> @@ -919,7 +988,7 @@ FillBufferDone (OMX_HANDLETYPE omx_handle,
>> GOmxPort *port;
>>
>> core = (GOmxCore *) app_data;
>> - port = g_omx_core_get_port (core, omx_buffer->nOutputPortIndex);
>> + port = get_port (core, omx_buffer->nOutputPortIndex);
>>
>> GST_CAT_LOG_OBJECT (gstomx_util_debug, core->object, "omx_buffer=
>> %p", omx_buffer);
>> got_buffer (core, port, omx_buffer);
>> diff --git a/omx/gstomx_util.h b/omx/gstomx_util.h
>> index f0cf045..4857265 100644
>> --- a/omx/gstomx_util.h
>> +++ b/omx/gstomx_util.h
>> @@ -114,9 +114,9 @@ struct GOmxPort
>> void g_omx_init (void);
>> void g_omx_deinit (void);
>>
>> -GOmxCore *g_omx_core_new (void);
>> +GOmxCore *g_omx_core_new (gpointer object, gpointer klass);
>> void g_omx_core_free (GOmxCore *core);
>> -void g_omx_core_init (GOmxCore *core, const gchar *library_name,
>> const gchar *component_name);
>> +void g_omx_core_init (GOmxCore *core);
>> void g_omx_core_deinit (GOmxCore *core);
>> void g_omx_core_prepare (GOmxCore *core);
>> void g_omx_core_start (GOmxCore *core);
>> @@ -127,9 +127,11 @@ void g_omx_core_set_done (GOmxCore *core);
>> void g_omx_core_wait_for_done (GOmxCore *core);
>> void g_omx_core_flush_start (GOmxCore *core);
>> void g_omx_core_flush_stop (GOmxCore *core);
>> -GOmxPort *g_omx_core_setup_port (GOmxCore *core,
>> OMX_PARAM_PORTDEFINITIONTYPE *omx_port);
>> +OMX_HANDLETYPE g_omx_core_get_handle (GOmxCore *core);
>> +GOmxPort *g_omx_core_get_port (GOmxCore *core, guint index);
>>
>> -GOmxPort *g_omx_port_new (GOmxCore *core);
>> +
>> +GOmxPort *g_omx_port_new (GOmxCore *core, guint index);
>> void g_omx_port_free (GOmxPort *port);
>> void g_omx_port_setup (GOmxPort *port, OMX_PARAM_PORTDEFINITIONTYPE
>> *omx_port);
>> void g_omx_port_push_buffer (GOmxPort *port, OMX_BUFFERHEADERTYPE
>> *omx_buffer);
>> @@ -142,4 +144,6 @@ void g_omx_port_enable (GOmxPort *port);
>> void g_omx_port_disable (GOmxPort *port);
>> void g_omx_port_finish (GOmxPort *port);
>>
>> +
>> +
>
> Definitely not needed.
>
>> #endif /* GSTOMX_UTIL_H */
>> --
>> 1.6.3.2
>
> 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.
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
> 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.
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)
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.
BR,
-R
>
> [1] http://www.kernel.org/pub/software/scm/git/docs/git-commit.html
>
> --
> Felipe Contreras
>
More information about the Gstreamer-openmax
mailing list