[FFmpeg-devel] [PATCH] avcodec/v4l2_m2m: fix draining process (dequeue without input)

[Jorge Ramirez-Ortiz]

On 09/27/2017 06:01 AM, wm4 wrote:
> On Tue, 26 Sep 2017 16:22:23 -0700
> Jorge Ramirez-Ortiz <jorge.ramirez-ortiz at linaro.org> wrote:
>
>> Stopping the codec when no more input is available causes captured
>> buffers that might be ready to be dequeued to be invalidated.
>>
>> This commit follows the V4L2 API more closely:
>> 1. on the last valid input buffer, it sets the V4L2_BUF_FLAG_LAST.
>> 2. ffmpeg then will continue dequeuing captured buffers until EPIPE is
>> raised by the driver (EOF condition)
>>
>> This also has the advantage of making the timeout on dequeuing capture
>> buffers while draining unnecessary.
>> ---
>>   libavcodec/v4l2_context.c | 162 ++++++++++++++++++----------------------------
>>   1 file changed, 64 insertions(+), 98 deletions(-)
> I can't really comment on the logic of this code. So LGTM, just some
> minor comments/questions.
>
>> -    /* 0. handle errors */
>> +    /* handle errors */
> (Apparently) unrelated cosmetic changes should usually be in separate
> patches, but that's probably not worth the trouble in this case.

ACK. will do on any following patches - or I can do it on this one if you want.

>> +    if (!frame) {
>> +        /* flag that we are draining */
>> +        ctx_to_m2mctx(ctx)->draining = 1;
>> +
>> +        /* send EOS */
>> +        avbuf->buf.m.planes[0].bytesused = 1;
>> +        avbuf->flags |= V4L2_BUF_FLAG_LAST;
>> +    } else {
>> +        ret = ff_v4l2_buffer_avframe_to_buf(frame, avbuf);
>> +        if (ret)
>> +            return ret;
>> +    }
> Wouldn't it be better to switch the if/else bodies and avoid the
> negation in the if condition?

yes
I am going to add a ff_v4l2_buffer_eos() to avoid exposing the bytesused at this 
level as well.
will fix

>
>> -    ret = ff_v4l2_buffer_avpkt_to_buf(pkt, avbuf);
>> -    if (ret)
>> -        return ret;
>> +    if (!pkt->size) {
>> +        /* flag that we are draining */
>> +        ctx_to_m2mctx(ctx)->draining = 1;
>> +
>> +        /* send EOS */
>> +        avbuf->buf.m.planes[0].bytesused = 1;
>> +        avbuf->flags |= V4L2_BUF_FLAG_LAST;
> What is going on here? Dummy buffer of size 1 to send the flag?

yes. we need to queue a dummy buffer to the input queue that carries that flag.
that way, the v4l2 device driver can raise EPIPE to indicate that the last 
buffer was processed.



> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel