Closed
Bug 999841
Opened 11 years ago
Closed 10 years ago
Canvas in FishIEtank benchmark hits size limit for skiagl
Categories
(Core :: Graphics: Canvas2D, defect)
Tracking
()
RESOLVED
FIXED
mozilla32
People
(Reporter: mwu, Assigned: bkelly)
References
()
Details
(Keywords: regression)
Attachments
(1 file, 1 obsolete file)
(deleted),
patch
|
snorp
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•10 years ago
|
||
Michael, were you ever able to test this on a mobile version of FishIETank that sets the viewport with a meta tag?
Reporter | ||
Comment 2•10 years ago
|
||
Nope - didn't find one.
Assignee | ||
Comment 3•10 years ago
|
||
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"/>
Assignee | ||
Comment 4•10 years ago
|
||
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)
Reporter | ||
Comment 5•10 years ago
|
||
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 6•10 years ago
|
||
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+
Assignee | ||
Comment 7•10 years ago
|
||
Assignee | ||
Comment 8•10 years ago
|
||
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.
Assignee | ||
Comment 9•10 years ago
|
||
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)
Updated•10 years ago
|
Attachment #8414665 -
Flags: review?(snorp) → review+
Assignee | ||
Comment 10•10 years ago
|
||
Assignee | ||
Comment 11•10 years ago
|
||
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
Comment 12•10 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
Assignee | ||
Comment 13•10 years ago
|
||
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)
Reporter | ||
Comment 14•10 years ago
|
||
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)
Reporter | ||
Comment 15•10 years ago
|
||
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?
Comment 16•10 years ago
|
||
(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.
Assignee | ||
Comment 17•10 years ago
|
||
(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.
Description
•