1319362 - Modify the default minimum rendering size for SkiaGL

Status: RESOLVED FIXED

(Core :: Graphics: Canvas2D, defect)

Description

[Ethan Lin[:ethlin]]

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.

Comment 1

[Ethan Lin[:ethlin]]

Attachment: change default value

I use the original value before the patch in bug 905227.

Comment 2

[Nicolas Silva [:nical]]

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.

Comment 3

[Lee Salzman [: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?

Comment 4

[Ethan Lin[:ethlin]]

(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.

Comment 5

[Ethan Lin[:ethlin]]

Attachment: Check area

I check area but not just dimension in this patch. This way should be more precise.

Comment 6

[Xidorn Quan [:xidorn] UTC+11]

(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?

Comment 7

[Ethan Lin[:ethlin]]

(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.

Comment 8

[Ethan Lin[:ethlin]]

try result:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5af9abe54a3697aa03e9767d2ea64b0c93b08186

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a38d735c9041
Use area but not just dimension to check SkiaGL minimum size. r=lsalzman

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a38d735c9041