[FFmpeg-devel] [PATCH] pthread: Fix ff_thread_get_format issues when called outside frame decode.

[Reimar Döffinger]

On 26.02.2015, at 23:46, Hendrik Leppkes <h.leppkes at gmail.com> wrote:
> On Thu, Feb 26, 2015 at 11:21 PM, Reimar Döffinger
> <Reimar.Doeffinger at gmx.de> wrote:
>> When ff_thread_get_format is called from the main thread, e.g.
>> during codec init it will access the thread_ctx as a PerThreadContext
>> even though it is a FrameThreadContext.
> 
> Where is this a problem exactly? I don't see a single codec that calls
> it during codec init.
> The only codecs that actually call it are h264 and hevc, and thats in
> slice header decoding, not codec init (and mpeg12, which doesn't do
> frame threading)

HEVC seems to call it during extradata parsing as far as I can tell, though I do not have a test-case.

>> In addition, when ff_thread_get_format is called during the context
>> update it would get confused because the state is not as expected.
>> Make it handle that case by adding a new state for this, signalling
>> that we are actually executing in the main thread and thus can
>> call get_format directly in all cases.
> 
> When would this ever happen? MT decoding works perfectly fine in a
> number of applications as it is.

When would what happen? If you disable thread-safe callbacks then HEVC will fail to decode anything with multithreading, so it's a fairly obvious issue. Though not many using HEVC I guess.
Note that I think this might just be a bug, since it means if you run with n threads then the HEVC decoder after each sps change will call get_format n times with exactly the same arguments.
The context update should probably just copy the selected format instead of ending up calling get_format due to reusing set_sps.

>> @@ -743,7 +754,10 @@ void ff_thread_flush(AVCodecContext *avctx)
>> int ff_thread_can_start_frame(AVCodecContext *avctx)
>> {
>>     PerThreadContext *p = avctx->internal->thread_ctx;
>> -    if ((avctx->active_thread_type&FF_THREAD_FRAME) && p->state != STATE_SETTING_UP &&
>> +    if (avctx->active_thread_type&FF_THREAD_FRAME)
>> +        return 0;
>> +    av_assert0(!p->main_thread);
>> +    if (p->state != STATE_SETTING_UP &&
>>         (avctx->codec->update_thread_context || !THREAD_SAFE_CALLBACKS(avctx))) {
>>         return 0;
>>     }
> 
> This hunk looks rather suspicious. Returning 0 unconditionally on
> active_thread_type & FF_THREAD_FRAME?

Yes, I flipped the condition there.

> In general, I'm rather cautious with any changes to the threading
> model, so these should be clearly warranted and explained properly.

My biggest issue is that we store two completely different structs in the same variable, and all those threading functions just assume that they will always be called only from the "right" places.
Otherwise they'd end up accessing things as the wrong struct.
I don't mind splitting the patch and/or fixing HEVC differently, but this should be improved IMO.
Though just having both a "main" and a "worker" thread_ctx variable for example might be a cleaner solution than what I have in this patch.