Re: [Gstreamer-openmax] [PATCH] add some utility macros to get/set core and port configs/params

[Clark, R. <ro...@ti...>]

On Mar 1, 2010, at 9:43 AM, Felipe Contreras wrote:

> On Sat, Feb 27, 2010 at 8:24 PM, Rob Clark <ro...@ti...> wrote:
>> btw, I'm thinking about this one again..
>> 
>> how about, in case of setting all params:
>> 
>>   OMX_AUDIO_PARAM_PCMMODETYPE param;
>> 
>>   G_OMX_INIT_PARAM (param);
> 
> I think it should be:
> 
> G_OMX_INIT_PARAM (OMX_AUDIO_PARAM_PCMMODETYPE, param);
> 
>>   ...  set params ...
>> 
>>   G_OMX_PORT_SET_PARAM (port, OMX_IndexParamAudioPcm, &param);
> 
> I don't see a big difference to:
> 
> param.nPortIndex = 1;
> OMX_SetParameter(omx_base->gomx->omx_handle, OMX_IndexParamAudioPcm, &param);
> 
>> or in case where you just want to set a few params:
>> 
>>   OMX_AUDIO_PARAM_PCMMODETYPE param;
> 
> You forgot the init I guess.


oh, btw, maybe it wasn't obvious, but current version of the patch combines the GET macro and INIT..

which is fine if you always follow GET/modify/SET pattern..  which seems like a reasonably good pattern to encourage.  In normal cases you aren't setting all parameters in one place.  Exceptions might be some param/config structs which only have one or two fields.

But it could be reasonable to to have an INIT function/macro as alternative to GET for those cases where GET/modify/SET really doesn't make sense.

I still like the idea of making it conceptually a method of the GOmxPort object  (ie. G_OMX_PORT_SET_PARAM(out_port, ...)) for port related params, since I think it makes it easier to read the code of the different gst-openmax element classes.


BR,
-R


> 
>>   G_OMX_PORT_GET_PARAM (port, OMX_IndexParamAudioPcm, &param);
>> 
>>   ...  set params ...
>> 
>>   G_OMX_PORT_SET_PARAM (port, OMX_IndexParamAudioPcm, &param);
> 
> Again, I don't see a big difference to:
> 
> param.nPortIndex = 1;
> OMX_GetParameter(omx_base->gomx->omx_handle, OMX_IndexParamAudioPcm, &param);
> 
> ... set params ...
> 
> OMX_SetParameter(omx_base->gomx->omx_handle, OMX_IndexParamAudioPcm, &param);
> 
> Cheers.
> 
> -- 
> Felipe Contreras