1615148 - Update wrench to latest winit and glutin versions

Status: RESOLVED FIXED

(Core :: Graphics: WebRender, enhancement, P5)

Description

[Jeff Muizelaar [:jrmuizel]]

I had a go at this for fun. I failed to get it to work so far.

Comment 1

[Jeff Muizelaar [:jrmuizel]]

Attachment: patch

Comment 2

[Jamie Nicol [:jnicol]]

I managed to get this to compile and run wrench show to render a yaml. But there's a lot of work left to resolve this. The issue is that the events loop API has changed: poll_events() has been removed and run_forever() has been renamed to just run(), and now actually runs forever and never returns:

pub fn run<F>(self, event_handler: F) -> !

Hijacks the calling thread and initializes the winit event loop with the provided closure. Since the closure is 'static, it must be a move closure if it needs to access any data from the calling context.

Currently wrench is very much in control of its events loop. It's called from a variety of places, and each of them does a different set of work after the loop has exited. This will no longer be possible, we need to restructure wrench around the new events loop.

(There is a also a run_return() function, which does return. But its use is strongly discouraged, and it is not supported on Android. This is how I managed to render a frame without doing the hard work.)

Comment 3

[Jeff Muizelaar [:jrmuizel]]

Jamie, can you attach your current work?

Comment 4

[Jamie Nicol [:jnicol]]

Attachment: Bug 1615148 - WIP update winit

Comment 5

[Jamie Nicol [:jnicol]]

This is where I've gotten to so far.

• Replaced the old event loop with the new style. Think it should work mostly the same but that definitely needs checked.
• The headless mode "event loop" is still commented out
• I managed to change it to use EventsLoop::run(), rather than run_return. I had to make the the callback a move closure, which means I commented out the wrench.renderer.deinit() call at the end. Probably a trivial fix.

It runs and seems to work correctly on Linux.

I installed the latest cargo-apk from crates.io to see if it works on android, but ran in to an error building freetype-sys. It appears that the old cargo-apk generated a cmake toolchain file, but the new one does not. I managed to create my own (or copied the one generated by the old cargo-apk version) to get past this. I'll raise an issue upstream.

Unfortunately I'm now running in to some errors compiling glutin.

Comment 6

[Jamie Nicol [:jnicol]]

I filed the cargo-apk freetype-sys error here.

The bad news is that whilst winit has now been updated to work with the new cargo-apk, glutin still has not been. There is a pull request here but doesn't seem to be getting much activity.

I think we might just need to wait for glutin to be fixed before we can proceed with the winit update.

In the meantime, we can fix our fork of the old cargo-apk to work with the new Cargo.lock file format. I'll file a separate bug about that.

Comment 7

[Takanori MATSUURA]

The latest version is 0.26.1.

Comment 8

[Jamie Nicol [:jnicol]]

Yep, it's an old bug! We plan to update to the latest version (with some extra patches to make it work on android)

Comment 9

[Jamie Nicol [:jnicol]]

Attachment: Bug 1615148 - Update wrench's winit and glutin dependencies. r?#gfx-reviewers

To versions 0.26 and 0.28 respectively. The most significant change is
that winit's event loop has been overhauled. poll_events() has been
removed, and you are strongly encouranged to use run() instead, which
takes full control of the events loop. There does exist a
run_return(), however, though we are advised against using it. We have
chosen to do so for now anyway to make porting easier. I have
attempted to match the existing semantics as closely as possible,
though there may be slight differences.

This patch additionally updates the version of rust used to build
wrench on CI.

Due to missing support in the 0.28 crates.io version of glutin,
android wrench builds no longer work following this change. This will
be rectified by the next patch in this series.

Comment 10

[Jamie Nicol [:jnicol]]

Attachment: Bug 1615148 - Fix wrench on android. r?#gfx-reviewers

This uses a patched version of glutin 0.28 which builds successfully
on Android. It has the caveat that the application is now responsible
to ensure we only create a GL context when the application has been
resumed and the window is valid. This patch does so by spinning an
event loop on startup until we receive a Resume event. This is a bit
of a hack, and will break if the app is minimised, but it is good
enough for wrench's use case.

Cargo-apk no longer supports specifying a separate target_sdk_version
and android_version, meaning we must use a target_sdk_version of
31. This means we no longer have permission to read from "/sdcard", so
wrench and its scripts have been updated to use the application's
"external data dir".

Finally, when running on CI we use a patched version of cargo-apk
which allows building with SDK version 31 and NDK r21d. We should be
able to switch to the upstream git version once we update to NDK r23.

Depends on D144417

Comment 11

[Jamie Nicol [:jnicol]]

I have this working on an unpatched winit 0.26 from crates.io, and a slightly patched glutin 0.28 to fix the compilation errors. A caveat with the glutin changes is that the application must ensure the window is valid prior to creating a GL context, but that's easy enough to add a hack for wrench's use case. Glutin changes are here.

Additionally, we need a slight modification to cargo-apk. We currently build with NDK version r21d, whose build/core/platforms.mk file contains NDK_MAX_PLATFORM_LEVEL := 30. But we build gecko with platform 31, and therefore our NDK package only contains the platform 31 directory. This means cargo-apk filters out the only platform directory and cannot find a platform. If I comment out the filter line, then it builds successfully with platform 31. I did look in to updating our NDK version, but it wasn't trivial. Alternatively perhaps we could include the platform 30 directory in our CI NDK bundle. But using a patched cargo-apk is probably easier for now.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a202bdbdf339
Update wrench's winit and glutin dependencies. r=gw
https://hg.mozilla.org/integration/autoland/rev/9aa546ee7d53
Fix wrench on android. r=jrmuizel

Comment 13

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/a202bdbdf339
https://hg.mozilla.org/mozilla-central/rev/9aa546ee7d53