Closed Bug 1068881 Opened 10 years ago Closed 9 years ago

Graphic glitch when scrolling page if zoom level <= 30%

Categories

(Core :: Graphics: ImageLib, defect)

34 Branch
x86_64
Windows 7
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla39
Tracking Status
firefox33 --- unaffected
firefox34 + wontfix
firefox35 + wontfix
firefox36 + wontfix
firefox37 + wontfix
firefox38 --- wontfix
firefox38.0.5 --- wontfix
firefox39 --- fixed
b2g-v2.2 --- fixed
b2g-master --- fixed

People

(Reporter: alice0775, Assigned: seth)

References

Details

(Keywords: regression)

Attachments

(5 files, 5 obsolete files)

870 bytes, text/html
Details
55.57 KB, image/png
Details
95.53 KB, image/png
Details
5.59 KB, patch
Details | Diff | Splinter Review
2.20 KB, patch
Details | Diff | Splinter Review
Attached file testcase html
[Tracking Requested - why for this release]: Regression since Nightly35.0a1

Steps To Reproduce:
1. Open attached
2. Scroll with mouse wheel very fast
   OR
   dragging thumb / scroll buttons / arrow keys

Actual Results:
Black bar is drawn.
Text overlaps
Attached image screenshot
Steps To Reproduce:
1. Open attached
2. Zoom out(Ctrl+-)
3. Scroll with mouse wheel very fast
   OR
   dragging thumb / scroll buttons / arrow key
   --- glitch appears

4. Scroll to middle
5. Reload (F5)
   --- glitch appears



Regression window(m-i)
Good:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8e28464849fa
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:35.0) Gecko/20100101 Firefox/35.0 ID:20140914145408
Bad:
https://hg.mozilla.org/integration/mozilla-inbound/rev/606aef8e8c16
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:35.0) Gecko/20100101 Firefox/35.0 ID:20140914152108
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=8e28464849fa&tochange=606aef8e8c16

Regressed by:
606aef8e8c16	Seth Fowler — Bug 1057903 - Refactor RasterImage to use DrawableFrameRef and generally clean up. r=tn


The glitch happens regardless of HWA on/off
This was fixed by Bug 1067207

Bad:
https://hg.mozilla.org/integration/mozilla-inbound/rev/612fedcc26e1
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:35.0) Gecko/20100101 Firefox/35.0 ID:20140916183757
Fixed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4746dd77b34b
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:35.0) Gecko/20100101 Firefox/35.0 ID:20140916190357
Fixed pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=612fedcc26e1&tochange=4746dd77b34b

Fixed by: 4746dd77b34b	Seth Fowler — Bug 1067207 - Compute imgFrame padding in unscaled coordinates. r=tn
Depends on: 1067207
Whiteboard: [fixed by Bug 1067207]
Err, 
Bug 1067207 does not fix the problem completely.

I can still reproduce the problem if Zoom level <= 30%
No longer depends on: 1067207
Whiteboard: [fixed by Bug 1067207]
[Tracking Requested - why for this release]:

So root cause is,

Regression window(m-i)
Good:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3488976ecf0f
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:34.0) Gecko/20100101 Firefox/34.0 ID:20140822131153
Bad:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3d51132a0099
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:34.0) Gecko/20100101 Firefox/34.0 ID:20140822131352
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=3488976ecf0f&tochange=3d51132a0099

Regressed by
3d51132a0099	Seth Fowler — Bug 1043560 - Refactor the imgIContainer::Draw API. r=tn,dholbert,jwatt,mwu,mattwoodrow,roc sr=jrmuize
Blocks: 1043560
No longer blocks: 1057903
Summary: Graphic glitch when scrolling page if zoom level < 100% → Graphic glitch when scrolling page if zoom level <= 30%
Version: 35 Branch → 34 Branch
Seth,

Can you have a look at this?
Flags: needinfo?(seth)
I'll take a look.
I haven't had a chance to debug this yet, but I have a strong feeling that bug 1067191 is caused by the same issue as this bug.
Depends on: 1067191
I probably won't get to this until next week because there are other issues that require urgent attention, but bug 1067191 has landed and will be in the Nightly that gets spun tonight. If anyone CC'd on this bug who runs Windows wants to check whether this bug is fixed now, that would be very helpful.
Attached image bug.png
with https://hg.mozilla.org/mozilla-central/rev/818f353b7aa6  build, i still get some visual corruption when zoomed in a (ctrl+ - 5-6 times) on the testpage.  The whole area just turns black.

The blackened area becomes normal after a few seconds if i click the mouse outside the scroll area.
(In reply to mayankleoboy1 from comment #10)
> i still
> get some visual corruption when zoomed in a (ctrl+ - 5-6 times) on the
> testpage.  The whole area just turns black.

Bummer! I had hoped that bug 1067191 fixed this issue too.

Thank you for checking, though!
Seth - Doesn't look like there's been any progress on this in the last month. Can you still work on this?
Flags: needinfo?(bas)
Confirmed this occurs on windows, with and without HWA, with and without OMTC. Considering the regression window that makes sense. Changing the component to match the reported regression window.
Assignee: nobody → seth
Component: Graphics → ImageLib
Flags: needinfo?(bas)
(In reply to Lawrence Mandel [:lmandel] (use needinfo) from comment #12)
> Seth - Doesn't look like there's been any progress on this in the last
> month. Can you still work on this?

I can. Unfortunately other things have taken precedence but I will circle back around to this soon.

Timothy Nikkel contributed an alternate STR for what is believed to be the same problem:

1. Visit twitter.com (must be logged in)
2. Hit ctrl-+ three times from the default zoom
3. Witness a horizontal black line between the clouds and the solid background color below them

He reproduced this on a non-Retina Mac. No scrolling needed to see the problem.
This is wontfix for 34. It would be good to fix this in 35 before it gets too late into the beta cycle.
Seth - any new news here?  We've got one beta left on Mon Dec 29th to consider a low risk backout for, no speculative patches.  If there's nothing we can do, will have to wontfix for 35 as well.
Flags: needinfo?(seth)
Flags: needinfo?(seth)
Without anything in our last beta this week, this has to be a wontfix for 35 as well.  Leaving the ni? so we can get an update on what to expect for 36.
Just like with 34 & 35, wontfix for 36.
Jet, please let us know what you plan to do wrt this bug? Thanks
Flags: needinfo?(bugs)
Right now I'm hoping to get a fix for this in for 37.
Flags: needinfo?(seth)
Flags: needinfo?(bugs)
37 merges to Beta on Feb 23. It is preferable to get a fix in before then.
There are only 3 Betas left in the 37 cycle. Do you still intend to fix this issue in 37?
Flags: needinfo?(seth)
I wanted to, but time just ran out on me. Let's see if we can get this into 38.
Flags: needinfo?(seth)
Note that now that this bug has shipped in 4 releases (6 months), I'm not going to track any further but I do trust that because this is still affected we will still work on this in 38.
OK, the problem is that the background is only one CSS pixel width and at extreme
zoom values the dest rect gets rounded to zero device pixels in width.

There are multiple ways we could potentially solve this. The cleanest approach
is to ensure that if aDest is nonempty then the dest rect is at least one device
pixel in each dimension. I'm not sure if we can get away with this; it may
trigger other corner cases. But I'd like to try it before going with something
more complicated.

Here's a patch that does that. I won't ask for review yet because I'm not sure
if there are other edge cases this will trip. The try job should help clarify
that:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2a65f975d0a6
By the way, I'm pretty sure the buggy rendering this causes is because we're optimizing away the color under the background image in the display list. (Or never adding it in the first place; not sure of the details of how this works.) That means that when we fail to draw because of the zero dest rect width, we end up just drawing nothing for that area, resulting in glitches.
OK, so the first patch I uploaded looked good on try. This version of the patch
is a little more complete in that it inflates the "scaled dest rect" to at least
one device pixel as well. The scaled dest rect is the version that includes the
scale on the context's transform. We need to inflate this one too, or else we
could end up in a similar situation where the transform scales the dest rect
down enough so that it rounds to zero (even though it's nonempty) and thus we
don't draw anything even though we have a much larger fill rect.

I ran a try job for this one too and it looks nice and green:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d40a8a599754

Ideally we also want a test for each case (full page zoom and transform). I'll
tackle those tomorrow.
Attachment #8583706 - Flags: review?(roc)
Attachment #8582831 - Attachment is obsolete: true
The only thing the previous try job didn't test is the conversion of the |if| that the scaled dest rects are nonempty to an assertion. It's worth checking that that doesn't fire. I pushed a linux-opt-only try job here for that:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2616129ca0c3
Comment on attachment 8583706 [details] [diff] [review]
Inflate the dest rect to at least one device pixel in ComputeSnappedImageDrawingParameters

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

::: layout/base/nsLayoutUtils.cpp
@@ +5845,5 @@
>    nsPoint anchor = aAnchor + (dest.TopLeft() - aDest.TopLeft());
>  
> +  // Inflate the dest rect to include at least one device pixel.
> +  dest.width = std::max(dest.width, aAppUnitsPerDevPixel);
> +  dest.height = std::max(dest.height, aAppUnitsPerDevPixel);

Can we use '1' instead of aAppUnitsPerDevPixel?

@@ +5879,5 @@
>                                : gfxSize(1.0, 1.0);
> +  gfxSize appUnitScaledDest(std::max(dest.width * destScale.width,
> +                                     double(aAppUnitsPerDevPixel)),
> +                            std::max(dest.height * destScale.height,
> +                                     double(aAppUnitsPerDevPixel)));

I'm not sure why we need this. Does some Moz2D backend round nonzero sizes < 1 device pixel to zero? Can we stop it doing that?
Attachment #8583706 - Flags: review?(roc)
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #28)
> Can we use '1' instead of aAppUnitsPerDevPixel?

In that case we'd have to do it after the conversion to device pixels. I'm trying to do it as early as possible to minimize the number of places we need to make the adjustment, since we end up referring to all of these variables (dest, devPixelDest, scaledDest, snappedScaledDest, etc.) later in the code.

> I'm not sure why we need this. Does some Moz2D backend round nonzero sizes <
> 1 device pixel to zero? Can we stop it doing that?

When we're doing other kinds of drawing where we only use the image's intrinsic size (e.g. canvas), conceptually we tile first and then scale down. But if we use that approach when we're also potentially using a downscaled version of the image (due to HQ scaling / downscale-during-decode / etc.), it makes pixel snapping unreliable - we need to introduce additional scaling in the ImageLib drawing code to scale up the downscaled version of the image, which in turn introduces rounding error that caused lots of rendering glitches. What we're trying to do in this code instead is conceptually to determine the final size we're going to draw the image at, and then tile *that*. That lets pixel snapping work correctly with downscaled images.

The downside is that we introduce this problem where if we have a large, nonempty fill rect, but the dest rect ends up rounding to zero width or height in device pixels, we end up drawing nothing.

Actually, having written all this out, I'm changing my mind about how we should solve this. It's probably not a big deal that the dest rect is a fraction of a pixel in width or height, as long as we inflate it when calling OptimalImageSizeForDest(). It's not actually important that the rect we pass to OptimalImageSizeForDest() be consistent with the dest rect we use in the other calculations. Let me verify that just doing that solves the problem, and I may have a new, simpler patch for you shortly.
OK, yeah, that solution works too, and doesn't have the possible side effects
that inflating the dest rect throughout the entire algorithm would have. I think
this is definitely a better approach.

So just to recap: in this version, we inflate the dest rect only for the
purposes of the OptimalImageSizeForDest() call. That means, in practice,
inflating snappedScaledDest, which is the version of the dest rect we pass to
OptimalImageSizeForDest(). We still bail early if scaledDest is empty; the
difference is that scaledDest is not rounded to the nearest pixel, so if it's
empty, it means that there's *really* nothing to draw. That should happen only
if the scale is zero or effectively zero, which should result in the fill being
empty too, so bailing in that case shouldn't cause the kind of glitching we're
fixing in this bug.

I've verified that this fixes the testcase in this bug on my machine.
Attachment #8584113 - Flags: review?(roc)
Attachment #8583706 - Attachment is obsolete: true
This needed a rebase so I went ahead and rebased it. I cancelled the previous
try job and pushed a new one:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b80c22a2958f
Attachment #8584121 - Flags: review?(roc)
Attachment #8584113 - Attachment is obsolete: true
Thanks for the review!

I pushed one patch too many in my latest inbound push and accidentally pushed this before the try results were back. I immediately backed out, but for reference, here are the changesets:

https://hg.mozilla.org/integration/mozilla-inbound/rev/c035d63ade9d
https://hg.mozilla.org/integration/mozilla-inbound/rev/ad2032c2de8c
Try looks good. I didn't get a chance to write tests for this bug today, but in the interest of getting this landed before the merge I went ahead and pushed.

https://hg.mozilla.org/integration/mozilla-inbound/rev/641902d237af
Keywords: leave-open
Not having much luck with my pushing workflow in this bug! Sorry, it's actually this rev:

https://hg.mozilla.org/integration/mozilla-inbound/rev/3a8dca82b1bc
OK, here are tests for this issue.

The main test is "== image-scrolling-zoom-1.html image-scrolling-zoom-1-ref.html".
The reference is a zoomed out scrollable div with an alternating white and black
background. The test starts with the same div scrolled to the bottom, then
scrolls to the top after MozReftestInvalidate arrives. This shows invalidation
issues (and fails) without the patch on this bug.

As a sanity check, I've included a second test "!=" test which compares the
reference (so no scrolling involved) with "image-scrolling-zoom-1-notref.html",
which is the same thing but with a plain white background. This ensures that we
don't trivially pass because we fail to paint the background image at all. This
fails as well without the patch in this bug.
Attachment #8595084 - Flags: review?(roc)
Thanks for the review!

The test needed some Linux-only fuzz. Other than that, try looks good.
Attachment #8595084 - Attachment is obsolete: true
Got backed out for an unrelated test failure, alas.
Flags: needinfo?(seth)
Status: NEW → RESOLVED
Closed: 9 years ago
Keywords: leave-open
Resolution: --- → FIXED
Comment on attachment 8584121 [details] [diff] [review]
Inflate the dest rect to at least one device pixel when computing the optimal image size for drawing

Approval Request Comment
[Feature/regressing bug #]: Bug 1043560.
[User impact if declined]: Invalidation issues resulting in visual glitches when small images are displayed on a zoomed page.
[Describe test coverage new/current, TreeHerder]: On m-c for a few weeks now. Covered with a test.
[Risks and why]: Low risk.
[String/UUID change made/needed]: None.
Attachment #8584121 - Flags: approval-mozilla-beta?
Attachment #8584121 - Flags: approval-mozilla-aurora?
Comment on attachment 8584121 [details] [diff] [review]
Inflate the dest rect to at least one device pixel when computing the optimal image size for drawing

Seth, if that OK with you, I am going to skip that for 38 as we have this bug for a while.
If you disagree, fill free to resubmit it for uplift.
Attachment #8584121 - Flags: approval-mozilla-beta?
Attachment #8584121 - Flags: approval-mozilla-beta-
Attachment #8584121 - Flags: approval-mozilla-aurora?
Attachment #8584121 - Flags: approval-mozilla-aurora+
Part 1 landed before the uplift, so 39 is already fixed. I'll uplift the test, though.
Flags: in-testsuite+
Target Milestone: --- → mozilla39
Thanks for the uplift, Ryan!

(In reply to Sylvestre Ledru [:sylvestre] from comment #46)
> Seth, if that OK with you, I am going to skip that for 38 as we have this
> bug for a while.
> If you disagree, fill free to resubmit it for uplift.

That's OK. Although this bug is really ugly when it happens, I think it's also pretty rare to hit in practice.
Test marked random on OSX due to apparent scrollbar drawing inconsistencies.
https://hg.mozilla.org/releases/mozilla-aurora/rev/d3912ffd1e54
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #50)
> Test marked random on OSX due to apparent scrollbar drawing inconsistencies.

Marked random on *Aurora*, just to be clear. It works fine on central. No idea what the issue is there.
Blocks: 1085783
Let's get this on B2G 37. Both because it's worth fixing in its own right, and
because uplifting it will make uplifting bug 1085783 much easier.
Attachment #8584121 - Attachment is obsolete: true
Comment on attachment 8598286 [details] [diff] [review]
Inflate the dest rect to at least one device pixel when computing the optimal image size for drawing. (B2G-37 Backport)

Approval Request Comment
[Feature/regressing bug #]: Bug 1043560. Also wanted to simplify the task of uplifting bug 1085783.
[User impact if declined]: Invalidation issues resulting in visual glitches when small images are displayed on a zoomed page.
[Describe test coverage new/current, TreeHerder]: On m-c for a few weeks now. Covered with a test.
[Risks and why]: Low risk.
[String/UUID change made/needed]: None.
Attachment #8598286 - Flags: approval-mozilla-b2g37?
Comment on attachment 8598286 [details] [diff] [review]
Inflate the dest rect to at least one device pixel when computing the optimal image size for drawing. (B2G-37 Backport)

approving, also helps with a blocker uplift.

NJpark, misgt be helpful to do some graphics test around this, so Ni'ing you for help.
Attachment #8598286 - Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+
Flags: needinfo?(npark)
Blocks: 1157542
Did a graphics sanity test on 2.2 build after this patch was merged, and did not find any issues.
Flags: needinfo?(npark)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: