Closed Bug 999841 Opened 6 years ago Closed 6 years ago

Canvas in FishIEtank benchmark hits size limit for skiagl

Categories

(Core :: Canvas: 2D, defect)

All
Gonk (Firefox OS)
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla32

People

(Reporter: mwu, Assigned: bkelly)

References

()

Details

(Keywords: regression)

Attachments

(1 file, 1 obsolete file)

The canvas in the FishIETank benchmark is sized based on window.innerWidth and window.innerHeight. This ends up much bigger than the device screen size, and the canvas doesn't get accelerated.
Michael, were you ever able to test this on a mobile version of FishIETank that sets the viewport with a meta tag?
Nope - didn't find one.
Its here:

  http://ie.microsoft.com/testdrive/Mobile/Performance/FishIETank/Default.html

I still think it will have trouble, though, because it hardcodes the width in its viewport tag:

  <meta name="viewport" content="width=480, initial-scale=1, maximum-scale=1, user-scalable=no"/>
After discussion in #gfx it sounds like using the full layout dimensions may be too permissive.  On the other hand, we default to a size of 980x480 which is larger than some screen.  (That default size is the de facto standard set by iOS.)

So I propose to simply use the MAX(default size, screen size) as the threshold.

This allows the mobile version of FishIETank to pass the skia size check.  Again this URL is:

  http://ie.microsoft.com/testdrive/Mobile/Performance/FishIETank/Default.html

Note, on my buri I am only getting 10fps on this site, but I verified it is passing the size check.  Perhaps there is another problem on master at the moment.
Assignee: nobody → bkelly
Status: NEW → ASSIGNED
Attachment #8413945 - Flags: review?(snorp)
Attachment #8413945 - Flags: feedback?(mwu)
Comment on attachment 8413945 [details] [diff] [review]
Allow skia canvas up to 980x480 even if screen is smaller. (v1)

Looks fine to me, but I can't easily test this at the moment - currently developing on a Flame.
Attachment #8413945 - Flags: feedback?(mwu) → feedback+
Comment on attachment 8413945 [details] [diff] [review]
Allow skia canvas up to 980x480 even if screen is smaller. (v1)

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

Seems reasonable.
Attachment #8413945 - Flags: review?(snorp) → review+
So talking on irc with :mwu and :kats, it seems we probably want to apply any scale factor for high res dpi as well.  Let me roll a patch with that and reflag.  Sorry for the churn.
Here's an updated patch that takes into account the dpi scaling as well.  Reflagging for review as its a bit different now.

Note, I cache the screen pixels and the scaling factor separately since we can fail to get these values in different ways.

I verified that I get a scale of 1.0 on my buri and a scale of 2.0 on my nexus 4.
Attachment #8413945 - Attachment is obsolete: true
Attachment #8414665 - Flags: review?(snorp)
Attachment #8414665 - Flags: review?(snorp) → review+
Try looks reasonably green.  The persistent orange in Android 4.0 Debug M(1) appears to be a known issue affecting m-c.

https://hg.mozilla.org/integration/mozilla-inbound/rev/c0d658d3f739
https://hg.mozilla.org/mozilla-central/rev/c0d658d3f739
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
Michael, do you think we need this in 1.4 to support the FishIETank bug you were working?  (mozilla32 is still 2.0, right?)
Flags: needinfo?(mwu)
1.4 has other issues that need to be fixed to really get FishIETank fast, so I don't think it's critical to get in. Won't hurt, though.
Flags: needinfo?(mwu)
Comment on attachment 8414665 [details] [diff] [review]
Allow skia canvas up to 980x480 even if screen is smaller. (v2)

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

::: content/canvas/src/CanvasRenderingContext2D.cpp
@@ +888,5 @@
> +    }
> +  }
> +
> +  int32_t threshold = gDefaultScale > 0 ? ceil(gDefaultScale * gScreenPixels)
> +                                        : gScreenPixels;

Shouldn't gDefaultScale be squared?
(In reply to Michael Wu [:mwu] from comment #15)
> Comment on attachment 8414665 [details] [diff] [review]
> Allow skia canvas up to 980x480 even if screen is smaller. (v2)
> 
> Review of attachment 8414665 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/canvas/src/CanvasRenderingContext2D.cpp
> @@ +888,5 @@
> > +    }
> > +  }
> > +
> > +  int32_t threshold = gDefaultScale > 0 ? ceil(gDefaultScale * gScreenPixels)
> > +                                        : gScreenPixels;
> 
> Shouldn't gDefaultScale be squared?

Yup, good catch.
Blocks: 1004469
(In reply to Michael Wu [:mwu] from comment #15)
> Shouldn't gDefaultScale be squared?

Thanks for the catch!  Patch in bug 1004469.
You need to log in before you can comment on or make changes to this bug.