748088 - Native android reftest are not testing OMTC+OGL

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Benoit Girard (:BenWa)]

I took a quick look and the reftest are testing some OMTC/IPC hybrid running on the main thread or something of the sort. They don't appear to be running OMTC with OGL on a compositing thread.

Comment 1

[Joel Maher ( :jmaher ) (UTC -8)]

Would this be the cause for the canvas tests not working (I believe they don't render) when we remove the browserIsRemote flag internally?  Currently on android native tests we run with browserIsRemote=True, but we don't have e10s enabled on the build.  I have been working on turning that off, but a lot of tests fail.

I am not a graphics expert, but if you offer some suggestions to the reftest harness, I would be happy to work on making that work and pushing through reviews, testing, landing.

Comment 2

[Benoit Girard (:BenWa)]

Right, I discussed it with the GFX team and that was their theory as well. We should switch all test to use browserIsRemote and flag failure appropriately. We also need a long term plan for fixing the reftest on android.

Comment 3

[Joel Maher ( :jmaher ) (UTC -8)]

So the goal is to run all Android tests with browserIsRemote = true ?  Right now I have failures on android XUL, but the same test passes on android Native.  That needs to get resolved in the real short term.

I think in Wednesday's mobile testing meeting we should figure out a strategy going forward for mobile reftests.

Comment 4

[Benoit Girard (:BenWa)]

(In reply to Benoit Girard (:BenWa) from comment #2)
> Right, I discussed it with the GFX team and that was their theory as well.
> We should switch all test to use browserIsRemote and flag failure
> appropriately. We also need a long term plan for fixing the reftest on
> android.

browserIsRemote=false*

Comment 5

[Ali Juma [:ajuma]]

When we run reftests with OMTC and browser.tabs.remote=false, the DrawWindow calls made by the reftest harness are actually getting the result of the widget's (content-side) BasicLayerManager drawing to a surface, but what we really want is the result of the compositor's (shadow-side) LayerManagerOGL drawing to a surface, since we want the reftest harness to "see" what's being drawn to the screen.

Here's how we can fix this. When a BasicLayerManager that has a ShadowManager gets an EndTransaction where a non-default target was set by the corresponding BeginTransactionWithTarget (that is, when mTarget != mDefaultTarget), it should send a sync IPC message to the ShadowManager asking it to draw into a shared memory buffer. The BasicLayerManager can then copy the contents of this buffer to the target it was given.

(Incidentally, the reason the reftest harness is currently seeing blank output for most tests when browser.tabs.remote=false is that BasicTiledThebesLayer, which always expects to have a shadow, quite rightly ignores the provided gfxContext in PaintThebes rather than painting to it.)

Comment 6

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

What browserIsRemote reports is meaningless in the context of native-fennec.  It doesn't affect what Gecko code runs during tests.  It does result in two optimizations in the reftest harness itself not being used, but that's pretty much it.  The results should be the same.

Comment 7

[Ali Juma [:ajuma]]

(In reply to Chris Jones [:cjones] [:warhammer] from comment #6)
> What browserIsRemote reports is meaningless in the context of native-fennec.
> It doesn't affect what Gecko code runs during tests.  It does result in two
> optimizations in the reftest harness itself not being used, but that's
> pretty much it.  The results should be the same.

Here's what I found, running reftests on a Tegra board:

-With browser.tabs.remote=true, a content process is created and used by the reftests. A CompositorParent/CompositorChild pair is also created, as usual, but this is not used by the reftests, and only a blank page is shown on screen during the reftests.

-With browser.tabs.remote=false, no content process is created. The reftests use the CompositorParent/CompositorChild, and are shown on screen. However, the DrawWindow calls made by the reftest harness read back from the CompositorChild's BasicLayerManager, not from the CompositorParent's LayerManagerOGL.

Comment 8

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Wow, that is incredibly broken and not how I thought things worked at all!

This is actually testing the configuration we'll have for b2g ...

Comment 9

[Ali Juma [:ajuma]]

Attachment: Part 1: Add a DrawToSurface message to PLayers.

Comment 10

[Ali Juma [:ajuma]]

Attachment: Part 2: Drawing to a non-default target should happen on the shadow side, not the shadowable side.

Together with Part 1, this implements the fix described in Comment 5.

Comment 11

[Ali Juma [:ajuma]]

Attachment: Part 1: Add a DrawToSurface message to PLayers.

Un-bitrotted.

Comment 12

[Ali Juma [:ajuma]]

Attachment: Part 2: Drawing to a non-default target should happen on the shadow side, not the shadowable side, v2

Patch updated to fix Linux crashes in reftest-ipc and crashtest-ipc.

Comment 13

[Benoit Girard (:BenWa)]

Comment on attachment 622729 [details] [diff] [review]
Part 1: Add a DrawToSurface message to PLayers.

Patch looks fine. Let's keep PLayers.ipdl documented.

Comment 14

[Ali Juma [:ajuma]]

Attachment: Part 1: Add a DrawToSurface message to PLayers.

Patch updated to address review comments.

Comment 15

[Benoit Girard (:BenWa)]

Comment on attachment 623230 [details] [diff] [review]
Part 2: Drawing to a non-default target should happen on the shadow side, not the shadowable side, v2

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

Great patch overall, r+ with these small improvements.

::: gfx/layers/basic/BasicLayers.cpp
@@ +3546,2 @@
>    }
>    BasicLayerManager::BeginTransactionWithTarget(aTarget);

You should clear mShadowTarget here...

@@ +3555,5 @@
>    BasicLayerManager::EndTransaction(aCallback, aCallbackData, aFlags);
>    ForwardTransaction();
> +  if (mShadowTarget) {
> +    ShadowLayerForwarder::ShadowDrawToTarget(mShadowTarget);
> +    mShadowTarget = nsnull;

... instead of here.

::: gfx/layers/ipc/ShadowLayers.cpp
@@ +388,5 @@
> +  nsRefPtr<gfxASurface> surface = OpenDescriptor(descriptorOut);
> +  aTarget->SetOperator(gfxContext::OPERATOR_SOURCE);
> +  aTarget->DrawSurface(surface, surface->GetSize());
> +
> +  DestroySharedSurface(&descriptorOut);

Nit: I feel like it's cleaner to do 'surface = nsnull' before this so we don't hold a surface to something that destroyed even if it's not used.

Comment 16

[Benoit Girard (:BenWa)]

(In reply to Benoit Girard (:BenWa) from comment #15)
> Comment on attachment 623230 [details] [diff] [review]
> Part 2: Drawing to a non-default target should happen on the shadow side,
> not the shadowable side, v2
> 
> Review of attachment 623230 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Great patch overall, r+ with these small improvements.
> 
> ::: gfx/layers/basic/BasicLayers.cpp
> @@ +3546,2 @@
> >    }
> >    BasicLayerManager::BeginTransactionWithTarget(aTarget);
> 
> You should clear mShadowTarget here...
> 
> @@ +3555,5 @@
> >    BasicLayerManager::EndTransaction(aCallback, aCallbackData, aFlags);
> >    ForwardTransaction();
> > +  if (mShadowTarget) {
> > +    ShadowLayerForwarder::ShadowDrawToTarget(mShadowTarget);
> > +    mShadowTarget = nsnull;
> 
> ... instead of here.
> 
Actually I read this too quickly. This is fine and also similar to what we do elsewhere.

Comment 17

[Ali Juma [:ajuma]]

Attachment: Part 2: Drawing to a non-default target should happen on the shadow side, not the shadowable side, v3

Patch updated to address review comments. Carrying forward r=BenWa.

Comment 18

[Ali Juma [:ajuma]]

Attachment: Part 1: Add a DrawToSurface message to PLayers, rebased.

Patch rebased.

Comment 19

[Ali Juma [:ajuma]]

Attachment: Part 2: Drawing to a non-default target should happen on the shadow side, not the shadowable side, v3, rebased.

Patch rebased.

Comment 20

[Joel Maher ( :jmaher ) (UTC -8)]

Attachment: adjust reftest manifests to get green r1-3 with browser.tabs.remote=false (1.0)

this is a working patch.  I still need to go through all other bugs out there and update or file new bugs to match this current set of 29 tests.

Comment 21

[Ali Juma [:ajuma]]

Comment on attachment 628415 [details] [diff] [review]
adjust reftest manifests to get green r1-3 with browser.tabs.remote=false (1.0)

Looks good to me. It's great to see that we're marking more tests as passing (18) than as failing (10).

Comment 22

[Joel Maher ( :jmaher ) (UTC -8)]

Attachment: adjust reftest manifests to get green r1-3 with browser.tabs.remote=false (2.0)

allow for crashtests to pass, there were 3 different scenarios that required manifest hacking.

Comment 23

[Joel Maher ( :jmaher ) (UTC -8)]

Attachment: adjust reftest manifests to get green r1-3 with browser.tabs.remote=false (2.1)

added 2 more test cases as well as documented with bug numbers.

Comment 24

[Ali Juma [:ajuma]]

Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=48134079201c

Comment 25

[Joel Maher ( :jmaher ) (UTC -8)]

inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/736da6c5bde2
https://hg.mozilla.org/integration/mozilla-inbound/rev/b15bcf12945e
https://hg.mozilla.org/integration/mozilla-inbound/rev/74ec0168aa18

Comment 26

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/74ec0168aa18
https://hg.mozilla.org/mozilla-central/rev/b15bcf12945e
https://hg.mozilla.org/mozilla-central/rev/736da6c5bde2