flypig.co.uk

Overnight I've been mulling options. They are to either strip out the code that chooses texture format based on device capabilities, or leave the existing large patch as it is. Going with the former will be more work and could result in incompatibilities on some devices, but has the potential to massively simplify the patch.

My decision is that I'm going to strip out the code. Already the way the decision is performed is messy. Stripping it all back and going with the bare essentials is exactly what's happened upstream; I think it makes sense to mirror these changes as much as possible. If it does cause problems for some devices, then we can look to reintroduce some of these changes. But at that point, it should be possible to do so in a much cleaner and more structured way.

The first step needed is to strip out the code from GLContext::ChooseGLFormats() so that it returns the values we saw yesterday, which were the following.

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

Stepping through the ChooseGLFormats() method we can see the consequences of these values, combined with the results returned from the GLES driver. When following this and subsequent comments it may be helpful to refer to the method as it looks upstream in ESR 78.

1763        formats.color_texType = LOCAL_GL_UNSIGNED_BYTE;
(gdb) p bpp16
$6 = false
(gdb) p caps.alpha
$7 = false
(gdb) n
1765        if (caps.alpha) {
(gdb) n
1772          formats.color_texFormat = LOCAL_GL_RGB;
(gdb) n
1773          formats.color_rbFormat = LOCAL_GL_RGB8;
(gdb) n
1779      if (IsSupported(GLFeature::packed_depth_stencil)) {
(gdb) n
1780        formats.depthStencil = LOCAL_GL_DEPTH24_STENCIL8;
(gdb) n
260         return mProfile == ContextProfile::OpenGLES;
(gdb) n
1785        if (IsExtensionSupported(OES_depth24)) {
(gdb) n
1794      formats.stencil = LOCAL_GL_STENCIL_INDEX8;
(gdb) n
1796      return formats;
(gdb) p formats
$8 = {color_texInternalFormat = 6407, color_texFormat = 6407, color_texType = 
    5121, color_rbFormat = 32849, depthStencil = 35056, depth = 33190, stencil 
    = 36168}
(gdb) 

The key values needed to determine flow through the method are these:

bpp16 == false
caps.alpha == false
IsSupported(GLFeature::packed_depth_stencil) == true
IsGLES() == true
(IsExtensionSupported(OES_depth24) == true

At the end of the method, we're left with the following capabilities:

color_texInternalFormat = 6407 = 0x1907 = LOCAL_GL_RGB
color_texFormat = 6407 = 0x1907 = LOCAL_GL_RGB
color_texType = 5121 = 0x1401 = LOCAL_GL_UNSIGNED_BYTE
color_rbFormat = 32849 = 0x8051 = LOCAL_GL_RGB8
depthStencil = 35056 = 0x88f0 = LOCAL_GL_DEPTH24_STENCIL8
depth = 33190 = 0x81a6 = LOCAL_GL_DEPTH_COMPONENT24
stencil = 36168 = 0x8d48 = LOCAL_GL_STENCIL_INDEX8

To perform the conversion from hex value to GLenum I cross-referenced against the gfx/gl/GLConsts.h file. Based on this analysis, I'm testing with the following newly reworked ChooseGLFormats() method:

GLFormats GLContext::ChooseGLFormats(const SurfaceCaps& caps) const {
  GLFormats formats;

  formats.color_texType = LOCAL_GL_UNSIGNED_BYTE;
  formats.color_texInternalFormat = LOCAL_GL_RGB;
  formats.color_texFormat = LOCAL_GL_RGB;
  formats.color_rbFormat = LOCAL_GL_RGB8;
  formats.depthStencil = LOCAL_GL_DEPTH24_STENCIL8;
  formats.depth = LOCAL_GL_DEPTH_COMPONENT24;
  formats.stencil = LOCAL_GL_STENCIL_INDEX8;

  return formats;
}

I should make clear that if this works, my plan is to remove this method entirely. I'm doing this check just to ensure I have everything correct before propagating these changes at a more fundamental level.

$ make -j1 -C obj-build-mer-qt-xr/gfx/
$ make -j16 -C `pwd`/obj-build-mer-qt-xr/toolkit

A quick test shows things are still working as expected, so the task now is to propagate this fixed configuration throughout the changes I've made. With any luck this will simplify things considerably, potentially allowing the use of SurfaceCaps to be eliminated entirely. The SurfaceCaps structure was removed between ESR 78 and ESR 91, so it'd be great if there's no need to restore it back again. Similarly for the AttachmentType class.

[...]

I've now spent a good few hours removing all references to SurfaceCaps from the code, to the extent that the structure isn't even defined in SurfaceTypes.h any more. That means big changes to the code: 79 new lines added, but more importantly 379 lines removed. That'll make a big difference to the final patch. I've also checked that the code compiles and, in theory, it's not doing anything different to the previous version that had the values fixed.

I'm going to test this out now, which means a partial build, but that will still take half an hour or so.

[...]

I've tested it; it all works well. So now I'm going to move on to the AttachmentType enumeration. Looking through the code, this attachment type is only ever set in three places: in the SharedSurface_EGLImage constructor and in one of the two SharedSurface_Basic constructors. In call cases it gets set to AttachmentType::GLTexture. Here are the three places:

SharedSurface_EGLImage::SharedSurface_EGLImage(GLContext* gl,
                                               const gfx::IntSize& size,
                                               const GLFormats& formats,
                                               GLuint prodTex, EGLImage image)
    : SharedSurface(
          SharedSurfaceType::EGLImageShare, AttachmentType::GLTexture, gl, size,
          false)  // Can't recycle, as mSync changes never update TextureHost.
      ,
[...]

SharedSurface_Basic::SharedSurface_Basic(const SharedSurfaceDesc& desc,
                                         UniquePtr<MozFramebuffer>&& fb)
    : SharedSurface(desc, std::move(fb), AttachmentType::GLTexture),

[...]

SharedSurface_Basic::SharedSurface_Basic(GLContext* gl, const IntSize& size,
                                         GLuint tex,
                                         bool ownsTex)
    : SharedSurface(SharedSurfaceType::Basic, AttachmentType::GLTexture, gl,
                    size, true),

Hopefully that gives the right idea. Given this is the case, it should be safe to remove the enumeration entirely and assume that the value is AttachmentType::GLTexture.

That simplifies the code a bunch more: we're now up to 98 inserted lines and 444 removed lines. Finally the GLFormats structure that's defined in GLContextTypes.h no longer exists in the upstream ESR 91 code. If you look back at the GLContext::ChooseGLFormats() listed out above, you'll notice that the values get set to default values; and in fact they never get changed. So this structure also looks to be a good candidate for removal.

Thankfully removing it is also pretty clean and straightforward. That leaves things in a much better state. These changes add a total of 95 lines but remove a total of 472 lines. I've checked that both the browser and WebView are still working as expected after these changes, so that feels like a good result for today.

That still leaves behind a potentially huge patch though. After these changes the patch still removes 145 lines and adds 1702 lines of code. We want both of these numbers to be as small as possible and that compares to 156 removals and 2090 additions before making these improvements.

I think we can do better; there are still further candidates for simplification. For example, one big change I had to make was to pass around the EGLDisplay value, which adds a new parameter to several methods. If I can remove this requirement, that'll take us closer again to the upstream ESR 91 code. But I've reached my limit for today, so this will have to wait until tomorrow.

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