1068190 - Add Layers Compositing GTest

Status: RESOLVED WONTFIX

(Core :: Graphics: Layers, defect)

Description

[Benoit Girard (:BenWa)]

Right now we can test compositing indirectly via reftest. This is not ideal because it requires us craft specific HTML document to get FrameLayerBuilder to construct a layer tree of the exact form that we need. If/When we make such a test case it means that changes to FrameLayerBuilder can break that test case and leave us with a test that isn't doing what it was designed to do.

I'm suggesting that we start off by supporting the basic compositor from GTest. We can draw to an offscreen surface and provide our own layer tree.

This will also make it possible to write more complicated tests. Well be able to reproduce a layers bug, get a layers dump and drop that into a GTest. Later we can extend this setup to test more sophistical async transformation to the layers. Something that is just too complex to do via reftest.

Comment 1

[Benoit Girard (:BenWa)]

Attachment: Poking with the setup

The setup looks to be very simple.

Comment 2

[Benoit Girard (:BenWa)]

Attachment: patch v1

Comment 3

[Benoit Girard (:BenWa)]

Comment on attachment 8527986 [details] [diff] [review]
patch v1

Review of attachment 8527986 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/moz.build
@@ +3,5 @@
>  # This Source Code Form is subject to the terms of the Mozilla Public
>  # License, v. 2.0. If a copy of the MPL was not distributed with this
>  # file, You can obtain one at http://mozilla.org/MPL/2.0/.
>  
> +CXXFLAGS += ['-O0', '-g']

Opps, need to remove this

Comment 4

[Benoit Girard (:BenWa)]

https://hg.mozilla.org/try/rev/a29454883517

Comment 5

[Markus Stange [:mstange]]

Comment on attachment 8527986 [details] [diff] [review]
patch v1

Review of attachment 8527986 [details] [diff] [review]:
-----------------------------------------------------------------

I like it. I'd like to see the mock widget idea at least attempted before we add all the null checks, though.

::: gfx/layers/basic/BasicCompositor.cpp
@@ +86,5 @@
>  }
>  
>  void BasicCompositor::Destroy()
>  {
> +  if (mWidget) {

All these mWidget null checks are a bit unfortunate. It would be nicer if we had a mock widget. Want to add one? nsBaseWidget should already have most of the functionality we need, I don't think there'd be too much left to implement.

::: gfx/layers/composite/LayerManagerComposite.cpp
@@ +334,5 @@
>  
>  already_AddRefed<PaintedLayer>
>  LayerManagerComposite::CreatePaintedLayer()
>  {
> +  MOZ_ASSERT(gIsGtest, "Should only be called on the drawing side");

These messages need to be changed if you don't want to confuse the reader of this code. Something like "Unless you're testing the compositor using GTests, this should only..."

::: gfx/layers/opengl/CompositorOGL.cpp
@@ +121,5 @@
>    }
>  
> +  if (!context) {
> +    if (mWidget) {
> +      context = gl::GLContextProvider::CreateForWindow(mWidget);

Hmm, so this might be a problem if mWidget is just an empty mock implementation. Might need some changes to GLContextProvider.

::: gfx/tests/gtest/TestCompositor.cpp
@@ +19,5 @@
> +
> +using namespace mozilla::gfx;
> +
> +/**
> + * Get a list of layers manager for the platform to run the test on.

layer managers

@@ +25,5 @@
> +static std::vector<RefPtr<LayerManagerComposite>> GetLayerManagers(vector<LayersBackend> aBackends) {
> +  vector<RefPtr<LayerManagerComposite>> managers;
> +
> +  for (size_t i = 0; i < aBackends.size(); i++) {
> +    auto aBackend = aBackends[i];

s/aBackend/backend for sanity

@@ +40,5 @@
> +    if (gfxPlatformGtk::GetPlatform()->UseXRender()) {
> +      //compositor = new X11BasicCompositor(mWidget);
> +      EXPECT_EQ
> +      abort(); // No yet support
> +    } else

Not a huge issue, but pulling out parts of this code into a CreateCompositorForBackend function would let you use early returns and remove the need for mixing C++ and preprocessor control flow syntax.

@@ +48,5 @@
> +    }
> +#ifdef XP_WIN
> +    } else if (aBackend == LayersBackend::LAYERS_D3D11) {
> +      //compositor = new CompositorD3D11();
> +      abort(); // No yet support

No support yet

@@ +56,5 @@
> +#endif
> +    }
> +
> +    if (!compositor) {
> +      printf_stderr("Failed to construct layer manager for the request backend\n");

requested

@@ +75,5 @@
> +/**
> + * This will return the default list of backends that
> + * units test should run against.
> + */
> +static vector<LayersBackend> GetPlatformBackends() {

Is this bracing style consistent with other GTests? (I don't know.) Because it's not consistent with Gecko...

@@ +145,5 @@
> +
> +TEST(Gfx, CompositorSimpleTree) {
> +  auto layerManagers = GetLayerManagers(GetPlatformBackends());
> +  for (size_t i = 0; i < layerManagers.size(); i++) {
> +    nsRefPtr<LayerManagerComposite> layerManager = layerManagers[i].get();

Is the .get() necessary?

@@ +146,5 @@
> +TEST(Gfx, CompositorSimpleTree) {
> +  auto layerManagers = GetLayerManagers(GetPlatformBackends());
> +  for (size_t i = 0; i < layerManagers.size(); i++) {
> +    nsRefPtr<LayerManagerComposite> layerManager = layerManagers[i].get();
> +    nsRefPtr<LayerManager> lmBase = layerManager.get();

same here

@@ +147,5 @@
> +  auto layerManagers = GetLayerManagers(GetPlatformBackends());
> +  for (size_t i = 0; i < layerManagers.size(); i++) {
> +    nsRefPtr<LayerManagerComposite> layerManager = layerManagers[i].get();
> +    nsRefPtr<LayerManager> lmBase = layerManager.get();
> +    nsTArray<nsRefPtr<Layer> > layers;

>>

@@ +157,5 @@
> +    };
> +    CreateLayerTree("c(ooo)", layerVisibleRegion, nullptr, lmBase, layers);
> +
> +    { // background
> +    ColorLayer* colorLayer = layers[1]->AsColorLayer();

strange indentation

@@ +175,5 @@
> +    colorLayer->SetBounds(colorLayer->GetVisibleRegion().GetBounds());
> +    }
> +
> +    RefPtr<DrawTarget> refDT = CreateDT();
> +    refDT->FillRect(Rect(0, 0, gCompWidth, gCompHeight), ColorPattern(Color(1.f, 0.f, 1.f, 1.0f)));

Getting even more pedantic: 1.f or 1.0f, choose one.

::: gfx/tests/gtest/TestLayers.cpp
@@ +68,5 @@
> +  virtual already_AddRefed<PaintedLayer> CreatePaintedLayer() {
> +    nsRefPtr<PaintedLayer> layer = new TestPaintedLayer(this);
> +    return layer.forget();
> +  }
> +  virtual already_AddRefed<ColorLayer> CreateColorLayer() { return nullptr; }

Don't you need to implement this for the CompositorSimpleTree test?

@@ +223,5 @@
>      }
>    }
>    if (rootLayer) {
>      rootLayer->ComputeEffectiveTransforms(Matrix4x4());
> +    manager->SetRoot(rootLayer);

I wonder why this wasn't needed before.

Comment 6

[Benoit Girard (:BenWa)]

.get() is needed for RefPtr -> nsRefPtr

Comment 7

[Benoit Girard (:BenWa)]

Attachment: patch v2 (missing mock widget)

Try closed.

Comment 8

[Benoit Girard (:BenWa)]

https://tbpl.mozilla.org/?tree=Try&rev=fb548ebb53bc

Comment 9

[Benoit Girard (:BenWa)]

https://tbpl.mozilla.org/?tree=Try&rev=9d6b7500efa0

Comment 10

[Benoit Girard (:BenWa)]

Fix debug assertions and non deterministic gfxPrefs ordering.
https://tbpl.mozilla.org/?tree=Try&rev=6b0f1281f597

Comment 11

[Benoit Girard (:BenWa)]

Attachment: patch v3 (now with 100% more mockwidget)

Comment 12

[Benoit Girard (:BenWa)]

https://tbpl.mozilla.org/?tree=Try&rev=3bc9bf47a672

Comment 13

[Markus Stange [:mstange]]

Comment on attachment 8530462 [details] [diff] [review]
patch v3 (now with 100% more mockwidget)

Review of attachment 8530462 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/gl/GLContextProviderCGL.mm
@@ +196,5 @@
>  
>  already_AddRefed<GLContext>
>  GLContextProviderCGL::CreateForWindow(nsIWidget *aWidget)
>  {
> +    // Used by mock widget to create an offscreen context

Might want to pull this out to the cross-platform place that calls GLContextProviderCGL::CreateForWindow to make it work on more platforms.

::: gfx/tests/gtest/TestCompositor.cpp
@@ +44,5 @@
> +      caps.preserve = false;
> +      caps.bpp16 = false;
> +      nsRefPtr<GLContext> context = GLContextProvider::CreateOffscreen(
> +        gfxIntSize(gCompWidth, gCompHeight), caps);
> +      return context.forget().take();

I hope you've tested this, because I can't tell you whether this is correct.

@@ +79,5 @@
> +  NS_IMETHOD_(InputContext) GetInputContext() { abort(); }
> +  NS_IMETHOD              ReparentNativeWidget(nsIWidget* aNewParent) { return NS_OK; }
> +private:
> +  ~MockWidget() {}
> +};

Interesting. A bit longer than I expected it to be, but still manageable, I think.

@@ +90,5 @@
> +
> +  RefPtr<Compositor> compositor;
> +
> +  if (backend == LayersBackend::LAYERS_OPENGL) {
> +    compositor = new CompositorOGL(new MockWidget(), // LEAK FIX ME

yes please

@@ +100,5 @@
> +    compositor = new BasicCompositor(new MockWidget()); // LEAK FIX ME
> +#ifdef XP_WIN
> +  } else if (backend == LayersBackend::LAYERS_D3D11) {
> +    //compositor = new CompositorD3D11();
> +    abort(); // No support yet

Not sure how common abort() usage is around these places. I thought MOZ_CRASH was the preferred way, but I could be wrong.

Comment 14

[Benoit Girard (:BenWa)]

Attachment: Part 2: Add container layer tests

I wrote a container layer test on the plane

Comment 15

[Markus Stange [:mstange]]

Comment on attachment 8531300 [details] [diff] [review]
Part 2: Add container layer tests

Review of attachment 8531300 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/composite/LayerManagerComposite.cpp
@@ +193,5 @@
>    mCompositor->SetTargetContext(aTarget, aRect);
>    mTarget = aTarget;
>    mTargetBounds = aRect;
> +
> +  mClonedLayerTreeProperties = LayerProperties::CloneFrom(GetRoot());

Is this fixing a real bug?

::: gfx/tests/gtest/TestCompositor.cpp
@@ +167,5 @@
>  
>    return dt;
>  }
>  
> +static RefPtr<DrawTarget> sTransactionDrawTarget = nullptr;

I think this causes a static initializer which we want to avoid IIRC. Can you use a raw pointer and manual refcounting instead?

@@ +194,5 @@
>        for (size_t channel = 0; channel < 4; channel++) {
>          uint8_t bit = bitmap[y * dss->Stride() + x * 4 + channel];
>          uint8_t bitRef = bitmapRef[y * dss->Stride() + x * 4 + channel];
> +        int16_t delta = (uint16_t)bitRef - (uint16_t)bit;
> +        if (delta >= 2 || delta <= -2) {

The fuzz threshold should probably be passed as an argument to EndTransactionAndCompare.

@@ +195,5 @@
>          uint8_t bit = bitmap[y * dss->Stride() + x * 4 + channel];
>          uint8_t bitRef = bitmapRef[y * dss->Stride() + x * 4 + channel];
> +        int16_t delta = (uint16_t)bitRef - (uint16_t)bit;
> +        if (delta >= 2 || delta <= -2) {
> +          if (equal) {

Don't you also want to dump these things for a non-equal test that doesn't contain different pixels?

@@ +300,5 @@
> +      nsIntRegion(nsIntRect(50, 50, 100, 100)),
> +    };
> +    nsRefPtr<Layer> root = CreateLayerTree("c(oc(oo))", layerVisibleRegion, nullptr, lmBase, layers);
> +
> +    // Note: We want to pick an opacity that will round the same way on all backends

Since you're adding fuzz, does that mean that using 0.4 doesn't actually work around the whole problem?

@@ +355,5 @@
> +      colorLayer->SetColor(gfxRGBA(0.1f, 0.2f, 0.3f, 1.f));
> +      colorLayer->SetBounds(colorLayer->GetVisibleRegion().GetBounds());
> +    }
> +    // False since we haven't updated our refDT yet
> +    EXPECT_FALSE(EndTransactionAndCompareNE(layerManager, refDT));

The NE already says "not equal", I think it's confusing to use EXPECT_FALSE instead of EXPECT_TRUE here. Can we flip the return value of EndTransactionAndCompare in the non-equal case?

Comment 16

[Benoit Girard (:BenWa)]

Attachment: patch v4 - fix leaks

Comment 17

[Benoit Girard (:BenWa)]

Attachment: Part 2: Add container layer tests

trivial fixes

Comment 18

[Markus Stange [:mstange]]

Comment on attachment 8533951 [details] [diff] [review]
patch v4 - fix leaks

Review of attachment 8533951 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/tests/gtest/TestCompositor.cpp
@@ +222,5 @@
> +TEST(Gfx, CompositorConstruct)
> +{
> +  auto layerManagers = GetLayerManagers(GetPlatformBackends());
> +  for (size_t i = 0; i < layerManagers.size(); i++) {
> +    RefPtr<LayerManagerComposite> layerManager = layerManagers[i].mLayerManager;

Does this do anything? If you want to test just construction, do you need the loop?

@@ +231,5 @@
> +{
> +  auto layerManagers = GetLayerManagers(GetPlatformBackends());
> +  for (size_t i = 0; i < layerManagers.size(); i++) {
> +    nsRefPtr<LayerManagerComposite> layerManager = layerManagers[i].mLayerManager.get();
> +    nsRefPtr<LayerManager> lmBase = layerManager.get();

I still think it would be preferable to have layer managers in nsRefPtrs all the time, so you don't need this RefPtr -> nsRefPtr conversion .get().

Comment 19

[Benoit Girard (:BenWa)]

(In reply to Markus Stange [:mstange] from comment #15)
> Comment on attachment 8531300 [details] [diff] [review]
> Part 2: Add container layer tests
> 
> Review of attachment 8531300 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/layers/composite/LayerManagerComposite.cpp
> @@ +193,5 @@
> >    mCompositor->SetTargetContext(aTarget, aRect);
> >    mTarget = aTarget;
> >    mTargetBounds = aRect;
> > +
> > +  mClonedLayerTreeProperties = LayerProperties::CloneFrom(GetRoot());
> 
> Is this fixing a real bug?

Yes because now with the container surface invalidation we have to always run the invalidation code otherwise we wont discard a changed surface.

> 
> ::: gfx/tests/gtest/TestCompositor.cpp
> @@ +167,5 @@
> >  
> >    return dt;
> >  }
> >  
> > +static RefPtr<DrawTarget> sTransactionDrawTarget = nullptr;
> 
> I think this causes a static initializer which we want to avoid IIRC. Can
> you use a raw pointer and manual refcounting instead?

Actually I can use StaticRefPtr

> 
> @@ +194,5 @@
> >        for (size_t channel = 0; channel < 4; channel++) {
> >          uint8_t bit = bitmap[y * dss->Stride() + x * 4 + channel];
> >          uint8_t bitRef = bitmapRef[y * dss->Stride() + x * 4 + channel];
> > +        int16_t delta = (uint16_t)bitRef - (uint16_t)bit;
> > +        if (delta >= 2 || delta <= -2) {
> 
> The fuzz threshold should probably be passed as an argument to
> EndTransactionAndCompare.

I think it's fine for now until we need it.

> 
> @@ +195,5 @@
> >          uint8_t bit = bitmap[y * dss->Stride() + x * 4 + channel];
> >          uint8_t bitRef = bitmapRef[y * dss->Stride() + x * 4 + channel];
> > +        int16_t delta = (uint16_t)bitRef - (uint16_t)bit;
> > +        if (delta >= 2 || delta <= -2) {
> > +          if (equal) {
> 
> Don't you also want to dump these things for a non-equal test that doesn't
> contain different pixels?
> 

Yes I guess so.

> @@ +300,5 @@
> > +      nsIntRegion(nsIntRect(50, 50, 100, 100)),
> > +    };
> > +    nsRefPtr<Layer> root = CreateLayerTree("c(oc(oo))", layerVisibleRegion, nullptr, lmBase, layers);
> > +
> > +    // Note: We want to pick an opacity that will round the same way on all backends
> 
> Since you're adding fuzz, does that mean that using 0.4 doesn't actually
> work around the whole problem?

It does. I think I can remove that now.

> 
> @@ +355,5 @@
> > +      colorLayer->SetColor(gfxRGBA(0.1f, 0.2f, 0.3f, 1.f));
> > +      colorLayer->SetBounds(colorLayer->GetVisibleRegion().GetBounds());
> > +    }
> > +    // False since we haven't updated our refDT yet
> > +    EXPECT_FALSE(EndTransactionAndCompareNE(layerManager, refDT));
> 
> The NE already says "not equal", I think it's confusing to use EXPECT_FALSE
> instead of EXPECT_TRUE here. Can we flip the return value of
> EndTransactionAndCompare in the non-equal case?

The idea was to be able to print a different debug message for == and != tests.

Comment 20

[Benoit Girard (:BenWa)]

(In reply to Markus Stange [:mstange] from comment #18)
> Comment on attachment 8533951 [details] [diff] [review]
> patch v4 - fix leaks
> 
> Review of attachment 8533951 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/tests/gtest/TestCompositor.cpp
> @@ +222,5 @@
> > +TEST(Gfx, CompositorConstruct)
> > +{
> > +  auto layerManagers = GetLayerManagers(GetPlatformBackends());
> > +  for (size_t i = 0; i < layerManagers.size(); i++) {
> > +    RefPtr<LayerManagerComposite> layerManager = layerManagers[i].mLayerManager;
> 
> Does this do anything? If you want to test just construction, do you need
> the loop?

Actually I just left it in as a copy-paste template/example. I can remove it if its misleading.

> 
> @@ +231,5 @@
> > +{
> > +  auto layerManagers = GetLayerManagers(GetPlatformBackends());
> > +  for (size_t i = 0; i < layerManagers.size(); i++) {
> > +    nsRefPtr<LayerManagerComposite> layerManager = layerManagers[i].mLayerManager.get();
> > +    nsRefPtr<LayerManager> lmBase = layerManager.get();
> 
> I still think it would be preferable to have layer managers in nsRefPtrs all
> the time, so you don't need this RefPtr -> nsRefPtr conversion .get().

Ok, I'll change that.

Comment 21

[Markus Stange [:mstange]]

(In reply to Benoit Girard (:BenWa) from comment #19)
> (In reply to Markus Stange [:mstange] from comment #15)
> > Comment on attachment 8531300 [details] [diff] [review]
> > Part 2: Add container layer tests
> > 
> > Review of attachment 8531300 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: gfx/layers/composite/LayerManagerComposite.cpp
> > @@ +193,5 @@
> > >    mCompositor->SetTargetContext(aTarget, aRect);
> > >    mTarget = aTarget;
> > >    mTargetBounds = aRect;
> > > +
> > > +  mClonedLayerTreeProperties = LayerProperties::CloneFrom(GetRoot());
> > 
> > Is this fixing a real bug?
> 
> Yes because now with the container surface invalidation we have to always
> run the invalidation code otherwise we wont discard a changed surface.

I mean, is this a bug users can hit while running Firefox regularly? Should we land this change in a separate bug and possibly uplift it?

Comment 22

[Benoit Girard (:BenWa)]

Ahh yes, it's already on aurora =\

Comment 23

[Benoit Girard (:BenWa)]

Attachment: Part 1: Add foundation for compositor unit tests. r=mstange

Feel free to have another look if you'd like

Comment 24

[Benoit Girard (:BenWa)]

Attachment: Part 2: Add container layer tests. r=mstange

Comment 25

[Markus Stange [:mstange]]

This all looks good to me.

Comment 26

[Benoit Girard (:BenWa)]

More include fiddling:
https://tbpl.mozilla.org/?tree=Try&rev=230e8b472a59

Comment 27

[Benoit Girard (:BenWa)]

https://tbpl.mozilla.org/?tree=Try&rev=f7a5df4880c4

Comment 28

[Benoit Girard (:BenWa)]

Attachment: Part 2: Add container layer tests. r=mstange

size_t -> int for warnings as errors.

Comment 29

[Benoit Girard (:BenWa)]

https://tbpl.mozilla.org/?tree=Try&rev=786498b4e431

Comment 30

[Benoit Girard (:BenWa)]

Attachment: Part 2: Add container layer tests. r=mstange

Comment 31

[Benoit Girard (:BenWa)]

Try run for just part 1, since part 2 is being held back by the fix it needs:
https://tbpl.mozilla.org/?tree=Try&rev=90ae2414e582

Comment 32

[Benoit Girard (:BenWa)]

Fix compare error:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5557801e541a

Comment 33

[Benoit Girard (:BenWa)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f3f3990e0508

Comment 34

[Benoit Girard (:BenWa)]

Attachment: Part 1: Add foundation for compositor unit tests. r=mstange

Comment 35

[Benoit Girard (:BenWa)]

leave open but I'll probably spin off part 2 into it's own bug and make this just about adding the framework for testing compositor changes.

Comment 36

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5680121a4909

Comment 37

[Benoit Girard (:BenWa)]

Attachment: Part 2: Add container layer tests. r=mstange (rebased)

Comment 38

[Firefox Bug Husbandry Bot]

Removing leave-open keyword from resolved bugs, per :sylvestre.