[FFmpeg-devel] [PATCH] Make avcodec_thread_init set the thread_count in the encoder when there isn't support for threads compiled in

[Stefano Sabatini]

On date Wednesday 2009-04-22 01:27:37 +0200, Michael Niedermayer encoded:
> On Tue, Apr 21, 2009 at 10:35:08PM +0200, Stefano Sabatini wrote:
> > On date Monday 2009-04-20 07:01:56 +0200, Michael Niedermayer encoded:
> > > On Sun, Mar 09, 2008 at 01:00:36PM +0100, Stefano Sabatini wrote:
> > > > On date Saturday 2008-03-08 20:31:25 +0100, Michael Dorr encoded:
> > > > > On Saturday 08 March 2008 07:25:49 pm Stefano Sabatini wrote:
> > > > > > On date Saturday 2008-03-08 18:55:26 +0100, Michael Niedermayer encoded:
> > > > > > > On Sat, Mar 08, 2008 at 06:33:27PM +0100, Stefano Sabatini wrote:
> > > > > > > > Hi,
> > > > > > > > as in $subject.
> > > > > > > >
> > > > > > > > If threads support is not enabled and threads count is set to a
> > > > > > > > value greater than MAX_THREADS (currently is 8), then encoder is
> > > > > > > > initialized with a value of thread_count equal to 1, then
> > > > > > > > enc->threads_count is set to the value set by the user, finally
> > > > > > > > libavcodec crashes when it tryies to free the first thread beyond
> > > > > > > > MAX_THREADS (this happens when quitting ffplay).
> > > > > > >
> > > > > > > the command line parsing code should not accept >MAX_THREADS
> > > > > >
> > > > > > [...]
> > > > > >
> > > > > > I agree, but MAX_THREADS is defined in libavcodec/mpegvideo.h, which
> > > > > > is not public, furthermore the patch implements the correct behaviour
> > > > > > for opening an encoder so I'd like to see it applied.
> > > > > 
> > > > > Shouldn't manually setting enc->thread_count be omitted completely, since it 
> > > > > gets set correctly in avcodec_thread_init anyway (and if thread_init isn't 
> > > > > called, it defaults to 1, which is also correct)? Theoretically, 
> > > > > avcodec_thread_init could fail, then enc->thread_count and the 'real' number 
> > > > > of threads will differ, and ffmpeg will crash ...
> > > > 
> > > > Hi,
> > > [...]
> > > > Index: libavcodec/utils.c
> > > > ===================================================================
> > > > --- libavcodec/utils.c	(revision 12393)
> > > > +++ libavcodec/utils.c	(working copy)
> > > > @@ -1350,6 +1350,7 @@
> > > >  
> > > >  #if !defined(HAVE_THREADS)
> > > >  int avcodec_thread_init(AVCodecContext *s, int thread_count){
> > > > +    s->thread_count = thread_count;
> > > >      return -1;
> > > >  }
> > > >  #endif
> > > 
> > > patch ok
> > 
> > Hi, just checked again this patch and it doesn't fix the problem.
> > 
> > MPV_common_init() executes:
> > 
> >     if(s->avctx->thread_count > MAX_THREADS || (s->avctx->thread_count > s->mb_height && s->mb_height)){
> >         av_log(s->avctx, AV_LOG_ERROR, "too many threads\n");
> >         return -1;
> >     }
> > 
> > so s->thread_context[i] are not initialized, then the codec is closed
> > anyway, and in MPV_common_end() we have:
> > 
> >     for(i=0; i<s->avctx->thread_count; i++){
> >         free_duplicate_context(s->thread_context[i]);
> >     }
> > 
> > resulting in a crash.
> > 
> > I suppose the solution is to set s->thread_count to a senseful value
> > (0?) *and* check the return value of avcodec_thread_init() as
> > suggested by Michael Dorr.
> 
> I approved this patch because it allows code to be simplified not
> because it adds some error checking, which it clearly does not.
> 
> I dont think this change introduces any new bugs, but i might miss
> something?

No it doesn't look like it introduces new bugs, but for sure it
doesn't fix the problem for which I posted it the first time.

So when appling I wouldn't know which reason to give for it (I don't
know what it currently simplifies...), that is feel free to apply
it yourself if it speeds up your work.

Regards.
-- 
FFmpeg = Furious & Freak Multimedia Patchable Elected Genius