[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
felipe.contreras at gmail.com
Wed Nov 4 06:31:04 PST 2009
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.
> 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.
> 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.
> {
> 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.
> + 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.
> 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?
> 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?
> /*
> * 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.
In general I try to answer these questions:
* What does the patch do?
* Why do we want it?
* What is the impact?
[1] http://www.kernel.org/pub/software/scm/git/docs/git-commit.html
--
Felipe Contreras
More information about the Gstreamer-openmax
mailing list