| Message ID | 1506468143-20607-1-git-send-email-jorge.ramirez-ortiz@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
On Tue, 26 Sep 2017 16:22:23 -0700 Jorge Ramirez-Ortiz <jorge.ramirez-ortiz@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. > + 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? > - 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?
On 09/27/2017 06:01 AM, wm4 wrote: > On Tue, 26 Sep 2017 16:22:23 -0700 > Jorge Ramirez-Ortiz <jorge.ramirez-ortiz@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@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
On Wed, 27 Sep 2017 09:03:58 -0700 Jorge Ramirez-Ortiz <jorge.ramirez-ortiz@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@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.
On 09/27/2017 09:31 AM, wm4 wrote: > On Wed, 27 Sep 2017 09:03:58 -0700 > Jorge Ramirez-Ortiz <jorge.ramirez-ortiz@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@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@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
diff --git a/libavcodec/v4l2_context.c b/libavcodec/v4l2_context.c index d675c55..f06a7ff 100644 --- a/libavcodec/v4l2_context.c +++ b/libavcodec/v4l2_context.c @@ -189,40 +189,6 @@ reinit_run: return 1; } -static int v4l2_stop_decode(V4L2Context *ctx) -{ - struct v4l2_decoder_cmd cmd = { - .cmd = V4L2_DEC_CMD_STOP, - }; - int ret; - - ret = ioctl(ctx_to_m2mctx(ctx)->fd, VIDIOC_DECODER_CMD, &cmd); - if (ret) { - /* DECODER_CMD is optional */ - if (errno == ENOTTY) - return ff_v4l2_context_set_status(ctx, VIDIOC_STREAMOFF); - } - - return 0; -} - -static int v4l2_stop_encode(V4L2Context *ctx) -{ - struct v4l2_encoder_cmd cmd = { - .cmd = V4L2_ENC_CMD_STOP, - }; - int ret; - - ret = ioctl(ctx_to_m2mctx(ctx)->fd, VIDIOC_ENCODER_CMD, &cmd); - if (ret) { - /* ENCODER_CMD is optional */ - if (errno == ENOTTY) - return ff_v4l2_context_set_status(ctx, VIDIOC_STREAMOFF); - } - - return 0; -} - static V4L2Buffer* v4l2_dequeue_v4l2buf(V4L2Context *ctx, int timeout) { struct v4l2_plane planes[VIDEO_MAX_PLANES]; @@ -236,6 +202,13 @@ static V4L2Buffer* v4l2_dequeue_v4l2buf(V4L2Context *ctx, int timeout) if (V4L2_TYPE_IS_OUTPUT(ctx->type)) pfd.events = POLLOUT | POLLWRNORM; + else { + /* on the capture queue */ + if (ctx_to_m2mctx(ctx)->draining) { + /* ignore driver requests for more input and just wait for a valid frame */ + pfd.events = POLLIN | POLLRDNORM | POLLPRI; + } + } for (;;) { ret = poll(&pfd, 1, timeout); @@ -243,50 +216,45 @@ static V4L2Buffer* v4l2_dequeue_v4l2buf(V4L2Context *ctx, int timeout) break; if (errno == EINTR) continue; - - /* timeout is being used to indicate last valid bufer when draining */ - if (ctx_to_m2mctx(ctx)->draining) - ctx->done = 1; - return NULL; } - /* 0. handle errors */ + /* handle errors */ if (pfd.revents & POLLERR) { av_log(logger(ctx), AV_LOG_WARNING, "%s POLLERR\n", ctx->name); return NULL; } - /* 1. handle resolution changes */ + /* handle resolution changes */ if (pfd.revents & POLLPRI) { ret = v4l2_handle_event(ctx); if (ret < 0) { /* if re-init failed, abort */ - ctx->done = EINVAL; + ctx->done = 1; return NULL; } if (ret) { /* if re-init was successfull drop the buffer (if there was one) - * since we had to reconfigure capture (unmap all buffers) - */ + * since we had to reconfigure capture (unmap all buffers) */ return NULL; } } - /* 2. dequeue the buffer */ + /* dequeue the buffer or provide more input */ if (pfd.revents & (POLLIN | POLLRDNORM | POLLOUT | POLLWRNORM)) { - if (!V4L2_TYPE_IS_OUTPUT(ctx->type)) { - /* there is a capture buffer ready */ - if (pfd.revents & (POLLIN | POLLRDNORM)) - goto dequeue; + if (V4L2_TYPE_IS_OUTPUT(ctx->type)) + goto dequeue; - /* the driver is ready to accept more input; instead of waiting for the capture - * buffer to complete we return NULL so input can proceed (we are single threaded) - */ - if (pfd.revents & (POLLOUT | POLLWRNORM)) - return NULL; - } + /* there is a capture buffer ready */ + if (pfd.revents & (POLLIN | POLLRDNORM)) + goto dequeue; + + /* the driver is ready to accept more input: instead of waiting for + * the capture buffer to complete, return NULL so input can proceed + * (we are single threaded after all) */ + if (pfd.revents & (POLLOUT | POLLWRNORM)) + return NULL; dequeue: memset(&buf, 0, sizeof(buf)); @@ -301,23 +269,25 @@ dequeue: ret = ioctl(ctx_to_m2mctx(ctx)->fd, VIDIOC_DQBUF, &buf); if (ret) { if (errno != EAGAIN) { - ctx->done = errno; + ctx->done = 1; if (errno != EPIPE) - av_log(logger(ctx), AV_LOG_DEBUG, "%s VIDIOC_DQBUF, errno (%s)\n", + av_log(logger(ctx), AV_LOG_ERROR, "%s VIDIOC_DQBUF, errno (%s)\n", ctx->name, av_err2str(AVERROR(errno))); } - } else { - avbuf = &ctx->buffers[buf.index]; - avbuf->status = V4L2BUF_AVAILABLE; - avbuf->buf = buf; - if (V4L2_TYPE_IS_MULTIPLANAR(ctx->type)) { - memcpy(avbuf->planes, planes, sizeof(planes)); - avbuf->buf.m.planes = avbuf->planes; - } + return NULL; } + + avbuf = &ctx->buffers[buf.index]; + avbuf->status = V4L2BUF_AVAILABLE; + avbuf->buf = buf; + if (V4L2_TYPE_IS_MULTIPLANAR(ctx->type)) { + memcpy(avbuf->planes, planes, sizeof(planes)); + avbuf->buf.m.planes = avbuf->planes; + } + return avbuf; } - return avbuf; + return NULL; } static V4L2Buffer* v4l2_getfree_v4l2buf(V4L2Context *ctx) @@ -476,50 +446,50 @@ int ff_v4l2_context_set_status(V4L2Context* ctx, int cmd) int ff_v4l2_context_enqueue_frame(V4L2Context* ctx, const AVFrame* frame) { - V4L2m2mContext *s = ctx_to_m2mctx(ctx); V4L2Buffer* avbuf; int ret; - if (!frame) { - ret = v4l2_stop_encode(ctx); - if (ret) - av_log(logger(ctx), AV_LOG_ERROR, "%s stop_encode\n", ctx->name); - s->draining= 1; - return 0; - } - avbuf = v4l2_getfree_v4l2buf(ctx); if (!avbuf) return AVERROR(ENOMEM); - ret = ff_v4l2_buffer_avframe_to_buf(frame, avbuf); - if (ret) - return ret; + 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; + } return ff_v4l2_buffer_enqueue(avbuf); } int ff_v4l2_context_enqueue_packet(V4L2Context* ctx, const AVPacket* pkt) { - V4L2m2mContext *s = ctx_to_m2mctx(ctx); V4L2Buffer* avbuf; int ret; - if (!pkt->size) { - ret = v4l2_stop_decode(ctx); - if (ret) - av_log(logger(ctx), AV_LOG_ERROR, "%s stop_decode\n", ctx->name); - s->draining = 1; - return 0; - } - avbuf = v4l2_getfree_v4l2buf(ctx); if (!avbuf) return AVERROR(ENOMEM); - 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; + } else { + ret = ff_v4l2_buffer_avpkt_to_buf(pkt, avbuf); + if (ret) + return ret; + } return ff_v4l2_buffer_enqueue(avbuf); } @@ -528,14 +498,12 @@ int ff_v4l2_context_dequeue_frame(V4L2Context* ctx, AVFrame* frame) { V4L2Buffer* avbuf = NULL; - /* if we are draining, we are no longer inputing data, therefore enable a - * timeout so we can dequeue and flag the last valid buffer. - * + /* * blocks until: * 1. decoded frame available * 2. an input buffer is ready to be dequeued */ - avbuf = v4l2_dequeue_v4l2buf(ctx, ctx_to_m2mctx(ctx)->draining ? 200 : -1); + avbuf = v4l2_dequeue_v4l2buf(ctx, -1); if (!avbuf) { if (ctx->done) return AVERROR_EOF; @@ -550,14 +518,12 @@ int ff_v4l2_context_dequeue_packet(V4L2Context* ctx, AVPacket* pkt) { V4L2Buffer* avbuf = NULL; - /* if we are draining, we are no longer inputing data, therefore enable a - * timeout so we can dequeue and flag the last valid buffer. - * + /* * blocks until: * 1. encoded packet available * 2. an input buffer ready to be dequeued */ - avbuf = v4l2_dequeue_v4l2buf(ctx, ctx_to_m2mctx(ctx)->draining ? 200 : -1); + avbuf = v4l2_dequeue_v4l2buf(ctx, -1); if (!avbuf) { if (ctx->done) return AVERROR_EOF;