ENH Support SDL-based packages and add pyxel #3508

ryanking13 · 2023-01-25T13:01:36Z

Description

This adds SDL-based package support to Pyodide and adds pyxel as a PoC. Most of the troubleshooting is done by @kitao and pyxel devs, a big thanks to them.

This is on top of #3505

I linked only js part of SDL libraries to the main module and linked the native parts to the side module. So the size increase of the main module is small enough (~0.1MB increase to the pyodide.asm.js).

Before (in #3505):

5145705 pyodide.asm.data
1204075 pyodide.asm.js
8862216 pyodide.asm.wasm

After:

5145705 pyodide.asm.data
1346524 pyodide.asm.js
8905514 pyodide.asm.wasm

Emscripten draws everything into Module.canvas, but the canvas attribute is not set automatically (which makes sense as we don't output any HTML), so it is the user's job to set it manually.

For now, I am thinking of just mentioning it in the FAQ entry, "you need to set pyodide._module.canvas = ...". But it would be probably nice to have an API to set canvas, as pyodide._module is a private attribute.

Checklists

Add a CHANGELOG entry
Add / update tests
Add new / update outdated documentation

Demo

https://ryanking13.github.io/pyodide-pyxel-test

(mostly borrowed from the pyxel repository: https://github.com/kitao/pyxel/tree/main/wasm)

This reverts commit 60e0026.

pmp-p · 2023-02-27T05:06:49Z

i'd be really curious to compare sizes when adding this -sUSE_WEBGL2 -sMIN_WEBGL_VERSION=2 -sOFFSCREENCANVAS_SUPPORT=1 -sFULL_ES2 -sFULL_ES3

I suspect SDL2 brings most of that stuff already in, but those params could help a lot of others packages like pyopengl(es) / moderngl / raylib / harfang / Panda3D and probably others.

ryanking13 · 2023-02-27T05:28:35Z

i'd be really curious to compare sizes when adding this -sUSE_WEBGL2 -sMIN_WEBGL_VERSION=2 -sOFFSCREENCANVAS_SUPPORT=1 -sFULL_ES2 -sFULL_ES3. I suspect SDL2 brings most of that stuff already in, but those params could help a lot of others packages like pyopengl(es) / moderngl / raylib / harfang / Panda3D and probably others.

Sounds great. USE_WEGBL2 flag will add library_webgl2.js (46.2 KB), and other flags will not add additional javascript files, so I think the size increase would be quite small.

pmp-p · 2023-03-01T08:31:28Z

i was a bit wrong including raylib this one need another binding -sUSE_GLFW=3 maybe it will be more heavy since not sdl2

but 46K seems great for such openness

JeffersGlass · 2023-03-03T21:17:03Z

Hi Y'all - Jeff here on the PyScript team. I'm quite excited about the possibility of using Pyxel (and other SDL-based packages) in the browser - having built a copy of Pyodide from this branch and tried out Pyxel, I'm hugely impressed by how much just works. And the ability for Pyxel to interact with the DOM, window events etc opens up a ton of really exciting possibilities.

If there's anything I can help with on this - testing, feedback, writing docs/tests, actual code - I'd be happy to contribute if it would be helpful.

ryanking13 · 2023-03-14T01:52:51Z

@szabolcsdombi Thanks for sharing your glue code!

This is ready to be reviewed now. I think we can improve multi canvas issues, GLFW support, and other things incrementally as a follow up as @pmp-p mentioned. For now, this PR adds support for pyxel so I think it is sufficient for this PR.

hoodmane · 2023-03-22T15:16:23Z

docs/usage/sdl.md

+```{admonition} This is experimental
+:class: warning
+
+SDL support in Pyodide is experimental.


Would still be nice to standardize our stability guarantees a little more. (No action needed.)

Yes indeed. I'll add some more explanation that we are depending on the non-documented behavior of Emscripten / SDL libraries.

packages/pyxel/meta.yaml

packages/pyxel/test_pyxel.py

hoodmane · 2023-03-22T15:29:35Z

src/core/pyproxy.ts

+    if (e && e === "unwind") {
+      return;
+    }


I think this might be handled better in API.fatal_error (we already handle exit there). Then all of these could change to

API.fatal_error(e); return; // in case it's not actually a fatal

since it is possible to execute arbitrary Python code from any of the other entrypoints in this file. Of course fatal_error should also change it's name to probable_fatal_error. We already handle os._exit() calls there so the name isn't quite accurate anymore.

Also, is this... really okay? This seems pretty disturbing as a behavior.

Does it actually happen?

Does it really not break Python interpreter invariants?

The linked issue is pretty vague.

I think this might be handled better in API.fatal_error (we already handle exit there). Then all of these could change to

Yeah, I also think putting this into API.fatal_error would be better. For now, there are some places that assume API.fatal_error will not return, so I think we should fix that first.

Does it actually happen?

Yes, it happens. (edit: fixed link)

Does it really not break Python interpreter invariants?

I believe so...? ~~It is just a JS exception.~~

I believe so...?

Does Pyxel have extra logic to unwind the Python stack?

No, pyxel just ignores when it gets "unwind".

Okay, I guess I should do some more experiments and be more careful about this. I'm not sure what can happen on Python interpreter side when Emscripten throws an exception.

What I see instead is:

Stack (most recent call first): File "<exec>", line 8 in g File "<exec>", line 5 in f File "<exec>", line 10 in <module> File "/lib/python311.zip/_pyodide/_base.py", line 310 in run File "/lib/python311.zip/_pyodide/_base.py", line 468 in eval_code

so there are some "dead frames" that are left on the Python stack.

Thanks for the check, then I think we should do

unwind Python stack frames manually (not sure this makes sense), or

tell people to not use simulate_infinite_loop=true when using emscripten_set_main_loop.

@pmp-p Do you have some idea about this? I think pygbag also use simulate_infinite_loop option.

Hmm, by the way, can we actually call that "dead frames"? Conceptually this runs infinite loops in that frame.

can we actually call that "dead frames"?

I think what you'll see if you call another Python function is that the new frames go on top of the old ones and the old frames are never re entered, and all objects owned by them are leaked. So I believe they are actually leaked frames. But I could be wrong.

I think what you'll see if you call another Python function is that the new frames go on top of the old ones and the old frames are never re entered, and all objects owned by them are leaked.

Yes, indeed the new functions go on top of the old ones.

Okay, but the issue is that I think the old frames are used inside the loop so we can't clear those frames. So maybe we should let users explicitly cancel the loop and maybe block other commands until the loop is running. I think this is going to be more complicated than I thought, so I'll open a separate issue.

hoodmane · 2023-03-22T15:32:33Z

src/tests/test_canvas.py

+
+        assert(() => pyodide._module.canvas === canvas);
+        assert(() => pyodide.canvas.getCanvas2D() === canvas);
+    """


Should we maybe clear it after so there aren't side effects?

Switched to selenium_standalone to avoid side effects of the feature flag.

src/js/api.ts

Co-authored-by: Hood Chatham <roberthoodchatham@gmail.com>

for more information, see https://pre-commit.ci

rth · 2023-03-29T07:08:20Z

So what's the situation with this PR for the release? Do we merge as is, accept that stack traces are not going to be unwound correctly after calling throw_unwind in this use case, and deal with https://github.com/pyodide/pyodide/issues/3697later. After all the test suite is passing so it probably won't affect existing users much. Or do we decide to postpone this PR for the 0.23 release?

ryanking13 · 2023-03-29T07:48:22Z

Personally, I would like to include this in 0.23.0, as I believe some folks would want to try this. But it is not a strong opinion, so if one of the other maintainers disagrees, I am okay to postpone this.

Maybe I can finish #3705 before releasing 0.23.0, but I can't guarantee it ( I'll be a bit busy for the next couple of weeks :( )

After all the test suite is passing so it probably won't affect existing users much.

Right, it doesn't affect existing users.

rth · 2023-03-29T07:51:58Z

Fine with me to merge this as is. @hoodmane ?

hoodmane · 2023-03-29T14:53:41Z

I think we should defer this to 0.23.1. The unwinding issue requires a bit more exploration IMO see #3508 (comment).

rth · 2023-03-29T15:07:32Z

Well, the problem is that it links more things into the main module and potentially impacts error catching depending on the outcome of the discussion, so if we put it in the 0.23.1 and it creates some changes of behavior for existing apps, that's not very reasonable for a bug fix release IMO. While if we put in the 0.23.0 even if has known issues when using SDL apps, and we fix those in 0.23.1 it might be better. Otherwise, it would mean 0.24 which is still far way.

Are your concerns in #3508 (comment) about this creating regressions for existing users, or that it would be suboptimal for users trying this functionality? In the latter case, I would say that the experimental tag could justify some of it.

hoodmane · 2023-03-29T15:10:07Z

I am okay with merging if we include some obvious warning that it doesn't work.

hoodmane · 2023-03-29T15:11:11Z

Maybe we could put it behind a feature flag or something? Like the way chrome ships broken code behind "experimental: unstable wip feature" flags...

hoodmane · 2023-03-29T15:13:11Z

Without unwind support, I think any application that runs for long enough will eventually fill the interpreter with dead frames so that any attempt to run code raises a stack overflow error.

rth · 2023-03-29T15:24:22Z

Something like:

To use this feature, currently you need to set,
```
pyodide._skip_unwind_fatal_error = true
```
To avoid producing fatal errors when Emscripten throws "unwind". 
However, with this option, the call stack is not unwound
properly and dead frames are remaining on the stack.
This is a known issue for the current implementation that should be fixed in following releases.

and then in,
pyproxy.ts

  } catch (e) {
    if (API._skip_unwind_fatal_error) {
         API.maybe_fatal_error(e);
     } else {
         API.fatal_error(e);
    }
    return;
  } finally {

?

We can always adjust the wording later in the docs, the main this to agree on the feature flag mechanism.

hoodmane · 2023-03-29T15:32:40Z

Looks great!

This reverts commit a0019bf.

ryanking13 · 2023-03-30T22:53:17Z

Thanks, for finishing this!

ryanking13 added 20 commits January 12, 2023 07:45

Draft: pyxel

0a2ae4e

Update configs

63a3362

Merge remote-tracking branch 'upstream/main' into pyxel

60195c9

Don't add module.canvas by default

b8f02b4

Update meta.yaml

55b5391

Update test code

711009f

Merge branch 'pyodide:main' into pyxel

ca4ecae

Merge remote-tracking branch 'upstream/main' into pyxel

fd0a0fe

Ignore unwind exception

002c2f1

Merge branch 'pyxel' of https://github.com/ryanking13/pyodide into pyxel

5adf4ad

Fix test

e4c7248

Disable AUTO_JS_LIBRARIES and AUDO_NATIVE_LIBRARIES flag

82f3b55

Merge remote-tracking branch 'upstream/main' into no-auto-libraries

173936a

Merge remote-tracking branch 'upstream/main' into pyxel

66ccc3d

Merge branch 'no-auto-libraries' into pyxel

6280b5d

Add libGL to main module

8bd2c67

Link libgl to side module not main module

60e0026

Revert "Link libgl to side module not main module"

022fb29

This reverts commit 60e0026.

Add comments on libgl

be09c87

Propagate unwind exception

248cf07

ryanking13 mentioned this pull request Jan 25, 2023

port pygame #289

Closed

ryanking13 added 3 commits January 31, 2023 08:08

Merge remote-tracking branch 'upstream/main' into pyxel

0b3702e

Update changelog

32628ed

Update docs

56c93d2

hoodmane mentioned this pull request Feb 11, 2023

cmu_graphics wheel #3572

Open

pmp-p mentioned this pull request Feb 27, 2023

Wasm static multiphase init for dynamic loading pygame-community/pygame-ce#1948

Merged

rth added this to the 0.23.0 milestone Mar 17, 2023

hoodmane mentioned this pull request Mar 17, 2023

0.23 release #3666

Closed

hoodmane reviewed Mar 22, 2023

View reviewed changes

Apply suggestions from code review

23b8e68

Co-authored-by: Hood Chatham <roberthoodchatham@gmail.com>

ryanking13 mentioned this pull request Mar 25, 2023

[Discussion] Handling "unwind" exception #3697

Open

rth and others added 2 commits March 29, 2023 06:40

Merge branch 'main' into pyxel

f54c905

[pre-commit.ci] auto fixes from pre-commit.com hooks

83d161f

for more information, see https://pre-commit.ci

ryanking13 added 2 commits March 29, 2023 07:33

Update sdl docs

cdb8dab

Move unwind catch into error_handling

588af97

rth added 6 commits March 29, 2023 22:58

Merge remote-tracking branch 'upstream/main' into pyxel

3f93082

Hide maybe_fatal_error behind a feature flag

98fbf0f

Update documentation

d8d0854

Use selenium_standalone for better test isolation

83e2d06

Version 0.23.0

a0019bf

Revert "Version 0.23.0"

19cff87

This reverts commit a0019bf.

rth mentioned this pull request Mar 29, 2023

Version 0.23.0 #3712

Merged

2 tasks

rth merged commit 2046310 into pyodide:main Mar 30, 2023

ryanking13 deleted the pyxel branch March 30, 2023 22:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ENH Support SDL-based packages and add pyxel #3508

ENH Support SDL-based packages and add pyxel #3508

ryanking13 commented Jan 25, 2023 •

edited

Loading

pmp-p commented Feb 27, 2023

ryanking13 commented Feb 27, 2023

pmp-p commented Mar 1, 2023

JeffersGlass commented Mar 3, 2023

ryanking13 commented Mar 14, 2023

hoodmane Mar 22, 2023

ryanking13 Mar 23, 2023

hoodmane Mar 22, 2023 •

edited

Loading

hoodmane Mar 22, 2023

ryanking13 Mar 23, 2023 •

edited

Loading

hoodmane Mar 23, 2023

ryanking13 Mar 23, 2023

hoodmane Mar 23, 2023

ryanking13 Mar 23, 2023 •

edited

Loading

ryanking13 Mar 23, 2023 •

edited

Loading

hoodmane Mar 24, 2023

ryanking13 Mar 25, 2023

hoodmane Mar 22, 2023

rth Mar 29, 2023

rth commented Mar 29, 2023

ryanking13 commented Mar 29, 2023

rth commented Mar 29, 2023

hoodmane commented Mar 29, 2023

rth commented Mar 29, 2023

hoodmane commented Mar 29, 2023

hoodmane commented Mar 29, 2023

hoodmane commented Mar 29, 2023

rth commented Mar 29, 2023

hoodmane commented Mar 29, 2023

ryanking13 commented Mar 30, 2023

ENH Support SDL-based packages and add pyxel #3508

ENH Support SDL-based packages and add pyxel #3508

Conversation

ryanking13 commented Jan 25, 2023 • edited Loading

Description

Checklists

Demo

pmp-p commented Feb 27, 2023

ryanking13 commented Feb 27, 2023

pmp-p commented Mar 1, 2023

JeffersGlass commented Mar 3, 2023

ryanking13 commented Mar 14, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hoodmane Mar 22, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryanking13 Mar 23, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryanking13 Mar 23, 2023 • edited Loading

Choose a reason for hiding this comment

ryanking13 Mar 23, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rth commented Mar 29, 2023

ryanking13 commented Mar 29, 2023

rth commented Mar 29, 2023

hoodmane commented Mar 29, 2023

rth commented Mar 29, 2023

hoodmane commented Mar 29, 2023

hoodmane commented Mar 29, 2023

hoodmane commented Mar 29, 2023

rth commented Mar 29, 2023

hoodmane commented Mar 29, 2023

ryanking13 commented Mar 30, 2023

ryanking13 commented Jan 25, 2023 •

edited

Loading

hoodmane Mar 22, 2023 •

edited

Loading

ryanking13 Mar 23, 2023 •

edited

Loading

ryanking13 Mar 23, 2023 •

edited

Loading

ryanking13 Mar 23, 2023 •

edited

Loading