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

[Jorge Ramirez-Ortiz]

On 09/27/2017 09:31 AM, wm4 wrote:
> On Wed, 27 Sep 2017 09:03:58 -0700
> Jorge Ramirez-Ortiz <jorge.ramirez-ortiz at linaro.org> wrote:
>
>> 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.
> Not necessary. In general we prefer separating unrelated changes, but I
> think there's a limit to which we have to cause additional work to
> follow this rule. (It's mainly in cases where the cosmetic changes are
> significant or in completely different code.)
>
>>>> +    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;
>>>>      /* handle the end of stream case */
>>>> +    } 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
> That sounds good. So you're going to send a new patch? Then I'll hold
> off applying this one.

yes, just testing.
it is much cleaner this way.

I looked [1] at how the  V4L2_BUF_FLAG_LAST is used in the Venus driver and it 
seems that I could either set bytesused to 0 _OR_ set the  V4L2_BUF_FLAG_LAST 
instead (the API suggests using the flag)
I will do both to make sure all cases are covered in drivers that might not use 
the flag.

[1] 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/media/platform/qcom/venus/helpers.c?h=v4.14-rc2#n325

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