search

emscripten-ports / SDL2_image

Fork of https://github.com/libsdl-org/SDL_image
https://github.com/libsdl-org/SDL_image
Other
16 stars 11 forks source link

IMG_Load should attempt to read from preloaded data #7

Closed gliheng closed 6 years ago

gliheng commented 6 years ago

I'm trying to pull data from web then put it on the canvas. After I pass the image data to emscripten_run_preload_plugins_data, I use IMG_Load to get a surface and I got a NULL pointer. I read SDL1 test case and it works fine, but SDL2 does not have a test case for emscripten_run_preload_plugins_data.

After some digging, I think IMG_Load should attempt to read from preloaded data and here's the fix for IMG_Load.

kripken commented 6 years ago

Thanks! Looks great.

kripken commented 6 years ago

I added a version_4 tag for this now. Would you like to open a PR in emscripten that adds a test for this, and updates the port to that version?

gliheng commented 6 years ago

Okay, I'll do that.

gliheng commented 6 years ago

And here's the pull request to emscripten with tests.

Beuc commented 6 years ago

Posted upstream for consideration at https://bugzilla.libsdl.org/show_bug.cgi?id=4203

Daft-Freak commented 5 years ago

A bit late, but this patch shouldn't be necessary.

IMG_Load calls IMG_LoadTyped_RW which has handling for preloaded images already. However, it fails as emscripten_run_preload_plugins_data doesn't actually create the filename it returns, so it fails to open the file and returns early. I tried switching that true to false and reverting to version_3 and the test still passes.

Beuc commented 5 years ago

Hmm, what patch are you writing about? The one I sent upstream is from the SDL2 emscripten port (but not in upstream https://hg.libsdl.org/SDL_image/file/tip/IMG.c). The other 2 referenced here are already merged?

Daft-Freak commented 5 years ago

This PR shouldn't be needed after my original patch (58d0d10569b633375a2a24958f62a29e7899083e). It looks like it might be hiding an Emscripten bug. It may make sense to upstream that patch though (with the fix from 38ddf6804e271da91691bd8eb42b2e72e49cff45), I just never looked into it.

EDIT: Though, the point of emscripten_run_preload_plugins_data is to avoid writing the file. And the "fake filename" was designed for the old JS IMG_Load. It still seems wrong to duplicate the Emscripten support for just that function though...

Beuc commented 5 years ago

It's worth nothing that the equivalent preloadedAudios wasn't ported to SDL2(_mixer) at all.