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

[wm4]

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;
> >> +    } 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.