[Mesa-dev] [PATCH 4/4] st/omx/dec: make decoder video buffer progressive

[Emil Velikov]

On 7 July 2016 at 18:16, Leo Liu <leo.liu at amd.com> wrote:
> Hi Emil,
>
> Have a look again, I think the logic is nothing wrong.
>
I did not mention that it's doing something "wrong" :-)

> reason In lines
>
>
> On 07/07/2016 11:39 AM, Emil Velikov wrote:
>>
>> On 6 July 2016 at 19:03, Leo Liu <leo.liu at amd.com> wrote:
>>>
>>> The idea of encode tunneling is to use video buffer directly for encoder,
>>> but currently the encoder doesn’t support interlaced surface, the OMX
>>> decoder set progressive surface before on that purpose.
>>>
>>> Since now we are polling the driver for interlacing information for
>>> decoder, we got the interlaced as preferred as other APIs(VDPAU, VA-API),
>>> thus breaking the transcode with tunneling.
>>>
>>> The solution is when with tunnel detected, re-allocate progressive target
>>> buffers, and then converting the interlaced decoder results to there.
>>>
>>> This has been tested with transcode results bit to bit matching as before
>>> with surface from progressive to progressive.
>>>
>>> Signed-off-by: Leo Liu <leo.liu at amd.com>
>>> ---
>>>   src/gallium/state_trackers/omx/vid_dec.c | 65
>>> +++++++++++++++++++++++++++++++-
>>>   src/gallium/state_trackers/omx/vid_dec.h |  6 ++-
>>>   2 files changed, 68 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/src/gallium/state_trackers/omx/vid_dec.c
>>> b/src/gallium/state_trackers/omx/vid_dec.c
>>> index a989c10..7842966 100644
>>> --- a/src/gallium/state_trackers/omx/vid_dec.c
>>> +++ b/src/gallium/state_trackers/omx/vid_dec.c
>>> @@ -167,6 +167,19 @@ static OMX_ERRORTYPE
>>> vid_dec_Constructor(OMX_COMPONENTTYPE *comp, OMX_STRING nam
>>>      if (!priv->pipe)
>>>         return OMX_ErrorInsufficientResources;
>>>
>>> +   if (!vl_compositor_init(&priv->compositor, priv->pipe)) {
>>> +      priv->pipe->destroy(priv->pipe);
>>> +      priv->pipe = NULL;
>>> +      return OMX_ErrorInsufficientResources;
>>> +   }
>>> +
>>> +   if (!vl_compositor_init_state(&priv->cstate, priv->pipe)) {
>>> +      vl_compositor_cleanup(&priv->compositor);
>>> +      priv->pipe->destroy(priv->pipe);
>>> +      priv->pipe = NULL;
>>> +      return OMX_ErrorInsufficientResources;
>>> +   }
>>> +()
>>
>> IIRC as vid_dec_Constructor() fails, the caller (bellagio?) explicitly
>> calls the destructor vid_dec_Destructor(). Thus the above teardown
>> should not be needed.
>
>
>
> We take reference of the structure of priv->compositor, and priv->cstate for
> init.
> the init return true or false, and there is no clear flag from compositor
> and cstate to reflect on the success of the init function.
> so we use priv->pipe as a clear flag in order to clean them up at the
> destructor.
>
One could use vl_compositor::pipe and/or ::upload,
vl_compositor_init() was a little more diligent making sure those the
struct is in a consistent state upon error. Same suggestion(s) apply
for vl_compositor_init_state().

Yes, updating(fixing?) those is not the goal is here, yet having
::compositor/::cstate teardown dependant on ::pipe is ambiguous and
error prone.  Something like the following is better imho,

if (priv->cstate.pipe)
      vl_compositor_cleanup_state(&priv->cstate);

As said before - the current code is not wrong by any means, it just
need some minor polish.

>
>>>      priv->sPortTypesParam[OMX_PortDomainVideo].nStartPortNumber = 0;
>>>      priv->sPortTypesParam[OMX_PortDomainVideo].nPorts = 2;
>>>      priv->ports = CALLOC(2, sizeof(omx_base_PortType *));
>>> @@ -218,8 +231,11 @@ static OMX_ERRORTYPE
>>> vid_dec_Destructor(OMX_COMPONENTTYPE *comp)
>>>         priv->ports=NULL;
>>>      }
>>>
>>> -   if (priv->pipe)
>>> +   if (priv->pipe) {
>>> +      vl_compositor_cleanup_state(&priv->cstate);
>>> +      vl_compositor_cleanup(&priv->compositor);
>>
>> Neither vl_compositor_cleanup_state() nor vl_compositor_cleanup() is
>> happy if upon deref. the value (pointer again) is NULL.
>>
>> omx/vid_enc.c could use similar cleanups ?
>
> so, if priv->pipe is true,
> it won't be NULL, the cstate and compositor is for sure there.
>
>
> Are I right?
>
Almost. Neither one of ::composuitor nor ::cstate is a pointer, thus
one cannot get a deref in the cleanup funcs. Regardless of the
priv->pipe value.

Thanks for confirming/dismissing my concerns :-)

Emil