[FFmpeg-devel] [PATCH] ffv1enc: reduce stack usage.

[wm4]

On Tue, 2 Sep 2014 21:18:24 +0200
Reimar Döffinger <Reimar.Doeffinger at gmx.de> wrote:

> On Tue, Sep 02, 2014 at 08:58:50PM +0200, wm4 wrote:
> > On Tue,  2 Sep 2014 20:32:57 +0200
> > Reimar Döffinger <Reimar.Doeffinger at gmx.de> wrote:
> > > @@ -933,6 +938,7 @@ static av_cold int encode_init(AVCodecContext *avctx)
> > >                  }
> > >              }
> > >          }
> > > +        av_freep(&best_state);
> > >      }
> > >  
> > >      if (s->version > 1) {
> > 
> > Is there any specific purpose to these changes? Sure, 64KB stack is
> > pushing a bit, but it should be fine on all normal systems.
> 
> First, that is only true if you assume that the calling application
> did not already use up all but 64 kB of stack.
> As a library, every byte we use on stack is a byte the calling
> application must ensure to leave free.
> Plus, "normal" system depends a lot on your definition.
> If you wanted to run 1000 threads on a 32 bit system, you certainly
> can't use 8MB stacks like we might be used to on our systems.
> Windows uses 1 MB stack by default.
> Since we are a library, I think it is reasonable to say that we
> definitely should not never grab more than 1/4th (and I find that rather
> high personally).
> That leaves us with 256kB. To keep a safety margin since we simply
> don't test all paths, I would suggest testing at least for 128 kB max.
> That does give us the freedom to allow 64 kB allocations if you should
> find my patches too intrusive, it just feels cutting it a bit close
> even on fairly common systems, not to mention that it might prevent
> us from running at all on more obscure systems.
> So I'd prefer to avoid it. However there is the question of which
> code mess/benefit ratio we want to accept.

I don't see anything wrong in the patch (well, maybe you should switch
the code to the "goto fail;" idiom). I was just wondering whether there
was a specific reason for this. Did it fail on a certain system?
Anyway, I'm not opposed to these patches; just curious.