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

[Thomas Guillem]

AH OK, I thought the codec should have returned all buffers at that state.

I modified that part of the code because I had a raise condition between
omx
free and HWbuffer free, but then I reworked lot of stuffs. So I should be
able
to put back the original code and have HWbuffer free working.

Thomas.


2014-06-24 18:31 GMT+02:00 Martin Storsjö <martin at martin.st>:

> 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
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel
>

-- 


This email and any files transmitted with it are confidential and are 
intended solely for the use of the individual or entity to which they are 
addressed. Access to this e-mail by anyone else is unauthorised. If you are 
not the intended recipient, any disclosure, copying, distribution or any 
action taken or omitted to be taken in reliance on it, is prohibited. 
E-mail messages are not necessarily secure. Archos does not accept 
responsibility for any changes made to this message after it was sent.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20140624/65ea4cf2/attachment.html>