flypig.co.uk

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.