Re: [FFmpeg-devel] [PATCH] ffv1enc: reduce stack usage.
On Tue, 2 Sep 2014 20:32:57 +0200
Reimar Döffinger reimar.doeffin...@gmx.de 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 reimar.doeffin...@gmx.de
---
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
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 reimar.doeffin...@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.
___
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 reimar.doeffin...@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.doeffin...@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.
___
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 reimar.doeffin...@gmx.de 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, 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 reimar.doeffin...@gmx.de 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: 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, 2 Sep 2014 21:54:36 +0200 Reimar Döffinger reimar.doeffin...@gmx.de 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
