List items
Items from the current list are shown below.
Gecko
All items from July 2024
5 Jul 2024 : Day 279 #
Today I'm continuing with the plan I hatched yesterday. I'm half way through Step 6 ("Build and check everything still works"), having built the packages overnight.
This morning I've done some basic testing: installed the packages, run the browser to view a page with some WebGL content, run the harbour-webview app.
I was a bit perturbed to find the browser failed to render WebGL if I executed it from the app drawer rather than the command line. But this got fixed when I deleted the startupCache:
So, Step 6 is completed.
Step 7: Take a deep breath. Commit the staged changes.
After making all of the changes and discarding anything unnecessary, there are now nineteen modified files to commit.
Deep breath.
Step 8: Admire the resulting, much cleaner, GLSCreenBuffer commit now at HEAD.
Looking through the diff, I think it'll make a decent patch. It remains uncomfortably big — not ideal — but I've now spent a lot of time cutting it down to the bare minimum and I'm satisfied that this is the best I can hope for, at least for now. Certainly it's not going to be worth spending more time trying to cut it down further.
Which leave me with just one more thing to do.
Step 9: Another deep breath. Force push everything to the server. Done.
At least, the plan was to force push. But interestingly, it turns out I don't need to force push at all. It's been so long since I started work on restoring GLScreenBuffer that I thought I must have pushed some of the changes to the remote. But it turns out I hadn't.
This is great news! It means I won't be doing anything destructive by pushing. No need to take any deep breaths after all!
So there we are. I can finally draw a line under the WebView rendering changes. It's a line drawn in pencil of course as I'm sure I'll find things I need to come back to, but for now it's time to move on.
The next thing I want to tackle is another Web-related acronym: WebRTC. I intentionally disabled this back on Day 38 in order to get the build to go through. And as we'll see there's a reason I didn't tackle it head on at the time. Now there's no avoiding it: it's time to swallow the bitter pill and get to it.
There are five patches which relate to WebRTC. Some look tractable. But others... well... I don't fancy the chances of that 2.4 MiB patch applying cleanly:
The problem is that the README.md file mentioned in the explanation is missing from the source tree. A bit of a search throws up a potentially useful comment in Bugzilla that links through to some text that's likely the README.md being referenced. It says the following:
Honestly that doesn't seem as bad as I'd feared. But this will of course constitute only one of the five patches that need to be applied. So tomorrow I'll start looking at the other patches to see what's really going to be involved here.
If you'd like to read any of my other gecko diary entries, they're all available on my Gecko-dev Diary page.
Comment
This morning I've done some basic testing: installed the packages, run the browser to view a page with some WebGL content, run the harbour-webview app.
I was a bit perturbed to find the browser failed to render WebGL if I executed it from the app drawer rather than the command line. But this got fixed when I deleted the startupCache:
$ rm ~/.local/share/org.sailfishos/browser/.mozilla/startupCache/*It's luck that I noticed this, but I'm glad I did: glitches like this can cause an almighty headache if you don't discover them early on. I also tested the Settings app to check the WebView still works on the various Account creation pages. Using the ESR 91 WebView I was successfully able to view the Twitter (nee X) Terms of Service; I didn't go as far as agreeing to them of course.
So, Step 6 is completed.
Step 7: Take a deep breath. Commit the staged changes.
After making all of the changes and discarding anything unnecessary, there are now nineteen modified files to commit.
$ git diff --stat --cached dom/base/nsGlobalWindowOuter.cpp | 6 - gfx/gl/GLContext.cpp | 34 +-- gfx/gl/GLContext.h | 6 +- gfx/gl/GLContextEGL.h | 6 +- gfx/gl/GLContextProviderEGL.cpp | 242 +++------------------ gfx/gl/GLLibraryEGL.cpp | 11 +- gfx/gl/GLLibraryEGL.h | 6 +- gfx/gl/GLScreenBuffer.cpp | 2 +- gfx/gl/GLTextureImage.cpp | 26 +-- gfx/gl/SharedSurface.cpp | 2 - gfx/gl/SharedSurface.h | 17 +- gfx/gl/SharedSurfaceEGL.h | 4 - gfx/gl/SharedSurfaceGL.cpp | 12 +- gfx/gl/SharedSurfaceGL.h | 16 ++ gfx/layers/CompositorTypes.h | 1 - gfx/layers/client/TextureClient.cpp | 42 +--- gfx/layers/client/TextureClient.h | 3 - gfx/layers/client/TextureClientSharedSurface.cpp | 1 - gfx/layers/ipc/CompositorVsyncScheduler.cpp | 1 + 19 files changed, 93 insertions(+), 345 deletions(-)Perhaps the more interesting thing to check is what will happen to the commit I'm merging this into. Part of the aim here has been to reduce the size of this commit to make it more manageable and I'm interested to know whether or not this has been successful. Here's the incumbent commit as it currently looks:
$ git diff --stat HEAD~..HEAD dom/base/nsGlobalWindowOuter.cpp | 6 + gfx/gl/GLContext.cpp | 74 +++++- gfx/gl/GLContext.h | 21 +- gfx/gl/GLContextEGL.h | 6 +- gfx/gl/GLContextProviderEGL.cpp | 240 +++++++++++++++++- gfx/gl/GLContextProviderImpl.h | 23 ++ gfx/gl/GLLibraryEGL.cpp | 11 +- gfx/gl/GLLibraryEGL.h | 6 +- gfx/gl/GLScreenBuffer.cpp | 274 +++++++++++++++++++++ gfx/gl/GLScreenBuffer.h | 132 +++++++++- gfx/gl/GLTextureImage.cpp | 34 ++- gfx/gl/SharedSurface.cpp | 116 ++++++++- gfx/gl/SharedSurface.h | 122 ++++++++- gfx/gl/SharedSurfaceEGL.cpp | 89 +++++-- gfx/gl/SharedSurfaceEGL.h | 34 ++- gfx/gl/SharedSurfaceGL.cpp | 64 ++++- gfx/gl/SharedSurfaceGL.h | 32 ++- gfx/gl/SurfaceTypes.h | 2 + gfx/gl/TextureImageEGL.cpp | 221 +++++++++++++++++ gfx/gl/TextureImageEGL.h | 80 ++++++ gfx/gl/moz.build | 1 + gfx/layers/CompositorTypes.h | 1 + gfx/layers/client/TextureClient.cpp | 42 +++- gfx/layers/client/TextureClient.h | 3 + gfx/layers/client/TextureClientSharedSurface.cpp | 50 ++++ gfx/layers/client/TextureClientSharedSurface.h | 25 ++ gfx/layers/ipc/CompositorVsyncScheduler.cpp | 2 - gfx/layers/opengl/CompositorOGL.cpp | 24 +- 28 files changed, 1631 insertions(+), 104 deletions(-)After merging in these changes to the commit, it'll look like this:
$ git diff --stat HEAD~ gfx/gl/GLContext.cpp | 40 ++- gfx/gl/GLContext.h | 17 +- gfx/gl/GLContextProviderEGL.cpp | 36 +++ gfx/gl/GLContextProviderImpl.h | 23 ++ gfx/gl/GLScreenBuffer.cpp | 274 +++++++++++++++++++++ gfx/gl/GLScreenBuffer.h | 132 +++++++++- gfx/gl/GLTextureImage.cpp | 10 + gfx/gl/SharedSurface.cpp | 114 ++++++++- gfx/gl/SharedSurface.h | 105 +++++++- gfx/gl/SharedSurfaceEGL.cpp | 89 +++++-- gfx/gl/SharedSurfaceEGL.h | 30 ++- gfx/gl/SharedSurfaceGL.cpp | 74 +++++- gfx/gl/SharedSurfaceGL.h | 46 ++++ gfx/gl/SurfaceTypes.h | 2 + gfx/gl/TextureImageEGL.cpp | 221 +++++++++++++++++ gfx/gl/TextureImageEGL.h | 80 ++++++ gfx/gl/moz.build | 1 + gfx/layers/client/TextureClientSharedSurface.cpp | 49 ++++ gfx/layers/client/TextureClientSharedSurface.h | 25 ++ gfx/layers/ipc/CompositorVsyncScheduler.cpp | 1 - gfx/layers/opengl/CompositorOGL.cpp | 24 +- 21 files changed, 1334 insertions(+), 59 deletions(-)There are big gains in reducing the diff for GLContextProviderEGL.cpp and the only losers are SharedSurfaceGL.cpp and SharedSurfaceGL.h. But the wins are much greater than the losses, so I'm happy with this. That means it's time to apply the changes to the previous commit.
Deep breath.
$ git log -1
commit 7437a9d17284bd9a4427502b1af6e94a9d7ee356 (HEAD ->
FIREFOX_ESR_91_9_X_RELBRANCH_patches)
Author: David Llewellyn-Jones <david@flypig.co.uk>
Date: Wed May 22 08:35:49 2024 +0100
Restore GLScreenBuffer and TextureImageEGL
TextureImageEGL was removed with Bug 1716559:
https://phabricator.services.mozilla.com/D117904
GLScreenBuffer was removed with Bug 1632249:
https://phabricator.services.mozilla.com/D75055
Both are used as part of the Sailfish OS WebView offscreen render
pipeline. This patch reintroduces these two classes and allows them to
be made use of.
$ git commit --amend
[FIREFOX_ESR_91_9_X_RELBRANCH_patches 1765dd0460a9] Restore GLScreenBuffer and
TextureImageEGL
Date: Wed May 22 08:35:49 2024 +0100
21 files changed, 1334 insertions(+), 59 deletions(-)
create mode 100644 gfx/gl/TextureImageEGL.cpp
create mode 100644 gfx/gl/TextureImageEGL.h
Step 7 completed. On to Step 8.Step 8: Admire the resulting, much cleaner, GLSCreenBuffer commit now at HEAD.
Looking through the diff, I think it'll make a decent patch. It remains uncomfortably big — not ideal — but I've now spent a lot of time cutting it down to the bare minimum and I'm satisfied that this is the best I can hope for, at least for now. Certainly it's not going to be worth spending more time trying to cut it down further.
Which leave me with just one more thing to do.
Step 9: Another deep breath. Force push everything to the server. Done.
At least, the plan was to force push. But interestingly, it turns out I don't need to force push at all. It's been so long since I started work on restoring GLScreenBuffer that I thought I must have pushed some of the changes to the remote. But it turns out I hadn't.
This is great news! It means I won't be doing anything destructive by pushing. No need to take any deep breaths after all!
$ git push
Enumerating objects: 92, done.
Counting objects: 100% (92/92), done.
Delta compression using up to 16 threads
Compressing objects: 100% (51/51), done.
Writing objects: 100% (51/51), 22.70 KiB | 2.84 MiB/s, done.
Total 51 (delta 40), reused 0 (delta 0), pack-reused 0
remote: Resolving deltas: 100% (40/40), completed with 37 local objects.
To github.com:llewelld/gecko-dev-mirror.git
c6ea49286566..1765dd0460a9 FIREFOX_ESR_91_9_X_RELBRANCH_patches ->
FIREFOX_ESR_91_9_X_RELBRANCH_patches
$ cd ..
$ git push
Enumerating objects: 13, done.
Counting objects: 100% (13/13), done.
Delta compression using up to 16 threads
Compressing objects: 100% (7/7), done.
Writing objects: 100% (7/7), 1.46 KiB | 373.00 KiB/s, done.
Total 7 (delta 5), reused 0 (delta 0), pack-reused 0
remote: Resolving deltas: 100% (5/5), completed with 5 local objects.
To github.com:llewelld/gecko-dev.git
9f64ce35a187..5b9c330998db sailfishos-esr91 -> sailfishos-esr91
Done!So there we are. I can finally draw a line under the WebView rendering changes. It's a line drawn in pencil of course as I'm sure I'll find things I need to come back to, but for now it's time to move on.
The next thing I want to tackle is another Web-related acronym: WebRTC. I intentionally disabled this back on Day 38 in order to get the build to go through. And as we'll see there's a reason I didn't tackle it head on at the time. Now there's no avoiding it: it's time to swallow the bitter pill and get to it.
There are five patches which relate to WebRTC. Some look tractable. But others... well... I don't fancy the chances of that 2.4 MiB patch applying cleanly:
$ grep -rInl "webrtc" * --include="*.patch" | xargs ls
-lah | awk '{print t$9 "\t" $5}'
0077-sailfishos-webrtc-Adapt-build-configuration-for-Sail.patch 5.6K
0078-sailfishos-webrtc-Regenerate-moz.build-files.-JB-537.patch 2.4M
0079-sailfishos-webrtc-Disable-desktop-sharing-feature-on.patch 2.1K
0080-sailfishos-webrtc-Enable-GMP-for-encoding-decoding.-.patch 33K
0081-sailfishos-webrtc-Implement-video-capture-module.-JB.patch 29K
In practice the really big patch — patch 0078 — is due to changes to the build system which have been auto-generated. Specifically it relates to components that would usually be built using GN, but where the build configuration has been automgically converted into the moz.build files used by the Gecko build system. Here's the text from the docs explaining what's going on here;GN is a third-party build tool used by chromium and some related projects that are vendored in mozilla-central. Rather than requiring GN to build or writing our own build definitions for these projects, we have support in the build system for translating GN configuration files into moz.build files. In most cases these moz.build files will be like any others in the tree (except that they shouldn’t be modified by hand), however those updating vendored code or building on platforms not supported by Mozilla automation may need to re-generate these files. This is a per-project process, described in dom/media/webrtc/third_party_build/gn-configs/README.md for webrtc. As of writing, it is very specific to webrtc, and likely doesn’t work as-is for other projects.
The problem is that the README.md file mentioned in the explanation is missing from the source tree. A bit of a search throws up a potentially useful comment in Bugzilla that links through to some text that's likely the README.md being referenced. It says the following:
Generate new gn json files and moz.build files for building libwebrtc in our tree
/!\ This is only supported on Linux and macOS. If you are on Windows, you can run the script under WSL.
1. The script should be run from the top directory of our firefox tree.
Adding new configurations to the build
Edit the main function in the python/mozbuild/mozbuild/gn_processor.py file.
/!\ This is only supported on Linux and macOS. If you are on Windows, you can run the script under WSL.
1. The script should be run from the top directory of our firefox tree.
./mach python python/mozbuild/mozbuild/gn_processor.py dom/media/webrtc/
third_party_build/gn-configs/webrtc.json
2. Checkin all the generated/modified files and try your build!Adding new configurations to the build
Edit the main function in the python/mozbuild/mozbuild/gn_processor.py file.
Honestly that doesn't seem as bad as I'd feared. But this will of course constitute only one of the five patches that need to be applied. So tomorrow I'll start looking at the other patches to see what's really going to be involved here.
If you'd like to read any of my other gecko diary entries, they're all available on my Gecko-dev Diary page.
4 Jul 2024 : Day 278 #
Finally, after much investigation, refinement and guesswork, I finally got the WebView and WebGL working simultaneously yesterday. The key realisation was that the SharedSurface_Basic class needed different functionality depending on whether it was used for one or the other. This was needed as a result of the changes that were made upstream to the WebGL rendering pipeline.
But solving this has also brought other benefits. The process I used to get here was to start with a version with working WebView but broken WebGL, then I started removing all of the GLScreenBuffer code (needed for the WebView) until I had working WebGL as well. But I overshot (embarrassingly and for reasons I won't go in to here) and so then started restoring the GLScreenBuffer code again gently until eventually I hit the critical point where adding or removing a single line resulted in either the WebGL or the WebView working, but not both. That gave me the info I needed to fix the issue, after which I added in just enough more of the remaining code to tidy things up.
As a result I've now added 101 lines to the previous code in order to fix things, but more importantly I've removed 349 lines as well:
I now have to format things into commits. As I just explained the current diff on my local machine removes a bunch of code and eventually I want to merge this diff with the last commit, described as follows:
The first of these involved changes added by me when I was scrabbling around trying to fix the WebView first time around. There were upstream changes to the way the EGLDisplay value was being handled that made me suspect it might be causing the WebView to fail. In retrospect this didn't turn out to be a factor, so I think it'll be safe to remove these changes. I'll just drop them.
The second is functionality that may be needed for native ports and the emulator. It doesn't seem to have any impact on my Xperia 10 III, but that's a libhyris port and doesn't appear to need these changes. Consequently I'm going to drop these changes too, but before I do I want to capture a diff of them, so that I can restore them later if it turns out they're important for native ports.
The third set of changes were to revert changeset D106879 the result of which was to restore the horrifically named mWorkaroundAnnoyingSharedSurfaceLifetimeIssues variable.
So it should now be safe to have this removed. It'll be good not to have that variable included any more. I'll drop this code and moreover I notice there are some further changes in the changeset which I can apply as well and which should safely reduce the size of the patch a little more.
Okay, here's my rubric for the next few steps.
Step 1. First I'm going to save out a reverse of the diff containing all of my current changes. This is in case something goes wrong, it'll give me something to refer back to so that I can restore things to the current state.
Step 2. Clean up and stage all of the changes that are clearly needed and not related to the three areas listed above.
Step 3. In this step I'm going to add all of the code that's not related to the Wayland surface.
Step 4. Create a diff of the changes needed to remove the Wayland surface changes. Reverse it so that it shows how to add rather than remove the changes.
Step 5. Apply all of the remaining changes from D106879. I'm going to do this carefully in hunks just so that I don't make any errors.
If you'd like to read any of my other gecko diary entries, they're all available on my Gecko-dev Diary page.
Comment
But solving this has also brought other benefits. The process I used to get here was to start with a version with working WebView but broken WebGL, then I started removing all of the GLScreenBuffer code (needed for the WebView) until I had working WebGL as well. But I overshot (embarrassingly and for reasons I won't go in to here) and so then started restoring the GLScreenBuffer code again gently until eventually I hit the critical point where adding or removing a single line resulted in either the WebGL or the WebView working, but not both. That gave me the info I needed to fix the issue, after which I added in just enough more of the remaining code to tidy things up.
As a result I've now added 101 lines to the previous code in order to fix things, but more importantly I've removed 349 lines as well:
$ git diff --stat dom/base/nsGlobalWindowOuter.cpp | 6 - gfx/gl/GLContext.cpp | 35 +-- gfx/gl/GLContext.h | 7 +- gfx/gl/GLContextEGL.h | 6 +- gfx/gl/GLContextProviderEGL.cpp | 242 +++------------------ gfx/gl/GLLibraryEGL.cpp | 11 +- gfx/gl/GLLibraryEGL.h | 6 +- gfx/gl/GLScreenBuffer.cpp | 2 +- gfx/gl/GLTextureImage.cpp | 26 +-- gfx/gl/SharedSurface.cpp | 3 +- gfx/gl/SharedSurface.h | 20 +- gfx/gl/SharedSurfaceEGL.cpp | 1 - gfx/gl/SharedSurfaceEGL.h | 4 - gfx/gl/SharedSurfaceGL.cpp | 12 +- gfx/gl/SharedSurfaceGL.h | 12 + gfx/layers/CompositorTypes.h | 1 - gfx/layers/client/TextureClient.cpp | 42 +--- gfx/layers/client/TextureClient.h | 3 - gfx/layers/client/TextureClientSharedSurface.cpp | 2 +- gfx/layers/ipc/CompositorVsyncScheduler.cpp | 1 + gfx/layers/opengl/CompositorOGL.cpp | 8 + 21 files changed, 101 insertions(+), 349 deletions(-)So the delta represents the removal of 248 lines, which is what I was hoping for. It should mean the patch for reintroducing GLSCreenBuffer will be much simpler than it would otherwise have been.
I now have to format things into commits. As I just explained the current diff on my local machine removes a bunch of code and eventually I want to merge this diff with the last commit, described as follows:
Author: David Llewellyn-Jones <david@flypig.co.uk>
Date: Wed May 22 08:35:49 2024 +0100
Restore GLScreenBuffer and TextureImageEGL
TextureImageEGL was removed with Bug 1716559:
https://phabricator.services.mozilla.com/D117904
GLScreenBuffer was removed with Bug 1632249:
https://phabricator.services.mozilla.com/D75055
Both are used as part of the Sailfish OS WebView offscreen render
pipeline. This patch reintroduces these two classes and allows them to
be made use of.
However, the code my local diff removes actually is there for a reason. Looking through it, I can see three separate reasons for why I made the changes in the first place:- To pass the EGLDisplay value through the system.
- To support Wayland surfaces.
- To ensure texture data is only destroyed on the main thread.
The first of these involved changes added by me when I was scrabbling around trying to fix the WebView first time around. There were upstream changes to the way the EGLDisplay value was being handled that made me suspect it might be causing the WebView to fail. In retrospect this didn't turn out to be a factor, so I think it'll be safe to remove these changes. I'll just drop them.
The second is functionality that may be needed for native ports and the emulator. It doesn't seem to have any impact on my Xperia 10 III, but that's a libhyris port and doesn't appear to need these changes. Consequently I'm going to drop these changes too, but before I do I want to capture a diff of them, so that I can restore them later if it turns out they're important for native ports.
The third set of changes were to revert changeset D106879 the result of which was to restore the horrifically named mWorkaroundAnnoyingSharedSurfaceLifetimeIssues variable.
commit 6f458ab05edc6c086e304c999365f6b375c46640
Author: sotaro <sotaro.ikeda.g@gmail.com>
Date: Tue Mar 2 10:48:55 2021 +0000
Bug 1695846 - Remove unused flags of TextureClient r=nical
Flags were used for WebGL SharedSurface
Differential Revision: https://phabricator.services.mozilla.com/D106879
I'm not able to recall off the top of my head why I reversed this changes, but the good news is that I did write about it, back on Day 187. Referring back I can see that at that time I thought it might have been the reason for the browser seizing up. I eventually found the actual underlying reason, which was unrelated. But I never removed this attempted workaround.So it should now be safe to have this removed. It'll be good not to have that variable included any more. I'll drop this code and moreover I notice there are some further changes in the changeset which I can apply as well and which should safely reduce the size of the patch a little more.
Okay, here's my rubric for the next few steps.
- Take a diff of the my current, uncommitted, local changes. But make it a reverse diff so that it switches additions and removals. This will allow me to get back to my current position from the last commit in case I screw something up.
- Stage all of the changes that aren't related to the three points above (EGLDisplay, Wayland surfaces and texture destruction) using git add -p -u . so that things are a bit cleaner. Add these changes to the latest commit using git commit --amend.
- Stage all of the changes related to 1 and 3 above (EGLDisplay and texture destruction) using git add -p -u .. Add these changes to the latest commit.
- Create a patch from the remaining changes, which should represent those needed to support Wayland surfaces. Keep this patch somewhere safe.
- Apply all of the remaining changes from D106879.
- Build and check that everything still works.
- Take a deep breath. Commit the staged changes.
- Admire the resulting, much cleaner, GLSCreenBuffer commit now at HEAD.
- Another deep breath. Force push everything to the server. Done.
Step 1. First I'm going to save out a reverse of the diff containing all of my current changes. This is in case something goes wrong, it'll give me something to refer back to so that I can restore things to the current state.
$ git diff -R > ../../glscreenbuffer-changes.diff
$ head ../../glscreenbuffer-changes.diff
diff --git b/dom/base/nsGlobalWindowOuter.cpp a/dom/base/nsGlobalWindowOuter.cpp
index 41c93c51cf3b..31a93ea1cc70 100644
--- b/dom/base/nsGlobalWindowOuter.cpp
+++ a/dom/base/nsGlobalWindowOuter.cpp
@@ -4542,10 +4542,16 @@ nsresult nsGlobalWindowOuter::SetFullscreenInternal(
FullscreenReason aReason,
if (rootItem != mDocShell)
return window->SetFullscreenInternal(aReason, aFullscreen);
+ // We don't have chrome from doc shell point of view. This commit
+ // sha1 3116f3bf53df offends fullscreen API to work without chrome
That looks good.Step 2. Clean up and stage all of the changes that are clearly needed and not related to the three areas listed above.
$ git add -p -u . [...]Great; all done, no issues.
Step 3. In this step I'm going to add all of the code that's not related to the Wayland surface.
$ git add -p -u . [...]This turned out to be more straightforward than I'd feared. The Wayland surface changes are now the only thing remaining and in fact all live inside the one file: GLContextProviderEGL.cpp.
Step 4. Create a diff of the changes needed to remove the Wayland surface changes. Reverse it so that it shows how to add rather than remove the changes.
$ git diff -R > ../../wayland-surface-changes.diff
flypig@chattan:~/Documents/Development/jolla/gecko-dev-esr91/gecko-dev/
gecko-dev$ head ../../wayland-surface-changes.diff
diff --git b/gfx/gl/GLContextProviderEGL.cpp a/gfx/gl/GLContextProviderEGL.cpp
index 1632d663d101..b5ff16a9e522 100644
--- b/gfx/gl/GLContextProviderEGL.cpp
+++ a/gfx/gl/GLContextProviderEGL.cpp
@@ -85,6 +85,14 @@
# endif
#endif
+#ifdef MOZ_WIDGET_QT
+#include <qpa/qplatformnativeinterface.h>
Okay, good, all looking fine.Step 5. Apply all of the remaining changes from D106879. I'm going to do this carefully in hunks just so that I don't make any errors.
$ git add -p -u . [...]Step 6. I'm going to do a full build, so this will be an overnight activity. It's a shame I can't get all of the steps completed today, but this is definitely the right thing to do. Steady as she goes.
$ cd .. $ sfdk build -d --with git_workaround [...]So, I've set the build going. I'll continue with the rest of Step 6 and the remaining steps tomorrow. We're getting there and, honestly, I think it's looking pretty good!
If you'd like to read any of my other gecko diary entries, they're all available on my Gecko-dev Diary page.
3 Jul 2024 : Day 277 #
I'm continuing to restore bits of WebView functionality today, in the hope that the renderer will click into place and start rendering. I've convinced myself it's only a matter of time now.
Yesterday I tried restoring the TextureClient destruction code, but that didn't have any obvious effects so I removed the changes again. Today I'm trying something else: restoring the augmented fBindFramebuffer() functionality.
This is unusual because in default Gecko fBindFramebuffer() is just a wrapper for glBindFramebuffer from the OpenGL library. In previous EmbedLite versions of Gecko the wrapper has been given additional functionality which we can find in GLContext.cpp and which looks like this:
The additional functionality isn't needed everywhere though, and in fact there are a few places where we then have to switch the calls from using fBindFramebuffer() to using the new raw wrapper instead:
Let's see. I've made these changes and set the partial build running. Soon I'll be able to test this out.
[...]
And it works! The WebView renders nicely. The browser renders nicely. Both of them render WebGL nicely (props to Shane for the nice shader example). Great!
Phew. This concludes the difficult part of this process. Sadly it doesn't mean the end of the journey just yet though, oh no: I still need to turn this into a tidy commit and figure out what to do with the remaining changes (the ones that don't appear to be necessary). And to avoid any undue excitement, there are more things to fix after that as well.
Those will all be for tomorrow and beyond though. For today, I'm going to celebrate (in a quiet contemplative way!) and enjoy the win.
If you'd like to read any of my other gecko diary entries, they're all available on my Gecko-dev Diary page.
Comment
Yesterday I tried restoring the TextureClient destruction code, but that didn't have any obvious effects so I removed the changes again. Today I'm trying something else: restoring the augmented fBindFramebuffer() functionality.
This is unusual because in default Gecko fBindFramebuffer() is just a wrapper for glBindFramebuffer from the OpenGL library. In previous EmbedLite versions of Gecko the wrapper has been given additional functionality which we can find in GLContext.cpp and which looks like this:
void GLContext::fBindFramebuffer(GLenum target, GLuint framebuffer) {
if (!mScreen) {
raw_fBindFramebuffer(target, framebuffer);
return;
}
switch (target) {
case LOCAL_GL_DRAW_FRAMEBUFFER_EXT:
mScreen->BindDrawFB(framebuffer);
return;
case LOCAL_GL_READ_FRAMEBUFFER_EXT:
mScreen->BindReadFB(framebuffer);
return;
case LOCAL_GL_FRAMEBUFFER:
mScreen->BindFB(framebuffer);
return;
default:
// Nothing we care about, likely an error.
break;
}
raw_fBindFramebuffer(target, framebuffer);
}
With this change the raw_fBindFramebuffer() becomes the new name for the glBindFramebuffer() wrapper. As you can see, now when fBindFramebuffer() is called some framebuffer binding is also performed.The additional functionality isn't needed everywhere though, and in fact there are a few places where we then have to switch the calls from using fBindFramebuffer() to using the new raw wrapper instead:
@ -215,11 +215,11 @@ void GLScreenBuffer::BindFB(GLuint fb) {
mInternalReadFB = (fb == 0) ? readFB : fb;
if (mInternalDrawFB == mInternalReadFB) {
+ mGL->raw_fBindFramebuffer(LOCAL_GL_FRAMEBUFFER, mInternalDrawFB);
- mGL->fBindFramebuffer(LOCAL_GL_FRAMEBUFFER, mInternalDrawFB);
} else {
MOZ_ASSERT(mGL->IsSupported(GLFeature::split_framebuffer));
+ mGL->raw_fBindFramebuffer(LOCAL_GL_DRAW_FRAMEBUFFER_EXT, mInternalDrawFB);
+ mGL->raw_fBindFramebuffer(LOCAL_GL_READ_FRAMEBUFFER_EXT, mInternalReadFB);
- mGL->fBindFramebuffer(LOCAL_GL_DRAW_FRAMEBUFFER_EXT, mInternalDrawFB);
- mGL->fBindFramebuffer(LOCAL_GL_READ_FRAMEBUFFER_EXT, mInternalReadFB);
}
#ifdef DEBUG
@@ -235,7 +235,7 @@ void GLScreenBuffer::BindDrawFB(GLuint fb) {
mUserDrawFB = fb;
mInternalDrawFB = (fb == 0) ? drawFB : fb;
+ mGL->raw_fBindFramebuffer(LOCAL_GL_DRAW_FRAMEBUFFER_EXT, mInternalDrawFB);
- mGL->fBindFramebuffer(LOCAL_GL_DRAW_FRAMEBUFFER_EXT, mInternalDrawFB);
#ifdef DEBUG
mInInternalMode_DrawFB = false;
@@ -249,7 +249,7 @@ void GLScreenBuffer::BindReadFB(GLuint fb) {
mUserReadFB = fb;
mInternalReadFB = (fb == 0) ? readFB : fb;
+ mGL->raw_fBindFramebuffer(LOCAL_GL_READ_FRAMEBUFFER_EXT, mInternalReadFB);
- mGL->fBindFramebuffer(LOCAL_GL_READ_FRAMEBUFFER_EXT, mInternalReadFB);
#ifdef DEBUG
mInInternalMode_ReadFB = false;
These aren't all of the uses though and in the remaining cases the new augmented functionality will be called instead. This is another nicely self-contained change without which I could perfectly well imagine the rendering might be broken. So restoring this functionality might have positive effects.Let's see. I've made these changes and set the partial build running. Soon I'll be able to test this out.
[...]
And it works! The WebView renders nicely. The browser renders nicely. Both of them render WebGL nicely (props to Shane for the nice shader example). Great!
Phew. This concludes the difficult part of this process. Sadly it doesn't mean the end of the journey just yet though, oh no: I still need to turn this into a tidy commit and figure out what to do with the remaining changes (the ones that don't appear to be necessary). And to avoid any undue excitement, there are more things to fix after that as well.
Those will all be for tomorrow and beyond though. For today, I'm going to celebrate (in a quiet contemplative way!) and enjoy the win.
If you'd like to read any of my other gecko diary entries, they're all available on my Gecko-dev Diary page.
2 Jul 2024 : Day 276 #
Last night the new packages built so that I now have a version with working WebGL and partially working WebView. Crucially, this version splits off the functionality needed for working WebGL from the WebView functionality, meaning I should now be able to reintroduce the WebView functionality without it affecting the WebGL rendering.
So the next steps will be to reintroduce as much of the WebView changes needed in order to get WebView working, but preferably no more than that.
Before getting on to that I have one other issue I want to address. Although the browser is now working, along with the WebGL rendering, there are also now spurious crashes happening, presumably as a result of some of these changes. The backtrace associated with the crash is basically useless, even with a fully installed and correct set of debug symbols:
As an attempt to fix this I've added in a line to initialise mFB to zero, which should be enough to prevent the new lines in the destructor from executing.
So I'm looking through the code that's been amended and it looks like there are some relatively self-contained changes related to destruction of TextureClient instances. By reverting the changes from the following files, I should get a slice of the changes back with minimal fuss:
So I'm going to restore thee changes and try something else tomorrow. I feel like getting the WebGL not to crash was a good step forwards today. It's a shame the TextureClient changes didn't work, but that's still something to cross off my list of changes to try. So that's progress.
If you'd like to read any of my other gecko diary entries, they're all available on my Gecko-dev Diary page.
Comment
So the next steps will be to reintroduce as much of the WebView changes needed in order to get WebView working, but preferably no more than that.
Before getting on to that I have one other issue I want to address. Although the browser is now working, along with the WebGL rendering, there are also now spurious crashes happening, presumably as a result of some of these changes. The backtrace associated with the crash is basically useless, even with a fully installed and correct set of debug symbols:
Thread 8 "GeckoWorkerThre" received signal SIGSEGV, Segmentation
fault.
[Switching to LWP 30258]
0x0000007fe5ee29a0 in ?? ()
(gdb) bt
#0 0x0000007fe5ee29a0 in ?? ()
Backtrace stopped: previous frame identical to this frame (corrupt stack?)
(gdb)
My hunch is that this relates to the changes to SharedSurface_Basic. In particular, although I changed the code so that a different constructor is used to create the SharedSurface_Basic objects depending on the type of rendering, the process of destruction, which used to rely on just the default destructor, now has bespoke functionality:
SharedSurface_Basic::~SharedSurface_Basic() {
if (!mDesc.gl || !mDesc.gl->MakeCurrent()) return;
if (mFB) mDesc.gl->fDeleteFramebuffers(1, &mFB);
if (mOwnsTex) mDesc.gl->fDeleteTextures(1, &mTex);
}
This code needs to be there for the WebView to work, but it was never there in the WebGL version. My guess is that as surfaces are destroyed periodically, they're calling some of this code which is then triggering a crash. In particular, I notice that mFB isn't being set in the WebGL constructor, which means it could have a value other than zero. If this were the case, it could be causing problems when this uninitialised value gets passed to the OpenGL library, which could in turn explain why the backtrace is so poor.As an attempt to fix this I've added in a line to initialise mFB to zero, which should be enough to prevent the new lines in the destructor from executing.
SharedSurface_Basic::SharedSurface_Basic(const SharedSurfaceDesc& desc,
UniquePtr<MozFramebuffer>&& fb)
: SharedSurface(desc, std::move(fb)),
mTex(0),
mOwnsTex(false),
+ mFB(0)
{
}
Great, that's done the trick! No more random crashes. So, now I can move on to the larger task of getting the WebView to work. It's a bigger task, but I'm also hoping it'll be relatively formulaic, given that I already have a working version to compare against. I just need to gradually reintroduce the code from that version until it all clicks into place and works. The plan is that, with the WebGL rendering now separated from it, what I'll end up with is a working WebView and working WebGL.So I'm looking through the code that's been amended and it looks like there are some relatively self-contained changes related to destruction of TextureClient instances. By reverting the changes from the following files, I should get a slice of the changes back with minimal fuss:
- gfx/layers/CompositorTypes.h
- gfx/layers/client/TextureClient.h
- gfx/layers/client/TextureClient.cpp
- gfx/layers/client/TextureClientSharedSurface.cpp
- gfx/layers/ipc/CompositorVsyncScheduler.cpp
$ git diff gfx/layers/CompositorTypes.h \
gfx/layers/client/TextureClient.h \
gfx/layers/client/TextureClient.cpp \
gfx/layers/client/TextureClientSharedSurface.cpp \
gfx/layers/ipc/CompositorVsyncScheduler.cpp \
> destroy-texture-changes.diff
$ git checkout gfx/layers/CompositorTypes.h \
gfx/layers/client/TextureClient.h \
gfx/layers/client/TextureClient.cpp \
gfx/layers/client/TextureClientSharedSurface.cpp \
gfx/layers/ipc/CompositorVsyncScheduler.cpp
I'm a little surprised to discover that after resetting the files to revert the changes the code still builds just fine without needing any further adjustments. But when I test out the new library I don't see any obvious positive (or indeed negative) effects. I was hoping for something different. I guess this isn't the critical difference that I'm looking for.So I'm going to restore thee changes and try something else tomorrow. I feel like getting the WebGL not to crash was a good step forwards today. It's a shame the TextureClient changes didn't work, but that's still something to cross off my list of changes to try. So that's progress.
If you'd like to read any of my other gecko diary entries, they're all available on my Gecko-dev Diary page.
1 Jul 2024 : Day 275 #
After completing a partial build yesterday we were left with working WebGL when using the browser and a partially working WebView. My suspicion was that a regression has resulted in the same hanging that we experienced much earlier in the process, but I have yet to investigate that.
More to the point, I want to find out where SharedSurface_Basic objects get created (there may be multiple) during each of the different permutations of rendering involving the browser, WebView, non-WebGL-adorned pages and pages with WebGL content.
So that I can do this properly using the debugger I left a build running overnight. When I got up this morning it hadn't yet completed, but it was in the next best state: everything was already being packaged up into rpms. I've never seen it fail from this point onwards and it would usually take no more than a few minutes from here, so this was good to see.
And indeed it did complete very quickly. So the build was successful and I now have packages to test. But before I do test them, I'm going to run with the original working WebGL packages to find out where the ShareSurface_Basic objects, if any, get created. To test this I've placed a breakpoint on the ShareSurface_Basic::Create() method.
What I find is that there's no call to ShareSurface_Basic::Create() when using the browser generally, unless there's WebGL content on the page. If there is WebGL content it gets called like this:
It'll also be useful to find out whether CompositorOGL::CreateContext() gets called when rendering browser pages. That's useful because it'll tell us why the code there isn't creating a SharedSurface_Basic object. So I've placed a breakpoint on the method and will step through to find out. This, again, is using the code installed from our original packages with working WebGL and on a page that includes WebGL content:
It turns out we get the same flow when rendering a page without WebGL, so this isn't a consequence of the WebGL on the page, it's just because we're using the browser rather than a WebView, as we can see here:
What does this all mean? It means that we know all of the following:
We can use all of this to our advantage. I've taken the SurfaceFactory_Basic class and created a new version, which I've called SurfaceFactory_GL. This performs the new constructor type. So we now have two versions, like this:
Alright, I've made this change and I want to see it in action. The partial build shows good results, but I want to check things with the debugger again. So I've set off another full build. Let's find out where things are at when it's done.
Before I sign off for the day, let me say that I'm getting increasingly hopeful about this. It really feels like we've got all the information needed to fix the problem and that we now just need to carefully move all of the metaphorical girders into their correct positions.
If you'd like to read any of my other gecko diary entries, they're all available on my Gecko-dev Diary page.
Comment
More to the point, I want to find out where SharedSurface_Basic objects get created (there may be multiple) during each of the different permutations of rendering involving the browser, WebView, non-WebGL-adorned pages and pages with WebGL content.
So that I can do this properly using the debugger I left a build running overnight. When I got up this morning it hadn't yet completed, but it was in the next best state: everything was already being packaged up into rpms. I've never seen it fail from this point onwards and it would usually take no more than a few minutes from here, so this was good to see.
And indeed it did complete very quickly. So the build was successful and I now have packages to test. But before I do test them, I'm going to run with the original working WebGL packages to find out where the ShareSurface_Basic objects, if any, get created. To test this I've placed a breakpoint on the ShareSurface_Basic::Create() method.
What I find is that there's no call to ShareSurface_Basic::Create() when using the browser generally, unless there's WebGL content on the page. If there is WebGL content it gets called like this:
Thread 8 "GeckoWorkerThre" hit Breakpoint 1, mozilla::gl::
SharedSurface_Basic::Create (desc=...)
at gfx/gl/SharedSurfaceGL.cpp:20
20 const SharedSurfaceDesc& desc) {
(gdb) bt
#0 mozilla::gl::SharedSurface_Basic::Create (desc=...)
at gfx/gl/SharedSurfaceGL.cpp:20
#1 0x0000007ff28a8130 in mozilla::gl::SurfaceFactory_Basic::CreateSharedImpl (
this=<optimized out>, desc=...)
at gfx/gl/SharedSurfaceGL.h:41
#2 0x0000007ff28aaee8 in mozilla::gl::SurfaceFactory::CreateShared (size=...,
this=0x7fc98a1980)
at ${PROJECT}/obj-build-mer-qt-xr/dist/include/mozilla/RefCounted.h:240
#3 mozilla::gl::SwapChain::Acquire (this=this@entry=0x7fc949ac48, size=...)
at gfx/gl/GLScreenBuffer.cpp:56
#4 0x0000007ff369d340 in mozilla::WebGLContext::PresentInto (
this=this@entry=0x7fc949a7b0, swapChain=...)
at ${PROJECT}/obj-build-mer-qt-xr/dist/include/mozilla/UniquePtr.h:290
#5 0x0000007ff369d7ac in mozilla::WebGLContext::Present (
this=this@entry=0x7fc949a7b0, xrFb=<optimized out>,
consumerType=consumerType@entry=mozilla::layers::TextureType::Unknown,
webvr=webvr@entry=false)
at dom/canvas/WebGLContext.cpp:9
36
#6 0x0000007ff36656a8 in mozilla::HostWebGLContext::Present (webvr=false,
t=mozilla::layers::TextureType::Unknown, xrFb=<optimized out>,
this=<optimized out>) at ${PROJECT}/obj-build-mer-qt-xr/dist/include/
mozilla/RefPtr.h:280
#7 mozilla::ClientWebGLContext::Run<void (mozilla::HostWebGLContext::*)(
unsigned long, mozilla::layers::TextureType, bool) const, &(mozilla::
HostWebGLCont
ext::Present(unsigned long, mozilla::layers::TextureType, bool) const),
unsigned long, mozilla::layers::TextureType const&, bool const&> (
this=<optimized out>, args#0=@0x7fdf2f26c0: 0, args#1=@0x7fdf2f26bf:
mozilla::layers::TextureType::Unknown, args#2=@0x7fdf2f26be: false)
at dom/canvas/ClientWebGLContext
.cpp:313
#8 0x0000007ff3665810 in mozilla::ClientWebGLContext::Present (
this=this@entry=0x7fc9564c10, xrFb=xrFb@entry=0x0, type=<optimized out>,
webvr=<optimized out>, webvr@entry=false)
at dom/canvas/ClientWebGLContext
.cpp:363
#9 0x0000007ff3691020 in mozilla::ClientWebGLContext::OnBeforePaintTransaction
(this=0x7fc9564c10)
at dom/canvas/ClientWebGLContext
.cpp:345
#10 0x0000007ff290058c in mozilla::layers::CanvasRenderer::
FirePreTransactionCallback (this=this@entry=0x7fc9840c50)
at gfx/layers/CanvasRenderer.cpp
:75
#11 0x0000007ff29b1228 in mozilla::layers::ShareableCanvasRenderer::
UpdateCompositableClient (this=0x7fc9840c50)
at gfx/layers/ShareableCanvasRen
derer.cpp:192
#12 0x0000007ff29f0dc4 in mozilla::layers::ClientCanvasLayer::RenderLayer (
this=0x7fc989ee20)
at gfx/layers/client/ClientCanva
sLayer.cpp:25
#13 0x0000007ff29efee4 in mozilla::layers::ClientLayer::RenderLayerWithReadback
(this=<optimized out>, aReadback=<optimized out>)
at gfx/layers/client/ClientLayer
Manager.h:365
#14 0x0000007ff2a00224 in mozilla::layers::ClientContainerLayer::RenderLayer (
this=0x7fc9898670)
at gfx/layers/Layers.h:1051
#15 0x0000007ff29efee4 in mozilla::layers::ClientLayer::RenderLayerWithReadback
(this=<optimized out>, aReadback=<optimized out>)
at gfx/layers/client/ClientLayer
Manager.h:365
#16 0x0000007ff2a00224 in mozilla::layers::ClientContainerLayer::RenderLayer (
this=0x7fc963f3f0)
at gfx/layers/Layers.h:1051
#17 0x0000007ff29efee4 in mozilla::layers::ClientLayer::RenderLayerWithReadback
(this=<optimized out>, aReadback=<optimized out>)
at gfx/layers/client/ClientLayer
Manager.h:365
#18 0x0000007ff2a00224 in mozilla::layers::ClientContainerLayer::RenderLayer (
this=0x7fc984bb20)
at gfx/layers/Layers.h:1051
#19 0x0000007ff2a06f08 in mozilla::layers::ClientLayerManager::
EndTransactionInternal (this=this@entry=0x7fc8b18820,
aCallback=aCallback@entry=
0x7ff46a379c <mozilla::FrameLayerBuilder::DrawPaintedLayer(mozilla::layers::
PaintedLayer*, gfxContext*, mozilla::gfx::IntRegionTyped<mozilla::gfx::Unkn
ownUnits> const&, mozilla::gfx::IntRegionTyped<mozilla::gfx::UnknownUnits>
const&, mozilla::layers::DrawRegionClip, mozilla::gfx::
IntRegionTyped<mozilla::g
fx::UnknownUnits> const&, void*)>,
aCallbackData=aCallbackData@entry=0x7fdf2f3268)
at gfx/layers/client/ClientLayerManager.cpp:341
#20 0x0000007ff2a11e08 in mozilla::layers::ClientLayerManager::EndTransaction (
this=0x7fc8b18820,
aCallback=0x7ff46a379c <mozilla::FrameLayerBuilder::DrawPaintedLayer(
mozilla::layers::PaintedLayer*, gfxContext*, mozilla::gfx::
IntRegionTyped<mozilla::gfx::UnknownUnits> const&, mozilla::gfx::
IntRegionTyped<mozilla::gfx::UnknownUnits> const&, mozilla::layers::
DrawRegionClip, mozilla::gfx::IntRegionTyped<mozilla::gfx::UnknownUnits>
const&, void*)>, aCallbackData=0x7fdf2f3268, aFlags=mozilla::layers::
LayerManager::END_DEFAULT)
at gfx/layers/client/ClientLayerManager.cpp:397
#21 0x0000007ff46a0bbc in nsDisplayList::PaintRoot (
this=this@entry=0x7fdf2f5078, aBuilder=aBuilder@entry=0x7fdf2f3268,
aCtx=aCtx@entry=0x0,
aFlags=aFlags@entry=13, aDisplayListBuildTime=...)
at layout/painting/nsDisplayList.cpp:2622
#22 0x0000007ff442cf18 in nsLayoutUtils::PaintFrame (
aRenderingContext=aRenderingContext@entry=0x0,
aFrame=aFrame@entry=0x7fc93460b0, aDirtyRegion=...,
aBackstop=aBackstop@entry=4294967295,
aBuilderMode=aBuilderMode@entry=nsDisplayListBuilderMode::Painting,
aFlags=aFlags@entry=(nsLayoutUtils::PaintFrameFlags::WidgetLayers |
nsLayoutUtils::PaintFrameFlags::ExistingTransaction | nsLayoutUtils::
PaintFrameFlags::NoComposite)) at ${PROJECT}/obj-build-mer-qt-xr/dist/
include/mozilla/MaybeStorageBase.h:80
#23 0x0000007ff43b760c in mozilla::PresShell::Paint (
this=this@entry=0x7fc92c2810, aViewToPaint=aViewToPaint@entry=0x7fc92a0d20,
aDirtyRegion=...,
aFlags=aFlags@entry=mozilla::PaintFlags::PaintLayers)
at layout/base/PresShell.cpp:6400
#24 0x0000007ff41ef4dc in nsViewManager::ProcessPendingUpdatesPaint (
this=this@entry=0x7fc92a0cb0, aWidget=aWidget@entry=0x7fc9250910)
at ${PROJECT}/obj-build-mer-qt-xr/dist/include/mozilla/gfx/RectAbsolute.h:43
[...]
#57 0x0000007fefbae89c in ?? () from /lib64/libc.so.6
(gdb)
That's a rather long backtrace, but it captures what I need to know, which is that it's not getting called from the CompositorOGL code, but rather from the painting code. That's because it's being created specifically for the WebGL entity on the page.It'll also be useful to find out whether CompositorOGL::CreateContext() gets called when rendering browser pages. That's useful because it'll tell us why the code there isn't creating a SharedSurface_Basic object. So I've placed a breakpoint on the method and will step through to find out. This, again, is using the code installed from our original packages with working WebGL and on a page that includes WebGL content:
Thread 37 "Compositor" hit Breakpoint 3, mozilla::layers::
CompositorOGL::CreateContext (this=this@entry=0x7ed81a1c80)
at gfx/layers/opengl/CompositorOGL.cpp:227
227 already_AddRefed<mozilla::gl::GLContext> CompositorOGL::CreateContext()
{
(gdb) n
231 nsIWidget* widget = mWidget->RealWidget();
(gdb) n
232 void* widgetOpenGLContext =
(gdb) n
234 if (widgetOpenGLContext) {
(gdb) p widgetOpenGLContext
$1 = (void *) 0x7ed81a6fe0
(gdb) n
mozilla::layers::CompositorOGL::Initialize (this=0x7ed81a1c80,
out_failureReason=0x7f1651d510)
at gfx/layers/opengl/CompositorOGL.cpp:395
395 mGLContext = CreateContext();
Stepping through the code, after reaching the condition on widgetOpenGLContext, we then find ourselves outside the CompositorOGL::CreateContext() method:
(gdb) bt 2
#0 mozilla::layers::CompositorOGL::Initialize (this=0x7ed81a1c80,
out_failureReason=0x7f1651d510)
at gfx/layers/opengl/CompositorOGL.cpp:395
#1 0x0000007ff2a66d88 in mozilla::layers::CompositorBridgeParent::
NewCompositor (this=this@entry=0x7fc89ab730, aBackendHints=...)
at gfx/layers/ipc/CompositorBridgeParent.cpp:1493
(More stack frames follow...)
(gdb)
That's because of the early return from near the start of the method, as we can see here:
already_AddRefed<mozilla::gl::GLContext> CompositorOGL::CreateContext() {
RefPtr<GLContext> context;
// Used by mock widget to create an offscreen context
nsIWidget* widget = mWidget->RealWidget();
void* widgetOpenGLContext =
widget ? widget->GetNativeData(NS_NATIVE_OPENGL_CONTEXT) : nullptr;
if (widgetOpenGLContext) {
GLContext* alreadyRefed = reinterpret_cast<GLContext*>(widgetOpenGLContext);
return already_AddRefed<GLContext>(alreadyRefed);
}
[...]
So what we can infer from this is, in short, that widget->GetNativeData(NS_NATIVE_OPENGL_CONTEXT) is set in the case of WebGL rendering. but not when it's WebView rendering.It turns out we get the same flow when rendering a page without WebGL, so this isn't a consequence of the WebGL on the page, it's just because we're using the browser rather than a WebView, as we can see here:
Thread 39 "Compositor" hit Breakpoint 3, mozilla::layers::
CompositorOGL::CreateContext (this=this@entry=0x7ed81a1c70)
at gfx/layers/opengl/CompositorOGL.cpp:227
227 already_AddRefed<mozilla::gl::GLContext> CompositorOGL::CreateContext()
{
(gdb) bt 3
#0 mozilla::layers::CompositorOGL::CreateContext (this=this@entry=0x7ed81a1c70)
at gfx/layers/opengl/CompositorOGL.cpp:227
#1 0x0000007ff2950ff8 in mozilla::layers::CompositorOGL::Initialize (
this=0x7ed81a1c70, out_failureReason=0x7f3633e510)
at gfx/layers/opengl/CompositorOGL.cpp:395
#2 0x0000007ff2a66d88 in mozilla::layers::CompositorBridgeParent::
NewCompositor (this=this@entry=0x7fc89a9d80, aBackendHints=...)
at gfx/layers/ipc/CompositorBridgeParent.cpp:1493
(More stack frames follow...)
(gdb) n
231 nsIWidget* widget = mWidget->RealWidget();
(gdb) n
232 void* widgetOpenGLContext =
(gdb) n
234 if (widgetOpenGLContext) {
(gdb) p widgetOpenGLContext
$2 = (void *) 0x7ed81a6fd0
(gdb) n
79 ${PROJECT}/obj-build-mer-qt-xr/dist/include/mozilla/RefPtr.h: No such
file or directory.
(gdb) n
mozilla::layers::CompositorOGL::Initialize (this=0x7ed81a1c70,
out_failureReason=0x7f3633e510)
at gfx/layers/opengl/CompositorOGL.cpp:395
395 mGLContext = CreateContext();
(gdb)
Okay, great. There's one other important piece of information that we need in order to unravel all this and that's that the code we added to the SharedSurface_Basic class in order to get the WebView working (it's not working yet, but we now know we'll need it) is actually only an extension of the SharedSurface_Basic code, without removing any existing code. In fact it adds a new constructor and new Create() method (as an override with a different signature), but without removing the original constructor or Create() method. Here are the old and new Create() methods as they appear side-by-side in the code:
static UniquePtr<SharedSurface_Basic> Create(const SharedSurfaceDesc&);
static UniquePtr<SharedSurface_Basic> Create(GLContext* gl,
const gfx::IntSize& size);
The only code that's not strictly an addition happens in the SurfaceFactory_Basic class where the Create() method that's used to create the SharedSurface_Basic object in the CreateSharedImpl() method is switched from the old one to the new one.
class SurfaceFactory_Basic final : public SurfaceFactory {
public:
explicit SurfaceFactory_Basic(GLContext& gl);
explicit SurfaceFactory_Basic(GLContext* gl,
const layers::TextureFlags& flags);
virtual UniquePtr<SharedSurface> CreateSharedImpl(
const SharedSurfaceDesc& desc) override {
- return SharedSurface_Basic::Create(desc);
+ return SharedSurface_Basic::Create(mDesc.gl, desc.size);
}
};
It would be helpful if I could confirm this, so I've built a version of the library that's exactly the same as the version with broken WebGL and I've switched it to use the original SharedSurface_Basic::Create() method. That's the only change. And when I run it, the WebGL is working. So that's confirmed, it's definitely the new code that's been added to the SharedSurface_Basic class that's causing the problem. It's great to have this finally pinned down, but what's even better is that it looks like the problem can be bypassed simply by choosing a different constructor.What does this all mean? It means that we know all of the following:
- The code added to the SharedSurface_Basic needed for WebView rendering is what's causing the WebGL to fail in the browser.
- The change that actually causes WebView rendering to fail is just one line: the switch of the Create() method in the SurfaceFactory_Basic::CreateSharedImpl() method.
- The calls to SurfaceFactory_Basic that are used to create the SharedSurface_Basic objects are in different places depending on whether they're used for the WebGL or the WebView. In the former case they happen in the painting loop. In the latter case they happen in CompositorOGL::CreateContext().
We can use all of this to our advantage. I've taken the SurfaceFactory_Basic class and created a new version, which I've called SurfaceFactory_GL. This performs the new constructor type. So we now have two versions, like this:
class SurfaceFactory_Basic final : public SurfaceFactory {
public:
explicit SurfaceFactory_Basic(GLContext& gl);
explicit SurfaceFactory_Basic(GLContext* gl,
const layers::TextureFlags& flags);
virtual UniquePtr<SharedSurface> CreateSharedImpl(
const SharedSurfaceDesc& desc) override {
return SharedSurface_Basic::Create(desc);
}
};
class SurfaceFactory_GL final : public SurfaceFactory {
public:
explicit SurfaceFactory_GL(GLContext& gl);
explicit SurfaceFactory_GL(GLContext* gl,
const layers::TextureFlags& flags);
virtual UniquePtr<SharedSurface> CreateSharedImpl(
const SharedSurfaceDesc& desc) override {
return SharedSurface_Basic::Create(mDesc.gl, desc.size);
}
};
Notice how they differ only in the parameters passed to the SharedSurface_Basic::Create() method. Together they'll allow us to create the SharedSurface_Basic objects needed for the two different pipelines. I may find I need something more nuanced in the future and, if so, I may end up having to create an entirely new type of SharedSurface. But if that does turn out to be the case then I now know it'll work out fine: it's all nice self-contained code.Alright, I've made this change and I want to see it in action. The partial build shows good results, but I want to check things with the debugger again. So I've set off another full build. Let's find out where things are at when it's done.
Before I sign off for the day, let me say that I'm getting increasingly hopeful about this. It really feels like we've got all the information needed to fix the problem and that we now just need to carefully move all of the metaphorical girders into their correct positions.
If you'd like to read any of my other gecko diary entries, they're all available on my Gecko-dev Diary page.