[Gstreamer-openmax] Proposal of new implementation

[giulio urlini]

Hi Felipe,
   your suggestions for code readability improvement are interesting,
and I'll take into account a rework of the framework in this
direction.
Some complications have a reason, like the use of DERIVEDCLASS that is
useful using doxygen, but I see your point here.

The support for interoperability is meant to be between OpenMAX
components of different vendors, and different loaders. So it implies
the possibility of the coexistence between components produced by
different companies. I must be pragmatic in this case, and I have to
admit that in the real world I don't see examples of interoperant
products, so the support for it is not a high priority.

I have to investigate the actual problems of interaction between
gst-omx and Bellagio, because there are no obvious reasons because it
does not work any more. I'll keep you updated on this list.

Best Regards,

Giulio


On Wed, Jan 26, 2011 at 18:47, Felipe Contreras
<felipe.contreras at gmail.com> wrote:
> Hi Giulio,
>
> On Wed, Jan 26, 2011 at 5:40 PM, giulio urlini <giulio.urlini at gmail.com> wrote:
>>   my two cents from Bellagio, with an opinion of pros and cons of
>> Bellagio framework.
>>
>> The Bellagio framework has been build so complicated in order to cover
>> all the definition and use cases of the standard, trying to reflect
>> the general opinion of the OpenMAX Khronos group about the behavior of
>> core and components in any use case. It covers also all the spec, with
>> a full interoperability support.
>
> Sure, but you can achieve the same goals with simpler code.
>
> For example:
>
> DERIVEDCLASS(omx_base_filter_PrivateType, omx_base_component_PrivateType)
> #define omx_base_filter_PrivateType_FIELDS
> omx_base_component_PrivateType_FIELDS \
>  /** @param pPendingOutputBuffer pending Output Buffer pointer */ \
>  OMX_BUFFERHEADERTYPE* pPendingOutputBuffer; \
>  /** @param BufferMgmtCallback function pointer for algorithm callback */ \
>  void (*BufferMgmtCallback)(OMX_COMPONENTTYPE* openmaxStandComp,
> OMX_BUFFERHEADERTYPE* inputbuffer, OMX_BUFFERHEADERTYPE*
> outputbuffer);
> ENDCLASS(omx_base_filter_PrivateType)
>
> This could be simplified a lot by doing something like:
>
> struct omx_base_filter {
>  struct omx_base_component parent;
>  OMX_BUFFERHEADERTYPE *pending_buffer;
>  ...
> };
>
> In fact, rather than having these base classes, it would probably be
> more useful if the functionality of these bases classes was provided
> to the components as utilities, that they might decide to use or not.
>
> Not to mention that the code is overly verbose:
>        omx_base_component_PrivateType* omx_base_component_Private;
>        omx_base_component_Private =
> (omx_base_component_PrivateType*)openmaxStandComp->pComponentPrivate;
>
> Could be easily simplified to:
>        omx_base_component_PrivateType *base;
>        base = standard->pComponentPrivate;
>
> Or here:
>        omx_base_component_Private->state = OMX_StateLoaded;
>        omx_base_component_Private->transientState = OMX_TransStateMax;
>        omx_base_component_Private->callbacks = NULL;
>        omx_base_component_Private->callbackData = NULL;
>        omx_base_component_Private->nGroupPriority = 100;
>        omx_base_component_Private->nGroupID = 0;
>        omx_base_component_Private->pMark.hMarkTargetComponent = NULL;
>
> s/omx_base_component_Private/base/
>
> Theres many many ways in which the code can be simplified, made easier
> to read, and more maintainable.
>
> Also, I'm not sure about the "full interoperability" support. I'm
> pretty sure if an implementation is compliant, it should work with
> gst-openmax without changes. Bellagio did at some point, and then it
> suddenly stopped. It might be the the API remains compatible, but the
> data streams are not, to me that's a break all the same.
>
>> Finally it has a support for multiple loaders mechanism, necessary to
>> allow the necessary flexibility since this part is not covered
>> intentionally by the standard.
>
> Well, I don't think it's necessary. If you need another loader, you
> use another omx implementation. What is there in an omx core other
> than the loader anyway?
>
> I have experimented with this, it's possible to load TI components
> (which don't use bellagio base clases) with bellagio loader. And it's
> also possible to load bellagio components into TI's omx core. At the
> end of the day the only difference of these implementations are the
> loaders.
>
>> For sure I'll take a look at your implementation, but my opinion is
>> that the spec itself, if considered entirely is complicated, and a
>> "reference" implementation would be complicated as well.
>
> Sure, if your objective is to implement as most as the standard as possible.
>
> I would prefer to have a trampoline implementation, which people can
> use to develop their own components. And for that purpose simplicity
> is key.
>
>> Actually in Linaro Bellagio has been recognized as the reference for
>> standard OpenMAX support, so probably concentrating the efforts on it
>> and on the improvement of the interaction with gst-omx can be useful
>> for everyone.
>>
>> https://wiki.linaro.org/WorkingGroups/Middleware/Multimedia/Specs/1105/StandardizeBellagioOmxCore
>>
>> We are totally open to it, and we are increasing the effort spent on
>> this project.
>
> Maybe. It would certainly be nice to get bellagio working on
> gst-openmax again, but I'm not going to do that any more. I think it's
> a better use of my time to implement ffmpeg wrappers for libomxil-g,
> and make it more extensible.
>
> Cheers.
>
> --
> Felipe Contreras
>