Closed Bug 706690 Opened 13 years ago Closed 13 years ago

Content scaling via CSS transform is jagged while layers are active

Categories

(Firefox for Android Graveyard :: General, defect, P2)

ARM
Android
defect

Tracking

(firefox11 fixed, fennec11+)

RESOLVED FIXED
Tracking Status
firefox11 --- fixed
fennec 11+ ---

People

(Reporter: nhirata, Assigned: cpeterson)

References

Details

(Whiteboard: [inbound])

Attachments

(1 file, 1 obsolete file)

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.
occurs on HTC Flyer : 20111130 build
Assignee: nobody → chrislord.net
Priority: -- → P2
Summary: panning around on a ftp site causes the letters to temporarily be pixalated. → Content scaling via CSS transform is jagged while layers are active
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
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.
According to the output from PrintDisplayList(), the display lists are identical.
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).
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.
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.
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.
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.
(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.
Those both sound good to me.
Attached patch Fix unnecessary scale clamping (obsolete) (deleted) — Splinter Review
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.
Attachment #579652 - Flags: review?(roc)
Attachment #579652 - Flags: feedback?(pwalton)
Just a note, I got feedback+ on the addition of a != operator on gfx3DMatrix from Matt Woodrow.
Attached patch Fix unnecessary scale clamping (deleted) — Splinter Review
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.
Attachment #579652 - Attachment is obsolete: true
Attachment #579652 - Flags: review?(roc)
Attachment #579652 - Flags: feedback?(pwalton)
Attachment #579653 - Flags: review?(roc)
Attachment #579653 - Flags: feedback?(pwalton)
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
Attachment #579653 - Flags: review?(roc) → review+
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Comment on attachment 579653 [details] [diff] [review] Fix unnecessary scale clamping f+ to get this out of my queue.
Attachment #579653 - Flags: feedback?(pwalton) → feedback+
tracking-fennec: --- → 11+
I continue to see something like this in nightly. Patrick, could this have regressed?
It was never actually fixed, in my experience. I think we need option (2) above.
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.
Assignee: chrislord.net → nobody
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
Assignee: nobody → cpeterson
Marking as fixed; with maple there is nothing more to do here.
Status: REOPENED → RESOLVED
Closed: 13 years ago13 years ago
Resolution: --- → FIXED
Depends on: 870192
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: