[Gstreamer-openmax] [PATCH 01/10] construct GOmxPort objects in element constructor

[Felipe Contreras]

On Mon, Mar 08, 2010 at 12:14:48AM +0100, Rob Clark wrote:
> The port objects are constructed up front (so they are valid at any point in the element's lifecycle), and bound to a port_index at construction time.
> 
> This will be useful later to enable properties that directly set port params.

It's not clear to me what this patch is doing, and why.

> ---
>  omx/gstomx_base_filter.c |   13 +++++------
>  omx/gstomx_base_sink.c   |   10 +++-----
>  omx/gstomx_base_src.c    |   10 +++-----
>  omx/gstomx_util.c        |   47 +++++++++++++++++++--------------------------
>  omx/gstomx_util.h        |    4 +-
>  5 files changed, 36 insertions(+), 48 deletions(-)
> 
> diff --git a/omx/gstomx_base_filter.c b/omx/gstomx_base_filter.c
> index 44841c3..ddb480f 100644
> --- a/omx/gstomx_base_filter.c
> +++ b/omx/gstomx_base_filter.c
> @@ -64,14 +64,14 @@ setup_ports (GstOmxBaseFilter *self)
>  
>      param.nPortIndex = 0;
>      OMX_GetParameter (core->omx_handle, OMX_IndexParamPortDefinition, &param);
> -    self->in_port = g_omx_core_setup_port (core, &param);
> +    g_omx_port_setup (self->in_port, &param);
>      gst_pad_set_element_private (self->sinkpad, self->in_port);
>  
>      /* Output port configuration. */
>  
>      param.nPortIndex = 1;
>      OMX_GetParameter (core->omx_handle, OMX_IndexParamPortDefinition, &param);
> -    self->out_port = g_omx_core_setup_port (core, &param);
> +    g_omx_port_setup (self->out_port, &param);
>      gst_pad_set_element_private (self->srcpad, self->out_port);
>  
>      if (g_getenv ("OMX_ALLOCATE_ON"))
> @@ -877,11 +877,10 @@ type_instance_init (GTypeInstance *instance,
>      self->use_timestamps = TRUE;
>  
>      /* GOmx */
> -    {
> -        GOmxCore *gomx;
> -        self->gomx = gomx = g_omx_core_new (self);
> -        gstomx_get_component_info (gomx, G_TYPE_FROM_CLASS (g_class));
> -    }
> +    self->gomx = g_omx_core_new (self);
> +    gstomx_get_component_info (self->gomx, G_TYPE_FROM_CLASS (g_class));
> +    self->in_port = g_omx_core_get_port (self->gomx, 0);
> +    self->out_port = g_omx_core_get_port (self->gomx, 1);

This code reorganization was not advertised, and it makes the actual
change more difficult to see.

>  
>      self->ready_lock = g_mutex_new ();
>  
> diff --git a/omx/gstomx_base_sink.c b/omx/gstomx_base_sink.c
> index 6838f08..1c7888c 100644
> --- a/omx/gstomx_base_sink.c
> +++ b/omx/gstomx_base_sink.c
> @@ -52,7 +52,7 @@ setup_ports (GstOmxBaseSink *self)
>  
>      param.nPortIndex = 0;
>      OMX_GetParameter (core->omx_handle, OMX_IndexParamPortDefinition, &param);
> -    self->in_port = g_omx_core_setup_port (core, &param);
> +    g_omx_port_setup (self->in_port, &param);
>      gst_pad_set_element_private (self->sinkpad, self->in_port);
>  }
>  
> @@ -417,11 +417,9 @@ type_instance_init (GTypeInstance *instance,
>      GST_LOG_OBJECT (self, "begin");
>  
>      /* GOmx */
> -    {
> -        GOmxCore *gomx;
> -        self->gomx = gomx = g_omx_core_new (self);
> -        gstomx_get_component_info (gomx, G_TYPE_FROM_CLASS (g_class));
> -    }
> +    self->gomx = g_omx_core_new (self);
> +    gstomx_get_component_info (self->gomx, G_TYPE_FROM_CLASS (g_class));
> +    self->in_port = g_omx_core_get_port (self->gomx, 0);

Unneeded reorganization.

>  
>      {
>          GstPad *sinkpad;
> diff --git a/omx/gstomx_base_src.c b/omx/gstomx_base_src.c
> index dd2be9e..a19c08f 100644
> --- a/omx/gstomx_base_src.c
> +++ b/omx/gstomx_base_src.c
> @@ -43,7 +43,7 @@ setup_ports (GstOmxBaseSrc *self)
>  
>      param.nPortIndex = 0;
>      OMX_GetParameter (core->omx_handle, OMX_IndexParamPortDefinition, &param);
> -    self->out_port = g_omx_core_setup_port (core, &param);
> +    g_omx_port_setup (self->out_port, &param);
>  
>      if (self->setup_ports)
>      {
> @@ -402,11 +402,9 @@ type_instance_init (GTypeInstance *instance,
>      GST_LOG_OBJECT (self, "begin");
>  
>      /* GOmx */
> -    {
> -        GOmxCore *gomx;
> -        self->gomx = gomx = g_omx_core_new (self);
> -        gstomx_get_component_info (gomx, G_TYPE_FROM_CLASS (g_class));
> -    }
> +    self->gomx = g_omx_core_new (self);
> +    gstomx_get_component_info (self->gomx, G_TYPE_FROM_CLASS (g_class));
> +    self->out_port = g_omx_core_get_port (self->gomx, 1);

Unneeded reorganization.

>  
>      GST_LOG_OBJECT (self, "end");
>  }
> diff --git a/omx/gstomx_util.c b/omx/gstomx_util.c
> index b2a96c8..d8cc7f0 100644
> --- a/omx/gstomx_util.c
> +++ b/omx/gstomx_util.c
> @@ -76,9 +76,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);
>  
>  static inline void
>  port_free_buffers (GOmxPort *port);
> @@ -133,7 +131,7 @@ core_for_each_port (GOmxCore *core,
>      {
>          GOmxPort *port;
>  
> -        port = g_omx_core_get_port (core, index);
> +        port = get_port (core, index);

g_omx_core_get_port() != get_port()

So this theoretically is changing the behavior, and that wasn't
mentioned in the commit message.

I think moving from g_omx_core_get_port() to get_port() is a good idea,
but perhaps should be a separate patch.

>  
>          if (port)
>              func (port);
> @@ -396,37 +394,30 @@ 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 *
> +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);

You are changing the semantics of the function; now it's not getting a
port; it's creating a port. It's more like g_omx_core_new_port().

-- 
Felipe Contreras