Closed Bug 1153539 Opened 5 years ago Closed 5 years ago

"Download finished"-animation is all blocky/low-res with OMTA

Categories

(Core :: Layout, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla40
Tracking Status
firefox40 --- fixed

People

(Reporter: elbart, Assigned: dbaron)

References

Details

(Keywords: regression)

Attachments

(4 files, 2 obsolete files)

The blue arrow-animation, which is played when a download is finished, is very blocky with OMTA enabled.
Summary: "Download finished"-animation is all blocky/low-res → "Download finished"-animation is all blocky/low-res with OMTA
Status: UNCONFIRMED → NEW
Component: CSS Parsing and Computation → Layout
Ever confirmed: true
Keywords: regression
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.
This has the advantage of being easily-reproducable on Linux.
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.
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.)
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.
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
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.
Do we know how OMTA is related to this?

I agree that the current behavior doesn't make sense.
(Note that it's not a straight backout because I want to leave https://hg.mozilla.org/mozilla-central/rev/ab404d8e7d88 .)
(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.
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.
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: 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)
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.
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)
Attachment #8600114 - Attachment is obsolete: true
Attachment #8600114 - Flags: review?(roc)
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-
(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.)
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)
Attachment #8601505 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/868b584b3eac
https://hg.mozilla.org/mozilla-central/rev/8fccf4962f1a
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Depends on: 1163817
You need to log in before you can comment on or make changes to this bug.