Last Comment Bug 706690 - Content scaling via CSS transform is jagged while layers are active
: Content scaling via CSS transform is jagged while layers are active
Status: RESOLVED FIXED
[inbound]
:
Product: Firefox for Android
Classification: Client Software
Component: General (show other bugs)
: unspecified
: ARM Android
: P2 normal (vote)
: ---
Assigned To: Chris Peterson [:cpeterson]
:
Mentors:
: 707790 707878 708287 708749 721790 (view as bug list)
Depends on: 870192
Blocks:
  Show dependency treegraph
 
Reported: 2011-11-30 15:21 PST by Naoki Hirata :nhirata (please use needinfo instead of cc)
Modified: 2013-05-10 14:24 PDT (History)
13 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
11+


Attachments
Fix unnecessary scale clamping (4.02 KB, patch)
2011-12-07 04:02 PST, Chris Lord [:cwiiis]
no flags Details | Diff | Review
Fix unnecessary scale clamping (2.19 KB, patch)
2011-12-07 04:04 PST, Chris Lord [:cwiiis]
roc: review+
pwalton: feedback+
Details | Diff | Review

Description Naoki Hirata :nhirata (please use needinfo instead of cc) 2011-11-30 15:21:33 PST
1. go to ftp.mozilla.org/pub/mozilla.org/mobile
2. pan

Expected: the letters/words should not get pixalated while panning
Actual: the letters temporarily get pixalated once the panning movement stops and then snaps to being antialiased.
Comment 1 Naoki Hirata :nhirata (please use needinfo instead of cc) 2011-11-30 15:22:24 PST
occurs on HTC Flyer : 20111130 build
Comment 2 Patrick Walton (:pcwalton) 2011-12-02 20:22:59 PST
Simple STR: Go to any site and pan around. You will see the text become jagged and then look nice a few ms later.

The jagged invalidation is triggered by the setting of the transform, and the nice-looking invalidation is triggered by the layers becoming inactive 100 ms afterward:

#0  0x728eac40 in nsWindow::Invalidate(nsIntRect const&, bool) ()
   from /Users/pwalton/Source/moz/birch/objdir-ff-android/dist/bin/libxul.so
...
   from /Users/pwalton/Source/moz/birch/objdir-ff-android/dist/bin/libxul.so
#16 0x723684f4 in nsIFrame::InvalidateFrameSubtree() ()
   from /Users/pwalton/Source/moz/birch/objdir-ff-android/dist/bin/libxul.so
#17 0x7236cc94 in LayerActivityTracker::NotifyExpired(LayerActivity*) ()
   from /Users/pwalton/Source/moz/birch/objdir-ff-android/dist/bin/libxul.so
#18 0x72367226 in nsExpirationTracker<LayerActivity, 4u>::AgeOneGeneration() ()
   from /Users/pwalton/Source/moz/birch/objdir-ff-android/dist/bin/libxul.so
#19 0x72367cf4 in nsExpirationTracker<LayerActivity, 4u>::TimerCallback(nsITimer
Comment 3 Patrick Walton (:pcwalton) 2011-12-02 20:25:12 PST
roc or Matt, do you have any idea what could be causing this? Zooming in shows that the text does not seem to be antialiased properly during the jagged paint.

We have a CSS scale transform and a translation placed on the browser element. The translation is always to pixel boundaries. Temporarily removing the scale transform fixes the issue, so this might have something to do with non-integer transforms.
Comment 4 Patrick Walton (:pcwalton) 2011-12-02 20:28:11 PST
According to the output from PrintDisplayList(), the display lists are identical.
Comment 5 Patrick Walton (:pcwalton) 2011-12-02 21:54:15 PST
Added some logging to nsTextFrame::PaintText(). Although the frame origins are identical between the ugly and nice text runs, the text baseline points (as computed by nsLayoutUtils::GetSnappedBaselineY()) are different. For the ugly paints they're snapped to pixel boundaries, so we have e.g. a baseline point like (2940,3600) for frame origin (2940,1327). For the nice paints the baseline point for the same frame origin is (2940,3552.5).
Comment 6 Patrick Walton (:pcwalton) 2011-12-02 22:49:17 PST
I believe this is because the Cairo matrix for the ugly paints is [ 0.5 0 ] [ 0 0.5 ], while the Cairo matrix for the nice paints is [ 0.489796 0 ] [ 0 0.489796 ], as I would expect. The scale that has been set is 0.489796.
Comment 7 Patrick Walton (:pcwalton) 2011-12-03 00:04:34 PST
One of the container layers here in the ugly case has the matrix [ 0.979592 0 ] [ 0 0.979592 ], which may be relevant since 0.5 = 0.489796 / 0.979592.
Comment 8 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-12-03 00:40:30 PST
This may simply be a matter of needing to make ChooseScaleAndSetTransform smarter in FrameLayerBuilder.cpp.

We're doing this:
    //Scale factors are normalized to a power of 2 to reduce the number of resolution changes
    scale = transform2d.ScaleFactors(true);
    // For frames with a changing transform that's not just a translation,
    // round scale factors up to nearest power-of-2 boundary so that we don't
    // keep having to redraw the content as it scales up and down. Rounding up to nearest
    // power-of-2 boundary ensures we never scale up, only down --- avoiding
    // jaggies. It also ensures we never scale down by more than a factor of 2,
    // avoiding bad downscaling quality.
    gfxMatrix frameTransform;
    if (aContainerFrame->AreLayersMarkedActive(nsChangeHint_UpdateTransformLayer) &&
        aTransform &&
        (!aTransform->Is2D(&frameTransform) || frameTransform.HasNonTranslationOrFlip())) {
      scale.width = gfxUtils::ClampToScaleFactor(scale.width);
      scale.height = gfxUtils::ClampToScaleFactor(scale.height);
    } else {

It sounds like we need to not clamp the scale if the new scale matches the old scale of the layer, or something like that.
Comment 9 Patrick Walton (:pcwalton) 2011-12-03 00:53:36 PST
Found that just before you posted :) I can confirm that this issue is resolved when commenting out that code.

This issue also occurs right after zooming in, in which case the new scale won't match the old scale, but we still want to render without jaggies. So I'm not sure what to do there. In the limit maybe we could just #ifdef this logic out when MOZ_JAVA_COMPOSITOR is on. Unless you have a better solution, of course.
Comment 10 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-12-03 01:12:29 PST
(In reply to Patrick Walton (:pcwalton) from comment #9)
> This issue also occurs right after zooming in, in which case the new scale
> won't match the old scale, but we still want to render without jaggies. So
> I'm not sure what to do there. In the limit maybe we could just #ifdef this
> logic out when MOZ_JAVA_COMPOSITOR is on. Unless you have a better solution,
> of course.

We really don't want to comment this out for you since that will mean that during zooming, we'll constantly re-render the content at every new zoom factor, which will make for slow zooming. It will also affect the handling of CSS transforms in Web content.

What we can do:
1) don't clamp the scale when the new desired scale matches the old desired scale. You can read the current transform on aLayer to check that.
2) Have some way of immediately marking the transform is inactive when you end a zoom or pan.
Comment 11 Patrick Walton (:pcwalton) 2011-12-03 01:13:59 PST
Those both sound good to me.
Comment 12 Naoki Hirata :nhirata (please use needinfo instead of cc) 2011-12-06 12:02:18 PST
*** Bug 707878 has been marked as a duplicate of this bug. ***
Comment 13 Chris Lord [:cwiiis] 2011-12-07 04:02:09 PST
Created attachment 579652 [details] [diff] [review]
Fix unnecessary scale clamping

I only just realised this was assigned to me, so here's a patch that implements suggestion 1, as I've understood from the comments.
Comment 14 Chris Lord [:cwiiis] 2011-12-07 04:02:48 PST
Just a note, I got feedback+ on the addition of a != operator on gfx3DMatrix from Matt Woodrow.
Comment 15 Chris Lord [:cwiiis] 2011-12-07 04:04:29 PST
Created attachment 579653 [details] [diff] [review]
Fix unnecessary scale clamping

Erk, and of course the patch doesn't even use it (remnants from an earlier revision) - sorry for churn, here's the patch with that part removed.
Comment 16 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-12-07 13:34:42 PST
Comment on attachment 579653 [details] [diff] [review]
Fix unnecessary scale clamping

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

::: layout/base/FrameLayerBuilder.cpp
@@ +1705,5 @@
> +      gfxMatrix oldFrameTransform2d;
> +      if (aLayer->GetTransform().Is2D(&oldFrameTransform2d)) {
> +        gfxSize oldScale = oldFrameTransform2d.ScaleFactors(true);
> +        if (oldScale == scale || oldScale == gfxSize(1.0, 1.0))
> +          clamp = false;

{} around the if body
Comment 18 Ed Morley [:emorley] 2011-12-08 08:44:54 PST
https://hg.mozilla.org/mozilla-central/rev/1e0a2b58599c
Comment 19 Brad Lassey [:blassey] (use needinfo?) 2011-12-08 10:39:56 PST
*** Bug 707790 has been marked as a duplicate of this bug. ***
Comment 20 Brad Lassey [:blassey] (use needinfo?) 2011-12-08 10:59:13 PST
*** Bug 708287 has been marked as a duplicate of this bug. ***
Comment 21 Mark Finkle (:mfinkle) (use needinfo?) 2011-12-08 11:49:53 PST
*** Bug 708749 has been marked as a duplicate of this bug. ***
Comment 22 Patrick Walton (:pcwalton) 2011-12-24 15:21:46 PST
Comment on attachment 579653 [details] [diff] [review]
Fix unnecessary scale clamping

f+ to get this out of my queue.
Comment 23 Doug Turner (:dougt) 2012-01-30 08:41:15 PST
I continue to see something like this in nightly.  Patrick, could this have regressed?
Comment 24 Patrick Walton (:pcwalton) 2012-01-30 09:26:48 PST
It was never actually fixed, in my experience. I think we need option (2) above.
Comment 25 Chris Lord [:cwiiis] 2012-01-30 10:15:56 PST
Let's reopen this then. I'm unasssigning from myself for now - please feel free to take it, but I will if I have time after dealing with other issues.
Comment 26 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-01-30 19:34:41 PST
I downloaded the latest nightly and tried it on my phone using the steps in comment #0. I don't seem to see the bug.
Comment 27 Doug Turner (:dougt) 2012-02-02 09:56:47 PST
*** Bug 721790 has been marked as a duplicate of this bug. ***
Comment 28 Patrick Walton (:pcwalton) 2012-02-17 21:59:56 PST
Marking as fixed; with maple there is nothing more to do here.

Note You need to log in before you can comment on or make changes to this bug.