Native android reftest are not testing OMTC+OGL

RESOLVED FIXED in mozilla15

Status

()

Core
Graphics
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: BenWa, Assigned: ajuma)

Tracking

unspecified
mozilla15
x86
Mac OS X
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 8 obsolete attachments)

2.69 KB, patch
ajuma
: review+
Details | Diff | Splinter Review
6.44 KB, patch
ajuma
: review+
Details | Diff | Splinter Review
14.27 KB, patch
jmaher
: review+
Details | Diff | Splinter Review
(Reporter)

Description

5 years ago
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.
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.
(Reporter)

Comment 2

5 years ago
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.
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.
(Reporter)

Updated

5 years ago
Depends on: 732498
(Reporter)

Comment 4

5 years ago
(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*
(Assignee)

Comment 5

5 years ago
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.)
Assignee: nobody → ajuma
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.
(Assignee)

Comment 7

5 years ago
(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.
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 ...
(Assignee)

Comment 9

5 years ago
Created attachment 622518 [details] [diff] [review]
Part 1: Add a DrawToSurface message to PLayers.
(Assignee)

Comment 10

5 years ago
Created attachment 622521 [details] [diff] [review]
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.
(Assignee)

Comment 11

5 years ago
Created attachment 622729 [details] [diff] [review]
Part 1: Add a DrawToSurface message to PLayers.

Un-bitrotted.
Attachment #622518 - Attachment is obsolete: true
(Assignee)

Comment 12

5 years ago
Created attachment 623230 [details] [diff] [review]
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.
Attachment #622521 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #622729 - Flags: review?(bgirard)
(Assignee)

Updated

5 years ago
Attachment #623230 - Flags: review?(bgirard)
(Reporter)

Comment 13

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

Patch looks fine. Let's keep PLayers.ipdl documented.
Attachment #622729 - Flags: review?(bgirard) → review-
(Assignee)

Comment 14

5 years ago
Created attachment 623686 [details] [diff] [review]
Part 1: Add a DrawToSurface message to PLayers.

Patch updated to address review comments.
Attachment #622729 - Attachment is obsolete: true
Attachment #623686 - Flags: review?(bgirard)
(Reporter)

Comment 15

5 years ago
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.
Attachment #623230 - Flags: review?(bgirard) → review+
(Reporter)

Updated

5 years ago
Attachment #623686 - Flags: review?(bgirard) → review+
(Reporter)

Comment 16

5 years ago
(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.
(Assignee)

Updated

5 years ago
Depends on: 755452
(Assignee)

Comment 17

5 years ago
Created attachment 624157 [details] [diff] [review]
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.
Attachment #623230 - Attachment is obsolete: true
Attachment #624157 - Flags: review+
(Assignee)

Comment 18

5 years ago
Created attachment 628014 [details] [diff] [review]
Part 1: Add a DrawToSurface message to PLayers, rebased.

Patch rebased.
Attachment #623686 - Attachment is obsolete: true
Attachment #628014 - Flags: review+
(Assignee)

Comment 19

5 years ago
Created attachment 628016 [details] [diff] [review]
Part 2: Drawing to a non-default target should happen on the shadow side, not the shadowable side, v3, rebased.

Patch rebased.
Attachment #624157 - Attachment is obsolete: true
Attachment #628016 - Flags: review+
Created attachment 628415 [details] [diff] [review]
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.
Attachment #628415 - Flags: review?(ajuma)
(Assignee)

Comment 21

5 years ago
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).
Attachment #628415 - Flags: review?(ajuma) → review+
No longer depends on: 739708
Created attachment 628480 [details] [diff] [review]
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.
Attachment #628415 - Attachment is obsolete: true
Attachment #628480 - Flags: review?(ajuma)
(Assignee)

Updated

5 years ago
Attachment #628480 - Flags: review?(ajuma) → review+
Created attachment 628900 [details] [diff] [review]
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.
Attachment #628480 - Attachment is obsolete: true
Attachment #628900 - Flags: review+
(Assignee)

Comment 24

5 years ago
Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=48134079201c
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
https://hg.mozilla.org/mozilla-central/rev/74ec0168aa18
https://hg.mozilla.org/mozilla-central/rev/b15bcf12945e
https://hg.mozilla.org/mozilla-central/rev/736da6c5bde2
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla15
Depends on: 773192
Duplicate of this bug: 732498
You need to log in before you can comment on or make changes to this bug.