diff mbox series

[FFmpeg-devel] avcodec/videotoolboxenc: don't wait when flushing data

Message ID 20200827025327.28334-1-lq@chinaffmpeg.org
State Accepted
Commit 1cbea3f9caa8d8641f749219a0c207320908778f
Headers show
Series [FFmpeg-devel] avcodec/videotoolboxenc: don't wait when flushing data
Related show

Checks

Context Check Description
andriy/default pending
andriy/make success Make finished
andriy/make_fate success Make fate finished

Commit Message

Steven Liu Aug. 27, 2020, 2:53 a.m. UTC
From: Tian Qi <tianqi@kuaishou.com>

because there is run in thread mode, few times will block
the workflow at the wait, so check the status is flushing data,
don't wait when flushing data.

Signed-off-by: Tian Qi <tianqi@kuaishou.com>
Signed-off-by: Steven Liu <lq@chinaffmpeg.org>
---
 libavcodec/videotoolboxenc.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

zhilizhao(赵志立) Aug. 26, 2020, 11:17 p.m. UTC | #1
> On Aug 27, 2020, at 10:53 AM, Steven Liu <lq@chinaffmpeg.org> wrote:
> 
> From: Tian Qi <tianqi@kuaishou.com>
> 
> because there is run in thread mode, few times will block
> the workflow at the wait, so check the status is flushing data,
> don't wait when flushing data.
> 
> Signed-off-by: Tian Qi <tianqi@kuaishou.com>
> Signed-off-by: Steven Liu <lq@chinaffmpeg.org>
> ---
> libavcodec/videotoolboxenc.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/libavcodec/videotoolboxenc.c b/libavcodec/videotoolboxenc.c
> index e89cfaeed8..f9626be18f 100644
> --- a/libavcodec/videotoolboxenc.c
> +++ b/libavcodec/videotoolboxenc.c
> @@ -292,7 +292,7 @@ static int vtenc_q_pop(VTEncContext *vtctx, bool wait, CMSampleBufferRef *buf, E
>         return 0;
>     }
> 
> -    while (!vtctx->q_head && !vtctx->async_error && wait) {
> +    while (!vtctx->q_head && !vtctx->async_error && wait && !vtctx->flushing) {
>         pthread_cond_wait(&vtctx->cv_sample_sent, &vtctx->lock);
>     }

I’m not familar with the code. I have some questions:
1. The commit message doesn’t give a detailed description of the real issue.

2. vtctx->flushing is modified without holding the lock, which is questionable.
And there is no cond_signal, how does it work?

> 
> @@ -308,6 +308,7 @@ static int vtenc_q_pop(VTEncContext *vtctx, bool wait, CMSampleBufferRef *buf, E
>         vtctx->q_tail = NULL;
>     }
> 
> +    vtctx->frame_ct_out++;

Looks like this modification doesn’t belong to the patch.

>     pthread_mutex_unlock(&vtctx->lock);
> 
>     *buf = info->cm_buffer;
> @@ -319,7 +320,6 @@ static int vtenc_q_pop(VTEncContext *vtctx, bool wait, CMSampleBufferRef *buf, E
>     }
>     av_free(info);
> 
> -    vtctx->frame_ct_out++;
> 
>     return 0;
> }
> -- 
> 2.25.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".
Richard Kern Aug. 28, 2020, 1:14 p.m. UTC | #2
On Wed, Aug 26, 2020 at 10:53 PM Steven Liu <lq@chinaffmpeg.org> wrote:

> From: Tian Qi <tianqi@kuaishou.com>
>
> because there is run in thread mode, few times will block
> the workflow at the wait, so check the status is flushing data,
> don't wait when flushing data.
>
I'd like to reproduce the issue and test the fix before the patch is
applied, so if you can provide a media file that causes the issue or code
snippet, it would be helpful.

It looks like this could happen after an encoding error. Do you see any
"Error encoding frame ..." logs?

>
> Signed-off-by: Tian Qi <tianqi@kuaishou.com>
> Signed-off-by: Steven Liu <lq@chinaffmpeg.org>
> ---
>  libavcodec/videotoolboxenc.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/libavcodec/videotoolboxenc.c b/libavcodec/videotoolboxenc.c
> index e89cfaeed8..f9626be18f 100644
> --- a/libavcodec/videotoolboxenc.c
> +++ b/libavcodec/videotoolboxenc.c
> @@ -292,7 +292,7 @@ static int vtenc_q_pop(VTEncContext *vtctx, bool wait,
> CMSampleBufferRef *buf, E
>          return 0;
>      }
>
> -    while (!vtctx->q_head && !vtctx->async_error && wait) {
> +    while (!vtctx->q_head && !vtctx->async_error && wait &&
> !vtctx->flushing) {
>          pthread_cond_wait(&vtctx->cv_sample_sent, &vtctx->lock);
>      }
>
> @@ -308,6 +308,7 @@ static int vtenc_q_pop(VTEncContext *vtctx, bool wait,
> CMSampleBufferRef *buf, E
>          vtctx->q_tail = NULL;
>      }
>
> +    vtctx->frame_ct_out++;
>      pthread_mutex_unlock(&vtctx->lock);
>
>      *buf = info->cm_buffer;
> @@ -319,7 +320,6 @@ static int vtenc_q_pop(VTEncContext *vtctx, bool wait,
> CMSampleBufferRef *buf, E
>      }
>      av_free(info);
>
> -    vtctx->frame_ct_out++;
>
>      return 0;
>  }
> --
> 2.25.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".
diff mbox series

Patch

diff --git a/libavcodec/videotoolboxenc.c b/libavcodec/videotoolboxenc.c
index e89cfaeed8..f9626be18f 100644
--- a/libavcodec/videotoolboxenc.c
+++ b/libavcodec/videotoolboxenc.c
@@ -292,7 +292,7 @@  static int vtenc_q_pop(VTEncContext *vtctx, bool wait, CMSampleBufferRef *buf, E
         return 0;
     }
 
-    while (!vtctx->q_head && !vtctx->async_error && wait) {
+    while (!vtctx->q_head && !vtctx->async_error && wait && !vtctx->flushing) {
         pthread_cond_wait(&vtctx->cv_sample_sent, &vtctx->lock);
     }
 
@@ -308,6 +308,7 @@  static int vtenc_q_pop(VTEncContext *vtctx, bool wait, CMSampleBufferRef *buf, E
         vtctx->q_tail = NULL;
     }
 
+    vtctx->frame_ct_out++;
     pthread_mutex_unlock(&vtctx->lock);
 
     *buf = info->cm_buffer;
@@ -319,7 +320,6 @@  static int vtenc_q_pop(VTEncContext *vtctx, bool wait, CMSampleBufferRef *buf, E
     }
     av_free(info);
 
-    vtctx->frame_ct_out++;
 
     return 0;
 }