On Wed, 27 Sep 2017 09:03:58 -0700
Jorge Ramirez-Ortiz <jorge.ramirez-or...@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-or...@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.
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
