Closed
Bug 1153539
Opened 9 years ago
Closed 9 years ago
"Download finished"-animation is all blocky/low-res with OMTA
Categories
(Core :: Layout, defect)
Core
Layout
Tracking
()
RESOLVED
FIXED
mozilla40
Tracking | Status | |
---|---|---|
firefox40 | --- | fixed |
People
(Reporter: elbart, Assigned: dbaron)
References
Details
(Keywords: regression)
Attachments
(4 files, 2 obsolete files)
11.58 KB,
image/png
|
Details | |
1.42 KB,
patch
|
roc
:
review+
|
Details | Diff | Splinter Review |
4.19 KB,
patch
|
roc
:
review+
|
Details | Diff | Splinter Review |
7.24 KB,
patch
|
roc
:
review+
|
Details | Diff | Splinter Review |
The blue arrow-animation, which is played when a download is finished, is very blocky with OMTA enabled.
Blocks: enable-omt-animations
Summary: "Download finished"-animation is all blocky/low-res → "Download finished"-animation is all blocky/low-res with OMTA
Updated•9 years ago
|
Status: UNCONFIRMED → NEW
Component: CSS Parsing and Computation → Layout
Ever confirmed: true
Keywords: regression
Comment 1•9 years ago
|
||
Sounds a lot like bug 1122526...
(In reply to Seth Fowler [:seth] from comment #1) > Sounds a lot like bug 1122526... It's still happening with the try-build of bug 947753 comment 27.
Assignee | ||
Comment 3•9 years ago
|
||
This has the advantage of being easily-reproducable on Linux.
Assignee | ||
Comment 4•9 years ago
|
||
Poking around a little bit, I've found that: without OMTA, we go through ImageHost::Composite 2 times with both pictureRect and textureSize 16x16, and then 8 times with both 128x128. with OMTA, we go through once with both 16x16 and once with both 128x128.
Assignee | ||
Comment 5•9 years ago
|
||
Well, after a bunch of debugging in the wrong place, this comes down to nsLayoutUtils::ComputeSuitableScaleForAnimation returning (1.0, 1.0) when that's clearly the wrong answer (and the non-OMTA path is zooming through (2.0, 2.0), (4.0, 4.0), and (8.0, 8.0)). (I haven't looked into why yet, but it certainly narrows down the problem.)
Assignee | ||
Comment 6•9 years ago
|
||
I have no idea why: https://hg.mozilla.org/mozilla-central/file/7723b15ea695/layout/base/nsLayoutUtils.cpp#l482 does what it does. In this case, aMaxScale is 8, aMinScale is 1, so it returns 1, which means we rasterize at the *smallest* size of the transform animation. I don't know why we wouldn't just want aMaxScale.
Assignee | ||
Comment 7•9 years ago
|
||
I think the solution is just to back out https://hg.mozilla.org/mozilla-central/rev/940b00462eb7 although I suppose I should read the bug to figure out why we did that.
Blocks: 972310
Assignee | ||
Comment 8•9 years ago
|
||
The original discussion goes from bug 945082 comment 82 to bug 945082 comment 106. I don't see why the approach there -- of examining the range of scales in the transform animation and using the point within that range closest to a 1.0 scale -- makes sense at all. It's making an implicit decision that you want poor rasterization for any animation that happens to animate scales greater than 1, but good rasterization for an animation that involves scales less than 1. That doesn't make any sense as an API to expose to the Web, and it's clearly giving unexpected results here (and means that OMT animations are a major quality regression relative to what we do without OMT animations) It seems like Firefox OS wants to opt in to poor (low end of the animation) rasterization for one particular animation. I think that should be done in some other way, and I propose to simply back out bug 972310.
Comment 9•9 years ago
|
||
Do we know how OMTA is related to this? I agree that the current behavior doesn't make sense.
Assignee | ||
Comment 10•9 years ago
|
||
(Note that it's not a straight backout because I want to leave https://hg.mozilla.org/mozilla-central/rev/ab404d8e7d88 .)
Assignee | ||
Comment 11•9 years ago
|
||
(In reply to Markus Stange [:mstange] from comment #9) > Do we know how OMTA is related to this? This code is only used when an element has OMT animations.
Assignee | ||
Comment 12•9 years ago
|
||
I think this will also fix bug 1122526. Then we can (at least for now) use mstange's original patch on bug 947753 (with improved comments), and I think that will be sufficient for having OMT animations at least on aurora, if not release.
Assignee | ||
Comment 13•9 years ago
|
||
This reverts the changes in changeset 940b00462eb7 (bug 972310), although it isn't a straight backout because the code was significantly refactored in ab404d8e7d88 (and I also fixed the coment formatting in FrameLayerBuilder.cpp and improved the comment wording in nsLayoutUtils.h). Prior to bug 972310, ChooseScaleAndSetTransform in FrameLayerBuilder.cpp would call GetMaximumAnimatedScale, which led to rasterizing (for the entire duration of the off-main-thread animation) at the largest scale present over the course of that animation, which avoids pixelation. This is roughly equivalent to the quality we get without off-main-thread animation, where we round up the scale to the nearest power of 2 and rerasterize as the animation progresses. Bug 972310 made us instead choose the scale within the range of the animation that is closest to 1.0. This means that animations that involve only scales smaller than 1.0 rasterize at the best quality, whereas animations involving only scales larger than 1.0 rasterize at the worst quality. This doesn't make sense as an API; the quality of the animation shouldn't be a function of what numbers are being used for the transform (especially considering how those numbers interact with other transforms). This change was made to optimize a particular animation in Firefox OS, where unlocking the lockscreen zooms in (to 2x) and fades out at the same time. This animation will need to be optimized some other way.
Attachment #8600114 -
Flags: review?(roc)
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → dbaron
Status: NEW → ASSIGNED
Andreas wanted to just use 1x scale all the time, like Safari does (or did at the time): https://bugzilla.mozilla.org/show_bug.cgi?id=945082#c92 So our currently policy is already strictly better quality than Safari's. Using max-scale has two problems: 1) extreme memory usage in some situations, because at one end of the animation the element might be completely visible with a small scale, but at the other end of the animation the element be scaled much bigger than the screen, so combining the latter scale with the former visibility region requires rasterizing to a size that's arbitrarily bigger than the screen 2) rasterizing those very large buffers can be very slow, especially in comparison to a browser like Safari that uses 1x. So I'm not all that comfortable with just backing that patch out.
> Andreas wanted to just use 1x scale all the time, like Safari does (or did at the time):
I mention this because that's the implicit "API exposed to the Web" for a long time.
Flags: needinfo?(dbaron)
Assignee | ||
Comment 16•9 years ago
|
||
So shouldn't we also do that for non-OMTA, then?
Flags: needinfo?(dbaron)
Yes, we probably should align OMTA with non-OMTA here. We could fix this bug by improving our heuristics. E.g. we could compute just the max-scale, and use largest scale <= max-scale that lets the scaled visible region fit in the screen size.
Assignee | ||
Comment 18•9 years ago
|
||
Attachment #8601504 -
Flags: review?(roc)
Assignee | ||
Comment 19•9 years ago
|
||
This fixes bug 1153539 with the Firefox download arrow being pixellated (verified by testing locally). I also confirmed what happens on a flame device with the transform unlocking the Firefox OS homescreen (bug 945082 / bug 972310). For that transform (which had a maximum scale of 2), I see four calls to GetSuitableScale: GetSuitableScale: aMaxScale=2.000000, displayVisibleRatio=1.000000 GetSuitableScale: aMaxScale=2.000000, displayVisibleRatio=1.000586 GetSuitableScale: aMaxScale=2.000000, displayVisibleRatio=1.000000 GetSuitableScale: aMaxScale=2.000000, displayVisibleRatio=1.000586 (Presumably the first and third are for width, and the second and fourth are for height.) I believe this shows that bug 972310 will remain fixed with this patch. I chose to use the pres context's visible area rather than the screen size because it seemed more appropriate, and also because (if memory serves correctly) it's much cheaper to get.
Attachment #8601505 -
Flags: review?(roc)
Assignee | ||
Comment 20•9 years ago
|
||
Attachment #8601506 -
Flags: review?(roc)
Assignee | ||
Comment 21•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d21eadfaf558
Assignee | ||
Updated•9 years ago
|
Attachment #8600114 -
Attachment is obsolete: true
Attachment #8600114 -
Flags: review?(roc)
Attachment #8601504 -
Flags: review?(roc) → review+
Comment on attachment 8601506 [details] [diff] [review] patch 3 - Stop computing minimum scale for off-main-thread animations of transforms, since it's no longer used Review of attachment 8601506 [details] [diff] [review]: ----------------------------------------------------------------- r+ if we end up doing this
Attachment #8601506 -
Flags: review?(roc) → review+
Comment on attachment 8601505 [details] [diff] [review] patch 2 - Compute scale for rasterizing off-main-thread transform animations based on maximum scale and ratio to display size Review of attachment 8601505 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/base/nsLayoutUtils.cpp @@ +488,5 @@ > + float(aVisibleDimension); > + // We want to rasterize based on the largest scale used during the > + // transform animation, unless that would make us rasterize something > + // larger than the screen. > + return std::min(aMaxScale, displayVisibleRatio); I think we actually need to take the minimum scale into account here. As written, this could reduce the maximum scale below the current minimum scale (maintaining the invariant that the visible dimensions are <= the display dimensions is hard), but we don't want to do that. So I think we should take the max of this result and the minimum scale. That would make patch 3 obsolete.
Attachment #8601505 -
Flags: review?(roc) → review-
Assignee | ||
Comment 24•9 years ago
|
||
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #23) > I think we actually need to take the minimum scale into account here. As > written, this could reduce the maximum scale below the current minimum scale > (maintaining the invariant that the visible dimensions are <= the display > dimensions is hard), but we don't want to do that. So I think we should take > the max of this result and the minimum scale. > > That would make patch 3 obsolete. Seems reasonable. (Although, is the only way it could happen if we somehow have a visible area (for painting) larger than the pres context's visible area? I guess that can happen with some things we do.)
Assignee | ||
Comment 25•9 years ago
|
||
This fixes bug 1153539 with the Firefox download arrow being pixellated (verified by testing locally). I also confirmed what happens on a flame device with the transform unlocking the Firefox OS homescreen (bug 945082 / bug 972310). For that transform (which had a maximum scale of 2), I see four calls to GetSuitableScale: GetSuitableScale: aMaxScale=2.000000, displayVisibleRatio=1.000000 GetSuitableScale: aMaxScale=2.000000, displayVisibleRatio=1.000586 GetSuitableScale: aMaxScale=2.000000, displayVisibleRatio=1.000000 GetSuitableScale: aMaxScale=2.000000, displayVisibleRatio=1.000586 (Presumably the first and third are for width, and the second and fourth are for height.) I believe this shows that bug 972310 will remain fixed with this patch. I chose to use the pres context's visible area rather than the screen size because it seemed more appropriate, and also because (if memory serves correctly) it's much cheaper to get.
Attachment #8601786 -
Flags: review?(roc)
Assignee | ||
Updated•9 years ago
|
Attachment #8601505 -
Attachment is obsolete: true
Attachment #8601786 -
Flags: review?(roc) → review+
Comment 26•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/868b584b3eac https://hg.mozilla.org/integration/mozilla-inbound/rev/8fccf4962f1a
Comment 27•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/868b584b3eac https://hg.mozilla.org/mozilla-central/rev/8fccf4962f1a
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox40:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
You need to log in
before you can comment on or make changes to this bug.
Description
•