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)
Tracking
(firefox11 fixed, fennec11+)
RESOLVED
FIXED
People
(Reporter: nhirata, Assigned: cpeterson)
References
Details
(Whiteboard: [inbound])
Attachments
(1 file, 1 obsolete file)
(deleted),
patch
|
roc
:
review+
pcwalton
:
feedback+
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Comment 1•13 years ago
|
||
occurs on HTC Flyer : 20111130 build
Updated•13 years ago
|
Assignee: nobody → chrislord.net
Priority: -- → P2
Updated•13 years ago
|
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
Comment 2•13 years ago
|
||
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•13 years ago
|
||
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•13 years ago
|
||
According to the output from PrintDisplayList(), the display lists are identical.
Comment 5•13 years ago
|
||
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•13 years ago
|
||
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•13 years ago
|
||
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.
Comment 9•13 years ago
|
||
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.
Comment 11•13 years ago
|
||
Those both sound good to me.
Comment 13•13 years ago
|
||
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)
Comment 14•13 years ago
|
||
Just a note, I got feedback+ on the addition of a != operator on gfx3DMatrix from Matt Woodrow.
Comment 15•13 years ago
|
||
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+
Comment 17•13 years ago
|
||
Whiteboard: [inbound]
Comment 18•13 years ago
|
||
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Comment 22•13 years ago
|
||
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+
Updated•13 years ago
|
tracking-fennec: --- → 11+
Updated•13 years ago
|
status-firefox11:
--- → fixed
Comment 23•13 years ago
|
||
I continue to see something like this in nightly. Patrick, could this have regressed?
Comment 24•13 years ago
|
||
It was never actually fixed, in my experience. I think we need option (2) above.
Comment 25•13 years ago
|
||
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 | ||
Updated•13 years ago
|
Assignee: nobody → cpeterson
Comment 28•13 years ago
|
||
Marking as fixed; with maple there is nothing more to do here.
Status: REOPENED → RESOLVED
Closed: 13 years ago → 13 years ago
Resolution: --- → FIXED
Updated•4 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•