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

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

On Mar 3, 2010, at 5:07 PM, Felipe Contreras wrote:

> On Tue, Mar 2, 2010 at 7:29 PM, Clark, Rob <ro...@ti...> wrote:
>> 
>> 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..
> 
> 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.


I was leaning towards the approach of providing an INIT and a GET+INIT macro.  But I could go either way with this.. I'm ok w/ separate INIT, GET, and SET APIs.

I'm more interested in having port versions which sets nPortIndex automatically..  but as you mention below, due to the silly way the OMX structs are defined (with copy/paste rather than inheritance), we can't really safely[1] do it without a macro :-(

that said, it is a pretty simple macro.  Especially if we remove the INIT part of the macro.


BR,
-R

---
[1] I did have a look thru all the standard config/param structs.. they *do* all have the same layout, so in theory we could implement as a function, and just cast the param/config void ptr to a OMX_PARAM_PORTDEFINITIONTYPE ptr and initialize nPortIndex..  but that isn't really an approach that helps me sleep well at night.  All it takes is for someone to define a custom config/param struct which doesn't follow the standard layout, and you get silent badness.  Whereas with the macro, you would catch the problem at compile time.


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