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

[Martin Storsjö]

On Thu, 26 Jun 2014, Thomas Guillem wrote:

> ---
> modules/codec/omxil/omxil.c |  246 +++++++++++++++++++++++--------------------
> 1 file changed, 132 insertions(+), 114 deletions(-)
>
> diff --git a/modules/codec/omxil/omxil.c b/modules/codec/omxil/omxil.c
> index d4e9c58..0d634fb 100644
> --- a/modules/codec/omxil/omxil.c
> +++ b/modules/codec/omxil/omxil.c
> @@ -356,6 +356,111 @@ static OMX_ERRORTYPE UpdatePixelAspect(decoder_t *p_dec)
> }
>
> /*****************************************************************************
> + * AllocateBuffers: Allocate Omx buffers
> + *****************************************************************************/
> +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)
> +        {
> +            p_port->i_buffers = i;
> +            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;
> +}
> +
> +/*****************************************************************************
> + * FreeBuffers: Free Omx buffers
> + *****************************************************************************/
> +static OMX_ERRORTYPE FreeBuffers(decoder_t *p_dec, OmxPort *p_port)
> +{
> +    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
> +
> +    for(i = 0; i < p_port->i_buffers; i++)
> +    {
> +        OMX_FIFO_GET(&p_port->fifo, p_buffer);
> +        if (p_buffer->pAppPrivate != NULL)
> +            decoder_DeletePicture( p_dec, p_buffer->pAppPrivate );

Note how there is a decoder_DeletePicture call here

> +        if (p_buffer->nFlags & SENTINEL_FLAG) {
> +            free(p_buffer);
> +            i--;
> +            continue;
> +        }
> +        omx_error = OMX_FreeBuffer( p_port->omx_handle,
> +                                    p_port->i_port_index, p_buffer );
> +
> +        if(omx_error != OMX_ErrorNone) break;
> +    }
> +    if( omx_error != OMX_ErrorNone )
> +       msg_Err( p_dec, "OMX_FreeBuffer failed (%x, %i, %i)",
> +                omx_error, (int)p_port->i_port_index, i );
> +
> +    p_port->i_buffers = 0;
> +    free( p_port->pp_buffers );
> +    p_port->pp_buffers = NULL;
> +
> +#ifdef OMXIL_EXTRA_DEBUG
> +    msg_Dbg( p_dec, "FreeBuffers(%d)::done", def->eDir );
> +#endif
> +
> +    return omx_error;
> +}
> +
> +/*****************************************************************************
>  * GetPortDefinition: set vlc format based on the definition of the omx port
>  *****************************************************************************/
> static OMX_ERRORTYPE GetPortDefinition(decoder_t *p_dec, OmxPort *p_port,
> @@ -505,9 +610,10 @@ static OMX_ERRORTYPE DeinitialiseComponent(decoder_t *p_dec,
>                                            OMX_HANDLETYPE omx_handle)
> {
>     decoder_sys_t *p_sys = p_dec->p_sys;
> +    OMX_BUFFERHEADERTYPE *p_buffer;
>     OMX_ERRORTYPE omx_error;
>     OMX_STATETYPE state;
> -    unsigned int i, j;
> +    unsigned int i;
>
>     if(!omx_handle) return OMX_ErrorNone;
>
> @@ -542,34 +648,10 @@ static OMX_ERRORTYPE DeinitialiseComponent(decoder_t *p_dec,
>         for(i = 0; i < p_sys->ports; i++)
>         {
>             OmxPort *p_port = &p_sys->p_ports[i];
> -            OMX_BUFFERHEADERTYPE *p_buffer;
> -
> -            for(j = 0; j < p_port->i_buffers; j++)
> -            {
> -                OMX_FIFO_GET(&p_port->fifo, p_buffer);
> -                if (p_buffer->nFlags & SENTINEL_FLAG) {
> -                    free(p_buffer);
> -                    j--;
> -                    continue;
> -                }

But no matching call to decoder_DeletePicture here. So while you are 
factorizing the code, you are effectively changing what the code does. 
Yes, the existing behaviour is probably a bug. But the fact that this 
patch actually changes the existing behaviour means that the patch needs 
to be tested in a completely different way, to make sure it doesn't 
trigger any other bug in previously (kind of, at least) working setups 
with OMX_UseBuffer.

Thus, to do this completely strictly, you'd first fix the existing code to 
be similar in the two codepaths that you factorize, then factorize 
separately. Or at least mention the fact in the commit message that this 
changes behaviour.

> @@ -582,6 +664,18 @@ static OMX_ERRORTYPE DeinitialiseComponent(decoder_t *p_dec,
>         OmxPort *p_port = &p_sys->p_ports[i];
>         free(p_port->pp_buffers);
>         p_port->pp_buffers = 0;
> +
> +        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_Warn( p_dec, "Stray buffer left in fifo, %p", p_buffer );
> +        }
>     }
>     omx_error = pf_free_handle( omx_handle );
>     return omx_error;

Here you also changed the structure of the code more than just 
factorizing, by only calling this at the end, not within FreeBuffers. This 
is probably also ok, but I'd rather do it in a separate patch afterward, 
or at the very least, mention it in the commit message.

The less you describe things in the commit message, and the more you hide 
innocent-looking behaviour changes, that the reviewer wants to know about, 
the more work it is to review them, when one has to go looking for them.

The patch itself is probably ok, but I will need to retest things with 
OMX_UseBuffer before pushing it.

// Martin