Re: [FFmpeg-devel] [PATCH] ffv1enc: reduce stack usage.
On Tue, Sep 02, 2014 at 09:44:05PM +0200, Michael Niedermayer wrote: > On Tue, Sep 02, 2014 at 08:32:57PM +0200, Reimar Döffinger wrote: > > A bit more complex than e.g. adding it to the context, but > > using the context for something that will be used only during > > initialization seemed a bit wasteful. > > > > Signed-off-by: Reimar Döffinger > > LGTM Pushed. ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] ffv1enc: reduce stack usage.
On Tue, Sep 02, 2014 at 09:54:36PM +0200, Reimar Döffinger wrote: > On Tue, Sep 02, 2014 at 09:28:27PM +0200, wm4 wrote: > > 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? > > Btw. while I did not test it, and ignoring HP-UX and AIX (16k and 96k > stack it seems), OpenBSD 4.6 release notes say: > Increased the default thread stack size to 256k for 32-bit hosts and to > 512k on 64-bit hosts. > > Before it was 64k, so it should fail on an OpenBSD system from 2009 :) And sorry if I write far more than you asked for, but there is also a security aspect to it. Those large objects on stack are usually arrays. If anything you do depends on user input, there is a risk that you will have a buffer overflow. At least without stack smashing protection (which I think we do not enable ourselves, though many distributions do by default), stack buffer overflows are near trivial to exploit. A head buffer overflow is at least more work, plus you can surround every allocation with guard pages if you are paranoid, which stops them - even if at a possibly massive cost in (virtual and physical) memory usage. I believe this was the reasoning behind the OpenBSD 64kB limit, stopping people from putting buffers on the stack. ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] ffv1enc: reduce stack usage.
On Tue, 2 Sep 2014 21:54:36 +0200 Reimar Döffinger wrote: > On Tue, Sep 02, 2014 at 09:28:27PM +0200, wm4 wrote: > > 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? > > Btw. while I did not test it, and ignoring HP-UX and AIX (16k and 96k > stack it seems), OpenBSD 4.6 release notes say: > Increased the default thread stack size to 256k for 32-bit hosts and to > 512k on 64-bit hosts. > > Before it was 64k, so it should fail on an OpenBSD system from 2009 :) Seems like a good enough reason. ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] ffv1enc: reduce stack usage.
On Tue, Sep 02, 2014 at 09:28:27PM +0200, wm4 wrote: > 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? Btw. while I did not test it, and ignoring HP-UX and AIX (16k and 96k stack it seems), OpenBSD 4.6 release notes say: Increased the default thread stack size to 256k for 32-bit hosts and to 512k on 64-bit hosts. Before it was 64k, so it should fail on an OpenBSD system from 2009 :) ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] ffv1enc: reduce stack usage.
On Tue, Sep 02, 2014 at 08:32:57PM +0200, Reimar Döffinger wrote: > A bit more complex than e.g. adding it to the context, but > using the context for something that will be used only during > initialization seemed a bit wasteful. > > Signed-off-by: Reimar Döffinger LGTM [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB The bravest are surely those who have the clearest vision of what is before them, glory and danger alike, and yet notwithstanding go out to meet it. -- Thucydides signature.asc Description: Digital signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] ffv1enc: reduce stack usage.
On Tue, Sep 02, 2014 at 09:28:27PM +0200, wm4 wrote: > On Tue, 2 Sep 2014 21:18:24 +0200 > Reimar Döffinger wrote: > > 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 generally prefer that, however the only way I can see that does not result in a total mess is if I first extract this part into a separate function (of you can see a trivial way to use goto here, I'd like to hear it). Probably a good idea anyway, but I didn't feel like getting completely sidetracked with unrelated code clean-up again. > I was just wondering whether there > was a specific reason for this. Did it fail on a certain system? No, only trying to run "make fate" with "ulimit -s 64". > Anyway, I'm not opposed to these patches; just curious. Sorry, if I sound defensive or anything, that would be because I am myself not completely convinced if and how far we should take it. ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] ffv1enc: reduce stack usage.
On Tue, 2 Sep 2014 21:18:24 +0200 Reimar Döffinger 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 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. ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] ffv1enc: reduce stack usage.
On Tue, Sep 02, 2014 at 08:58:50PM +0200, wm4 wrote: > On Tue, 2 Sep 2014 20:32:57 +0200 > Reimar Döffinger 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. ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] ffv1enc: reduce stack usage.
On Tue, 2 Sep 2014 20:32:57 +0200 Reimar Döffinger wrote: > A bit more complex than e.g. adding it to the context, but > using the context for something that will be used only during > initialization seemed a bit wasteful. > > Signed-off-by: Reimar Döffinger > --- > libavcodec/ffv1enc.c | 8 +++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/libavcodec/ffv1enc.c b/libavcodec/ffv1enc.c > index b63ed42..4611f94 100644 > --- a/libavcodec/ffv1enc.c > +++ b/libavcodec/ffv1enc.c > @@ -862,9 +862,11 @@ static av_cold int encode_init(AVCodecContext *avctx) > } > if (avctx->stats_in) { > char *p = avctx->stats_in; > -uint8_t best_state[256][256]; > +uint8_t (*best_state)[256] = av_malloc_array(256, 256); > int gob_count = 0; > char *next; > + if (!best_state) > +return AVERROR(ENOMEM); > > av_assert0(s->version >= 2); > > @@ -875,6 +877,7 @@ static av_cold int encode_init(AVCodecContext *avctx) > if (next == p) { > av_log(avctx, AV_LOG_ERROR, > "2Pass file invalid at %d %d [%s]\n", j, i, > p); > +av_freep(&best_state); > return AVERROR_INVALIDDATA; > } > p = next; > @@ -888,6 +891,7 @@ static av_cold int encode_init(AVCodecContext *avctx) > av_log(avctx, AV_LOG_ERROR, > "2Pass file invalid at %d %d %d %d > [%s]\n", > i, j, k, m, p); > +av_freep(&best_state); > return AVERROR_INVALIDDATA; > } > p = next; > @@ -896,6 +900,7 @@ static av_cold int encode_init(AVCodecContext *avctx) > gob_count = strtol(p, &next, 0); > if (next == p || gob_count <= 0) { > av_log(avctx, AV_LOG_ERROR, "2Pass file invalid\n"); > +av_freep(&best_state); > return AVERROR_INVALIDDATA; > } > p = next; > @@ -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. ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel