Skip to content

Increase the stack size to 128 KB#143

Merged
renpytom merged 1 commit into
renpy:fixfrom
Kassy2048:fix-avif-crash2
Nov 5, 2024
Merged

Increase the stack size to 128 KB#143
renpytom merged 1 commit into
renpy:fixfrom
Kassy2048:fix-avif-crash2

Conversation

@Kassy2048
Copy link
Copy Markdown
Contributor

It is 64 KB by default, but aom needs at least 64800 bytes for decoding frames (see the arrays allocated in "av1/common/restoration.c" with size RESTORATION_PROC_UNIT_PELS). IMHO, allocating 64 KB from the stack is criminal, but that's Google code, so I guess they know better...
We already allocate at least 200 MB for the memory, so using 128 KB for the stack sounds reasonable (it is 1 MB by default on some x86 OS for instance).

The cause of the avif decode crash shows up when compiling the web version with -sASSERTIONS=2 (which I didn't use of course, because I love spending hours debugging wasm instructions🥴). Also, the "-fsanitize" build options that are supposed to debug memory problems don't work at all with Ren'Py.

On a side note, upgrading libavif and aom to the latest version (v1.1.1 and v3.10.0 respectively) seems to work well with Ren'Py (but the AVIF_CODEC_AOM cmake parameter for libavif needs to be set to "SYSTEM" instead of "1" now).

Fix renpy/renpy#5871

(New PR against the fix branch this time)

It is 64 KB by default, but aom needs at least 64800 bytes for decoding
frames (see the arrays allocated in "av1/common/restoration.c" with size
RESTORATION_PROC_UNIT_PELS). We already allocate at least 200 MB for the
memory, so using 128 KB for the stack sounds reasonable (it is 1 MB by
default on some x86 OS for instance).

Fix renpy/renpy#5871
@qTich
Copy link
Copy Markdown
Contributor

qTich commented Nov 4, 2024

This fixes the problem, but possibly but after changing the codec to dav1d - #136.
Apparently you will need to increase the stack size even higher - https://code.videolan.org/videolan/dav1d/-/issues/442

@Kassy2048
Copy link
Copy Markdown
Contributor Author

I initially set the stack size to 1 MB and nothing broke, so at least there is no technical problem with increasing it to more than 128 KB if needed to switch to dav1d. As with aom, it is probably only needed for frames/images that use the SGRPROJ restoration filter.

@renpytom renpytom merged commit 2e6d168 into renpy:fix Nov 5, 2024
@renpytom
Copy link
Copy Markdown
Member

renpytom commented Nov 5, 2024

I've merged 128 for now.

1MB probably makes sense for the master branch, especially since there's only one thread on the web platform, so we don't have to worry about the stack size of it. (At least until threading on the web works.)

renpytom added a commit that referenced this pull request Nov 5, 2024
Since we only have the one thread.

Per discussion in #143.
@renpytom
Copy link
Copy Markdown
Member

renpytom commented Nov 5, 2024

I just bumped it up to 1024KB - there's no reason not to have it this big, as again, one thread.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants