Last Comment Bug 748088 - Native android reftest are not testing OMTC+OGL
: Native android reftest are not testing OMTC+OGL
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: unspecified
: x86 Mac OS X
: -- normal (vote)
: mozilla15
Assigned To: Ali Juma [:ajuma]
:
Mentors:
: 732498 (view as bug list)
Depends on: 732498 755452 773192
Blocks:
  Show dependency treegraph
 
Reported: 2012-04-23 13:20 PDT by Benoit Girard (:BenWa)
Modified: 2012-10-17 04:28 PDT (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Part 1: Add a DrawToSurface message to PLayers. (2.49 KB, patch)
2012-05-09 14:46 PDT, Ali Juma [:ajuma]
no flags Details | Diff | Splinter Review
Part 2: Drawing to a non-default target should happen on the shadow side, not the shadowable side. (5.85 KB, patch)
2012-05-09 14:50 PDT, Ali Juma [:ajuma]
no flags Details | Diff | Splinter Review
Part 1: Add a DrawToSurface message to PLayers. (2.60 KB, patch)
2012-05-10 07:56 PDT, Ali Juma [:ajuma]
bgirard: review-
Details | Diff | Splinter Review
Part 2: Drawing to a non-default target should happen on the shadow side, not the shadowable side, v2 (6.27 KB, patch)
2012-05-11 11:32 PDT, Ali Juma [:ajuma]
bgirard: review+
Details | Diff | Splinter Review
Part 1: Add a DrawToSurface message to PLayers. (2.68 KB, patch)
2012-05-14 08:47 PDT, Ali Juma [:ajuma]
bgirard: review+
Details | Diff | Splinter Review
Part 2: Drawing to a non-default target should happen on the shadow side, not the shadowable side, v3 (6.27 KB, patch)
2012-05-15 12:59 PDT, Ali Juma [:ajuma]
ajuma.bugzilla: review+
Details | Diff | Splinter Review
Part 1: Add a DrawToSurface message to PLayers, rebased. (2.69 KB, patch)
2012-05-29 10:24 PDT, Ali Juma [:ajuma]
ajuma.bugzilla: review+
Details | Diff | Splinter Review
Part 2: Drawing to a non-default target should happen on the shadow side, not the shadowable side, v3, rebased. (6.44 KB, patch)
2012-05-29 10:26 PDT, Ali Juma [:ajuma]
ajuma.bugzilla: review+
Details | Diff | Splinter Review
adjust reftest manifests to get green r1-3 with browser.tabs.remote=false (1.0) (11.91 KB, patch)
2012-05-30 12:06 PDT, Joel Maher ( :jmaher)
ajuma.bugzilla: review+
Details | Diff | Splinter Review
adjust reftest manifests to get green r1-3 with browser.tabs.remote=false (2.0) (13.59 KB, patch)
2012-05-30 14:21 PDT, Joel Maher ( :jmaher)
ajuma.bugzilla: review+
Details | Diff | Splinter Review
adjust reftest manifests to get green r1-3 with browser.tabs.remote=false (2.1) (14.27 KB, patch)
2012-05-31 14:24 PDT, Joel Maher ( :jmaher)
jmaher: review+
Details | Diff | Splinter Review

Description Benoit Girard (:BenWa) 2012-04-23 13:20:01 PDT
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) 2012-04-23 13:47:26 PDT
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) 2012-04-23 15:43:48 PDT
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) 2012-04-23 18:23:01 PDT
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) 2012-04-27 08:18:51 PDT
(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] 2012-05-03 14:07:13 PDT
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 2012-05-03 15:48:11 PDT
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] 2012-05-03 16:50:02 PDT
(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 2012-05-03 16:57:32 PDT
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] 2012-05-09 14:46:03 PDT
Created attachment 622518 [details] [diff] [review]
Part 1: Add a DrawToSurface message to PLayers.
Comment 10 Ali Juma [:ajuma] 2012-05-09 14:50:41 PDT
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.
Comment 11 Ali Juma [:ajuma] 2012-05-10 07:56:04 PDT
Created attachment 622729 [details] [diff] [review]
Part 1: Add a DrawToSurface message to PLayers.

Un-bitrotted.
Comment 12 Ali Juma [:ajuma] 2012-05-11 11:32:22 PDT
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.
Comment 13 Benoit Girard (:BenWa) 2012-05-14 08:31:24 PDT
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] 2012-05-14 08:47:08 PDT
Created attachment 623686 [details] [diff] [review]
Part 1: Add a DrawToSurface message to PLayers.

Patch updated to address review comments.
Comment 15 Benoit Girard (:BenWa) 2012-05-14 08:50:35 PDT
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) 2012-05-14 08:56:12 PDT
(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] 2012-05-15 12:59:20 PDT
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.
Comment 18 Ali Juma [:ajuma] 2012-05-29 10:24:59 PDT
Created attachment 628014 [details] [diff] [review]
Part 1: Add a DrawToSurface message to PLayers, rebased.

Patch rebased.
Comment 19 Ali Juma [:ajuma] 2012-05-29 10:26:49 PDT
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.
Comment 20 Joel Maher ( :jmaher) 2012-05-30 12:06:23 PDT
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.
Comment 21 Ali Juma [:ajuma] 2012-05-30 12:22:51 PDT
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) 2012-05-30 14:21:14 PDT
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.
Comment 23 Joel Maher ( :jmaher) 2012-05-31 14:24:29 PDT
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.
Comment 24 Ali Juma [:ajuma] 2012-05-31 14:47:32 PDT
Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=48134079201c
Comment 27 Joel Maher ( :jmaher) 2012-10-17 04:28:36 PDT
*** Bug 732498 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.