Emscripten_UpdateWindowFramebuffer fails if pixel data is higher than 2gb in address space

[sbc100]

Specifically the line var src = pixels >> 2 needs updating.

It should probably be:

#if __wasm64__
   var src = pixels  / 4;
#else
   var src = pixels >>> 0; 
#endif 

(I can work look into uploading a PR for this)

[icculus]

Yes, please send a PR when you get a chance; this feels like something I'll get wrong if I try to do it myself. :)

[ericoporto]

SDL/src/video/emscripten/SDL_emscriptenframebuffer.c

Line 92 in cd197be

var src = pixels >> 2;

Trying to understand the issuepixels is a void* (I guess of size either 4 or 8 depending on it being a wasm64) and HEAP32 sees the memory like a sequence of 32-bit int. The SDL_Surface->pixels is allocated using SDL_SIMDAlloc which takes the size in bytes.

>>> 0 is a trick to cast to 32-bit int, it's a weird change assuming the current >> 2 is working (this is the same as / 4 here?)

[sbc100]

Sorry I meant to write >>> 2. The triple right shift is an unsigned shift (which is want) and the double right shift is a signed shift (which don't work for 32-bit addresses in the upper half of the range).

[sbc100]

If this is not in performance critical code you could just use a divide by 4 operation in all cases and keep the code simple.

[icculus]

Just coming back to this, do we just need that one var src = pixels >> 2; line fixed? I'll put that in revision control today if so!

[sbc100]

Any place >> or >>> is used on a pointer value would need to be fixed. I don't know if var src = pixels >> 2 is the only one.

[slouken]

We are scoping work for the SDL 3.2.0 release, so please let us know if this is a showstopper for you.

[sbc100]

This would be a showstopper for supporting memory64 AKA wasm64. It looks like there are 3 places that would need updating:

src/video/emscripten/SDL_emscriptenevents.c:                        var idx = ptr >> 2;
src/video/emscripten/SDL_emscriptenframebuffer.c:        var src = pixels >> 2;
src/video/emscripten/SDL_emscriptenmouse.c:        var src = pixels >> 2;

Should I send a RP for this? If so could it also be backported to the SDL2 branch (which emscritpen uses)?

[slouken]

This would be a showstopper for supporting memory64 AKA wasm64. It looks like there are 3 places that would need updating:

src/video/emscripten/SDL_emscriptenevents.c:                        var idx = ptr >> 2;
src/video/emscripten/SDL_emscriptenframebuffer.c:        var src = pixels >> 2;
src/video/emscripten/SDL_emscriptenmouse.c:        var src = pixels >> 2;

Should I send a RP for this? If so could it also be backported to the SDL2 branch (which emscritpen uses)?

Yes please. :) We can backport it, that's fine.

[icculus]

I'm surprised there are only three places we need this fix.

[sbc100]

I'm surprised there are only three places we need this fix.

I'll do a more thorough search. I can also very that all the emscripten-side tests pass under wasm64 after making the change.

[slouken]

FYI, the fix for this is in the latest SDL2 release:
https://github.com/libsdl-org/SDL/releases/tag/release-2.30.9

[sbc100]

Thanks: emscripten-core/emscripten#22830