Closed Bug 731603 Opened 9 years ago Closed 9 years ago

Maple: BeginDrawing/EndDrawing take > 8ms/1ms

Categories

(Firefox for Android Graveyard :: General, defect, P2)

ARM
Android
defect

Tracking

(firefox13 affected, firefox14 fixed, blocking-fennec1.0 beta+)

RESOLVED FIXED
Firefox 14
Tracking Status
firefox13 --- affected
firefox14 --- fixed
blocking-fennec1.0 --- beta+

People

(Reporter: jrmuizel, Assigned: kats)

References

Details

(Whiteboard: [gfx])

Attachments

(3 files, 1 obsolete file)

We can hopefully eliminate most of the work that's happening here.
Blocks: 729391
See also bug 730687
Depends on: 731897
blocking-fennec1.0: --- → ?
No longer blocks: land-maple
blocking-fennec1.0: ? → beta+
Priority: -- → P2
Whiteboard: [gfx]
Assignee: nobody → bugmail.mozilla
Duplicate of this bug: 729653
Summary: Maple: EndDrawing takes 1ms → Maple: BeginDrawing/EndDrawing take > 8ms/1ms
Waiting on try run at https://tbpl.mozilla.org/?tree=Try&rev=b166950cf592 to see if the robocop stuff works with this.
Comment on attachment 607194 [details] [diff] [review]
(1/2) Propagate resolution back to java in syncViewportInfo

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

Looks fine.

::: gfx/layers/ipc/CompositorParent.cpp
@@ +292,5 @@
> +    nsIntPoint scrollOffset = metrics->mViewportScrollOffset;
> +    displayPort.x += scrollOffset.x;
> +    displayPort.y += scrollOffset.y;
> +
> +    mozilla::AndroidBridge::Bridge()->SyncViewportInfo(displayPort, 1/rootScaleX, mScrollOffset, mXScale, mYScale);

Are there any problems with only using the X axis for the scale factor? I realise they should be the same, but I think there's the possibility of them being different by fractional amounts for optimisation reasons?
Attachment #607194 - Flags: review?(chrislord.net) → review+
Comment on attachment 607195 [details] [diff] [review]
(2/2) Get rid of endDrawing/beginDrawing and fix touch event mapping

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

Like it, but this should also remove the draw-metadata function in browser.js, right? I'd also like to know more about the possible robotium breakage, but if that's a problem, I think this should still land and an extra bug should be filed for that.

::: mobile/android/base/gfx/GeckoLayerClient.java
@@ +379,5 @@
> +            /* Used by robocop for testing purposes */
> +            boolean drawnAreaChanged = !mRootLayer.getPosition().equals(new Rect(x, y, x + width, y + height))
> +                                    || !FloatUtils.fuzzyEquals(mRootLayer.getResolution(), resolution);
> +            if (drawnAreaChanged) {
> +                mDrawListener.drawFinished();

This assumes that every draw will have a different position or resolution? This isn't equivalent to what was there before, there ought to be a comment (or this function renamed).

Perhaps this could be remedied by tracking when a layer transaction happens in LayerManagerOGL and setting a flag on the first render after a transaction?

What depends on this function, and does it need to be more correct than this?
Attachment #607195 - Flags: review?(chrislord.net) → review+
(In reply to Chris Lord [:cwiiis] from comment #6)
> Comment on attachment 607194 [details] [diff] [review]
> Are there any problems with only using the X axis for the scale factor? I
> realise they should be the same, but I think there's the possibility of them
> being different by fractional amounts for optimisation reasons?

TBH, I don't really know. But we're using the X scale only in SetFirstPaintViewport as well so I figured it would be best to keep it consistent.

(In reply to Chris Lord [:cwiiis] from comment #7)
> Like it, but this should also remove the draw-metadata function in
> browser.js, right?

Good point, I'll add another patch that takes that stuff out.

> I'd also like to know more about the possible robotium
> breakage, but if that's a problem, I think this should still land and an
> extra bug should be filed for that.
> 
> ::: mobile/android/base/gfx/GeckoLayerClient.java
> @@ +379,5 @@
> > +            /* Used by robocop for testing purposes */
> > +            boolean drawnAreaChanged = !mRootLayer.getPosition().equals(new Rect(x, y, x + width, y + height))
> > +                                    || !FloatUtils.fuzzyEquals(mRootLayer.getResolution(), resolution);
> > +            if (drawnAreaChanged) {
> > +                mDrawListener.drawFinished();
> 
> This assumes that every draw will have a different position or resolution?
> This isn't equivalent to what was there before, there ought to be a comment
> (or this function renamed).
> 
> Perhaps this could be remedied by tracking when a layer transaction happens
> in LayerManagerOGL and setting a flag on the first render after a
> transaction?
> 
> What depends on this function, and does it need to be more correct than this?

Yeah, this isn't equivalent to what was there before, and based on the way it's used in the tests I thought it would be correct enough. But your solution is pretty easy to implement so I should stop being lazy and just implement it. :)

As for what depends on that function - we use it to determine when we can grab the pixels via glReadPixels and expect them to reflect the newly drawn stuff. In one test we also use it as a negative test, to make sure we aren't spuriously drawing while in overscroll.
(In reply to Kartikaya Gupta (:kats) from comment #8)
> (In reply to Chris Lord [:cwiiis] from comment #7)
> > Like it, but this should also remove the draw-metadata function in
> > browser.js, right?
> 
> Good point, I'll add another patch that takes that stuff out.

Turns out there's a showSurface call in GeckoApp that also uses this now, so I can't just rip it out. :(
Need this for robocop testing.
Attachment #607292 - Flags: review?(ajuma)
Updated version of the last patch, with the robocop stuff pulled out into the previous patch. Carrying r+
Attachment #607195 - Attachment is obsolete: true
Attachment #607293 - Flags: review+
Attachment #607292 - Flags: review?(ajuma) → review+
Assignee: bugmail.mozilla → nobody
Component: Graphics → General
OS: Mac OS X → Android
Product: Core → Fennec Native
QA Contact: thebes → general
Hardware: x86 → ARM
Merged into m-c:
https://hg.mozilla.org/mozilla-central/rev/3f893a92408a
https://hg.mozilla.org/mozilla-central/rev/f93f7553317f
https://hg.mozilla.org/mozilla-central/rev/22af7bc45a18
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Version: unspecified → Trunk
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.