Closed Bug 630743 Opened 9 years ago Closed 9 years ago

Snap displayport upates to device pixels

Categories

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

ARM
Android
defect

Tracking

(fennec2.0+)

VERIFIED FIXED
Tracking Status
fennec 2.0+ ---

People

(Reporter: kbrosnan, Assigned: stechz)

References

Details

(Keywords: polish)

Attachments

(2 files)

This seems to be a general issue. Opening any page and scrolling can result in a slight wiggle of text after scrolling has been completed. Some lines of text move by about 1px.

Mozilla/5.0 (Android; Linux armv7l; rv:2.0b11pre) Gecko/20110201 Firefox/4.0b11pre Fennec/4.0b5pre ID:20110201042403
HTC G2 Android 2.2
Video showing this in a couple spots http://dl.dropbox.com/u/686313/scrolling.webm
Priority: -- → P2
tracking-fennec: --- → 2.0+
It might be possible that we end up setting resolution/scroll offsets that cause the vertical space between textruns to have a sub-pixel fraction, then when rendered the textruns are being snapped up/down to the nearest pixel position "randomly", differently.  If so, a simple fix/workaround might be to ensure the scroll offset is only moved by whole device pixels, so that the rounding always goes the same way.  Just a guess though.
Assignee: nobody → crowderbt
See Also: → 630743, 635035
To clarify comment 2, my guess is, say an "a" glyph somehow ends up positioned 10.75 device pixels above a "b" glyph.  If "a" is draw at device pixel y=0 and snapped, it will snap to y=0, and if "b" is snapped it will snap to y=11.  But if "a" is "scrolled" to device pixel y=1.5, and both are snapped again, "a" will be drawn at y=2 and "b" at y=12.  So the distance between the glyphs would shrink by 1 device pixel after the scroll.  This is still just a guess, I don't know if that can happen in our code.

I'm not at all familiar with the text layout/drawing code, though Rob is :), but some things to do/check initially would be
 (1) Find a small+simple+reliable test case with STR for fennec proper
 (2) Log the resolution, displayport, and scroll offsets from before a text "jump" and after
 (3) Get the test working in test-ipcbrowser.xul with steps like bug 634759 comment 8
 (4) Log paint info from before a jump and after, MOZ_DUMP_PAINT_LIST=1

That should be enough info to figure out what to do next.
(In reply to comment #2)
> If so, a simple fix/workaround might be to
> ensure the scroll offset is only moved by whole device pixels, so that the
> rounding always goes the same way.  Just a guess though.

That's what we want.
This kind of problem might be the underlying cause of the few-pixel jitter in buffer sizes in bug 635035.
Not sure if this is reproducible using test-ipcbrowser.xul, since that chrome doesn't seem to ever refresh areas with checkerboarding during scrolling...  Am I doing something wrong?
mbrubeck, you put this bug in its own "see also", I'm assuming that you meant some other bug
Fennec does not currently snap displayport to device pixels, at least on purpose. Would it be okay to pass in floats with fractions that snap to device pixels, or do I need to find integers that somehow snap to device pixels? The latter is harder--we may want to ensure we are preserving the float fraction.
I'll get a mobile patch up for this soon. It should be very easy to do.
As we discussed on IRC, here's approximately what needs to happen
 - find a desired displayport' in device-pixel coordinates
 - from that, compute displayport in float CSS pixels given the desired resolution

A similar computation needs to happen for scroll offsets.  It looks like window.scrollTo() only allows integer CSS pixels, which isn't going to be sufficient.  That should be relatively easy to fix though.

The displayport components are rounded to app units currently.  If that turns out not to provide enough precision, we can investigate the best fix.

If the platform knows the frontend is shooting for device-pixel snapping, it can snap away and win should be had by all.
Claiming this and making this the frontend part of the bug. We need to file a followup for the platform to fix scrollTo.
Assignee: crowderbt → ben
Status: NEW → ASSIGNED
Summary: Text shifts slightly after scrolling → Snap displayport upates to device pixels
Here's a relatively simple test case

 (1) Load http://people.mozilla.com/~cjones/test-630743.html
 (2) Pan the URL bar out of view
 (3) Press "dance"

This uses window.scrollBy(), so may be more appropriate for bug 637852.  I'll make another test with "pan targets" one can manually pan to.  (I also found a couple of other interesting bugs with this.)
Attached file Test
The previous test didn't ensure that the target scroll offsets fell on device pixels.  This version does that to the best of my ability; the scale is set to 0.75, so there should be 4 CSS pixels per 3 device pixels.  The test moves the scroll offset by 40 CSS pixels right/down, which should translate to 30 device pixels.  On my galaxy tab in portrait, I see dancing; if the displayport aligned to device pixels, I don't think we should see dancing text.

Oddly, sometimes this test fails to scroll vertically, as if the window.scrollTo(0, +dy) is being ignored.
Oh -- STR for comment 13 are the same as in comment 12.  It's important the URL bar be scroll out of view so that the initial scroll offset falls on device pixel <0, 0>.
(Although it shouldn't matter since the offset is only moved by device pixels; text positioning should round the same way.)
Ben, how is this patch looking?  We should land this and bug 637852 ASAP so we have a non-glitchy baseline for the followup optimizations (and a solid base to ship from if the optimizations slip 4.0).
Comment on attachment 516446 [details] [diff] [review]
Snap displayport updates to device pixels

Ugly, but it works.
Attachment #516446 - Flags: review?(mbrubeck)
Attachment #516446 - Flags: review?(mbrubeck) → review+
Whiteboard: [needs-landing]
Seems to make the dancing text go away on my tab in the test above.
Actually I tested before http://hg.mozilla.org/mobile-browser/rev/0811c1cc3170 and then with this patch after.  Rechecking without that change.
Comment on attachment 516446 [details] [diff] [review]
Snap displayport updates to device pixels

Text is still jumping in the test with this patch applied before http://hg.mozilla.org/mobile-browser/rev/0811c1cc3170.  Either the test is wrong or the patch isn't quite right.
Attachment #516446 - Flags: feedback-
Matt reports that the test is not accounting for the initial-scale being multiplied by the frontend.
Comment on attachment 516446 [details] [diff] [review]
Snap displayport updates to device pixels

With the fixed-up test, on my galaxy tab in portrait, starting from the base rev http://hg.mozilla.org/mobile-browser/rev/52c9d06b9c97
 - without the patch I see text jumping after the test has run for a while, >10sec or so
 - with the patch applied, I don't see any jumping text

This patch definitely fixes some bug.  I vote for landing this and focusing our efforts on bug 637852.
Attachment #516446 - Flags: feedback- → feedback+
http://hg.mozilla.org/mobile-browser/rev/b039de59c004
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
kevin, can you verify the fix?
Verified Mozilla/5.0 (Android; Linux armv7l; rv:2.0b13pre) Gecko/20110317 Firefox/4.0b13pre Fennec/4.0b6pre ID:20110317040030

Note bug 637852 that has some followup work. Already marked 2.0next+.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.