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. -- John GnuPG fingerprint: D0EC B3DB C372 D1F1 0B01 83F0 49F1 D7B2 60D4 D0F7
signature.asc
Description: OpenPGP digital signature
_______________________________________________ libav-devel mailing list [email protected] https://lists.libav.org/mailman/listinfo/libav-devel
