flypig.co.uk

Before I get started, I want to mention that I've been heartened by all of the encouraging comments on the Sailfish Forum recently. I've not had a chance to reply there — I will do — but let me just say that while I appreciate the incredibly generous offers of donations, there's absolutely no need. It might seem a bit strange, but I'm do this for my own enjoyment and because just like everyone else I'd love to see the browser get a boost to the next version. But I also know there are many fabulous Sailfish developers who are far more deserving than I am, so if you want to splash the cash, you'll find willing recipients on the excellent The big Thank You & Coffee thread and I encourage you to donate to them!

With that said, let's get in to the day's development. Yesterday I was able to remove sixteen unused methods from my offscreen rendering patch. That's good progress, but it still leaves us with a very large patch. Large enough that it's worth continuing with the process in the hope of trimming it down further.

There are a few places where small changes upstream have caused the code to diverge, but in a way that could potentially be ironed out. If I can do this it'll remove code from the patch that can safely follow the upstream changes instead.

Chief among these is a change to the way GLContext is passed between methods. It's quite a large structure and so in ESR 78 it's typically passed as a pointer. In ESR 91 this has been changed in places so that it's now passed by reference.

In C++ there's not much practical difference between these apart from syntax: dereferencing isn't needed in the latter case. But the syntax changes propagate throughout the methods it's passed to. This can lead to what appear to be significant changes where they're actually pretty minor.

Let's take an example. Here's a method taken from ESR 78:

GLuint CreateTexture(GLContext* aGL, GLenum aInternalFormat, GLenum aFormat,
                     GLenum aType, const gfx::IntSize& aSize, bool linear) {
  GLuint tex = 0;
  aGL->fGenTextures(1, &tex);
  ScopedBindTexture autoTex(aGL, tex);

  aGL->fTexParameteri(LOCAL_GL_TEXTURE_2D, LOCAL_GL_TEXTURE_MIN_FILTER,
                      linear ? LOCAL_GL_LINEAR : LOCAL_GL_NEAREST);
  aGL->fTexParameteri(LOCAL_GL_TEXTURE_2D, LOCAL_GL_TEXTURE_MAG_FILTER,
                      linear ? LOCAL_GL_LINEAR : LOCAL_GL_NEAREST);
  aGL->fTexParameteri(LOCAL_GL_TEXTURE_2D, LOCAL_GL_TEXTURE_WRAP_S,
                      LOCAL_GL_CLAMP_TO_EDGE);
  aGL->fTexParameteri(LOCAL_GL_TEXTURE_2D, LOCAL_GL_TEXTURE_WRAP_T,
                      LOCAL_GL_CLAMP_TO_EDGE);

  aGL->fTexImage2D(LOCAL_GL_TEXTURE_2D, 0, aInternalFormat, aSize.width,
                   aSize.height, 0, aFormat, aType, nullptr);

  return tex;
}

The equivalent code in ESR 91 looks like this:

UniquePtr<Texture> CreateTexture(GLContext& gl, const gfx::IntSize& size) {
  const GLenum target = LOCAL_GL_TEXTURE_2D;
  const GLenum format = LOCAL_GL_RGBA;

  auto tex = MakeUnique<Texture>(gl);
  ScopedBindTexture autoTex(&gl, tex->name, target);

  gl.fTexParameteri(target, LOCAL_GL_TEXTURE_MIN_FILTER, LOCAL_GL_LINEAR);
  gl.fTexParameteri(target, LOCAL_GL_TEXTURE_MAG_FILTER, LOCAL_GL_LINEAR);
  gl.fTexParameteri(target, LOCAL_GL_TEXTURE_WRAP_S, LOCAL_GL_CLAMP_TO_EDGE);
  gl.fTexParameteri(target, LOCAL_GL_TEXTURE_WRAP_T, LOCAL_GL_CLAMP_TO_EDGE);

  gl.fTexImage2D(target, 0, format, size.width, size.height, 0, format,
                 LOCAL_GL_UNSIGNED_BYTE, nullptr);

  return tex;
}

As you can see, several lines have their access operators changed from a dot (".") to an arrow ("->"). That's direct access vs. dereferenced access.

The changes needed to get offscreen rendering to work mean I've switched out the ESR 91 version for a copy of the ESR 78 version. While there are clearly differences between the two, they're smaller than they look at first glance. If I can refactor the code so it's more like the ESR 91 version, that should save effort in the future when the patch is applied to the next upstream changes beyond ESR 91.

This isn't just an idle preference. Much of the work I've spent on the upgrade to ESR 91 has been caused by having to refactor patches where the underlying code has changed upstream. I predict that for every line of code I can remove from a patch I'm going to save future developers two or three times the effort having not to worry about that change in the future. The smaller and more efficient the patches, the less work is needed when reapplying them later.

Apart from the pointer vs. reference difference, another important difference between these two methods is that the ESR 78 version has the format, type and linear flag passed in, whereas in ESR 91 they're all defined statically.

Let's tackle the last of these first: the linear flag. In the ESR 78 version of the method this defaults to a value of true if left unspecified. It looks like the only place this is called is from CreateTextureForOffscreen() where the parameter is left as its default value. So I'm going to remove the parameter and assume it's always true. The compiler will tell us if there are cases where ti's set to false which I missed.

To understand whether the other parameters could change we have to take a look at the GLContext::ChooseGLFormats() method, which is where the format value is chosen. And the return value depends on the SurfaceCaps structure that's passed in.

Some potential changes of the SurfaceCaps are performed in GLContextProviderEGL::CreateOffscreen(), but looking carefully at this, that can only happen if canOffscreenUseHeadless is set to false, which itself can only happen if MOZ_WIDGET_ANDROID is defined. It's never defined for us (we're not Android!) So I can safely remove not just the assumption, but the related code as well. That's much cleaner.

Now we go through to CompositorOGL::CreateContext() where the SurfaceCaps objects are originally created. Here's the code that creates them:

    SurfaceCaps caps = SurfaceCaps::ForRGB();
    caps.bpp16 = gfxVars::OffscreenFormat() == SurfaceFormat::R5G6B5_UINT16;

By default gfxVars::OffscreenFormat() will return SurfaceFormat::X8R8G8B8_UINT32. I don't see any reason why that would be changed, which would mean that caps.bpp16 is by default set to false. What does the SurfaceCaps::ForRGB() method return? Apparently these values, according to the code:

  bool any = false;
  bool color = true;
  bool alpha = false;
  bool bpp16 = false;
  bool depth = false;
  bool stencil = false;
  bool premultAlpha = true;

This tallies with the values I'm seeing in practice using the debugger as well:

(gdb) p minOffscreenCaps
$1 = {any = false, color = true, alpha = false, bpp16 = false, depth = false, 
    stencil = false, premultAlpha = true, surfaceAllocator = {mRawPtr = 0x0}}

Armed with this knowledge I can now simplify the GLContext::ChooseGLFormats() method appropriately. Although the internal format might change, the color_texFormat value is now always set to LOCAL_GL_RGB. That gets passed in to CreateTexture() as the third parameter (aFormat). So we can remove that parameter and simply set it to LOCAL_GL_RGB in all cases. In ESR 91 this is almost what's happening as well, except there it's set to LOCAL_GL_RGBA (an extra alpha channel). I think this is a difference we're going to have to maintain. We're also going to have to keep the aInternalFormat parameter as this might change depending on the GLES capabilities of the device.

Given all of this, I'm not convinced we're going to get much more out of trying to simplify this CreateTexture() method.

It's getting quite late here now and I think I've reached the end of my viable energy for the day. I'll have to return to the topic of simplification tomorrow. Overnight I'll be pondering whether to lock the texture down to 24-bit RGB or allow other textures (primarily 16-bit textures) to be supported as well. Checking the ESR 91 code, it seems to be always assuming a LOCAL_GL_RGBA texture. Maybe we'd be safe just to do that?

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