Status

()

defect
RESOLVED WONTFIX
5 years ago
2 years ago

People

(Reporter: BenWa, Unassigned)

Tracking

Trunk
x86
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 11 obsolete attachments)

30.67 KB, patch
BenWa
: review+
Details | Diff | Splinter Review
11.74 KB, patch
BenWa
: review+
Details | Diff | Splinter Review
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.
Posted patch Poking with the setup (obsolete) — Splinter Review
The setup looks to be very simple.
Posted patch patch v1 (obsolete) — Splinter Review
Assignee: nobody → bgirard
Attachment #8490547 - Attachment is obsolete: true
Status: NEW → ASSIGNED
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 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.
Attachment #8527986 - Flags: feedback+
.get() is needed for RefPtr -> nsRefPtr
Try closed.
Attachment #8527986 - Attachment is obsolete: true
Fix debug assertions and non deterministic gfxPrefs ordering.
https://tbpl.mozilla.org/?tree=Try&rev=6b0f1281f597
Attachment #8528134 - Attachment is obsolete: true
Attachment #8530462 - Flags: review?(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.
Attachment #8530462 - Flags: review?(mstange) → review+
I wrote a container layer test on the plane
Attachment #8531300 - Flags: review?(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?
Attachment #8531300 - Flags: review?(mstange) → review+
Posted patch patch v4 - fix leaks (obsolete) — Splinter Review
Attachment #8530462 - Attachment is obsolete: true
Attachment #8533951 - Flags: review?(mstange)
trivial fixes
Attachment #8531300 - Attachment is obsolete: true
Attachment #8533952 - Flags: review+
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().
Attachment #8533951 - Flags: review?(mstange) → review+
(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.
(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.
(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?
Ahh yes, it's already on aurora =\
Depends on: 1109314
Feel free to have another look if you'd like
Attachment #8533951 - Attachment is obsolete: true
Attachment #8533996 - Flags: review+
Attachment #8533952 - Attachment is obsolete: true
Attachment #8533998 - Flags: review+
This all looks good to me.
size_t -> int for warnings as errors.
Attachment #8533998 - Attachment is obsolete: true
Attachment #8535051 - Flags: review+
Attachment #8535051 - Attachment is obsolete: true
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
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.
Keywords: leave-open
Attachment #8535099 - Attachment is obsolete: true
Attachment #8551504 - Flags: review+
Assignee: bgirard → nobody
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → WONTFIX
Removing leave-open keyword from resolved bugs, per :sylvestre.
Keywords: leave-open
You need to log in before you can comment on or make changes to this bug.