| Message ID | 20200406042203.10681-2-andriy.gelman@gmail.com |
|---|---|
| State | Accepted |
| Commit | 2a9d62356152d4ef079416101664f26d2562c681 |
| Headers | show |
| Series | [FFmpeg-devel,1/2] avcodec/v4l2_context: Use av_freep | expand |
| Context | Check | Description |
|---|---|---|
| andriy/ffmpeg-patchwork | success | Make fate finished |
Lgtm, But if don't prevent enqueue frame buffer of capture port, unlikely to happen this case. And I think we can get a proper num_frame_buffers by g_ctrl V4L2_CID_MIN_BUFFERS_FOR_CAPTURE and V4L2_CID_MIN_BUFFERS_FOR_OUTPUT. Best regards, Ming Qian Tel#:86-512-6805-6630 -----Original Message----- From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Andriy Gelman Sent: Monday, April 6, 2020 12:22 PM To: ffmpeg-devel@ffmpeg.org Cc: Andriy Gelman <andriy.gelman@gmail.com> Subject: [EXT] [FFmpeg-devel] [PATCH 2/2] avcodec/v4l2_context: Log warning when all capture buffers are in userspace Caution: EXT Email From: Andriy Gelman <andriy.gelman@gmail.com> v4l2_m2m uses device memory mapped buffers to store dequeued frames/packets (reference counted by AVBufferRef). When the reference count drops to zero, the buffer ownership is returned back to the device, so that they can re-filled with frames/packets. There are some cases when all the buffers are in userspace (i.e. due to internal buffering in ffmpeg). On the s5p-mfc this causes an infinite wait when polling to dequeue the buffers, which can be prevented by increasing the total number of capture buffers. This commit adds a warning when this occurs. Signed-off-by: Andriy Gelman <andriy.gelman@gmail.com> --- libavcodec/v4l2_context.c | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/libavcodec/v4l2_context.c b/libavcodec/v4l2_context.c index 31af10d28e..5145772c45 100644 --- a/libavcodec/v4l2_context.c +++ b/libavcodec/v4l2_context.c @@ -285,6 +285,18 @@ static V4L2Buffer* v4l2_dequeue_v4l2buf(V4L2Context *ctx, int timeout) }; int i, ret; + if (!V4L2_TYPE_IS_OUTPUT(ctx->type) && ctx->buffers) { + for (i = 0; i < ctx->num_buffers; i++) { + if (ctx->buffers[i].status == V4L2BUF_IN_DRIVER) + break; + } + if (i == ctx->num_buffers) + av_log(logger(ctx), AV_LOG_WARNING, "All capture buffers returned to " + "userspace. Increase num_capture_buffers " + "to prevent device deadlock or dropped " + "packets/frames.\n"); + } + /* if we are draining and there are no more capture buffers queued in the driver we are done */ if (!V4L2_TYPE_IS_OUTPUT(ctx->type) && ctx_to_m2mctx(ctx)->draining) { for (i = 0; i < ctx->num_buffers; i++) { -- 2.25.1
Hi Ming, Thanks for looking over the patch. On Thu, 09. Apr 07:13, Ming Qian wrote: > Lgtm, > > But if don't prevent enqueue frame buffer of capture port, unlikely to happen this case. The problem happens when all the capture packets/frames are buffered internally by ffmpeg (i.e. the buffer state is V4L2BUF_RET_USER). I see it on the s5p-mfc even with your patch: https://patchwork.ffmpeg.org/project/ffmpeg/patch/20200316020208.27547-1-ming.qian@nxp.com/ > And I think we can get a proper num_frame_buffers by g_ctrl V4L2_CID_MIN_BUFFERS_FOR_CAPTURE and V4L2_CID_MIN_BUFFERS_FOR_OUTPUT. > The values for V4L2_CID_MIN_BUFFERS_FOR_CAPTURE and V4L2_CID_MIN_BUFFERS_FOR_OUTPUT are the minimum values for the hardware device and are lower than our defaults. I don't think we should use the minimum values because the frames/packets may be buffered. There is another option to request additional buffers via CREATE_BUFS ioctl, but it's not supported by all hardware (thanks to ndufresne on #v4l for suggesting). I feel adding this ioctl should be a separate patch and we still need a warning if it's not supported. I'll apply this patch on Wednesday if no one objects. Thanks, Andriy > > > > -----Original Message----- > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Andriy Gelman > Sent: Monday, April 6, 2020 12:22 PM > To: ffmpeg-devel@ffmpeg.org > Cc: Andriy Gelman <andriy.gelman@gmail.com> > Subject: [EXT] [FFmpeg-devel] [PATCH 2/2] avcodec/v4l2_context: Log warning when all capture buffers are in userspace > > Caution: EXT Email > > From: Andriy Gelman <andriy.gelman@gmail.com> > > v4l2_m2m uses device memory mapped buffers to store dequeued frames/packets (reference counted by AVBufferRef). When the reference count drops to zero, the buffer ownership is returned back to the device, so that they can re-filled with frames/packets. > > There are some cases when all the buffers are in userspace (i.e. due to internal buffering in ffmpeg). On the s5p-mfc this causes an infinite wait when polling to dequeue the buffers, which can be prevented by increasing the total number of capture buffers. This commit adds a warning when this occurs. > > Signed-off-by: Andriy Gelman <andriy.gelman@gmail.com> > --- > libavcodec/v4l2_context.c | 12 ++++++++++++ > 1 file changed, 12 insertions(+) > > diff --git a/libavcodec/v4l2_context.c b/libavcodec/v4l2_context.c index 31af10d28e..5145772c45 100644 > --- a/libavcodec/v4l2_context.c > +++ b/libavcodec/v4l2_context.c > @@ -285,6 +285,18 @@ static V4L2Buffer* v4l2_dequeue_v4l2buf(V4L2Context *ctx, int timeout) > }; > int i, ret; > > + if (!V4L2_TYPE_IS_OUTPUT(ctx->type) && ctx->buffers) { > + for (i = 0; i < ctx->num_buffers; i++) { > + if (ctx->buffers[i].status == V4L2BUF_IN_DRIVER) > + break; > + } > + if (i == ctx->num_buffers) > + av_log(logger(ctx), AV_LOG_WARNING, "All capture buffers returned to " > + "userspace. Increase num_capture_buffers " > + "to prevent device deadlock or dropped " > + "packets/frames.\n"); > + } > + > /* if we are draining and there are no more capture buffers queued in the driver we are done */ > if (!V4L2_TYPE_IS_OUTPUT(ctx->type) && ctx_to_m2mctx(ctx)->draining) { > for (i = 0; i < ctx->num_buffers; i++) { > -- > 2.25.1 > > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fffmpeg.org%2Fmailman%2Flistinfo%2Fffmpeg-devel&data=02%7C01%7Cming.qian%40nxp.com%7C38355e6d2f7b41f639f708d7d9e2f2e9%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637217441076225890&sdata=KmGkJyH%2FxR3jBkrTLI89EWTYhrpNchkJM7ldqjHKUSw%3D&reserved=0 > > To unsubscribe, visit link above, or email ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe". > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
On Mon, 27. Apr 16:28, Andriy Gelman wrote: > Hi Ming, > > Thanks for looking over the patch. > > On Thu, 09. Apr 07:13, Ming Qian wrote: > > Lgtm, > > > > But if don't prevent enqueue frame buffer of capture port, unlikely to happen this case. > > The problem happens when all the capture packets/frames are buffered internally > by ffmpeg (i.e. the buffer state is V4L2BUF_RET_USER). > > I see it on the s5p-mfc even with your patch: > https://patchwork.ffmpeg.org/project/ffmpeg/patch/20200316020208.27547-1-ming.qian@nxp.com/ > > > And I think we can get a proper num_frame_buffers by g_ctrl V4L2_CID_MIN_BUFFERS_FOR_CAPTURE and V4L2_CID_MIN_BUFFERS_FOR_OUTPUT. > > > > The values for V4L2_CID_MIN_BUFFERS_FOR_CAPTURE and > V4L2_CID_MIN_BUFFERS_FOR_OUTPUT are the minimum values for the hardware device > and are lower than our defaults. I don't think we should use the minimum values > because the frames/packets may be buffered. > > There is another option to request additional buffers via > CREATE_BUFS ioctl, but it's not supported by all hardware (thanks to ndufresne > on #v4l for suggesting). I feel adding this ioctl should be a separate patch and we > still need a warning if it's not supported. > > I'll apply this patch on Wednesday if no one objects. > Applied. Thanks,
diff --git a/libavcodec/v4l2_context.c b/libavcodec/v4l2_context.c index 31af10d28e..5145772c45 100644 --- a/libavcodec/v4l2_context.c +++ b/libavcodec/v4l2_context.c @@ -285,6 +285,18 @@ static V4L2Buffer* v4l2_dequeue_v4l2buf(V4L2Context *ctx, int timeout) }; int i, ret; + if (!V4L2_TYPE_IS_OUTPUT(ctx->type) && ctx->buffers) { + for (i = 0; i < ctx->num_buffers; i++) { + if (ctx->buffers[i].status == V4L2BUF_IN_DRIVER) + break; + } + if (i == ctx->num_buffers) + av_log(logger(ctx), AV_LOG_WARNING, "All capture buffers returned to " + "userspace. Increase num_capture_buffers " + "to prevent device deadlock or dropped " + "packets/frames.\n"); + } + /* if we are draining and there are no more capture buffers queued in the driver we are done */ if (!V4L2_TYPE_IS_OUTPUT(ctx->type) && ctx_to_m2mctx(ctx)->draining) { for (i = 0; i < ctx->num_buffers; i++) {