[Gstreamer-openmax] [PATCH 01/10] construct GOmxPort objects in element constructor
Rob Clark
rob at ti.com
Mon Mar 8 08:32:05 PST 2010
On Mar 8, 2010, at 9:57 AM, Felipe Contreras wrote:
> 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.
Basically it is creating port in element constructor, instead of later at setup_ports() time..
The internal function within gstomx_util.c used to access ports (g_omx_core_get_port()) is renamed to get_port() because it is not public API. A new g_omx_core_get_port() which constructs the port if it has not already been constructed is created. (So maybe more correctly it would be g_omx_core_get_port_or_construct_new_port_if_it_does_not_exist() ;-))
But since this is only used from type_instance_init() fxns, it could be replaced by g_omx_core_new() which does not check if the port already exists yet or not and is simply a constructor function.
BR,
-R
>
>> ---
>> 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, ¶m);
>> - self->in_port = g_omx_core_setup_port (core, ¶m);
>> + g_omx_port_setup (self->in_port, ¶m);
>> gst_pad_set_element_private (self->sinkpad, self->in_port);
>>
>> /* Output port configuration. */
>>
>> param.nPortIndex = 1;
>> OMX_GetParameter (core->omx_handle, OMX_IndexParamPortDefinition, ¶m);
>> - self->out_port = g_omx_core_setup_port (core, ¶m);
>> + g_omx_port_setup (self->out_port, ¶m);
>> 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, ¶m);
>> - self->in_port = g_omx_core_setup_port (core, ¶m);
>> + g_omx_port_setup (self->in_port, ¶m);
>> 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, ¶m);
>> - self->out_port = g_omx_core_setup_port (core, ¶m);
>> + g_omx_port_setup (self->out_port, ¶m);
>>
>> 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
More information about the Gstreamer-openmax
mailing list