[vlc-devel] [RFC PATCH 07/11] omxil: factorize FreeBuffers and AllocateBuffers

[Martin Storsjö]

On Tue, 24 Jun 2014, Thomas Guillem wrote:

> ---
> modules/codec/omxil/omxil.c |  236 ++++++++++++++++++++++---------------------
> 1 file changed, 123 insertions(+), 113 deletions(-)
>
> diff --git a/modules/codec/omxil/omxil.c b/modules/codec/omxil/omxil.c
> index df75c93..c1ed20d 100644
> --- a/modules/codec/omxil/omxil.c
> +++ b/modules/codec/omxil/omxil.c
> @@ -355,6 +355,113 @@ static OMX_ERRORTYPE UpdatePixelAspect(decoder_t *p_dec)
>     return omx_err;
> }
>
> +static OMX_ERRORTYPE AllocateBuffers(decoder_t *p_dec, OmxPort *p_port)
> +{
> +    decoder_sys_t *p_sys = p_dec->p_sys;
> +    OMX_ERRORTYPE omx_error = OMX_ErrorUndefined;
> +    OMX_PARAM_PORTDEFINITIONTYPE *def = &p_port->definition;
> +    unsigned int i;
> +
> +#ifdef OMXIL_EXTRA_DEBUG
> +    msg_Dbg( p_dec, "AllocateBuffers(%d)", def->eDir );
> +#endif
> +
> +    p_port->i_buffers = p_port->definition.nBufferCountActual;
> +
> +    p_port->pp_buffers = calloc(p_port->i_buffers, sizeof(OMX_BUFFERHEADERTYPE*));
> +    if( !p_port->pp_buffers )
> +    {
> +        return OMX_ErrorInsufficientResources;
> +    }
> +
> +    for(i = 0; i < p_port->i_buffers; i++)
> +    {
> +#if 0
> +#define ALIGN(x,BLOCKLIGN) (((x) + BLOCKLIGN - 1) & ~(BLOCKLIGN - 1))
> +        char *p_buf = malloc(p_port->definition.nBufferSize +
> +                             p_port->definition.nBufferAlignment);
> +        p_port->pp_buffers[i] = (void *)ALIGN((uintptr_t)p_buf, p_port->definition.nBufferAlignment);
> +#endif
> +
> +        if(p_port->b_direct)
> +            omx_error =
> +                OMX_UseBuffer( p_sys->omx_handle, &p_port->pp_buffers[i],
> +                               p_port->i_port_index, 0,
> +                               p_port->definition.nBufferSize, (void*)1);
> +        else
> +            omx_error =
> +                OMX_AllocateBuffer( p_sys->omx_handle, &p_port->pp_buffers[i],
> +                                    p_port->i_port_index, 0,
> +                                    p_port->definition.nBufferSize);
> +
> +        if(omx_error != OMX_ErrorNone) break;
> +        OMX_FIFO_PUT(&p_port->fifo, p_port->pp_buffers[i]);
> +    }
> +    CHECK_ERROR(omx_error, "AllocateBuffers failed (%x, %i)",
> +                omx_error, (int)p_port->i_port_index );
> +
> +
> +#ifdef OMXIL_EXTRA_DEBUG
> +    msg_Dbg( p_dec, "AllocateBuffers(%d)::done", def->eDir );
> +#endif
> +error:
> +    return omx_error;
> +}
> +
> +static OMX_ERRORTYPE FreeBuffers(decoder_t *p_dec, OmxPort *p_port)
> +{
> +    decoder_sys_t *p_sys = p_dec->p_sys;
> +    OMX_PARAM_PORTDEFINITIONTYPE *def = &p_port->definition;
> +    OMX_ERRORTYPE omx_error = OMX_ErrorNone;
> +    OMX_BUFFERHEADERTYPE *p_buffer;
> +    unsigned int i;
> +
> +#ifdef OMXIL_EXTRA_DEBUG
> +    msg_Dbg( p_dec, "FreeBuffers(%d)", def->eDir );
> +#endif
> +
> +    /* wait for omx to release all buffers */
> +    while (1) {
> +        OMX_FIFO_PEEK(&p_port->fifo, p_buffer);
> +        if (!p_buffer) break;
> +
> +        OMX_FIFO_GET(&p_port->fifo, p_buffer);
> +        if (p_buffer->nFlags & SENTINEL_FLAG) {
> +            free(p_buffer);
> +            continue;
> +        }
> +        msg_Dbg( p_dec, "Stray buffer left in fifo, %p", p_buffer );
> +    }
> +
> +    for(i = 0; i < p_port->i_buffers; i++)
> +    {
> +        p_buffer =  p_port->pp_buffers[i];
> +
> +        if( p_buffer )
> +        {
> +            if( p_buffer->pAppPrivate != NULL )
> +                decoder_DeletePicture( p_dec, p_buffer->pAppPrivate );
> +            omx_error = OMX_FreeBuffer( p_sys->omx_handle,
> +                                        p_port->i_port_index, p_buffer );
> +            if(omx_error != OMX_ErrorNone) break;
> +        }
> +    }

Why did you change the order of this code compared to how it was before? 
This will not work as intended. Previously this was structured like this:

     for (0..i_buffers) {
         OMX_FIFO_GET
     }
     while (1)
         // Get any remaining buffer (sentinels?)

Now you just remove any buffers currently in the FIFO, and then assume you 
have got them all.

At this point, we've signalled the codec to return them to us (by setting 
the state to loaded, or by disabling the port), but the codec might not 
have returned all of them yet - and while the codec still owns one of the 
buffers, we cannot free it. (I've spent a lot of time debugging quite 
non-obvious bugs relating exactly to this piece of code.)

Thus, we need to make sure that we actually remove exactly 'i_buffers' 
buffers from the fifo. (And if there are sentinel buffers in the fifo 
those should not be counted.) By looping 'i_buffers' times and doing 
OMX_FIFO_GET each time, we make sure the code blocks until there is one 
buffer that we can free.

Thus, please keep the original structure of the code that frees the 
buffers - but factorizing it definitely is a good step.

// Martin