pthread_frame: fix rv40 threaded decode of clip starting with a bframe

Message ID 1408466716-7757-1-git-send-email-stebbins@jetheaddev.com
State New
Headers show

Commit Message

John Stebbins Aug. 19, 2014, 4:45 p.m.
Finish setting thread state, even if there is an error returned by
update_context_from_thread.
---
Only Luca gave feedback on my previous WIP patch set giving alternatives for
this issue.  But this option was his favorite and also happens to be mine.

I'm hoping Janne can give it a review since I am told he is most familiar
with the threading code.

 libavcodec/pthread_frame.c | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

Comments

Janne Grunau Aug. 21, 2014, 11:11 a.m. | #1
On 2014-08-19 09:45:16 -0700, John Stebbins wrote:
> Finish setting thread state, even if there is an error returned by
> update_context_from_thread.
> ---
> Only Luca gave feedback on my previous WIP patch set giving alternatives for
> this issue.  But this option was his favorite and also happens to be mine.
> 
> I'm hoping Janne can give it a review since I am told he is most familiar
> with the threading code.
> 
>  libavcodec/pthread_frame.c | 11 +++--------
>  1 file changed, 3 insertions(+), 8 deletions(-)
> 
> diff --git a/libavcodec/pthread_frame.c b/libavcodec/pthread_frame.c
> index a658f3e..c3846bb 100644
> --- a/libavcodec/pthread_frame.c
> +++ b/libavcodec/pthread_frame.c
> @@ -298,6 +298,7 @@ static int submit_packet(PerThreadContext *p, AVPacket *avpkt)
>      FrameThreadContext *fctx = p->parent;
>      PerThreadContext *prev_thread = fctx->prev_thread;
>      const AVCodec *codec = p->avctx->codec;
> +    int err = 0;
>  
>      if (!avpkt->size && !(codec->capabilities & CODEC_CAP_DELAY)) return 0;
>  
> @@ -306,7 +307,6 @@ static int submit_packet(PerThreadContext *p, AVPacket *avpkt)
>      release_delayed_buffers(p);
>  
>      if (prev_thread) {
> -        int err;
>          if (prev_thread->state == STATE_SETTING_UP) {
>              pthread_mutex_lock(&prev_thread->progress_mutex);
>              while (prev_thread->state == STATE_SETTING_UP)
> @@ -315,10 +315,6 @@ static int submit_packet(PerThreadContext *p, AVPacket *avpkt)
>          }
>  
>          err = update_context_from_thread(p->avctx, prev_thread->avctx, 0);
> -        if (err) {
> -            pthread_mutex_unlock(&p->mutex);
> -            return err;
> -        }

I'm not 100% sure this is safe. update_context_from_thread is used to 
set the current context. I don't think the context will be in safe state 
to start decoding of the next frame.

It's safe in the rv40 start with b-frame error case since the context is 
not initialized at all and reflects that.

Most (maybe all) decoder contexts have context_initialized variable. We 
could require that update_thread_context() reset that to 0 if it fails.  
That would need a documentation update and a review of all existing 
code.

>      }
>  
>      av_packet_unref(&p->avpkt);
> @@ -358,7 +354,7 @@ FF_ENABLE_DEPRECATION_WARNINGS
>      fctx->prev_thread = p;
>      fctx->next_decoding++;
>  
> -    return 0;
> +    return err;
>  }

I'm not sure if passing the error from update_context_from_thread along 
makes much sense if we decode the next frame anyway. It also may cause 
problems with the delaying logic.

>  int ff_thread_decode_frame(AVCodecContext *avctx,
> @@ -374,6 +370,7 @@ int ff_thread_decode_frame(AVCodecContext *avctx,
>       * Submit a packet to the next decoding thread.
>       */
>  
> +    if (fctx->next_decoding >= avctx->thread_count) fctx->next_decoding = 0;
>      p = &fctx->threads[fctx->next_decoding];
>      err = update_context_from_user(p->avctx, avctx);
>      if (err) return err;
> @@ -426,8 +423,6 @@ int ff_thread_decode_frame(AVCodecContext *avctx,
>  
>      update_context_from_thread(avctx, p->avctx, 1);
>  
> -    if (fctx->next_decoding >= avctx->thread_count) fctx->next_decoding = 0;
> -
>      fctx->next_finished = finished;
>  
>      /* return the size of the consumed packet if no error occurred */

There might have been a reason why the wrap-around was here? if there is 
no reason I think it would be best to add the wrap-around logic to the 
only place which increases next_decoding in submit_frame().

Janne
John Stebbins Aug. 21, 2014, 2:55 p.m. | #2
On 08/21/2014 04:11 AM, Janne Grunau wrote:
> On 2014-08-19 09:45:16 -0700, John Stebbins wrote:
>> Finish setting thread state, even if there is an error returned by
>> update_context_from_thread.
>> ---
>> Only Luca gave feedback on my previous WIP patch set giving alternatives for
>> this issue.  But this option was his favorite and also happens to be mine.
>>
>> I'm hoping Janne can give it a review since I am told he is most familiar
>> with the threading code.
>>
>>  libavcodec/pthread_frame.c | 11 +++--------
>>  1 file changed, 3 insertions(+), 8 deletions(-)
>>
>> diff --git a/libavcodec/pthread_frame.c b/libavcodec/pthread_frame.c
>> index a658f3e..c3846bb 100644
>> --- a/libavcodec/pthread_frame.c
>> +++ b/libavcodec/pthread_frame.c
>> @@ -298,6 +298,7 @@ static int submit_packet(PerThreadContext *p, AVPacket *avpkt)
>>      FrameThreadContext *fctx = p->parent;
>>      PerThreadContext *prev_thread = fctx->prev_thread;
>>      const AVCodec *codec = p->avctx->codec;
>> +    int err = 0;
>>  
>>      if (!avpkt->size && !(codec->capabilities & CODEC_CAP_DELAY)) return 0;
>>  
>> @@ -306,7 +307,6 @@ static int submit_packet(PerThreadContext *p, AVPacket *avpkt)
>>      release_delayed_buffers(p);
>>  
>>      if (prev_thread) {
>> -        int err;
>>          if (prev_thread->state == STATE_SETTING_UP) {
>>              pthread_mutex_lock(&prev_thread->progress_mutex);
>>              while (prev_thread->state == STATE_SETTING_UP)
>> @@ -315,10 +315,6 @@ static int submit_packet(PerThreadContext *p, AVPacket *avpkt)
>>          }
>>  
>>          err = update_context_from_thread(p->avctx, prev_thread->avctx, 0);
>> -        if (err) {
>> -            pthread_mutex_unlock(&p->mutex);
>> -            return err;
>> -        }
> I'm not 100% sure this is safe. update_context_from_thread is used to 
> set the current context. I don't think the context will be in safe state 
> to start decoding of the next frame.
>
> It's safe in the rv40 start with b-frame error case since the context is 
> not initialized at all and reflects that.
>
> Most (maybe all) decoder contexts have context_initialized variable. We 
> could require that update_thread_context() reset that to 0 if it fails.  
> That would need a documentation update and a review of all existing 
> code.

I was afraid this might be the case.  I think I'll investigate further to see if I can figure out exactly what causes
the deadlock that prevents 'q' from exiting avplay though.

>>      }
>>  
>>      av_packet_unref(&p->avpkt);
>> @@ -358,7 +354,7 @@ FF_ENABLE_DEPRECATION_WARNINGS
>>      fctx->prev_thread = p;
>>      fctx->next_decoding++;
>>  
>> -    return 0;
>> +    return err;
>>  }
> I'm not sure if passing the error from update_context_from_thread along 
> makes much sense if we decode the next frame anyway. It also may cause 
> problems with the delaying logic.
>
>>  int ff_thread_decode_frame(AVCodecContext *avctx,
>> @@ -374,6 +370,7 @@ int ff_thread_decode_frame(AVCodecContext *avctx,
>>       * Submit a packet to the next decoding thread.
>>       */
>>  
>> +    if (fctx->next_decoding >= avctx->thread_count) fctx->next_decoding = 0;
>>      p = &fctx->threads[fctx->next_decoding];
>>      err = update_context_from_user(p->avctx, avctx);
>>      if (err) return err;
>> @@ -426,8 +423,6 @@ int ff_thread_decode_frame(AVCodecContext *avctx,
>>  
>>      update_context_from_thread(avctx, p->avctx, 1);
>>  
>> -    if (fctx->next_decoding >= avctx->thread_count) fctx->next_decoding = 0;
>> -
>>      fctx->next_finished = finished;
>>  
>>      /* return the size of the consumed packet if no error occurred */
> There might have been a reason why the wrap-around was here? if there is 
> no reason I think it would be best to add the wrap-around logic to the 
> only place which increases next_decoding in submit_frame().

I checked the usage of next_decoding to be certain that it was safe to move where I put it.
You can't do the wrap-around in submit_frame because of the way next_decoding is used in ff_thread_decode_frame.

Patch

diff --git a/libavcodec/pthread_frame.c b/libavcodec/pthread_frame.c
index a658f3e..c3846bb 100644
--- a/libavcodec/pthread_frame.c
+++ b/libavcodec/pthread_frame.c
@@ -298,6 +298,7 @@  static int submit_packet(PerThreadContext *p, AVPacket *avpkt)
     FrameThreadContext *fctx = p->parent;
     PerThreadContext *prev_thread = fctx->prev_thread;
     const AVCodec *codec = p->avctx->codec;
+    int err = 0;
 
     if (!avpkt->size && !(codec->capabilities & CODEC_CAP_DELAY)) return 0;
 
@@ -306,7 +307,6 @@  static int submit_packet(PerThreadContext *p, AVPacket *avpkt)
     release_delayed_buffers(p);
 
     if (prev_thread) {
-        int err;
         if (prev_thread->state == STATE_SETTING_UP) {
             pthread_mutex_lock(&prev_thread->progress_mutex);
             while (prev_thread->state == STATE_SETTING_UP)
@@ -315,10 +315,6 @@  static int submit_packet(PerThreadContext *p, AVPacket *avpkt)
         }
 
         err = update_context_from_thread(p->avctx, prev_thread->avctx, 0);
-        if (err) {
-            pthread_mutex_unlock(&p->mutex);
-            return err;
-        }
     }
 
     av_packet_unref(&p->avpkt);
@@ -358,7 +354,7 @@  FF_ENABLE_DEPRECATION_WARNINGS
     fctx->prev_thread = p;
     fctx->next_decoding++;
 
-    return 0;
+    return err;
 }
 
 int ff_thread_decode_frame(AVCodecContext *avctx,
@@ -374,6 +370,7 @@  int ff_thread_decode_frame(AVCodecContext *avctx,
      * Submit a packet to the next decoding thread.
      */
 
+    if (fctx->next_decoding >= avctx->thread_count) fctx->next_decoding = 0;
     p = &fctx->threads[fctx->next_decoding];
     err = update_context_from_user(p->avctx, avctx);
     if (err) return err;
@@ -426,8 +423,6 @@  int ff_thread_decode_frame(AVCodecContext *avctx,
 
     update_context_from_thread(avctx, p->avctx, 1);
 
-    if (fctx->next_decoding >= avctx->thread_count) fctx->next_decoding = 0;
-
     fctx->next_finished = finished;
 
     /* return the size of the consumed packet if no error occurred */