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

[Felipe Contreras]

On Tue, Mar 2, 2010 at 7:29 PM, Clark, Rob <rob at ti.com> wrote:
>
> On Mar 1, 2010, at 9:43 AM, Felipe Contreras wrote:
>
>> On Sat, Feb 27, 2010 at 8:24 PM, Rob Clark <rob at ti.com> 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..

Oh, well I don't like that approach.

> 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.

I'm not a fan of patronizing users. And as you said; some structs have
one or two fields, and it's perfectly fine not to call Get in those
cases.

> 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 don't like the idea of hiding things under the carpet. If the code
is doing: init ->get -> set, then that's what the code should look
like.

> 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.

I'm ok with a function that does this, but not with a macro, and I
don't think there's a way to do that.

Cheers.

-- 
Felipe Contreras