Modify the default minimum rendering size for SkiaGL

RESOLVED FIXED in Firefox 53

Status

()

Core
Canvas: 2D
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: ethlin, Assigned: ethlin)

Tracking

(Blocks: 1 bug)

unspecified
mozilla53
Points:
---

Firefox Tracking Flags

(firefox53 fixed)

Details

(URL)

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

a year ago
The original min rendering size for SkiaGL is 16x16. Bug 905227 changed the limitation to 128x128, so it became hard to use SkiaGL. But Bug 905227 is for turning on SkiaGL on B2G, I don't see why we use the same limitation for browser.

I tested the website reported in bug 1313675 and the playback is smoother after changing the limitation.
(Assignee)

Updated

a year ago
Blocks: 1313675
(Assignee)

Comment 1

a year ago
Created attachment 8813077 [details] [diff] [review]
change default value

I use the original value before the patch in bug 905227.
Attachment #8813077 - Flags: review?(nical.bugzilla)
Comment on attachment 8813077 [details] [diff] [review]
change default value

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

I don't know enough about the implications of allowing skiagl for smaller sizes, so deferring the review to Lee. One thing to note is that if we allow very small canvases, we'll suddenly see skiagl used for ad banners in a lot of places where it wasn't used before since some of the standard ad sizes are less than 128 thick (although, that may already be the case with hidpi screens, not sure). I don't know if it's a good or a bad thing.
Attachment #8813077 - Flags: review?(nical.bugzilla) → review?(lsalzman)
Comment on attachment 8813077 [details] [diff] [review]
change default value

I think given the original concerns noted in bug 905227, it is unwise to merely set the value back to 16x16. There is a cost to having a bunch of small SkiaGL canvases, and that will outweigh the benefits here. Can we get away with making this larger, at least 32x32, or possibly 64x64?
Flags: needinfo?(ethlin)
(Assignee)

Comment 4

a year ago
(In reply to Lee Salzman [:lsalzman] from comment #3)
> Comment on attachment 8813077 [details] [diff] [review]
> change default value
> 
> I think given the original concerns noted in bug 905227, it is unwise to
> merely set the value back to 16x16. There is a cost to having a bunch of
> small SkiaGL canvases, and that will outweigh the benefits here. Can we get
> away with making this larger, at least 32x32, or possibly 64x64?

The perf problem in bug 1313675 is about text rendering. I found many text sizes in the page are about 900x50, so we will not use SkiaGL to render them though the text areas are larger than 128x128.
Maybe we should check area value.
Flags: needinfo?(ethlin)
(Assignee)

Comment 5

a year ago
Created attachment 8813498 [details] [diff] [review]
Check area

I check area but not just dimension in this patch. This way should be more precise.
Attachment #8813077 - Attachment is obsolete: true
Attachment #8813077 - Flags: review?(lsalzman)
Attachment #8813498 - Flags: review?(lsalzman)
(In reply to Ethan Lin[:ethlin] from comment #4)
> The perf problem in bug 1313675 is about text rendering. I found many text
> sizes in the page are about 900x50, so we will not use SkiaGL to render them
> though the text areas are larger than 128x128.
> Maybe we should check area value.

Do they render each danmaku in a separate canvas and then move those canvases each frame, or render all danmakus in a large canvas and draw all again each frame?
Attachment #8813498 - Flags: review?(lsalzman) → review+
(Assignee)

Comment 7

a year ago
(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #6)
> (In reply to Ethan Lin[:ethlin] from comment #4)
> > The perf problem in bug 1313675 is about text rendering. I found many text
> > sizes in the page are about 900x50, so we will not use SkiaGL to render them
> > though the text areas are larger than 128x128.
> > Maybe we should check area value.
> 
> Do they render each danmaku in a separate canvas and then move those
> canvases each frame, or render all danmakus in a large canvas and draw all
> again each frame?

The page renders each danmaku in a separate canvas and then calls DrawImage to draw on a large canvas.
(Assignee)

Updated

a year ago
Keywords: checkin-needed

Comment 9

a year ago
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a38d735c9041
Use area but not just dimension to check SkiaGL minimum size. r=lsalzman
Keywords: checkin-needed

Comment 10

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/a38d735c9041
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
You need to log in before you can comment on or make changes to this bug.