flypig.co.uk

My first task today is to figure out when and how often DeclarativeWebContainer::clearWindowSurface() gets called on ESR 78. As I discovered yesterday it's called only once on the latest ESR 91 build and that's the same build where things are broken now even for the sailfish-browser. So my hope is that this will lead to some insight as to why it's broken.

My suspicion is that it should be called often. In fact, each time the screen is updated. If that's the case then it will give me a clear issue to track down: why is it not being called every update on ESR 91.

For context, recall that DeclarativeWebContainer::clearWindowSurface() is not part of the gecko code itself, but rather part of the sailfish-browser code.

So, I'm firing up the debugger to find out.

$ gdb sailfish-browser
[...]
(gdb) b DeclarativeWebContainer::clearWindowSurface
Breakpoint 1 at 0x3c750: file ../core/declarativewebcontainer.cpp, line 681.
(gdb) r
Starting program: /usr/bin/sailfish-browser 
[...]
Created LOG for EmbedLite
[...]

Thread 1 "sailfish-browse" hit Breakpoint 1, DeclarativeWebContainer::
    clearWindowSurface (this=0x55559bbc60) at ../core/
    declarativewebcontainer.cpp:681
681         m_context->makeCurrent(this);
(gdb) bt
#0  DeclarativeWebContainer::clearWindowSurface (this=0x55559bbc60) at ../core/
    declarativewebcontainer.cpp:681
#1  0x0000005555594e68 in DeclarativeWebContainer::exposeEvent (
    this=0x55559bbc60) at ../core/declarativewebcontainer.cpp:815
#2  0x0000007fb83603dc in QWindow::event(QEvent*) () from /usr/lib64/
    libQt5Gui.so.5
#3  0x0000005555594b8c in DeclarativeWebContainer::event (this=0x55559bbc60, 
    event=0x7fffffecf8) at ../core/declarativewebcontainer.cpp:770
#4  0x0000007fb7941144 in QCoreApplication::notify(QObject*, QEvent*) () from /
    usr/lib64/libQt5Core.so.5
#5  0x0000007fb79412e8 in QCoreApplication::notifyInternal2(QObject*, QEvent*) (
    ) from /usr/lib64/libQt5Core.so.5
#6  0x0000007fb8356488 in QGuiApplicationPrivate::processExposeEvent(
    QWindowSystemInterfacePrivate::ExposeEvent*) () from /usr/lib64/
    libQt5Gui.so.5
#7  0x0000007fb83570b4 in QGuiApplicationPrivate::processWindowSystemEvent(
    QWindowSystemInterfacePrivate::WindowSystemEvent*) ()
   from /usr/lib64/libQt5Gui.so.5
#8  0x0000007fb83356e4 in QWindowSystemInterface::sendWindowSystemEvents(
    QFlags<QEventLoop::ProcessEventsFlag>) () from /usr/lib64/libQt5Gui.so.5
#9  0x0000007faf65ac4c in ?? () from /usr/lib64/libQt5WaylandClient.so.5
#10 0x0000007fb70dfd34 in g_main_context_dispatch () from /usr/lib64/
    libglib-2.0.so.0
#11 0x0000007fb70dffa0 in ?? () from /usr/lib64/libglib-2.0.so.0
#12 0x0000007fb70e0034 in g_main_context_iteration () from /usr/lib64/
    libglib-2.0.so.0
#13 0x0000007fb7993a90 in QEventDispatcherGlib::processEvents(QFlags<QEventLoop:
    :ProcessEventsFlag>) () from /usr/lib64/libQt5Core.so.5
#14 0x0000007fb793f608 in QEventLoop::exec(QFlags<QEventLoop::
    ProcessEventsFlag>) () from /usr/lib64/libQt5Core.so.5
#15 0x0000007fb79471d4 in QCoreApplication::exec() () from /usr/lib64/
    libQt5Core.so.5
#16 0x000000555557b360 in main (argc=<optimized out>, argv=<optimized out>) at 
    main.cpp:201
(gdb) c
Continuing.
[...]
Created LOG for EmbedLiteLayerManager
[...]

Thread 38 &quot;Compositor&quot; hit Breakpoint 1, DeclarativeWebContainer::
    clearWindowSurface (this=0x55559bbc60) at ../core/
    declarativewebcontainer.cpp:681
681         m_context->makeCurrent(this);
(gdb) bt
#0  DeclarativeWebContainer::clearWindowSurface (this=0x55559bbc60) at ../core/
    declarativewebcontainer.cpp:681
#1  0x0000005555591910 in DeclarativeWebContainer::createGLContext (
    this=0x55559bbc60) at ../core/declarativewebcontainer.cpp:1193
#2  0x0000007fb796b204 in QMetaObject::activate(QObject*, int, int, void**) () 
    from /usr/lib64/libQt5Core.so.5
#3  0x0000007fbfb9f5c8 in QMozWindowPrivate::RequestGLContext (
    this=0x5555b81290, context=@0x7edb46b2f0: 0x0, surface=@0x7edb46b2f8: 0x0, 
    display=@0x7edb46b300: 0x0) at qmozwindow_p.cpp:133
#4  0x0000007fbca9a374 in mozilla::embedlite::nsWindow::GetGLContext (
    this=0x7f80ce9e90)
    at mobile/sailfishos/embedshared/nsWindow.cpp:408
#5  0x0000007fba682718 in mozilla::layers::CompositorOGL::CreateContext (
    this=0x7e98003420)
    at gfx/layers/opengl/CompositorOGL.cpp:228
#6  0x0000007fba6a33a4 in mozilla::layers::CompositorOGL::Initialize (
    this=0x7e98003420, out_failureReason=0x7edb46b720)
    at gfx/layers/opengl/CompositorOGL.cpp:374
#7  0x0000007fba77aff4 in mozilla::layers::CompositorBridgeParent::
    NewCompositor (this=this@entry=0x7f80ce94a0, aBackendHints=...)
    at gfx/layers/ipc/CompositorBridgeParent.cpp:1534
#8  0x0000007fba784660 in mozilla::layers::CompositorBridgeParent::
    InitializeLayerManager (this=this@entry=0x7f80ce94a0, aBackendHints=...)
    at gfx/layers/ipc/CompositorBridgeParent.cpp:1491
#9  0x0000007fba7847a8 in mozilla::layers::CompositorBridgeParent::
    AllocPLayerTransactionParent (this=this@entry=0x7f80ce94a0, 
    aBackendHints=..., aId=...)
    at gfx/layers/ipc/CompositorBridgeParent.cpp:1587
#10 0x0000007fbca81234 in mozilla::embedlite::EmbedLiteCompositorBridgeParent::
    AllocPLayerTransactionParent (this=0x7f80ce94a0, aBackendHints=..., 
    aId=...)
    at mobile/sailfishos/embedthread/EmbedLiteCompositorBridgeParent.cpp:77
#11 0x0000007fba05f3f0 in mozilla::layers::PCompositorBridgeParent::
    OnMessageReceived (this=0x7f80ce94a0, msg__=...) at 
    PCompositorBridgeParent.cpp:1391
[...]
#27 0x0000007fb735989c in thread_start () at ../sysdeps/unix/sysv/linux/aarch64/
    clone.S:78
(gdb) c
Continuing.
[...]

Thread 38 &quot;Compositor&quot; hit Breakpoint 1, DeclarativeWebContainer::
    clearWindowSurface (this=0x55559bbc60) at ../core/
    declarativewebcontainer.cpp:681
681         m_context->makeCurrent(this);
(gdb) bt
#0  DeclarativeWebContainer::clearWindowSurface (this=0x55559bbc60) at ../core/
    declarativewebcontainer.cpp:681
#1  0x0000005555591830 in DeclarativeWebContainer::clearWindowSurfaceTask (
    data=0x55559bbc60) at ../core/declarativewebcontainer.cpp:671
#2  0x0000007fbca81e10 in details::CallFunction<0ul, void (*)(void*), void*> (
    arg=..., function=<optimized out>)
    at ipc/chromium/src/base/task.h:52
#3  DispatchTupleToFunction<void (*)(void*), void*> (arg=..., 
    function=<optimized out>)
    at ipc/chromium/src/base/task.h:53
#4  RunnableFunction<void (*)(void*), mozilla::Tuple<void*> >::Run (
    this=<optimized out>)
    at ipc/chromium/src/base/task.h:324
#5  0x0000007fb9bfe4e4 in nsThread::ProcessNextEvent (aResult=0x7edb46be77, 
    aMayWait=<optimized out>, this=0x7f80cc0580)
    at xpcom/threads/nsThread.cpp:1211
[...]
#15 0x0000007fb735989c in thread_start () at ../sysdeps/unix/sysv/linux/aarch64/
    clone.S:78
(gdb) c
Continuing.
[...]

It turns out I'm wrong about this DeclarativeWebContainer::clearWindowSurface() method. It gets called three times as the page is loading and that's it. Checking the code, it's triggered by signals that come from qtmozembed. These are, in order:

1. DeclarativeWebContainer::exposeEvent().
2. DeclarativeWebContainer::createGLContext() triggered by the QMozWindow::requestGLContext() signal.
3. WebPageFactory::aboutToInitialize() on page creation.

Here are some of the calls and signals which potentially could result in a call to clearWindowSurface(). Not all of these are actually happening during the runs I've been testing:

DeclarativeWebContainer::exposeEvent()

connect(m_mozWindow.data(), &QMozWindow::requestGLContext, this, 
    &DeclarativeWebContainer::createGLContext, Qt::DirectConnection);

connect(m_mozWindow.data(), &QMozWindow::compositorCreated, this, 
    &DeclarativeWebContainer::postClearWindowSurfaceTask);

connect(m_model.data(), &DeclarativeTabModel::tabClosed, this, 
    &DeclarativeWebContainer::releasePage);

DeclarativeWebContainer::clearSurface()

On ESR 91 only one of these is happening: the first one triggered by a call to exposeEvent(). So what of the others? Placing breakpoints in various places I'm able to identify that CompositorOGL::CreateContext() is called on ESR 91. This appears in the call stack of the second breakpoint hit on ESR 78, so this should in theory also be calling clearWindowSurface() on ESR 91, but it's not.

What happens is that there's a break in the call stack. While the following aren't called:

#0  clearWindowSurface() at ../core/declarativewebcontainer.cpp:681
#1  createGLContext() at ../core/declarativewebcontainer.cpp:1193
#2  QMetaObject::activate() /usr/lib64/libQt5Core.so.5
#3  QMozWindowPrivate::RequestGLContext() at qmozwindow_p.cpp:133
#4  GetGLContext() mobile/sailfishos/embedshared/nsWindow.cpp:408

The following are called. These are all prior to the others in the callstack on ESR 78, which means that the break is happening in the CompositorOGL::CreateContext() method.

#5  CompositorOGL::CreateContext() gfx/layers/opengl/CompositorOGL.cpp:228
#6  CompositorOGL::Initialize() gfx/layers/opengl/CompositorOGL.cpp:374
#7  NewCompositor() gfx/layers/ipc/CompositorBridgeParent.cpp:1534
#8  InitializeLayerManager() gfx/layers/ipc/CompositorBridgeParent.cpp:1491

So tomorrow I'll need to take a look at the CompositorOGL::CreateContext() code. Based on what I've seen today, something different is likely to be happening in there compared to what was happening with the working version. By looking at the diff and seeing what's changed, it should be possible to figure out what. More on this tomorrow.

If you'd like to read any of my other gecko diary entries, they're all available on my Gecko-dev Diary page.

Today has been a bit of a disheartening day from a Gecko perspective. So disheartening in fact that I considered pausing this blog, taking some time off and then trying to fix it in private to avoid having to admit how badly things are going right now.

But it's important to show the downs as well as the ups. Software development can be a messy business at times, things rarely go to plan and even when they do there's still often an awful lot of angst and frustration preceding the enjoyment of getting something working.

So here it is, warts and all.

Overnight I rebuilt the packages with the new patches installed. Running the WebView showed no changes in the output: still a white screen, no page rendering or coloured backgrounds. I can live with that, it's not what I wanted but it's also no worse than before.

So I decided to head off in the direction that was set last Thursday when I laid plans to check the implementation that happens between the DeclarativeWebContainer::clearWindowSurface() method and the CompositorOGL::EndFrame() method. This direction is in response to the useful discussion in last week's Sailfish Community Meeting.

First up I wanted to establish what was happening on the window side, so starting at clearWindowSurface() I added some code first to change the colour used to clear the texture from white to green.

This didn't affect the rendering, which remains white. Okay, so far so normal. That's a little unexpected, but nevertheless adds new and useful information.

I then added some code to read off the colour at the centre of the texture. This is pretty simple code and, as before, I added some debug output lines so we can find out what's going wrong. Here's what the method looks like now:

void DeclarativeWebContainer::clearWindowSurface()
{
    Q_ASSERT(m_context);
    // The GL context should always be used from the same thread in which it 
    was created.
    Q_ASSERT(m_context->thread() == QThread::currentThread());
    m_context->makeCurrent(this);
    QOpenGLFunctions_ES2* funcs = 
    m_context->versionFunctions<QOpenGLFunctions_ES2>();
    Q_ASSERT(funcs);

    funcs->glClearColor(0.0, 1.0, 0.0, 0.0);
    funcs->glClear(GL_COLOR_BUFFER_BIT);
    m_context->swapBuffers(this);

    QSize screenSize = QGuiApplication::primaryScreen()->size();
    size_t bufferSize = screenSize.width() * screenSize.height() * 4;
    uint8_t* buf = static_cast<uint8_t*>(calloc(sizeof(uint8_t), bufferSize));

    funcs->glReadPixels(0, 0, screenSize.width(), screenSize.height(),
                            GL_RGBA, GL_UNSIGNED_BYTE, buf);

    int xpos = screenSize.width() / 2;
    int ypos = screenSize.height() / 2;
    int pos = xpos * ypos * 4;

    volatile char red = buf[pos];
    volatile char green = buf[pos + 1];
    volatile char blue = buf[pos + 2];
    volatile char alpha = buf[pos + 3];

    printf(&quot;Colour: (%d, %d, %d, %d)\n&quot;, red, green, blue, alpha);
    free(buf);
}

Everything after the call to swapBuffers() is newly added.

Executing this I was surprised to find that there was no new output from this. So I tried to set a breakpoint on it, but gdb claims it doesn't exist in the binary. A bit of reflection made me realise why: this is code in sailfish-browser, which forms part of the browser, not the WebView.

So for the WebView I'll need to find the similar equivalent call. Before trying to find out where the correct call lives, I thought I'd first see what happens when I run the browser instead of the WebView. Will gdb have more luck with that?

And this is where things start to go wrong. When running with the browser the method is called once. The screen turns green. But there's no other rendering. The fact the screen goes green is good. The fact there's no other rendering is bad. Very bad.

It means that over the last few weeks while I've been trying to fix the WebView render pipeline, I've been doing damage to the browser render pipeline in the process. I honestly thought they didn't interact at this level, but it turns out I was wrong.

So now I have both a broken WebView and a broken browser to fix. Grrrr.

While I was initially quite downhearted about this, on reflection it's not quite as bad as it sounds. The working browser version is still safely stored in the repository, so if necessary I can just revert back. But I've also got all of the changes on top of this easily visible using git locally on my system. So I can at least now work through the changes to see whether I can figure out what's responsible.

I'm not going to make more progress on this tonight, so I'll have to return to it tomorrow to try to understand what I've broken.

So very frustrating. But that's software development for you.

If you'd like to read any of my other gecko diary entries, they're all available on my Gecko-dev Diary page.

I've been continuing my patch audit today. It remains a bit of a lengthy and laborious process, but I have at least managed to get through them all.

Until now I've been keeping track of patches that have been applied, patches that aren't needed — either because changes have become redundant or because the code has changed so much that they're just no longer applicable — and patches that haven't yet been applied.

As I worked through today I found a new category to log: patches that haven't been applied but really should be. For example there are patches to ensure libcontentaction is used for handling URLs, or patches for managing extensions better on mobile. These are changes that won't necessarily have an immediately obvious effect on whether or not the browser runs, but will have functionality or efficiency impacts that need to be included.

The list of all such patches is longer than I was expecting:

0056, 0057, 0058, 0059, 0060, 0061, 0062, 0063, 0067, 0068, 0069, 0074, 0077, 0078, 0079, 0080, 0081, 0086.

You might well ask, if I know for sure that these should be applied, why didn't I just apply them? That's because I don't want to muddy the waters with them while I focus on fixing the offscreen render pipeline. If I applied these patches now, not only would it make debugging harder, it would also make the partitioning of code changes into clean patches harder as well. What I am sure about, however, is that none of these are affecting the render pipeline problem.

On top of these the following patches haven't been applied, but potentially should be. I say potentially because these are patches which either probably should be applied but I've not had time to properly decide yet, or where it's possible the reason for the patch was fixed upstream, but it's not entirely clear yet.

For example this includes patches for integrating with FFmpeg version 5.0 and above, patches to fix bugs or fixes for memory leaks. When I checked these they definitely hadn't yet been applied, but maybe they can safely be skipped. I should go through all of these more carefully at some point. Here's the list:

0008, 0012, 0014, 0041, 0042, 0043, 0044, 0045, 0046, 0049, 0050, 0051, 0052, 0064, 0066, 0073, 0076, 0082, 0083, 0084, 0085, 0087, 0089, 0090, 0091, 0094, 0095, 0096, 0097, 0099.

Then there are the patches that are not needed. I didn't find any more of these, so the list remains the same as yesterday with the following:

0004, 0005, 0013, 0035.

Finally the patches that have already been applied:

0001, 0002, 0003, 0006, 0007, 0009, 0010, 0011, 0015, 0016, 0017, 0018, 0019, 0020, 0021, 0022, 0023, 0024, 0025, 0026, 0027, 0028, 0029, 0030, 0031, 0032, 0033, 0034, 0036, 0037, 0038, 0039, 0040, 0047, 0048, 0053, 0054, 0055, 0065, 0070, 0071, 0072, 0075, 0088, 0092, 0093, 0099.

That's 47 patches in total; just under half. Plus the four that won't get applied at all sums to 51 patches dealt with, leaving 48 that still need dealing with; just under half.

Of those patches that have been applied, the following are the ones that hadn't been applied but looked potentially relevant to the render pipeline issue I'm tryig to fix. So I went ahead and applied all of these just in case:

0053, 0054, 0055, 0065, 0075.

My suspicion is that none of these will be important enough to fix the issue, but all contributions are helpful. The next step is to build myself some new packages, install them and see where we are.

Then, tomorrow, I can finally move on to the task that was discussed at the community meeting last Thursday. I'm going to start testing the render status at points in between DeclarativeWebContainer::clearWindowSurface() and CompositorOGL::EndFrame(). This will help narrow down where the working render turns into a broken render. That'll leave me in a much better position to focus on why.

I'm just starting the build now.

If you'd like to read any of my other gecko diary entries, they're all available on my Gecko-dev Diary page.

After re-reading patch 0053 yesterday, today I've been reintegrating the TextureImageEGL class back into the codebase. For context, patch 0053 makes a very small change to a single file, the TextureImageEGL.h file. But when I tried to check whether the patch had been applied or not I discovered the file it's supposed to apply to has been removed entirely from the ESR 91 codebase entirely.

The upstream changeset that removed the file mentions that the class doesn't add anything over and above what BasicTextureImage already offers. When I compared the two classes myself I came to pretty much the same conclusion: there really isn't much that's different between them. The key difference seems to be that TextureImageEGL allows for a wide variety of surface formats, whereas BasicTextureImage supports only RGBA format. There's also a slight difference in that TextureImageEGL seems to be more aggressive in resizing the texture.

Neither of these strike me as significant enough to prevent rendering, but you never know. So I've copied over the two relevant files — TextureImageEGL.h and TextureImageEGL.cpp — and integrated them into the build system. I also had to adjust the code in a few places in order to support the new EglDisplay class and some slight differences in the overridable parent method signatures that necessitated altering the signatures in the class.

Since then I've been trying to perform a partial rebuild of the code, trying to avoid having to do a full rebuild. Previously I would have run a full build overnight, but I've been getting increasingly confident with the way the build system works. Consequently I persuaded mach to rebuild the configuration based on the advice it offered up:

$ sfdk engine exec
$ sb2 -t SailfishOS-esr78-aarch64.default
$ source `pwd`/obj-build-mer-qt-xr/rpm-shared.env
$ make -j1 -C obj-build-mer-qt-xr/gfx/gl/
make: Entering directory '${PROJECT}/obj-build-mer-qt-xr/gfx/gl'
${PROJECT}/gecko-dev/config/rules.mk:338: *** Build configuration changed. 
    Build with |mach build| or run |mach build-backend| to regenerate build 
    config.  Stop.
make: Leaving directory '${PROJECT}/obj-build-mer-qt-xr/gfx/gl'

Following that advice seems to give good results:

$ ./gecko-dev/mach build-backend
 0:05.01 ${PROJECT}/obj-build-mer-qt-xr/_virtualenvs/common/bin/python 
    ${PROJECT}/obj-build-mer-qt-xr/config.status
Reticulating splines...
 0:05.22 File already read. Skipping: ${PROJECT}/gecko-dev/intl/components/
    moz.build
 0:08.32 File already read. Skipping: ${PROJECT}/gecko-dev/gfx/angle/targets/
    angle_common/moz.build
Finished reading 984 moz.build files in 19.20s
Read 11 gyp files in parallel contributing 0.00s to total wall time
Processed into 5326 build config descriptors in 17.35s
RecursiveMake backend executed in 28.37s
  1959 total backend files; 0 created; 2 updated; 1957 unchanged; 0 deleted; 21 
    -> 664 Makefile
FasterMake backend executed in 2.77s
  8 total backend files; 0 created; 1 updated; 7 unchanged; 0 deleted
Total wall time: 69.45s; CPU time: 68.55s; Efficiency: 99%; Untracked: 1.75s
Glean could not be found, so telemetry will not be reported. You may need to 
    run |mach bootstrap|.

I can then follow my usual partial build commands in order to get a newly built version of the library out.

After the build I went through the usual process of transferring and installing the new library to my phone. Sadly I'm still left with a blank screen, so this wasn't enough to get the render working. That may mean these changes can safely reverted, but I'll leave them as they are for now and aim to come back to that later.

That's because it should be easier to remove redundant code once things are working than to guess exactly which changes are needed to get things to work. Or at least, that's my rationale.

This leaves me having applied another patch today (patch 0053), so that we now have the following applied:

0001, 0002, 0003, 0006, 0007, 0009, 0010, 0011, 0015, 0016, 0017, 0018, 0019, 0020, 0021, 0022, 0023, 0024, 0025, 0026, 0027, 0028, 0029, 0030, 0031, 0032, 0033, 0034, 0036, 0037. 0038, 0039, 0040, 0047, 0048, 0053

The following remain uneeded:

0004, 0005, 0013, 0035,

While the following haven't yet been applied:

0008, 0012, 0014, 0041, 0042, 0043, 0044, 0045, 0046, 0049, 0050, 0051, 0052,

Tomorrow I'll continue working through the patches. It's slow going, but this interlude with TextureImageEGL serves to highlight that there are still relevant patches still to be applied, which means it's worth checking. But I also want to reiterate that I've not forgotten the discussion from the Sailfish Community Meeting last Thursday. I still have some work to do following the advice provided there.

If you'd like to read any of my other gecko diary entries, they're all available on my Gecko-dev Diary page.

After a motivational day getting really useful feedback from Raine, Damien and others, I'm back to going through patches today. I will of course need to act on the helpful suggestions, which I#m sure will be key to getting the rendering to work, but having started on my journey of checking patches I need to now finish it. This entire activity is an in-order execution pipeline you see.

I had reached patch 0038, otherwise named "Fix mesa egl display and buffer initialisation" and originally written by Adam Pigg, Frajo and myself back in 2021. It's a big one and also a relevant one to the current problem, so I'm going to spend a bit of time working my way through it.

The good news is that the process of checking, while a little laborious, doesn't require a lot of concentration. So it's good work to do while I'm still half asleep on my morning commute.

[...]

Having worked through patch 0038 it all looks present and correct, at least to the extent that makes sense with the way the new code is structured. I still have 61 patches to check through so I'm going to continue on with them.

So far the following are all applied, present and correct:

0001, 0002, 0003, 0006, 0007, 0009, 0010, 0011, 0015, 0016, 0017, 0018, 0019, 0020, 0021, 0022, 0023, 0024, 0025, 0026, 0027, 0028, 0029, 0030, 0031, 0032, 0033, 0034, 0036, 0037. 0038, 0039, 0040, 0047, 0048,

The following are no longer needed:

0004, 0005, 0013, 0035,

While the following haven't been applied, but might potentially be important for the browser overall, so may need to be applied at some point in the future:

0008, 0012, 0014, 0041, 0042, 0043, 0044, 0045, 0046, 0049, 0050, 0051, 0052,

I got up to patch 0051 and then I hit something unexpected. Patch 0051 makes "TextureImageEGL hold a reference to GLContext". There's a detailed explanation in the patch about why this is needed: it ensures that freed memory isn't used during the shutdown of EmbedLite port objects. The actual change required is minimal:

  protected:
   typedef gfxImageFormat ImageFormat;
 
-  GLContext* mGLContext;
+  RefPtr<GLContext> mGLContext;
 
   gfx::SurfaceFormat mUpdateFormat;
   EGLImage mEGLImage;

Changes don't come much simpler than this. But when I tried to check whether it had been applied I ran in to problems: the TextureImageEGL.h file which it's intended to apply to doesn't exist.

Well that's odd.

It would appear that the TextureImageEGL class has been completely removed. And because it's created in a factory it's apparently not affected any of the other EmbedLite changes. Here's the creation code from ESR 78:

already_AddRefed<TextureImage> CreateTextureImage(
    GLContext* gl, const gfx::IntSize& aSize,
    TextureImage::ContentType aContentType, GLenum aWrapMode,
    TextureImage::Flags aFlags, TextureImage::ImageFormat aImageFormat) {
  switch (gl->GetContextType()) {
    case GLContextType::EGL:
      return CreateTextureImageEGL(gl, aSize, aContentType, aWrapMode, aFlags,
                                   aImageFormat);
    default: {
      GLint maxTextureSize;
      gl->fGetIntegerv(LOCAL_GL_MAX_TEXTURE_SIZE, &maxTextureSize);
      if (aSize.width > maxTextureSize || aSize.height > maxTextureSize) {
        NS_ASSERTION(aWrapMode == LOCAL_GL_CLAMP_TO_EDGE,
                     &quot;Can't support wrapping with tiles!&quot;);
        return CreateTiledTextureImage(gl, aSize, aContentType, aFlags,
                                       aImageFormat);
      } else {
        return CreateBasicTextureImage(gl, aSize, aContentType, aWrapMode,
                                       aFlags);
      }
    }
  }
}

Notice how in ESR 91 the code for generating TextureImageEGL objects has been completely removed:

already_AddRefed<TextureImage> CreateTextureImage(
    GLContext* gl, const gfx::IntSize& aSize,
    TextureImage::ContentType aContentType, GLenum aWrapMode,
    TextureImage::Flags aFlags, TextureImage::ImageFormat aImageFormat) {
  GLint maxTextureSize;
  gl->fGetIntegerv(LOCAL_GL_MAX_TEXTURE_SIZE, &maxTextureSize);
  if (aSize.width > maxTextureSize || aSize.height > maxTextureSize) {
    NS_ASSERTION(aWrapMode == LOCAL_GL_CLAMP_TO_EDGE,
                 &quot;Can't support wrapping with tiles!&quot;);
    return CreateTiledTextureImage(gl, aSize, aContentType, aFlags,
                                   aImageFormat);
  } else {
    return CreateBasicTextureImage(gl, aSize, aContentType, aWrapMode, aFlags);
  }
}

Let's find out why that is.

$ git log -1 -S "CreateTextureImageEGL" -- gfx/gl/GLTextureImage.cpp
commit a7817816b708c8b1fc3362d21cbcadbff3c46741
Author: Jeff Muizelaar <jmuizelaar@mozilla.com>
Date:   Tue Jun 15 21:10:47 2021 +0000

    Bug 1716559 - Remove TextureImageEGL. r=jnicol,jgilbert
    
    TextureImageEGL doesn't seem to provide any value beyond
    BasicTextureImage. It's last usage was bug 814159.
    
    Removing this has the side effect of using BasicTextureImage
    for small images instead of always using TilingTextureImage.
    
    Differential Revision: https://phabricator.services.mozilla.com/D117904

It surprises me that I've not run in to this before, but one of the benefits of keeping this diary is that I can very quickly verify this:... I have not.

The upstream diff suggests that TextureImageEGL provides no value, but it would be easy to miss the value if it applied only to a project that's external to gecko and which isn't officially supported. So I wonder if it's potentially offering value to us?

Unfortunately I've run out of time today to investigate this further today. I'd especially like to compare the functionality of TextureImageEGL from ESR 78 with that of BasicTextureImage in ESR 91. That will be my task for tomorrow morning.

If you'd like to read any of my other gecko diary entries, they're all available on my Gecko-dev Diary page.