flypig.co.uk

Overnight I've been running a build. For the last few days — including last night — I've been on holiday lodging in a hotel, which has been lovely, but sadly today is the last day which means I'll be spending today travelling. It would have been great to have the build complete overnight so that I can start my edit-rebuild-test cycle while I travel. But the build failed, which means I have some more fixing to do first.

Here's the build error.

98:13.71 In file included from Unified_cpp_gfx_gl1.cpp:2:
98:13.71 ${PROJECT}/gecko-dev/gfx/gl/SharedSurfaceEGL.cpp: In member function 
    ‘virtual bool mozilla::gl::SharedSurface_EGLImage::ReadPixels(GLint, GLint, 
    GLsizei, GLsizei, GLenum, GLenum, GLvoid*)’:
98:13.71 ${PROJECT}/gecko-dev/gfx/gl/SharedSurfaceEGL.cpp:186:38: error: ‘void 
    mozilla::gl::GLContext::raw_fDeleteFramebuffers(GLsizei, const GLuint*)’ is 
    private within this context
98:13.71    gl->raw_fDeleteFramebuffers(1, &fbo);
98:13.71                                       ^

The problem here is that I moved raw_fDeleteFramebuffers() into the private block of the class definition it's part of. It ought to be in the private block, but there's obviously an attempt being made in SharedSurfaceEGL.cpp to call it from another class.

My suspicion is that this is part of the debug code I added. It looks like this is being used by a ReadPixels() method I added for the purposes of extracting pixel colours:

bool SharedSurface_EGLImage::ReadPixels(GLint x, GLint y, GLsizei width, 
    GLsizei height,
                        GLenum format, GLenum type, GLvoid* pixels) {
  const auto& gl = GLContextEGL::Cast(mDesc.gl);

  // See https://stackoverflow.com/a/53993894
  GLuint fbo;
  gl->fGenFramebuffers(1, &fbo);
  gl->raw_fBindFramebuffer(LOCAL_GL_FRAMEBUFFER, fbo);
  gl->fFramebufferTexture2D(LOCAL_GL_FRAMEBUFFER, LOCAL_GL_COLOR_ATTACHMENT0, 
    LOCAL_GL_TEXTURE_2D, mProdTex, 0);

  gl->raw_fReadPixels(x, y, width, height, format, type, pixels);

  gl->raw_fBindFramebuffer(LOCAL_GL_FRAMEBUFFER, 0);
  gl->raw_fDeleteFramebuffers(1, &fbo);

  return true;
}

There's a little nuance here though. This SharedSurface_EGLImage::ReadPixels() method is called from the GLScreenBuffer::ReadPixels() method, like so:

bool GLScreenBuffer::ReadPixels(GLint x, GLint y, GLsizei width, GLsizei height,
                                GLenum format, GLenum type, GLvoid* pixels) {
  // If the currently bound framebuffer is backed by a SharedSurface
  // then it might want to override how we read pixel data from it.
  // This is normally only the default framebuffer, but we can also
  // have SharedSurfaces bound to other framebuffers when doing
  // readback for BasicLayers.
  SharedSurface* surf;
  if (GetReadFB() == 0) {
    surf = SharedSurf();
  } else {
    surf = mGL->mFBOMapping[GetReadFB()];
  }
  if (surf) {
    return surf->ReadPixels(x, y, width, height, format, type, pixels);
  }

  return false;
}

This method exists in the GLScreenBuffer class of ESR 78 as well, so it's initially surprising that this is happy to call surf->ReadPixels() given SharedSurface_EGLImage::ReadPixels() doesn't exist.

Looking in to this more deeply, the reason this doesn't cause a problem in ESR 78 appears to be because the SharedSurface_EGLImage version of the method is an override. So if I were to remove it, the inherited version of the method would be called instead:

  virtual bool ReadPixels(GLint x, GLint y, GLsizei width, GLsizei height,
                          GLenum format, GLenum type, GLvoid* pixels) {
    return false;
  }

Making this change would allow things to compile, but it's not going to be adding a great deal of value. So I could just get rid of the SharedSurface_EGLImage version of the call and rely on this default implementation. But I can't at the moment see where the GLScreenBuffer version of the call is used so it seems to me it would be better to remove the GLScreenBuffer variant of the method as well.

Which is what I've done. If this method is being used it'll cause an error during compilation, which will slow things down, but will at least make clear where the caller lives. And if it goes through then we've managed to find a nice simplification, which is after all the purpose of this effort.

I've set the build off, but while it's running it might be a good idea to check in case something that consumes libxul.so is expecting access to this ReadPixels() method. One of the reasons I'm wondering is that there are certainly situations in which the texture buffer is read, such as when the images used for the tab previews and bookmark icons are generated. So it's possible it's a method we need to keep.

In QtMozEmbed there is something similar, but it calls GLES directly, rather than via libxul.so:

QImage gl_read_framebuffer(const QRect &rect)
{
    QSize size = rect.size();
    int x = rect.x();
    int y = rect.y();

    while (glGetError());

    QImage img(size, QImage::Format_RGB32);
    GLint fmt = GL_BGRA_EXT;
    glReadPixels(x, y, size.width(), size.height(), fmt, GL_UNSIGNED_BYTE, 
    img.bits());
    if (!glGetError())
        return img.mirrored();

    QImage rgbaImage(size, QImage::Format_RGBX8888);
    glReadPixels(x, y, size.width(), size.height(), GL_RGBA, GL_UNSIGNED_BYTE, 
    rgbaImage.bits());
    if (!glGetError())
        return rgbaImage.mirrored();
    return QImage();
}

There's also something similar in the sailfish-browser code. However, looking carefully at this shows that it's debug code I added myself. It's not needed for the final app and in fact, it's about time I removed it.

$ git diff
diff --git a/apps/core/declarativewebcontainer.cpp b/apps/core/
    declarativewebcontainer.cpp
index 60d5327c..1e2d63ce 100644
--- a/apps/core/declarativewebcontainer.cpp
+++ b/apps/core/declarativewebcontainer.cpp
@@ -694,9 +694,28 @@ void DeclarativeWebContainer::clearWindowSurface()
     QOpenGLFunctions_ES2* funcs = 
    m_context->versionFunctions<QOpenGLFunctions_ES2>();
     Q_ASSERT(funcs);
 
-    funcs->glClearColor(1.0, 1.0, 1.0, 0.0);
+    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);
 }
 
 void DeclarativeWebContainer::dumpPages() const

Thankfully these debug changes are all neatly encapsulated and caught by git, so removing them is as simple as performing a git checkout.

$ git checkout apps/core/declarativewebcontainer.cpp
Updated 1 path from the index
$ git diff
$ 

I don't dare run a sailfish-browser build while the gecko build is already running (maybe it's safe... but I'm not certain). So I've lodged the fact I need to in my mental task queue. There were also some changes to embedlite-components and sailfish-components-webview which I've removed for the former and will need to commit for the latter. They're not related to ReadPixels() but I'm making another mental note here to rebuild and reinstall them later once the gecko build has completed.

Well, the build is continuing so that's it for today. More tomorrow.

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