diff mbox series

[FFmpeg-devel] avcodec/v4l2_buffers: don't prevent enqueue capture buffer to driver

Message ID 20200316020208.27547-1-ming.qian@nxp.com
State Accepted
Commit 7afd34050c1190e59e572a7882916a7dd4670d55
Headers show
Series [FFmpeg-devel] avcodec/v4l2_buffers: don't prevent enqueue capture buffer to driver
Related show

Checks

Context Check Description
andriy/ffmpeg-patchwork pending
andriy/ffmpeg-patchwork success Applied patch
andriy/ffmpeg-patchwork success Configure finished
andriy/ffmpeg-patchwork success Make finished
andriy/ffmpeg-patchwork success Make fate finished

Commit Message

Ming Qian March 16, 2020, 2:02 a.m. UTC
the draining is set when the output port is finished,
but it doesn't mean the capture port is finished.
especially for decoder, there may be a stream buffer to store several
frames.
so the decoder still need capture buffer even if the draining is set.

Signed-off-by: Ming Qian <ming.qian@nxp.com>
---
 libavcodec/v4l2_buffers.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Andriy Gelman March 17, 2020, 3:07 a.m. UTC | #1
On Mon, 16. Mar 10:02, Ming Qian wrote:
> the draining is set when the output port is finished,
> but it doesn't mean the capture port is finished.
> especially for decoder, there may be a stream buffer to store several
> frames.
> so the decoder still need capture buffer even if the draining is set.
> 
> Signed-off-by: Ming Qian <ming.qian@nxp.com>
> ---
>  libavcodec/v4l2_buffers.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/libavcodec/v4l2_buffers.c b/libavcodec/v4l2_buffers.c
> index dc1b9eaf24..02f23d954b 100644
> --- a/libavcodec/v4l2_buffers.c
> +++ b/libavcodec/v4l2_buffers.c
> @@ -222,7 +222,7 @@ static void v4l2_free_buffer(void *opaque, uint8_t *unused)
>              if (!atomic_load(&s->refcount))
>                  sem_post(&s->refsync);
>          } else {
> -            if (s->draining) {
> +            if (s->draining && V4L2_TYPE_IS_OUTPUT(avbuf->context->type)) {
>                  /* no need to queue more buffers to the driver */
>                  avbuf->status = V4L2BUF_AVAILABLE;
>              }

It makes sense, but did you have some dropped frames without this? 

Thanks,
BYHYKCHKIO WEIINZWLM March 17, 2020, 3:39 a.m. UTC | #2
When digging the application, I found this vulnerability in the program,
because when the sws_scale function was called, the application did not
check the validity of the parameters, resulting in an address access
exception vulnerability. So I want to know if this is a problem in FFmpage,
so I sent you an email to confirm it.

On Tue, Mar 17, 2020 at 11:30 AM Andriy Gelman <andriy.gelman@gmail.com>
wrote:

> On Mon, 16. Mar 10:02, Ming Qian wrote:
> > the draining is set when the output port is finished,
> > but it doesn't mean the capture port is finished.
> > especially for decoder, there may be a stream buffer to store several
> > frames.
> > so the decoder still need capture buffer even if the draining is set.
> >
> > Signed-off-by: Ming Qian <ming.qian@nxp.com>
> > ---
> >  libavcodec/v4l2_buffers.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/libavcodec/v4l2_buffers.c b/libavcodec/v4l2_buffers.c
> > index dc1b9eaf24..02f23d954b 100644
> > --- a/libavcodec/v4l2_buffers.c
> > +++ b/libavcodec/v4l2_buffers.c
> > @@ -222,7 +222,7 @@ static void v4l2_free_buffer(void *opaque, uint8_t
> *unused)
> >              if (!atomic_load(&s->refcount))
> >                  sem_post(&s->refsync);
> >          } else {
> > -            if (s->draining) {
> > +            if (s->draining &&
> V4L2_TYPE_IS_OUTPUT(avbuf->context->type)) {
> >                  /* no need to queue more buffers to the driver */
> >                  avbuf->status = V4L2BUF_AVAILABLE;
> >              }
>
> It makes sense, but did you have some dropped frames without this?
>
> Thanks,
> --
> Andriy
> _______________________________________________
> 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".
Ming Qian March 17, 2020, 10:28 a.m. UTC | #3
Yes, I have meet some hang issue on nxp's imx platform.
On nxp's imx platform, there is a stream buffer in the v4l2 decoder driver.
So driver may cache some frames in driver, if without this patch, after the draining is set to 1, the v4l2 capture buffer won't be enqueued to driver any more, it leds to hang.
Andriy Gelman April 9, 2020, 1:21 a.m. UTC | #4
On Tue, 17. Mar 10:28, Ming Qian wrote:
> Yes, I have meet some hang issue on nxp's imx platform.
> On nxp's imx platform, there is a stream buffer in the v4l2 decoder driver.
> So driver may cache some frames in driver, if without this patch, after the draining is set to 1, the v4l2 capture buffer won't be enqueued to driver any more, it leds to hang.

Did you try increasing the -num_capture_buffers option? It may solve your
problem.

There is currently a check that sets ctx->done=1 when all the capture buffers
are dequeued (v4l2_context.c:300), and this patch would prevent it (although
it's not needed if an eos event is received).

Perhaps Mark/Aman could also comment?

Thanks,
Andriy

> 
> ________________________________
> 发件人: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> 代表 Andriy Gelman <andriy.gelman@gmail.com>
> 发送时间: 2020年3月16日 20:07
> 收件人: FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org>
> 主题: [EXT] Re: [FFmpeg-devel] [PATCH] avcodec/v4l2_buffers: don't prevent enqueue capture buffer to driver
> 
> Caution: EXT Email
> 
> On Mon, 16. Mar 10:02, Ming Qian wrote:
> > the draining is set when the output port is finished,
> > but it doesn't mean the capture port is finished.
> > especially for decoder, there may be a stream buffer to store several
> > frames.
> > so the decoder still need capture buffer even if the draining is set.
> >
> > Signed-off-by: Ming Qian <ming.qian@nxp.com>
> > ---
> >  libavcodec/v4l2_buffers.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/libavcodec/v4l2_buffers.c b/libavcodec/v4l2_buffers.c
> > index dc1b9eaf24..02f23d954b 100644
> > --- a/libavcodec/v4l2_buffers.c
> > +++ b/libavcodec/v4l2_buffers.c
> > @@ -222,7 +222,7 @@ static void v4l2_free_buffer(void *opaque, uint8_t *unused)
> >              if (!atomic_load(&s->refcount))
> >                  sem_post(&s->refsync);
> >          } else {
> > -            if (s->draining) {
> > +            if (s->draining && V4L2_TYPE_IS_OUTPUT(avbuf->context->type)) {
> >                  /* no need to queue more buffers to the driver */
> >                  avbuf->status = V4L2BUF_AVAILABLE;
> >              }
> 
> It makes sense, but did you have some dropped frames without this?
> 
> Thanks,
> --
> Andriy
Ming Qian April 9, 2020, 2:14 a.m. UTC | #5
Did you try increasing the -num_capture_buffers option? It may solve your problem.
1. We can't increase the num_capture_buffers indefinitely.
2. There is a ring buffer in driver, so the number of frames who is stored in the ring buffer may be a large value, This depends on the speed of the input and output. So it's likely that when output port is done, there are a large count of frames who have not been decoded,  if the capture port prevent qbuf to driver, there will no frame buffer to store the decoded frame.

There is currently a check that sets ctx->done=1 when all the capture buffers are dequeued (v4l2_context.c:300), and this patch would prevent it (although it's not needed if an eos event is received).
Yes, but I think it's not appropriate to judge whether the capture port is done by checking all the capture buffers are dequeued.



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: Thursday, April 9, 2020 9:21 AM
To: FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org>
Subject: Re: [FFmpeg-devel] 回复: [EXT] Re: [PATCH] avcodec/v4l2_buffers: don't prevent enqueue capture buffer to driver

Caution: EXT Email

On Tue, 17. Mar 10:28, Ming Qian wrote:
> Yes, I have meet some hang issue on nxp's imx platform.
> On nxp's imx platform, there is a stream buffer in the v4l2 decoder driver.
> So driver may cache some frames in driver, if without this patch, after the draining is set to 1, the v4l2 capture buffer won't be enqueued to driver any more, it leds to hang.

Did you try increasing the -num_capture_buffers option? It may solve your problem.

There is currently a check that sets ctx->done=1 when all the capture buffers are dequeued (v4l2_context.c:300), and this patch would prevent it (although it's not needed if an eos event is received).

Perhaps Mark/Aman could also comment?

Thanks,
Andriy

>
> ________________________________
> 发件人: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> 代表 Andriy Gelman 
> <andriy.gelman@gmail.com>
> 发送时间: 2020年3月16日 20:07
> 收件人: FFmpeg development discussions and patches 
> <ffmpeg-devel@ffmpeg.org>
> 主题: [EXT] Re: [FFmpeg-devel] [PATCH] avcodec/v4l2_buffers: don't 
> prevent enqueue capture buffer to driver
>
> Caution: EXT Email
>
> On Mon, 16. Mar 10:02, Ming Qian wrote:
> > the draining is set when the output port is finished, but it doesn't 
> > mean the capture port is finished.
> > especially for decoder, there may be a stream buffer to store 
> > several frames.
> > so the decoder still need capture buffer even if the draining is set.
> >
> > Signed-off-by: Ming Qian <ming.qian@nxp.com>
> > ---
> >  libavcodec/v4l2_buffers.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/libavcodec/v4l2_buffers.c b/libavcodec/v4l2_buffers.c 
> > index dc1b9eaf24..02f23d954b 100644
> > --- a/libavcodec/v4l2_buffers.c
> > +++ b/libavcodec/v4l2_buffers.c
> > @@ -222,7 +222,7 @@ static void v4l2_free_buffer(void *opaque, uint8_t *unused)
> >              if (!atomic_load(&s->refcount))
> >                  sem_post(&s->refsync);
> >          } else {
> > -            if (s->draining) {
> > +            if (s->draining && 
> > + V4L2_TYPE_IS_OUTPUT(avbuf->context->type)) {
> >                  /* no need to queue more buffers to the driver */
> >                  avbuf->status = V4L2BUF_AVAILABLE;
> >              }
>
> It makes sense, but did you have some dropped frames without this?
>
> Thanks,
> --
> Andriy

--
Andriy
Andriy Gelman April 9, 2020, 4:27 a.m. UTC | #6
On Thu, 09. Apr 02:14, Ming Qian wrote:
> Did you try increasing the -num_capture_buffers option? It may solve your problem.
> 1. We can't increase the num_capture_buffers indefinitely.
> 2. There is a ring buffer in driver, so the number of frames who is stored in the ring buffer may be a large value, This depends on the speed of the input and output. So it's likely that when output port is done, there are a large count of frames who have not been decoded,  if the capture port prevent qbuf to driver, there will no frame buffer to store the decoded frame.
> 
> There is currently a check that sets ctx->done=1 when all the capture buffers are dequeued (v4l2_context.c:300), and this patch would prevent it (although it's not needed if an eos event is received).

> Yes, but I think it's not appropriate to judge whether the capture port is done by checking all the capture buffers are dequeued.
> 

I tend to agree with you. Even if eos event is not setup/supported, we should
be able to set ctx->done=1 via the EPIPE error.  

But let's wait for some more comments.
Andriy Gelman April 27, 2020, 3:50 a.m. UTC | #7
On Thu, 09. Apr 00:27, Andriy Gelman wrote:
> On Thu, 09. Apr 02:14, Ming Qian wrote:
> > Did you try increasing the -num_capture_buffers option? It may solve your problem.
> > 1. We can't increase the num_capture_buffers indefinitely.
> > 2. There is a ring buffer in driver, so the number of frames who is stored in the ring buffer may be a large value, This depends on the speed of the input and output. So it's likely that when output port is done, there are a large count of frames who have not been decoded,  if the capture port prevent qbuf to driver, there will no frame buffer to store the decoded frame.
> > 
> > There is currently a check that sets ctx->done=1 when all the capture buffers are dequeued (v4l2_context.c:300), and this patch would prevent it (although it's not needed if an eos event is received).
> 
> > Yes, but I think it's not appropriate to judge whether the capture port is done by checking all the capture buffers are dequeued.
> > 
> 
> I tend to agree with you. Even if eos event is not setup/supported, we should
> be able to set ctx->done=1 via the EPIPE error.  
> 
> But let's wait for some more comments. 
> 

In [1] it says that capture buffers should be enqueued back to the device while
draining.

I'll apply this patch on Wednesday if no one objects.


[1] https://patchwork.kernel.org/patch/11110031/

"...The client must continue to handle both queues independently,
   smilarly to normal encode operation. This includes:

   * queuing and dequeuing ``CAPTURE`` buffers, until a buffer marked with the
     ``V4L2_BUF_FLAG_LAST`` flag is dequeued, ... "
Andriy Gelman April 30, 2020, 2:28 a.m. UTC | #8
On Sun, 26. Apr 23:50, Andriy Gelman wrote:
> On Thu, 09. Apr 00:27, Andriy Gelman wrote:
> > On Thu, 09. Apr 02:14, Ming Qian wrote:
> > > Did you try increasing the -num_capture_buffers option? It may solve your problem.
> > > 1. We can't increase the num_capture_buffers indefinitely.
> > > 2. There is a ring buffer in driver, so the number of frames who is stored in the ring buffer may be a large value, This depends on the speed of the input and output. So it's likely that when output port is done, there are a large count of frames who have not been decoded,  if the capture port prevent qbuf to driver, there will no frame buffer to store the decoded frame.
> > > 
> > > There is currently a check that sets ctx->done=1 when all the capture buffers are dequeued (v4l2_context.c:300), and this patch would prevent it (although it's not needed if an eos event is received).
> > 
> > > Yes, but I think it's not appropriate to judge whether the capture port is done by checking all the capture buffers are dequeued.
> > > 
> > 
> > I tend to agree with you. Even if eos event is not setup/supported, we should
> > be able to set ctx->done=1 via the EPIPE error.  
> > 
> > But let's wait for some more comments. 
> > 
> 
> In [1] it says that capture buffers should be enqueued back to the device while
> draining.
> 
> I'll apply this patch on Wednesday if no one objects.
> 

Applied.

Thanks,
diff mbox series

Patch

diff --git a/libavcodec/v4l2_buffers.c b/libavcodec/v4l2_buffers.c
index dc1b9eaf24..02f23d954b 100644
--- a/libavcodec/v4l2_buffers.c
+++ b/libavcodec/v4l2_buffers.c
@@ -222,7 +222,7 @@  static void v4l2_free_buffer(void *opaque, uint8_t *unused)
             if (!atomic_load(&s->refcount))
                 sem_post(&s->refsync);
         } else {
-            if (s->draining) {
+            if (s->draining && V4L2_TYPE_IS_OUTPUT(avbuf->context->type)) {
                 /* no need to queue more buffers to the driver */
                 avbuf->status = V4L2BUF_AVAILABLE;
             }