flypig.co.uk

This morning I wake up to find the build has failed while attempting to compile dom/canvas/WebGLContext.cpp. This follows from all of the changes I've been making to try to bring back GLScreenBuffer. Yesterday I used partial builds, performed inside the scratchbox build target, to test my changes. Now that I've tried to build the whole of gecko it's failed with the following error:

66:19.17 ${PROJECT}/obj-build-mer-qt-xr/dist/include/mozilla/UniquePtr.h:
    In instantiation of ‘typename mozilla::detail::UniqueSelector<T>::
    SingleObject mozilla::MakeUnique(Args&& ...) [with T = mozilla::gl::
    SurfaceFactory_Basic; Args = {mozilla::gl::GLContext&};
    typename mozilla::detail::UniqueSelector<T>::SingleObject = mozilla::
    UniquePtr<mozilla::gl::SurfaceFactory_Basic, mozilla::DefaultDelete
    <mozilla::gl::SurfaceFactory_Basic> >]’:
66:19.17 ${PROJECT}/gecko-dev/dom/canvas/WebGLContext.cpp:929:67:
    required from here
66:19.17 ${PROJECT}/obj-build-mer-qt-xr/dist/include/mozilla/UniquePtr.h:609:23:
    error: no matching function for call to ‘mozilla::gl::SurfaceFactory_Basic::
    SurfaceFactory_Basic(mozilla::gl::GLContext&)’
66:19.17    return UniquePtr<T>(new T(std::forward<Args>(aArgs)...));
66:19.17                        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
66:19.17 In file included from ${PROJECT}/gecko-dev/dom/canvas/
    WebGLContext.cpp:52,
66:19.17                  from Unified_cpp_dom_canvas1.cpp:119:
66:19.18 ${PROJECT}/gecko-dev/gfx/gl/SharedSurfaceGL.h:31:12: note: candidate: ‘mozilla::gl::SurfaceFactory_Basic::SurfaceFactory_Basic(mozilla::gl::
    GLContext*, const mozilla::gl::SurfaceCaps&, const
    mozilla::layers::TextureFlags&)’
66:19.18    explicit SurfaceFactory_Basic(GLContext* gl,
66:19.18             ^~~~~~~~~~~~~~~~~~~~
66:19.18 ${PROJECT}/gecko-dev/gfx/gl/SharedSurfaceGL.h:31:12: note:
    candidate expects 3 arguments, 1 provided

This can happen and, in fact, I was almost expecting it. A partial build will only builds the files inside a particular subdirectory and its children (depending on how the build scripts are structured). So if there's some other bit of code in a different branch of the directory hierarchy that uses something in this subdirectory, then it won't be compiled against the changes. If the interface changes and the a consumer of that interface is neither updated nor compiled against it, then we end up where we are now.

It's not an ideal sign though as it suggests the changes I've made aren't self-contained. Ideally they would be. But let's run with it and focus on fixing the issue.

The good news is that I can trigger the same error back inside the scratchbox target by building the directory containing the file that's causing the error:

$ make -j1 -C obj-build-mer-qt-xr/dom/canvas
[...]
${PROJECT}/gecko-dev/dom/canvas/WebGLContext.cpp:929:67:   required from here
${PROJECT}/obj-build-mer-qt-xr/dist/include/mozilla/UniquePtr.h:609:23: error:
    no matching function
[...]

This makes it much easier to test my fixes. So with all this in place I can get down to fixing the error. Here's the code causing the problem:

  if (!swapChain->mFactory) {
    NS_WARNING("Failed to make an ideal SurfaceFactory.");
    swapChain->mFactory = MakeUnique<gl::SurfaceFactory_Basic>(*gl);
  }

These lines and all the code around it have changed quite considerably since ESR 78, but I'm still able to find a portion of code in the ESR 78 codebase that looks to be doing something similar:

  if (!factory) {
    // Absolutely must have a factory here, so create a basic one
    factory = MakeUnique<gl::SurfaceFactory_Basic>(gl, gl->Caps(), flags);
    mBackend = layers::LayersBackend::LAYERS_BASIC;
  }

The error is caused by the fact I restored the previously removed SurfaceCaps and TextureFlags parameters to the SurfaceFactory_Basic() method. All of these end up being fed into the SurfaceFactory constructor.

Performing a diff on the change we've made to SurfaceFactory_Basic() we can see how things are and how they were for comparison:

$ git diff gfx/gl/SharedSurfaceGL.cpp
[...]
-SurfaceFactory_Basic::SurfaceFactory_Basic(GLContext& gl)
-    : SurfaceFactory({&gl, SharedSurfaceType::Basic,
-                      layers::TextureType::Unknown, true}) {}
+SurfaceFactory_Basic::SurfaceFactory_Basic(GLContext* gl,
+                                           const SurfaceCaps& caps,
+                                           const layers::TextureFlags& flags)
+    : SurfaceFactory({gl, SharedSurfaceType::Basic,
+                  layers::TextureType::Unknown, true}, caps, nullptr, flags) {}

This doesn't really help us though: the caps and flags, which are the ones causing the problem, get passed straight through. The important change is further down. The reason I'm chasing these is because I need to find sensible default values to set them to. There may not be sensible defaults, but if there are then it will allow us to create a new method that doesn't require these additional parameters, which is what we need for the code to compile.

So, we need to look inside SurfaceFactory.cpp and specifically at the SurfaceFactory constructor. I fear this is going to get messy.

$ git diff gfx/gl/SharedSurface.cpp
[...]
-SurfaceFactory::SurfaceFactory(const PartialSharedSurfaceDesc& partialDesc)
-    : mDesc(partialDesc), mMutex("SurfaceFactor::mMutex") {}
[...]
+SurfaceFactory::SurfaceFactory(const PartialSharedSurfaceDesc& partialDesc,
+                             const SurfaceCaps& caps,
+                             const RefPtr<layers::LayersIPCChannel>& allocator,
+                             const layers::TextureFlags& flags)
+    : mDesc(partialDesc),
+      mCaps(caps),
+      mAllocator(allocator),
+      mFlags(flags),
+      mFormats(partialDesc.gl->ChooseGLFormats(caps)),
+      mMutex("SurfaceFactor::mMutex")
+{
+  ChooseBufferBits(caps, &mDrawCaps, &mReadCaps);
+}

The constructor itself is just storing the values and calling ChooseBufferBits() with some of them in order to set the mDrawCaps and mReadCaps member variables. It's unlikely that mDrawCaps or mReadCaps are being used by the WebGLContext code because I added them in as part of these changes (it's possible they're used by a method that's called in WebGLContext, but I don't believe that's the case).

The mCaps, mAllocator and mFlags members only seem to get used directly in SurfaceFactory::NewTexClient(). They're public, so that doesn't mean they don't get used elsewhere, but I suspect the only other places they're used are in new code I've added for GLScreenBuffer. This won't be used by WebGLContext.

Similarly the NewTexClient() method is only called within GLScreenBuffer::Swap() and GLScreenBuffer::Resize(). The WebGLContext code doesn't have to worry or care about these as the WebGLContext code is entirely orthogonal.

In conclusion, it should be safe to set all the new parameters to some default or null values. So I've added in a new version of the SurfaceFactory_Basic constructor like this:

SurfaceFactory_Basic::SurfaceFactory_Basic(GLContext& gl)
    : SurfaceFactory({&gl, SharedSurfaceType::Basic,
            layers::TextureType::Unknown, true}, SurfaceCaps(), nullptr, 0) {}

I've run partial builds on the original source directory that contains the changes we've been looking at, as well as the directory containing the code that failed during the full build, like this:

$ make -j1 -C obj-build-mer-qt-xr/gfx/gl/
[...]
$ make -j1 -C obj-build-mer-qt-xr/dom/canvas
[...]

Both complete successfully without errors. Hooray!

Okay, it's now time to set off a full build again. It's quite possible something else will fail, but running the build is the simplest way to find out. This will, of course, take a while. If it's done by the end of the day there may be time to test it this evening; let's see.

In the meantime, it's worth noting that ultimately the mCaps, mAllocator and mFlags parameters we've been discussing only seem to get used by the two different versions of GLScreenBuffer::CreateFactory(). However I can't find any code that actually calls either of these. So it's quite possible that eventually all of the code related to these parameters will be found to be redundant and can be removed. But we'll come back to that later.

[...]

Sadly this wasn't the last of the errors; the build failed again. The error is rather interesting:

193:24.93 In file included from Unified_cpp_gfx_layers6.cpp:128:
193:24.93 ${PROJECT}/gecko-dev/gfx/layers/client/TextureClientSharedSurface.cpp:
    In static member function ‘static already_AddRefed<mozilla::layers::
    SharedSurfaceTextureClient> mozilla::layers::SharedSurfaceTextureClient::
    Create(mozilla::UniquePtr<mozilla::gl::SharedSurface>,
    mozilla::gl::SurfaceFactory*, mozilla::layers::LayersIPCChannel*,
    mozilla::layers::TextureFlags)’:
193:24.94 ${PROJECT}/gecko-dev/gfx/layers/client/
    TextureClientSharedSurface.cpp:102:63: error:
    ‘class mozilla::gl::SharedSurface’ has no member named ‘GetTextureFlags’
193:24.94    TextureFlags flags = aFlags | TextureFlags::RECYCLE | surf->GetTextureFlags();
193:24.94                                                                ^~~~~~~~~~~~~~~
193:24.94 ${PROJECT}/gecko-dev/gfx/layers/client/
    TextureClientSharedSurface.cpp:104:51: error: no matching function for call
    to ‘mozilla::layers::SharedSurfaceTextureData::SharedSurfaceTextureData(
    std::remove_reference<mozilla::UniquePtr<mozilla::gl::SharedSurface>&>::
    type)’
193:24.94        new SharedSurfaceTextureData(std::move(surf));
193:24.94                                                    ^
193:24.94 ${PROJECT}/gecko-dev/gfx/layers/client/
    TextureClientSharedSurface.cpp:34:1: note: candidate:
    ‘mozilla::layers::SharedSurfaceTextureData::SharedSurfaceTextureData(
    const mozilla::layers::SurfaceDescriptor&, mozilla::gfx::SurfaceFormat,
    mozilla::gfx::IntSize)’
193:24.94  SharedSurfaceTextureData::SharedSurfaceTextureData(
193:24.94  ^~~~~~~~~~~~~~~~~~~~~~~~
193:24.94 ${PROJECT}/gecko-dev/gfx/layers/client/
    TextureClientSharedSurface.cpp:34:1: note:   candidate expects 3 arguments,
    1 provided

Interesting because this is in part of the code that I made changes to in order to get the partial build to complete, but it turns out I wasn't actually building this code at all, it was just using the header.

So I'll need to add in the GetTextureFlags() to SharedSurface.h. It's a simple method because all of the functionality is supposed to come from it being overridden. So I've added this in to SharedSurface.h:

  // Specifies to the TextureClient any flags which
  // are required by the SharedSurface backend.
  virtual layers::TextureFlags GetTextureFlags() const;

In addition to this I've inserted a simple implementation for it to SharedSurface.cpp:

layers::TextureFlags SharedSurface::GetTextureFlags() const {
  return layers::TextureFlags::NO_FLAGS;
}

But to actually match the functionality of ESR 78 I've also added this override, directly into SharedSurfaceEGL.h:

  virtual layers::TextureFlags GetTextureFlags() const override {
    return layers::TextureFlags::DEALLOCATE_CLIENT;
  }

They're all super-simple methods and with any luck shouldn't trigger any additional errors. I now have to do a partial build of the now three the directories that I've touched with changes:

$ make -j1 -C obj-build-mer-qt-xr/gfx/gl/
[...]
$ make -j1 -C obj-build-mer-qt-xr/dom/canvas
[...]
$ make -j1 -C obj-build-mer-qt-xr/gfx/layers
[...]

Happily these all compile successfully and without errors. So it's time to kick off another full build again. I don't expect this to be done before I go to bed this evening, so I'll have to pick this up again tomorrow. In the morning we'll find out what new gruesome errors there are to deal with!

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