flypig.co.uk

Yesterday I promised to do two things. First to analyse the structure of the code around GLContextProviderEGL::CreateOffscreen() to find out why there were two calls to GetAppDisplay() on ESR 78 but only one on ESR 91. Second I said I was going to step through all the places where the display is used in ESR 91 to compare against what we saw with ESR 78.

For the first of these, we have three points of reference. Let's focus first on the ESR 78 code which in practice uses GetAppDisplay() in only one place:

#0  mozilla::gl::GetAppDisplay () at gfx/gl/GLContextProviderEGL.cpp:175
#1  0x0000007fb8e81a2c in mozilla::gl::GLContextProviderEGL::CreateOffscreen (
    size=..., minCaps=..., 
    flags=flags@entry=mozilla::gl::CreateContextFlags::REQUIRE_COMPAT_PROFILE, 
    out_failureId=out_failureId@entry=0x7fa516f378)
    at gfx/gl/GLContextProviderEGL.cpp:1404

The code this breakpoint is attached to looks like this:

  if (!GLLibraryEGL::EnsureInitialized(
          forceEnableHardware, out_failureId, GetAppDisplay())) {  // Needed 
    for IsANGLE().
    return nullptr;
  }

This is a really crucial call because it ends up resulting in a call to GLLibraryEGL::DoEnsureInitialized() which is responsible for setting up everything about the EGL library. Having set that up we then drop back to CreateOffscreen() where CreateHeadless() is called to set up all the textures and surfaces. Finally GLContext::InitOffscreen() is called which looks like this:

bool GLContext::InitOffscreen(const gfx::IntSize& size,
                              const SurfaceCaps& caps) {
  if (!CreateScreenBuffer(size, caps)) return false;

  if (!MakeCurrent()) {
    return false;
  }
  fBindFramebuffer(LOCAL_GL_FRAMEBUFFER, 0);
  fScissor(0, 0, size.width, size.height);
  fViewport(0, 0, size.width, size.height);

  mCaps = mScreen->mCaps;
  MOZ_ASSERT(!mCaps.any);

  return true;
}

With that done, all of the main initialisation steps for the display are complete.

Let's compare that to ESR 91. With the new code the call to GLLibraryEGL::EnsureInitialized() has been removed. Instead the code drops straight through to CreateHeadless(). The implementation of CreateHeadless() is a little different: instead of just calling CreateEGLPBufferOffscreenContext() it first makes a call to DefaultEglDisplay() which is why we get this first breakpoint hit:

#0  mozilla::gl::GetAppDisplay ()
    at gfx/gl/GLContextProviderEGL.cpp:161
#1  0x0000007ff112e910 in mozilla::gl::GLContextProviderEGL::CreateHeadless (
    desc=..., out_failureId=out_failureId@entry=0x7f1f5ac1c8)
    at gfx/gl/GLContextProviderEGL.cpp:1434
#2  0x0000007ff112f260 in mozilla::gl::GLContextProviderEGL::CreateOffscreen (
    size=..., minCaps=...,
    flags=flags@entry=mozilla::gl::CreateContextFlags::REQUIRE_COMPAT_PROFILE, 
    out_failureId=out_failureId@entry=0x7f1f5ac1c8)
    at gfx/gl/GLContextProviderEGL.cpp:1476

That call will try to get the default EGL library and on the first occasion, when the "get" fails, it will call GLLibraryEGL::Create(). This will then end up calling GLLibraryEGL::Init() which is the closest equivalent we have in ESR 91 to GLLibraryEGL::DoEnsureInitialized(). I'm not going to have time today, but I should try to set some time aside over the coming days to compare the two. They're both long and complex methods, which makes them good candidates for problematic divergence.

It's this rather peculiar restructuring of the code that has led to GetAppDisplay() being called twice on ESR 91. Because the call to GLLibraryEGL::Create() also requires a display, but the display isn't passed in to DefaultEglLibrary(). Consequently another call to GetAppDisplay() is being made inside DefaultEglLibrary() as you can see from the second breakpoint hit:

#0  mozilla::gl::GetAppDisplay ()
    at gfx/gl/GLContextProviderEGL.cpp:161
#1  0x0000007ff111907c in mozilla::gl::DefaultEglLibrary (
    out_failureId=out_failureId@entry=0x7f1f5ac1c8)
    at gfx/gl/GLContextProviderEGL.cpp:1519
#2  0x0000007ff112e920 in mozilla::gl::DefaultEglDisplay (aDisplay=0x1, 
    out_failureId=0x7f1f5ac1c8)
    at gfx/gl/GLContextEGL.h:29
#3  mozilla::gl::GLContextProviderEGL::CreateHeadless (desc=..., 
    out_failureId=out_failureId@entry=0x7f1f5ac1c8)
    at gfx/gl/GLContextProviderEGL.cpp:1434
#4  0x0000007ff112f260 in mozilla::gl::GLContextProviderEGL::CreateOffscreen (
    size=..., minCaps=..., 
    flags=flags@entry=mozilla::gl::CreateContextFlags::REQUIRE_COMPAT_PROFILE, 
    out_failureId=out_failureId@entry=0x7f1f5ac1c8)
    at gfx/gl/GLContextProviderEGL.cpp:1476

This second call turns out to be unnecessary: it'd be better to add an aDisplay parameter to DefaultEglLibrary(), making things line up better with the code in ESR 78. Having said that, a quick search for DefaultEglLibrary() shows it's also used in one other place, so if I change its signature I'll have to update this other call as well:

already_AddRefed<GLContext> GLContextEGLFactory::CreateImpl(
    EGLNativeWindowType aWindow, bool aHardwareWebRender, bool aUseGles) {
  nsCString failureId;
  const auto lib = gl::DefaultEglLibrary(&failureId);
  if (!lib) {
    gfxCriticalNote << &quot;Failed[3] to load EGL library: &quot; << 
    failureId.get();
    return nullptr;
  }
  const auto egl = lib->CreateDisplay(true, &failureId, GetAppDisplay());
[...]

Since there's already a call to GetAppDisplay() it wouldn't be the end of the world to just move it up a little inside the method and store the result for use in both places. So this is also on my task list for tomorrow.

All of the differences in flow here between ESR 78 and ESR 91 make things really messy. But I don't think I should revert all of the changes in ESR 91 just yet, I'll need to take things in stages. First I'll compare the EGL library creation, then I'll remove this additional call to GetAppDisplay() and then, if none of these have fixed things, I'll make the larger changes to align the two flows more closely.

It feels like this has been a helpful analysis, even if I've not written any code or made any changes today. But I have made a plan and some immediate tasks for me to perform tomorrow. So definitely not wasted effort.

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