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
_______________________________________________
libav-devel mailing list
[email protected]
https://lists.libav.org/mailman/listinfo/libav-devel
