flypig.co.uk

Yesterday I restored (manually, because the underlying code has changed so much) patch 0038 back into the codebase. But it doesn't yet quite compile. There are a few sticking point errors that need to be resolved. The first I'm working on looks like this:

In file included from Unified_cpp_gfx_gl0.cpp:47:
${PROJECT}/gecko-dev/gfx/gl/GLContextProviderEGL.cpp: In function ‘void* 
    mozilla::gl::CreateEmulatorBufferSurface(mozilla::gl::GLLibraryEGL*, 
    EGLConfig, mozilla::gfx::IntSize&)’:
${PROJECT}/gecko-dev/gfx/gl/GLContextProviderEGL.cpp:991:55: error: ‘class 
    mozilla::gl::GLLibraryEGL’ has no member named ‘Display’; did you mean 
    ‘fGetDisplay’?
   EGLSurface surface = egl->fCreateWindowSurface(egl->Display(), config, 
    eglwindow, 0);                                           ^~~~~~~
                                                             fGetDisplay

This is code I copied over from ESR 78. The problem here is that egl->Display() no longer exists in ESR 91, for reasons we've already discussed, but most notably because there is no longer one single display in operation: there could be multiple. In ESR 78 the EGLLibrary::Display() method looked like this:

  EGLDisplay Display() const {
    MOZ_ASSERT(mInitialized);
    return mEGLDisplay;
  }

As you can see it just returns an mEGLDisplay value — the single canonical display for the application — every time. On ESR 91 this method has been removed entirely. It took me a while to decide which alternative method was the best match, but eventually I settled on this one:

  std::shared_ptr<EglDisplay> DefaultDisplay(nsACString* const out_failureId, 
    EGLDisplay aDisplay);

The difficulty, as you can see, is that while the former required no parameters, the latter has acquired two new parameters. I have to be honest that I'm a bit uncomfortable with a method that takes in an EGLDisplay in order to return an EglDisplay: it should really be the other way around. But I'm going with the flow on this.

The choice of parameters turns out to be relatively straightforward. The out_failureId error string that's actually an output rather than an input is just a pointer which gets pointed at a string literal if something goes wrong. But I'm ignoring errors anyway at this point, so I can just pass in a dummy variable. The aDisplay input value should be set to the result of GetAppDisplay() since this is the whole point of the changes that were introduced in ESR 78 to support the emulator and native devices.

After these changes the code now looks like this:

  EGLSurface surface = egl->fCreateWindowSurface(egl->DefaultDisplay(
    out_failureId, GetAppDisplay())->mDisplay, config, eglwindow, 0);

This line now compiles, so time to move on to the second error, which looks like this:

In file included from Unified_cpp_gfx_gl0.cpp:2:
${PROJECT}/gecko-dev/gfx/gl/AndroidSurfaceTexture.cpp: In static member 
    function ‘static already_AddRefed<mozilla::gl::GLContextEGL> mozilla::gl::
    AndroidSharedBlitGL::CreateContextImpl(bool)’:
${PROJECT}/gecko-dev/gfx/gl/AndroidSurfaceTexture.cpp:78:52: error: too few 
    arguments to function  std::shared_ptr<mozilla::gl::EglDisplay> mozilla::gl:
    :DefaultEglDisplay(nsACString*, EGLDisplay)’
     const auto egl = gl::DefaultEglDisplay(&ignored);
                                                    ^
In file included from ${PROJECT}/gecko-dev/gfx/gl/AndroidSurfaceTexture.cpp:9,
                 from Unified_cpp_gfx_gl0.cpp:2:
${PROJECT}/gecko-dev/gfx/gl/GLContextEGL.h:27:36: note: declared here
 inline std::shared_ptr<EglDisplay> DefaultEglDisplay(
                                    ^~~~~~~~~~~~~~~~~

There's a similar thing going on here, in that there should be an input parameter. But this is a little different because of where the code lives. It's being compiled in as part of the AndroidSurfaceTexture.cpp file. I'm wondering why this file is getting included at all, given the name seems to suggest it's rather Android focused. Nevertheless I've fixed this particular error by passing in EGL_DISPLAY_NONE as the parameter. To be honest I'm hoping this code never actually gets called so that the input value is somewhat irrelevant. Based on this assumption, this change should do the trick.

The third error is more unexpected and looks like this:

In file included from Unified_cpp_gfx_gl0.cpp:47:
${PROJECT}/gecko-dev/gfx/gl/GLContextProviderEGL.cpp: In function ‘void* 
    mozilla::gl::CreateEmulatorBufferSurface(mozilla::gl::GLLibraryEGL*, 
    EGLConfig, mozilla::gfx::IntSize&)’:
${PROJECT}/gecko-dev/gfx/gl/GLContextProviderEGL.cpp:996:36: error: 
    ‘nsTHashMap<nsPtrHashKey<void>, mozilla::gl::WaylandGLSurface*>’ {aka 
    ‘class nsBaseHashtable<nsPtrHashKey<void>, mozilla::gl::WaylandGLSurface*, 
    mozilla::gl::WaylandGLSurface*, nsDefaultConverter<mozilla::gl::
    WaylandGLSurface*, mozilla::gl::WaylandGLSurface*> >’} has no member named 
    ‘LookupForAdd’; did you mean ‘LookupResult’?
     auto entry = sWaylandGLSurface.LookupForAdd(surface);
                                    ^~~~~~~~~~~~
                                    LookupResult

On the face of it it's unclear to me what's going wrong and how this should be fixed, but a dig through the git history shows that this error has been caused by change D104216 which relates to Bugzilla bug 1688833. The bug description has this to say:
 
Okay, I'll be honest, I'm not really sure what's going on here. More importantly the diff has examples of how this has been changed in other places and which match pretty closely to the situation I'm trying to solve. So I've copied those snippets of code and refactored them for the ESR 91 code. The result is that I've changed the following piece of code:

  if (surface) {
    WaylandGLSurface* waylandData = new WaylandGLSurface(wlsurface, eglwindow);
    auto entry = sWaylandGLSurface.LookupForAdd(surface);
    entry.OrInsert([&waylandData]() { return waylandData; });
  }

So that it now looks like this:

  if (surface) {
    WaylandGLSurface* waylandData = new WaylandGLSurface(wlsurface, eglwindow);
    sWaylandGLSurface.WithEntryHandle(surface, [&](auto&& entry) {
      entry.OrInsert(waylandData);
    })  
  }

Does that look sensible? Well, it compiles and it's consistent with how the change was made in other parts of the code; and right now that's good enough for me.

Having made these changes I'm pleased to discover that the code now compiles fully. I'm able to kick off the partial build and watch it generate the libxul.so file that we need. Having copied over and installed that file, when running the app I discover that... nothing overt has changed.

That's too bad. But what would be interesting to see now is what the actual values for the display are. I'll look in to that tomorrow.

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