[Mesa-dev] [PATCHv2 1/3] omx: query driver encode capatibility earlier

[Christian König]

Am 16.02.2014 13:53, schrieb Emil Velikov:
> On 16/02/14 10:20, Christian König wrote:
>> Am 15.02.2014 20:35, schrieb Emil Velikov:
>>> Query of the driver support AVC_HIGH encode before reporting
>>> the total number of supported components.
>>>
>>> v2: drop if (name_specific_length) check
>> I considered this as well, but as far as I understand it that's not
>> necessary correct.
>>
> Hi Christian
>
> I'm not sure I see your concern.
>
> If you consider this an individual project one will not implement nor
> advertise an encoder (second component) if they know it is not going to
> work (would that be missing hardware capability or other reason). Same
> thing here - there is no point in saying "we have two components" only
> to die later after realising that is not the case.

As far as I understood it the number of components and the name of them 
should be statically and never change without rebulding the component 
registry. Actually that we concatenate the component name and have 
dynamically specific names/roles is already quite questionable

See for example the ".omxregister" file that the bellagio loader builds 
up while loading the library. Not sure if it was bellagios caching or 
that of gstreamer, but while working on the code I had to clear and 
rebuild them a couple of times to make it work.

> I suspect that this may even help with the following bug
> https://bugs.freedesktop.org/show_bug.cgi?id=74944

Actually to fix this bug we shouldn't check at all while being queried 
for available components, but rather fail while creating them. To be 
honest I don't have much experience with applications using this 
interface (except for gstreamer), so I can't really judge what's the 
right approach here.

Anyway, the proper fix for this bug is probably to use the pipe loader 
instead.

>> Have you checked the spec on how exactly we should do it?
>>
> I have skimmed through the openmax IL 1.1.2 spec and it does not seem to
> cover this topic. Or perhaps I should be looking at the
> libomxil-bellagio documentation ?

I couldn't find anything clear on this topic either.

Christian.

> Updated patches with improved commit message/comments should clear any
> confusion caused.
>
> http://patchwork.freedesktop.org/patch/20396/
> http://patchwork.freedesktop.org/patch/20397/
> http://patchwork.freedesktop.org/patch/20398/
>
> -Emil
>> Christian.
>>
>>> Signed-off-by: Emil Velikov <emil.l.velikov at gmail.com>
>>> ---
>>>    src/gallium/state_trackers/omx/entrypoint.c | 17 +++++++++++++++--
>>>    src/gallium/state_trackers/omx/vid_enc.c    | 25
>>> +++++++++----------------
>>>    2 files changed, 24 insertions(+), 18 deletions(-)
>>>
>>> diff --git a/src/gallium/state_trackers/omx/entrypoint.c
>>> b/src/gallium/state_trackers/omx/entrypoint.c
>>> index 52b2495..fa654ba 100644
>>> --- a/src/gallium/state_trackers/omx/entrypoint.c
>>> +++ b/src/gallium/state_trackers/omx/entrypoint.c
>>> @@ -50,10 +50,23 @@ static unsigned omx_usecount = 0;
>>>      int omx_component_library_Setup(stLoaderComponentType **stComponents)
>>>    {
>>> +   struct vl_screen *vscreen = omx_get_screen();
>>> +   struct pipe_screen *screen = vscreen ? vscreen->pscreen : NULL;
>>>       OMX_ERRORTYPE r;
>>> +   int num_components = 1;
>>> +
>>> +   /*
>>> +    * Increment the number of components if the driver supports
>>> +    * PIPE_VIDEO_PROFILE_MPEG4_AVC_HIGH encoding.
>>> +    */
>>> +   if (screen->get_video_param(screen,
>>> PIPE_VIDEO_PROFILE_MPEG4_AVC_HIGH,
>>> +                               PIPE_VIDEO_ENTRYPOINT_ENCODE,
>>> +                               PIPE_VIDEO_CAP_SUPPORTED) {
>>> +      num_components++;
>>> +   }
>>>         if (stComponents == NULL)
>>> -      return 2;
>>> +      return num_components;
>>>         /* component 0 - video decoder */
>>>       r = vid_dec_LoaderComponent(stComponents[0]);
>>> @@ -65,7 +78,7 @@ int
>>> omx_component_library_Setup(stLoaderComponentType **stComponents)
>>>       if (r != OMX_ErrorNone)
>>>          return r;
>>>    -   return 2;
>>> +   return num_components;
>>>    }
>>>      struct vl_screen *omx_get_screen(void)
>>> diff --git a/src/gallium/state_trackers/omx/vid_enc.c
>>> b/src/gallium/state_trackers/omx/vid_enc.c
>>> index 6e65274..564f5b6 100644
>>> --- a/src/gallium/state_trackers/omx/vid_enc.c
>>> +++ b/src/gallium/state_trackers/omx/vid_enc.c
>>> @@ -106,12 +106,7 @@ OMX_ERRORTYPE
>>> vid_enc_LoaderComponent(stLoaderComponentType *comp)
>>>       comp->componentVersion.s.nRevision = 0;
>>>       comp->componentVersion.s.nStep = 1;
>>>       comp->constructor = vid_enc_Constructor;
>>> -
>>> -   if (screen->get_video_param(screen,
>>> PIPE_VIDEO_PROFILE_MPEG4_AVC_HIGH,
>>> -                   PIPE_VIDEO_ENTRYPOINT_ENCODE,
>>> PIPE_VIDEO_CAP_SUPPORTED))
>>> -      comp->name_specific_length = 1;
>>> -   else
>>> -      comp->name_specific_length = 0;
>>> +   comp->name_specific_length = 1;
>>>         omx_put_screen();
>>>    @@ -129,19 +124,17 @@ OMX_ERRORTYPE
>>> vid_enc_LoaderComponent(stLoaderComponentType *comp)
>>>       if (!comp->role_specific)
>>>          goto error_arrays;
>>>    -   if (comp->name_specific_length) {
>>> -      comp->name_specific[0] = CALLOC(1, OMX_MAX_STRINGNAME_SIZE);
>>> -      if (!comp->name_specific[0])
>>> -         goto error_specific;
>>> +   comp->name_specific[0] = CALLOC(1, OMX_MAX_STRINGNAME_SIZE);
>>> +   if (!comp->name_specific[0])
>>> +      goto error_specific;
>>>    -      vid_enc_name_avc(comp->name_specific[0]);
>>> +   vid_enc_name_avc(comp->name_specific[0]);
>>>    -      comp->role_specific[0] = CALLOC(1, OMX_MAX_STRINGNAME_SIZE);
>>> -      if (!comp->role_specific[0])
>>> -         goto error_specific;
>>> +   comp->role_specific[0] = CALLOC(1, OMX_MAX_STRINGNAME_SIZE);
>>> +   if (!comp->role_specific[0])
>>> +      goto error_specific;
>>>    -      strcpy(comp->role_specific[0], OMX_VID_ENC_AVC_ROLE);
>>> -   }
>>> +   strcpy(comp->role_specific[0], OMX_VID_ENC_AVC_ROLE);
>>>         return OMX_ErrorNone;
>>>