| Message ID | 20230718220017.3336-1-mailingradian@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel,RESEND] avcodec/v4l2_context: always log POLLERR when buffers are uninitialized | expand |
| Context | Check | Description |
|---|---|---|
| yinshiyou/make_loongarch64 | success | Make finished |
| yinshiyou/make_fate_loongarch64 | success | Make fate finished |
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
Will this patch be applied or receive any comments? I have been waiting more than 2 weeks since the original submission (https://ffmpeg.org/pipermail/ffmpeg-devel/2023-July/311580.html) and have not received a response since. I am trying to get mpv to work again with the v4l2_m2m codec, as it started emitting segmentation faults. This is partly due to some incorrect usage of libavcodec in mpv, and partly due to the error/warning handling in libavcodec. An mpv maintainer dismissed this as a bug in ffmpeg, and this change is simpler than what mpv would need for a fix, so I am hoping that this patch goes in. I don't have write access so I can't apply it myself, even if there are no objections. On Tue, Jul 18, 2023 at 06:00:17PM -0400, Richard Acayan wrote: > The error handler for POLLERRs assumes that the timeout is only zero > when v4l2_dequeue_v4l2buf is called by v4l2_getfree_v4l2buf. This > assumption is incorrect, as ff_v4l2_context_dequeue_frame also calls > this function with a timeout of zero. Do not check for unavailable > buffers if the buffers are uninitialized. > > See https://trac.ffmpeg.org/ticket/9957 for the original bug report. > > Signed-off-by: Richard Acayan <mailingradian@gmail.com> > --- > libavcodec/v4l2_context.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/libavcodec/v4l2_context.c b/libavcodec/v4l2_context.c > index a40be94690..69ddf80723 100644 > --- a/libavcodec/v4l2_context.c > +++ b/libavcodec/v4l2_context.c > @@ -327,7 +327,7 @@ start: > if (pfd.revents & POLLERR) { > /* if we are trying to get free buffers but none have been queued yet > no need to raise a warning */ > - if (timeout == 0) { > + if (timeout == 0 && ctx->buffers) { > for (i = 0; i < ctx->num_buffers; i++) { > if (ctx->buffers[i].status != V4L2BUF_AVAILABLE) > av_log(logger(ctx), AV_LOG_WARNING, "%s POLLERR\n", ctx->name); > -- > 2.41.0 >
On Fri, 21 Jul 2023, Richard Acayan wrote: > Will this patch be applied or receive any comments? I have been waiting > more than 2 weeks since the original submission > (https://ffmpeg.org/pipermail/ffmpeg-devel/2023-July/311580.html) and > have not received a response since. > > I am trying to get mpv to work again with the v4l2_m2m codec, as it > started emitting segmentation faults. This is partly due to some > incorrect usage of libavcodec in mpv, and partly due to the > error/warning handling in libavcodec. An mpv maintainer dismissed this > as a bug in ffmpeg, and this change is simpler than what mpv would need > for a fix, so I am hoping that this patch goes in. > > I don't have write access so I can't apply it myself, even if there are > no objections. > > On Tue, Jul 18, 2023 at 06:00:17PM -0400, Richard Acayan wrote: >> The error handler for POLLERRs assumes that the timeout is only zero >> when v4l2_dequeue_v4l2buf is called by v4l2_getfree_v4l2buf. This >> assumption is incorrect, as ff_v4l2_context_dequeue_frame also calls >> this function with a timeout of zero. Do not check for unavailable >> buffers if the buffers are uninitialized. >> >> See https://trac.ffmpeg.org/ticket/9957 for the original bug report. >> >> Signed-off-by: Richard Acayan <mailingradian@gmail.com> >> --- >> libavcodec/v4l2_context.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/libavcodec/v4l2_context.c b/libavcodec/v4l2_context.c >> index a40be94690..69ddf80723 100644 >> --- a/libavcodec/v4l2_context.c >> +++ b/libavcodec/v4l2_context.c >> @@ -327,7 +327,7 @@ start: >> if (pfd.revents & POLLERR) { >> /* if we are trying to get free buffers but none have been queued yet >> no need to raise a warning */ >> - if (timeout == 0) { >> + if (timeout == 0 && ctx->buffers) { This would warn if no buffers are allocated yet, but this is not a warning condition as far as I understand the issue, simply a different API usage pattern. So maybe it is better to simply do a if (!ctx->buffers) return NULL; in the beginning of the if (timeout == 0) block. Thanks, Marton >> for (i = 0; i < ctx->num_buffers; i++) { >> if (ctx->buffers[i].status != V4L2BUF_AVAILABLE) >> av_log(logger(ctx), AV_LOG_WARNING, "%s POLLERR\n", ctx->name); >> -- >> 2.41.0 >> > _______________________________________________ > 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 Sun, Jul 23, 2023 at 10:15:50AM +0200, Marton Balint wrote: > > > On Fri, 21 Jul 2023, Richard Acayan wrote: > >> Will this patch be applied or receive any comments? I have been waiting >> more than 2 weeks since the original submission >> (https://ffmpeg.org/pipermail/ffmpeg-devel/2023-July/311580.html) and >> have not received a response since. >> >> I am trying to get mpv to work again with the v4l2_m2m codec, as it >> started emitting segmentation faults. This is partly due to some >> incorrect usage of libavcodec in mpv, and partly due to the >> error/warning handling in libavcodec. An mpv maintainer dismissed this >> as a bug in ffmpeg, and this change is simpler than what mpv would need >> for a fix, so I am hoping that this patch goes in. >> >> I don't have write access so I can't apply it myself, even if there are >> no objections. >> >> On Tue, Jul 18, 2023 at 06:00:17PM -0400, Richard Acayan wrote: >>> The error handler for POLLERRs assumes that the timeout is only zero >>> when v4l2_dequeue_v4l2buf is called by v4l2_getfree_v4l2buf. This >>> assumption is incorrect, as ff_v4l2_context_dequeue_frame also calls >>> this function with a timeout of zero. Do not check for unavailable >>> buffers if the buffers are uninitialized. >>> >>> See https://trac.ffmpeg.org/ticket/9957 for the original bug report. >>> >>> Signed-off-by: Richard Acayan <mailingradian@gmail.com> >>> --- >>> libavcodec/v4l2_context.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/libavcodec/v4l2_context.c b/libavcodec/v4l2_context.c >>> index a40be94690..69ddf80723 100644 >>> --- a/libavcodec/v4l2_context.c >>> +++ b/libavcodec/v4l2_context.c >>> @@ -327,7 +327,7 @@ start: >>> if (pfd.revents & POLLERR) { >>> /* if we are trying to get free buffers but none have been queued yet >>> no need to raise a warning */ >>> - if (timeout == 0) { >>> + if (timeout == 0 && ctx->buffers) { > > This would warn if no buffers are allocated yet, but this is not a warning > condition as far as I understand the issue, simply a different API usage > pattern. I assumed that the usage was "incorrect" because the V4L2 API didn't like it. > So maybe it is better to simply do a > > if (!ctx->buffers) > return NULL; > > in the beginning of the if (timeout == 0) block. The sequence in mpv seems to be supported by libavcodec though, so I will do this in v2. > > Thanks, > Marton > >>> for (i = 0; i < ctx->num_buffers; i++) { >>> if (ctx->buffers[i].status != V4L2BUF_AVAILABLE) >>> av_log(logger(ctx), AV_LOG_WARNING, "%s POLLERR\n", ctx->name); >>> -- >>> 2.41.0 >>>
diff --git a/libavcodec/v4l2_context.c b/libavcodec/v4l2_context.c index a40be94690..69ddf80723 100644 --- a/libavcodec/v4l2_context.c +++ b/libavcodec/v4l2_context.c @@ -327,7 +327,7 @@ start: if (pfd.revents & POLLERR) { /* if we are trying to get free buffers but none have been queued yet no need to raise a warning */ - if (timeout == 0) { + if (timeout == 0 && ctx->buffers) { for (i = 0; i < ctx->num_buffers; i++) { if (ctx->buffers[i].status != V4L2BUF_AVAILABLE) av_log(logger(ctx), AV_LOG_WARNING, "%s POLLERR\n", ctx->name);
The error handler for POLLERRs assumes that the timeout is only zero when v4l2_dequeue_v4l2buf is called by v4l2_getfree_v4l2buf. This assumption is incorrect, as ff_v4l2_context_dequeue_frame also calls this function with a timeout of zero. Do not check for unavailable buffers if the buffers are uninitialized. See https://trac.ffmpeg.org/ticket/9957 for the original bug report. Signed-off-by: Richard Acayan <mailingradian@gmail.com> --- libavcodec/v4l2_context.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)