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)
Tracking
()
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
|
bajaj
:
approval-mozilla-b2g37+
|
Details | Diff | Splinter Review |
[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
Reporter | ||
Comment 1•10 years ago
|
||
Reporter | ||
Comment 2•10 years ago
|
||
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
Blocks: 1057903
Keywords: regressionwindow-wanted
Reporter | ||
Comment 3•10 years ago
|
||
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
Reporter | ||
Comment 4•10 years ago
|
||
Err, Bug 1067207 does not fix the problem completely. I can still reproduce the problem if Zoom level <= 30%
Reporter | ||
Comment 5•10 years ago
|
||
[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
status-firefox33:
--- → unaffected
tracking-firefox34:
--- → ?
Summary: Graphic glitch when scrolling page if zoom level < 100% → Graphic glitch when scrolling page if zoom level <= 30%
Version: 35 Branch → 34 Branch
Updated•10 years ago
|
Assignee | ||
Comment 7•10 years ago
|
||
I'll take a look.
Assignee | ||
Comment 8•10 years ago
|
||
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
Assignee | ||
Comment 9•10 years ago
|
||
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.
Comment 10•10 years ago
|
||
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.
Assignee | ||
Comment 11•10 years ago
|
||
(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!
Comment 12•10 years ago
|
||
Seth - Doesn't look like there's been any progress on this in the last month. Can you still work on this?
Updated•10 years ago
|
Flags: needinfo?(bas)
Comment 13•10 years ago
|
||
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)
Assignee | ||
Comment 14•10 years ago
|
||
(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.
Comment 15•10 years ago
|
||
This is wontfix for 34. It would be good to fix this in 35 before it gets too late into the beta cycle.
Comment 16•9 years ago
|
||
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.
Updated•9 years ago
|
Flags: needinfo?(seth)
Updated•9 years ago
|
Flags: needinfo?(seth)
Comment 17•9 years ago
|
||
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.
Comment 18•9 years ago
|
||
Just like with 34 & 35, wontfix for 36. Jet, please let us know what you plan to do wrt this bug? Thanks
Assignee | ||
Comment 19•9 years ago
|
||
Right now I'm hoping to get a fix for this in for 37.
Flags: needinfo?(seth)
Updated•9 years ago
|
Flags: needinfo?(bugs)
Comment 20•9 years ago
|
||
37 merges to Beta on Feb 23. It is preferable to get a fix in before then.
Comment 21•9 years ago
|
||
There are only 3 Betas left in the 37 cycle. Do you still intend to fix this issue in 37?
Flags: needinfo?(seth)
Assignee | ||
Comment 22•9 years ago
|
||
I wanted to, but time just ran out on me. Let's see if we can get this into 38.
Flags: needinfo?(seth)
Comment 23•9 years ago
|
||
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.
status-firefox38:
--- → affected
status-firefox39:
--- → affected
Assignee | ||
Comment 24•9 years ago
|
||
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
Assignee | ||
Comment 25•9 years ago
|
||
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.
Assignee | ||
Comment 26•9 years ago
|
||
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)
Assignee | ||
Updated•9 years ago
|
Attachment #8582831 -
Attachment is obsolete: true
Assignee | ||
Comment 27•9 years ago
|
||
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)
Assignee | ||
Comment 29•9 years ago
|
||
(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.
Assignee | ||
Comment 30•9 years ago
|
||
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)
Assignee | ||
Updated•9 years ago
|
Attachment #8583706 -
Attachment is obsolete: true
Assignee | ||
Comment 31•9 years ago
|
||
Here's a new try job: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ec2abcb2d032
Attachment #8584113 -
Flags: review?(roc) → review+
Assignee | ||
Comment 32•9 years ago
|
||
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)
Assignee | ||
Updated•9 years ago
|
Attachment #8584113 -
Attachment is obsolete: true
Attachment #8584121 -
Flags: review?(roc) → review+
Assignee | ||
Comment 33•9 years ago
|
||
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
Assignee | ||
Comment 34•9 years ago
|
||
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
Assignee | ||
Comment 35•9 years ago
|
||
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
Assignee | ||
Comment 37•9 years ago
|
||
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)
Assignee | ||
Comment 38•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1c5176cfc6db
Attachment #8595084 -
Flags: review?(roc) → review+
Assignee | ||
Comment 39•9 years ago
|
||
Thanks for the review! The test needed some Linux-only fuzz. Other than that, try looks good.
Assignee | ||
Updated•9 years ago
|
Attachment #8595084 -
Attachment is obsolete: true
Assignee | ||
Comment 40•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/dc6d37458b88
Comment 41•9 years ago
|
||
sorry had to back this out in https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=ce2a46e97e38 for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=9167961&repo=mozilla-inbound
Flags: needinfo?(seth)
Assignee | ||
Comment 42•9 years ago
|
||
Got backed out for an unrelated test failure, alas.
Flags: needinfo?(seth)
Assignee | ||
Comment 43•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/a58fd2d77c96
Assignee | ||
Updated•9 years ago
|
Assignee | ||
Comment 45•9 years ago
|
||
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 46•9 years ago
|
||
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+
Comment 47•9 years ago
|
||
Part 1 landed before the uplift, so 39 is already fixed. I'll uplift the test, though.
Assignee | ||
Comment 49•9 years ago
|
||
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.
Updated•9 years ago
|
Comment 50•9 years ago
|
||
Test marked random on OSX due to apparent scrollbar drawing inconsistencies. https://hg.mozilla.org/releases/mozilla-aurora/rev/d3912ffd1e54
Assignee | ||
Comment 51•9 years ago
|
||
(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.
Assignee | ||
Comment 52•9 years ago
|
||
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.
Assignee | ||
Updated•9 years ago
|
Attachment #8584121 -
Attachment is obsolete: true
Assignee | ||
Comment 53•9 years ago
|
||
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 54•9 years ago
|
||
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+
Updated•9 years ago
|
Flags: needinfo?(npark)
Comment 56•9 years ago
|
||
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.
Description
•