flypig.co.uk

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:

$ 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:
 

1. To pass the EGLDisplay value through the system.
2. To support Wayland surfaces.
3. 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.

1. 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.
2. 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.
3. 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.
4. Create a patch from the remaining changes, which should represent those needed to support Wayland surfaces. Keep this patch somewhere safe.
5. Apply all of the remaining changes from D106879.
6. Build and check that everything still works.
7. Take a deep breath. Commit the staged changes.
8. Admire the resulting, much cleaner, GLSCreenBuffer commit now at HEAD.
9. Another deep breath. Force push everything to the server. Done.

That looks very much like a plan. Time to execute it.

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.