Closed Bug 1351323 Opened 3 years ago Closed 3 years ago
New GPU accelerated tab throbbers appear slightly fuzzy, especially on high-resolution displays
See the attached video. The top spinner is the spinner on release, the one below is the GPU accelerated one on Nightly since bug 759252 landed. shorlander noticed this, and I offered to file it. Essentially, this appears to be the result of us rasterizing the spinner once to a layer (with anti-aliasing artifacts), and then rotating that layer (with the anti-aliasing artifacts). Not sure if there's much that can be done here, but filing just in case.
(In reply to Mike Conley (:mconley) (QF workweek until March 31) from comment #0) > Essentially, this appears > to be the result of us rasterizing the spinner once to a layer (with > anti-aliasing artifacts), and then rotating that layer (with the > anti-aliasing artifacts). Sounds like a platform issue, unless there's some CSS that we should use here (image-rendering: -moz-crisp-edges?), but I don't think so.
Component: Theme → SVG
Product: Firefox → Core
When I pause the video it looks to me like the "tail" is a lot darker in the SVG version than the PNG version. Isn't that the problem? If so I think the fundamental issue might be that a circumferential gradient was used to create the PNG version (exactly what we'd want), but since SVG doesn't support that (I've long wanted to add it to the spec) we try to approximate things with a radial gradient and a mask in the SVG version. For my own reference as much as anything else, the two live files are: https://hg.mozilla.org/releases/mozilla-release/raw-file/62f7f4595c88/toolkit/themes/shared/icons/loading.png https://hg.mozilla.org/mozilla-central/raw-file/76ad1c764e5c/browser/themes/shared/tabbrowser/loading.svg
If you need SVG to be able to style the color of the spinner then I think the only way to do this is to get rid of the gradient, and instead make the contents of the mask a raster image that actually contains a circumferential gradient. The mask will obviously have to be high enough resolution to work with all resolutions that we need it to.
And please get rid of the SVG DOCTYPE cruft. It makes the XML parser do a bunch of work that is unnecessary here and will slow down loading of the image.
I'm not sure we're on the same page as to what this bug is about. My understand is that the inner and outer edges of the rotating gradient are fuzzy rather than the gradient itself.
Ah, I see. So I'm just seeing different ugliness then. ;) I'm not sure there's much that can be done about the anti-aliasing. Obviously when we render to the layer's surface we anti-alias, and that anti-aliasing is not going to be ideal when the layer is rotated (or no longer grid aligned). needinfo'ing Matt for his thoughts from a gfx point of view. Maybe there's a way to request that the layer uses a higher resolution or something.
[Tracking Requested - why for this release]: Regression I can confirm that throbbers appears blurry, but it's visible even on non HiDPI displays.
Severity: normal → major
Has Regression Range: --- → yes
Has STR: --- → yes
OS: Unspecified → All
Hardware: Unspecified → All
Version: unspecified → 55 Branch
(In reply to Mike Conley (:mconley) (QF workweek until March 31) from comment #0) > Created attachment 8852025 [details] > fuzzy-spinners.mov Protip: System Prefs -> A11Y -> Zoom, uncheck "smooth images". Makes it easier to see whats happening with the Actual Pixels™. > shorlander noticed this, and I offered to file it. Essentially, this appears > to be the result of us rasterizing the spinner once to a layer (with > anti-aliasing artifacts), and then rotating that layer (with the > anti-aliasing artifacts). This sounds familiar, ahh: bug 1008324 comment 8. AIUI this ends up just rotating the bitmap, and that's not a visually perfect operation, especially for tiny images with high contrast... 6 degrees per frame (at 60fps), and rotating a 16x16 or 32x32 bitmap isn't going to look good. Maybe we want to use the same animation technique here as we're going to use elsewhere in Photon? A sprite sheet of frames, and use CSS to animate between them. (Which would also conveniently remove the limitation that our throbber design is limited to a simple rotation.) I suppose one obvious downside is that these animations run longer (~1 second), so that's ~60 frames of SVG. But maybe not a big deal?
(In reply to Justin Dolske [:Dolske] from comment #9) > Maybe we want to use the same animation technique here as we're going to use > elsewhere in Photon? A sprite sheet of frames, and use CSS to animate > between them. (Which would also conveniently remove the limitation that our > throbber design is limited to a simple rotation.) I suppose one obvious > downside is that these animations run longer (~1 second), so that's ~60 > frames of SVG. But maybe not a big deal? I filed bug 1353422 for this.
The anti-aliasing spinning with the throbbers aren't helping, but I believe (having spoken to mattwoodrow last week), that there are some changes that could be made to the SVG to make this better. mattwoodrow, can you remind me what those suggestions were?
ensure that the SVGs are drawn on 0.5 pixel boundaries as much as possible per https://www.cairographics.org/FAQ/#sharp_lines You could try turning antialiasing off for some shapes depending on your requirements. shape-rendering="crispEdges" but you might think that the result of that is worse than the anti-aliasing issue. Note that you can apply this on a per element basis or to the SVG as a whole via the root element.
Not sure we have many other options other than what has been suggested already. crispEdges is worth a try for sure. We don't have a way to request a higher resolution for a given layer unfortunately.
I tried to double the size of the source image and scale it down along with the rotation in one go. Unfortunately this doesn't work as apparently @keyframes can't handle multiple transforms... a rather unfortunate limitation. I haven't tried shape-rendering="crispEdges" so far. It makes no sense to me that rotating the pixelated source would be a net improvement here.
Fixed by backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/d0f1f01d369641c422db403eb5550b40cae59889 https://hg.mozilla.org/integration/mozilla-inbound/rev/5110f7c040365cacc750021b6b2aeeaa74728e7e
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.