flypig.co.uk

I'm tidying things up. If you've been following previous posts you'll know there was some visual celebration provided by artist-in-residence thigg when the rendering started working, but now things need to be put in order and pushed to the repository. Given this I need to get the code into a fit state, which means removing debug code, refactoring the changes, making sure everything still works correctly.

In other words, it's time to clean things up. And thigg has created a rather apt image to illustrate this process as well. It's not all glamorous you know!  
My first clean-up task is a bit of an odd one. In some of the debug output I've pasted on previous days you'll have seen lines that look a bit like this:

frame014.data: Colour before: (8, 0, 152, 255), 1
frame015.data: Colour before: (1, 0, 255, 255), 1
frame016.data: Colour before: (223, 5, 0, 255), 1
frame017.data: Colour before: (71, 94, 162, 255), 1

You may recall that I added these in to test the colour at specific pixels of the texture used for offscreen rendering. I also introduced some code to export to disc images from the surface as textures. These produced some rubbish looking images which neither I nor the community ever successfully figured out (I now suspect they were just memory artefacts).

For example, I added a bunch of code into the GLScreenBuffer::Swap() method that looks like this:

    result = ReadPixels(0, 0, size.width, size.height, LOCAL_GL_RGB, 
    LOCAL_GL_UNSIGNED_BYTE, buf);

Right now this and all the related code can still be found in the source tree, not just in the ESR 91 code, but in the ESR 78 code as well. None of it is needed for a working browser and it's outlived its usefulness, so I'll need to remove it before pushing the latest changes to remote. All fine. But can I recall which specific parts are needed and which can be removed? No, no I can't.

I have a solution though. If I compare the changes with the original ESR 78 code it should become clear which bits I've added for debugging purposes and which are needed for offscreen rendering. But that means I first have to remove the debug code from ESR 78 so I have something to compare against.

Removing the code from ESR 78 is possible but it's also harder than it should be because it's now mixed in with all of the changes made by the patches. I didn't apply these to the ESR 78 as commits, but rather let the build processes apply them automatically, so they've been applied as a bunch of local changes, all mixed together.

To get things back to where I need them to be I need to reset the ESR 78 project folder to a clean state, then apply all the patches again. This will mess up the build state (meaning that in order to a build I'll have to build everything rather than just what's changed), but that's okay because in general I don't need to run ESR 78 builds very often. Let's hope it stays that way.

There are a few changes I made to the spec file that I want to keep, but everything else can be cleaned out.

$ cd gecko-dev-project/gecko-dev/gecko-dev
$ git reset --hard
$ git clean -xdf
$ cd ..
$ git stash
$ sfdk apply
$ git stash pop

The penultimate

apply

step applies the patches. After this I now have a nice clean ESR 78 source tree to compare my ESR 91 code against.

This has allowed me to remove a bunch of debug code from the GLContext.h file of ESR 91.

Now I'm wondering whether I can fold the GLScreenBuffer class into SwapChain. There's definitely some overlap, but also quite a bit of divergence too. Below is an annotated listing of the SwapChain header. Lines prefixed with a "+" symbol also appear in GLScreenBuffer. Those prefixed with a "-" appear only in SwapChain.

class SwapChain final {
  friend class SwapChainPresenter;

 public:
+  UniquePtr<SurfaceFactory> mFactory;
-  bool mPreserve = false;
-  std::shared_ptr<SharedSurface> mPrevFrontBuffer;

 private:
-  std::queue<std::shared_ptr<SharedSurface>> mPool;
-  std::shared_ptr<SharedSurface> mFrontBuffer;
-  SwapChainPresenter* mPresenter = nullptr;

 public:
+  SwapChain();
+  virtual ~SwapChain();

-  void ClearPool();
-  const auto& FrontBuffer() const { return mFrontBuffer; }
-  UniquePtr<SwapChainPresenter> Acquire(const gfx::IntSize&);

+  const gfx::IntSize& Size() const;
-  const gfx::IntSize& OffscreenSize() const;
+  bool Resize(const gfx::IntSize & size);
+  bool PublishFrame(const gfx::IntSize& size) { return Swap(size); }
+  void Morph(UniquePtr<SurfaceFactory> newFactory);

private:
+  bool Swap(const gfx::IntSize& size);
};

As well as these there are also a large number of methods in GLScreenBuffer which don't appear in SwapChain. These are less critical though, because as additions there won't be any attempt to make use of them in the existing code.

If I were to merge GLScreenBuffer into SwapChain I'd need to create an implementations for all of the lines indicated with a "-" in the class definition above. They can be dummy implementations (or make use of the existing implementations from the SwapChain class), so this process doesn't have to be complicated, but it would need to be done.

The fact that SwapChain is marked as final means that I'm not able to inherit from it. I could remove the final specifier of course, but that may not be a great idea. Some other parts of the code might be making assumptions based on it.

I think I'm going to have a go at performing this merge, but as it happens I'm not able to make these changes straight away anyway. That's because I need to run a full rebuild. I tried running a partial build and its come back complaining that the build config has changed:

$ sfdk engine exec
$ sb2 -t SailfishOS-esr78-aarch64.default
$ source `pwd`/obj-build-mer-qt-xr/rpm-shared.env
$ make -j1 -C obj-build-mer-qt-xr/gfx/gl/
make: Entering directory 'obj-build-mer-qt-xr/gfx/gl'
config/rules.mk:338: *** Build configuration changed. Build with |mach build| 
    or run |mach build-backend| to regenerate build config.  Stop.
make: Leaving directory 'obj-build-mer-qt-xr/gfx/gl'

For the next few days I'm planning to make small changes to the code, followed by rebuilds and tests to check things are working (a tight edit-rebuild-test cycle). To make the most of this I need the partial build working, which means running a full build now.

Well, by running the full build and watching it fail I have at least determined that the following changes are necessary:

diff --git a/gfx/gl/GLContext.h b/gfx/gl/GLContext.h
index 863414aaeb44..0991afbf045c 100644
--- a/gfx/gl/GLContext.h
+++ b/gfx/gl/GLContext.h
@@ -37,3 +38,2 @@
 #include "GLDefs.h"
-#include "GLScreenBuffer.h"
 #include "GLTypes.h"
@@ -42,2 +42,3 @@
 #include "mozilla/WeakPtr.h"
+#include "SurfaceTypes.h"
 
@@ -61,2 +62,5 @@ class GLReadTexImageHelper;
 struct SymLoadStruct;
+class SwapChain;
+class SurfaceCaps;
+class GLScreenBuffer;
 }  // namespace gl

Having restored them, the build is off again. Now it's back to the full build again; until this is done, there's not much point making any more changes to the code today.

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