1085783 - Under certain condition images can't resize correctly, resulted in blurry

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[fireattack [:fireattack]]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:34.0) Gecko/20100101 Firefox/34.0
Build ID: 20141014134955

Steps to reproduce:

Test page: http://plaza.ufl.edu/bpeng/firefoxbug.htm

I actually find this problem from a real web page. After hours testing I got the minimized condition (CSS, html, etc.) to reproduce this bug. For this one, I can't change any of these parameters, otherwise the bug disappears. It's not the only combination of parameters causes bug though- I just represent one for you to verify.


Actual results:

The image is abnormally blurry. If you can't tell the difference, just modify the HTML like add "width=180" to <img>. You will suddenly notice the difference (for the lazy: http://plaza.ufl.edu/bpeng/firefoxbug2.htm )

The cause of this problem is, the image is wrongly displayed in 179px width while it's rendered in 180px width (a shown in box model), which causes blur. Actually, if you use ruler to measure, you'll find the width of image is 179.

The cause of this, ultimately, I guess should be non-integer distance because of my combination of CSS condition. This should be avoided- even the distance of some CSS condition is non-integer, it should still try the most recent integer and render and display image correctly.

By the way, Chrome don't have this problem.

Comment 1

[fireattack [:fireattack]]

OK after some test in old version, I can confirm it's a new bug for firefox 34 and later. 

I tested in 28 and 33, image is not blurry. Though they still have the problem as I mentioned before: the image is 179px instead of 180px (which is shown in box model), but it DOES rendered in 179px, therefore, no blur.

Comment 2

[fireattack [:fireattack]]

Another related bug is sometimes, the measurement of element's size from the select tool and the box model is not the same (not in my example, but i can bulid another test page for this easily).

Comment 3

[Loic]

Do you have a screenshot of the blurriness?

Comment 4

[fireattack [:fireattack]]

(In reply to Loic from comment #3)
> Do you have a screenshot of the blurriness?

http://i.imgur.com/z1mW9Js.png

Comment 5

[:Gijs (he/him)]

I can reproduce this on Windows 8 but not on OS X (although with a retina screen, so that might not mean much).

getBoundingClientRect on the image reports an exact width of 179.6666717529297 . I expect a testcase that specifically assigns that width to the image might also work?

Benjamin, would you be able to find a regression window here using mozregression? ( http://mozilla.github.io/mozregression/ )

Assuming this wasn't the fault of something that was uplifted for beta 34, something like:

mozregression --good 2014-07-20 --bad 2014-09-03

should work.

Comment 6

[fireattack [:fireattack]]

(In reply to :Gijs Kruitbosch from comment #5)
> I can reproduce this on Windows 8 but not on OS X (although with a retina
> screen, so that might not mean much).
> 
> getBoundingClientRect on the image reports an exact width of
> 179.6666717529297 . I expect a testcase that specifically assigns that width
> to the image might also work?
> 
> Benjamin, would you be able to find a regression window here using
> mozregression? ( http://mozilla.github.io/mozregression/ )
> 
> Assuming this wasn't the fault of something that was uplifted for beta 34,
> something like:
> 
> mozregression --good 2014-07-20 --bad 2014-09-03
> 
> should work.

last good: daa84204a11a 08-25
first bad: dc352a7bf234 08-26

(the windows terminal is a nightmare, I can't copy..

Comment 7

[fireattack [:fireattack]]

https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=daa84204a11a&tochange=dc352a7bf234

Comment 8

[Loic]

(In reply to Benjamin Peng from comment #4)
> (In reply to Loic from comment #3)
> > Do you have a screenshot of the blurriness?
> 
> http://i.imgur.com/z1mW9Js.png

I have to admit I don't really see the difference between both images, anyway they look a little bit different at higher zoom. On my screen, with FF34, I don't see an obvious blurriness compared to FF33.
Maybe that bug doesn't show up on hidpi screens.

Comment 9

[Loic]

After writting my previous comment, I got the idea to reset the screen resolution from 125% to 100%, and I saw the difference. :)
And I confirm the regression range.

Comment 10

[Alice0775 White]

(In reply to Benjamin Peng from comment #7)
> https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=daa84204a11a&tochange=dc352a7bf234

This should be m-c
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=daa84204a11a&tochange=dc352a7bf234

Comment 11

[Alice0775 White]

[Tracking Requested - why for this release]:

[Tracking Requested - why for this release]:

[Tracking Requested - why for this release]:

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:
https://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=jrmuizel

Comment 12

[fireattack [:fireattack]]

Thanks for correction, Alice.

BTW, I should emphasize that even before 34, the image is not blurred BUT the size of width is WRONG: the width should be 179.667 which should be rounded to 180, not 179. When we're fixing this bug, better handle this rounding problem as well.

Comment 13

[Lawrence Mandel [:lmandel] (use needinfo)]

This is now wontfix for 34. We can consider a fix in a later release.

Comment 14

[:Gijs (he/him)]

Seth, can you have a look and/or delegate? :-)

Comment 15

[fireattack [:fireattack]]

Any update? To be honest this annoying bug highly affects experience on image-heavy websites (to be specific, the ones rely on browsers' image high-quality resizing instead of thumb everything themselves). I by chance browse one everyday.

Comment 16

[Lukas Blakk [:lsblakk] use ?needinfo]

Without anything actionable at this stage in 35 cycle, have to wontfix again.

Comment 17

[Sylvestre Ledru [:Sylvestre]]

Jet & Seth consider that this regression is a blocker for the release. Wontfix for 36

Comment 18

[fireattack [:fireattack]]

Any updates?

Comment 19

[Seth Fowler [:seth] [:s2h]]

(In reply to Benjamin Peng from comment #18)
> Any updates?

No updates yet, but I do plan to look at this again very soon.

Comment 20

[Seth Fowler [:seth] [:s2h]]

(In reply to Benjamin Peng from comment #12)
> BTW, I should emphasize that even before 34, the image is not blurred BUT
> the size of width is WRONG: the width should be 179.667 which should be
> rounded to 180, not 179. When we're fixing this bug, better handle this
> rounding problem as well.

We're drawing the image into a rect located at (106.56..., 99.79...) with size (179.6..., 220). (All values in layout device pixels.) That means the upper right corner of the right is at (286.23..., 99.79...).

So when UserToDevicePixelSnapped() snaps these coordinates, we end up with the upper left corner having a x coordinate of 107 and the upper right corner having an x coordinate of 286. 286 - 107 = 179.

So the region we're drawing into has a width of 179. This value appears correct to me. That's not where the bug is.

Comment 21

[Seth Fowler [:seth] [:s2h]]

(In reply to Seth Fowler [:seth] from comment #20)
> upper right corner of the right

"upper right corner of the rect", sorry.

Comment 22

[fireattack [:fireattack]]

(In reply to Seth Fowler [:seth] from comment #20)
> (In reply to Benjamin Peng from comment #12)
> > BTW, I should emphasize that even before 34, the image is not blurred BUT
> > the size of width is WRONG: the width should be 179.667 which should be
> > rounded to 180, not 179. When we're fixing this bug, better handle this
> > rounding problem as well.
> 
> We're drawing the image into a rect located at (106.56..., 99.79...) with
> size (179.6..., 220). (All values in layout device pixels.) That means the
> upper right corner of the right is at (286.23..., 99.79...).
> 
> So when UserToDevicePixelSnapped() snaps these coordinates, we end up with
> the upper left corner having a x coordinate of 107 and the upper right
> corner having an x coordinate of 286. 286 - 107 = 179.
> 
> So the region we're drawing into has a width of 179. This value appears
> correct to me. That's not where the bug is.

First of all, thanks for your investigation.

It depends on your criteria of "correctness". 

Personally, I don't understand why you think ensuring a undefined variable (x coordinate of upper right corner of the image, which IMO is a driven measure, not a driving one) is more important than ensuring defined (not directly though) variable: width of the image.

But anyway, both could be correct: IE render the image as 179px width, Chrome chooses 180px. It's just a choice. However, I think the former one is better, here is why:

I agree with you this implementation difference per se is not where causes the bug. The bug is because, Firefox's high quality resizing engine is calculating resized image result (and store it to memory, I guess) based on 180px. And if you stretch this image to fit it into a 179px box when rendering the web page, obviously it will be blurry.

The thing is, the "resizing engine/code", may have no idea what the coordinate of the image would be. All it knows is the size: 179.6px here. So it chooses closest integer, 180px. It won't know that due to stupid rounding problem, the box for image could be magically 179px instead of 180px.

That's why I don't prefer your way to calculate the box coordinate. Always ensuring same integer size of image is easier for resizing engine to do its job, and it seems like the only way if we don't change it for now..

----

Disclaimer: I have no knowledge what the workflow of Firefox's rendering is like, so the above is all based on assumption.

Comment 23

[Seth Fowler [:seth] [:s2h]]

(In reply to Benjamin Peng from comment #22)
> I agree with you this implementation difference per se is not where causes
> the bug. The bug is because, Firefox's high quality resizing engine is
> calculating resized image result (and store it to memory, I guess) based on
> 180px. And if you stretch this image to fit it into a 179px box when
> rendering the web page, obviously it will be blurry.

Yep, there's an inconsistency between the two calculations.

I've located the source of the problem, and I'll be uploading a patch to fix it shortly.

Comment 24

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 1) - Snap both the fill and dest rects using UserToDeviceSnapped() when pixel snapping images

This patch should fix the bug. It needs a try job before it's ready for review, though.

Comment 25

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 2) - Clean up variable names in ComputeSnappedImageDrawingParameters to be more consistent

I find the variable names somewhat inconsistent in
ComputeSnappedImageDrawingParameters, particularly after the changes in part 1.
This patch cleans that up a bit. (Most importantly: |fill| and |dest| are now in
the same space.)

Comment 26

[Seth Fowler [:seth] [:s2h]]

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

Comment 27

[Seth Fowler [:seth] [:s2h]]

Here's what try turned up:

- This patch fixes layout/reftests/bugs/446100-1b.html and layout/reftests/bugs/446100-1g.html, which previously failed on mobile.

- On Windows, this patch turns layout/reftests/backgrounds/background-tiling-zoom-1.html a very tiny bit more fuzzy - we go from fuzzy(30,474) to fuzzy(31,474). This seems OK.

- On OS X, this patch causes catastrophic failure of layout/reftests/backgrounds/background-tiling-zoom-1.html. Not sure what's going on there; need to investigate further.

Comment 28

[Seth Fowler [:seth] [:s2h]]

(In reply to Seth Fowler [:seth] from comment #27)
> - On OS X, this patch causes catastrophic failure of
> layout/reftests/backgrounds/background-tiling-zoom-1.html. Not sure what's
> going on there; need to investigate further.

OK, I fixed this. We need to use a non-snapped dest rect for the calculation in the "complex" branch of the code. I'll upload an updated version of the patch.

Comment 29

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 1) - Snap both the fill and dest rects using UserToDeviceSnapped() when pixel snapping images

Here's the updated version of part 1.

Comment 30

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 2) - Clean up variable names in ComputeSnappedImageDrawingParameters to be more consistent

Part 2 needed a rebase against part 1.

Comment 31

[Seth Fowler [:seth] [:s2h]]

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

Comment 32

[Seth Fowler [:seth] [:s2h]]

OK, this new version is solid green on try. Nice.

Comment 33

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8591889 [details] [diff] [review]
(Part 1) - Snap both the fill and dest rects using UserToDeviceSnapped() when pixel snapping images

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

IIUC snapping scaledDest has the downside that it can significantly change the tiling. For example if an image is scaled to 2.5 pixels wide, we might snap it to 3 pixels wide, then over a fill area of 300 pixels we would get 100 repetitions instead of 120. I'm a bit leery of changing gross rendering like that. What do you think?

Comment 34

[Seth Fowler [:seth] [:s2h]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #33)
> IIUC snapping scaledDest has the downside that it can significantly change
> the tiling. For example if an image is scaled to 2.5 pixels wide, we might
> snap it to 3 pixels wide, then over a fill area of 300 pixels we would get
> 100 repetitions instead of 120. I'm a bit leery of changing gross rendering
> like that. What do you think?

Well, there are two code paths in ComputeSnappedImageDrawingParameters, the "simple" one (where the anchor's at the upper left of the dest rect and the fill rect is the same as the dest rect) and the "complex" one that covers all other cases.

In the "simple" case, we actually do not take |scaledDest| into account at all; we just compute the transform from |subimage| to |fill|.

In the "complex" case, we use |scaledDest| only if we didn't snap it; otherwise, we compute a non-snapped version. So nothing should change there.

This patch is just about making sure that we round in the same way when computing the fill and the image size.

I considered this in some detail and noticed multiple bugs in the code when I was doing so. One was a bug in part 1 (we should always scale |scaledDest| by |currentMatrix.ScaleFactors()|, whether we snapped or not) and one was a pre-existing bug (I'm pretty sure we should actually do the same for |fill|). I'll upload a fixed version of part 1 and add some comments about the current situation in part 2, and I'll file a followup to fix the pre-existing bug.

Comment 35

[Seth Fowler [:seth] [:s2h]]

Note that the bug in part 1 would've been caught by the testcase for bug 1068881. I'll write that today.

Comment 36

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 1) - Snap both the fill and dest rects using UserToDeviceSnapped() when pixel snapping images

This new version fixes the bug mentioned in comment 34.

Comment 37

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 2) - Clean up variable names in ComputeSnappedImageDrawingParameters to be more consistent

Rebased and added some extra comments as mentioned in comment 34.

Comment 38

[Seth Fowler [:seth] [:s2h]]

Attachment: Snap both the fill and dest rects using UserToDeviceSnapped() when pixel snapping images

So I began writing a patch to make sure that |dest| and |fill| were always in
the same space by scaling |fill| by |currentMatrix.ScaleFactors()| as well, as
mentioned in comment 34. Once I actually tried writing the patch, it was obvious
to me that this was wrong, as making the math work out would require scaling by
the inverse of the scale factors later, and that's a very bad idea from a
numerical accuracy standpoint.

The right thing to do here is actually to:

- On the non-snapping code path, *don't* multiply |dest| by
  |currentMatrix.ScaleFactors()|. This way |dest| and |fill| are always in the
  same space. This is the key change from the previous patches I've uploaded in
  this bug.

- Multiply by |currentMatrix.ScaleFactors()| directly in the computations where
  we need it - most importantly, the optimal image size computation.

- Document the fact that |fill| and |dest| take |currentMatrix| into account on
  one code path and not on the other, to help make it easier to interpret this
  code the next time we need to change it.

This new patch implements the changes above. We still snap |dest| on the
snapping code path, ensuring that we round in the same way when computing the
fill and the image size, so it still fixes this bug. And I've verified that bug
1068881 remains fixed.

I feel much better about this version of the patch. Now we just need to verify
it's green on try. I've run a subset of the tests locally and they've come up
green, which is a good sign.

Comment 39

[Seth Fowler [:seth] [:s2h]]

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

Comment 40

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 2) - Add a test for rounding behavior when high-quality downscaling

OK, here's the other piece of the puzzle: the test.

The testcase is similar to the one given in this bug, but with a new, CC-0 image.

We need to wait before taking the reftest snapshot because HQ downscaling is
currently an async process. That means that this test will be intermittent until
bug 1045926 is complete. (Which is targeted for this quarter, FWIW.) This is the
same approach we use in |image/test/reftest/downscaling|, and in practice we
haven't had a big problem with intermittent oranges there. If this one does turn
out to be more problematic, we can just disable it until bug 1045926 is done,
but I'd prefer to at least get it in the tree right away.

Locally, this testcase fails without the patch in this bug and passes with it.

Comment 41

[Seth Fowler [:seth] [:s2h]]

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

Comment 42

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8595520 [details] [diff] [review]
(Part 2) - Add a test for rounding behavior when high-quality downscaling

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

::: layout/reftests/pixel-rounding/image-high-quality-scaling-1-ref.html
@@ +13,5 @@
> +      document.documentElement.removeAttribute('class');
> +    }
> +  </script>
> +</head>
> +<body onload="setTimeout(snapshot, 50)">

I suggest increasing the timeout to 250ms for now

Comment 43

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 2) - Add a test for rounding behavior when high-quality downscaling

Thanks for the review! This version of the patch increases the timeout to 250ms.

Comment 44

[Seth Fowler [:seth] [:s2h]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/60bbc4230cd7
https://hg.mozilla.org/integration/mozilla-inbound/rev/bc6db8bcf480

Comment 45

[Carsten Book [:Tomcat]]

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

Comment 46

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0bd6d2ca50a0
https://hg.mozilla.org/integration/mozilla-inbound/rev/0220e2406fab

Comment 47

[Seth Fowler [:seth] [:s2h]]

This got backed out because the test fails intermittently. I suppose the odds of failure are greater in this case compared to our other HQ downscaling tests, because the image is bigger (hence scaling takes longer) and both the test _and_ the reference are images which get HQ scaled.

I'm going to go ahead and mark the test as random and re-land. I'll file a followup to reenable once DDD is flipped on everywhere.

Comment 48

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 2) - Add a test for rounding behavior when high-quality downscaling

Added skip annotation.

Comment 49

[Seth Fowler [:seth] [:s2h]]

(Went with skip rather than random because I don't think this test adds any useful code coverage. It's not really useful unless we actually inspect the results of the test.)

Comment 50

[Seth Fowler [:seth] [:s2h]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b339f54f20f6
https://hg.mozilla.org/integration/mozilla-inbound/rev/1088d4ec5fad

Comment 51

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/b339f54f20f6
https://hg.mozilla.org/mozilla-central/rev/1088d4ec5fad

Comment 52

[Seth Fowler [:seth] [:s2h]]

Comment on attachment 8592564 [details] [diff] [review]
Snap both the fill and dest rects using UserToDeviceSnapped() when pixel snapping images

Approval Request Comment
[Feature/regressing bug #]: Bug 1043560.
[User impact if declined]: Blurry images in some situations.
[Describe test coverage new/current, TreeHerder]: On m-c. Test landed but will be intermittent until DDD lands, so it's currently disabled.
[Risks and why]: Low risk.
[String/UUID change made/needed]: None.

Comment 53

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8592564 [details] [diff] [review]
Snap both the fill and dest rects using UserToDeviceSnapped() when pixel snapping images

Lame CC from bug 1068881
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.

Comment 54

[Seth Fowler [:seth] [:s2h]]

(In reply to Sylvestre Ledru [:sylvestre] from comment #53)
> Lame CC from bug 1068881
> 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.

Fine with me. I do think we should get it into 39, though, at a minimum.

Comment 55

[Ryan VanderMeulen [:RyanVM]]

I guess there's the question of whether this is wanted for b2g37 (v2.2) or not as well. Going to assume the ship has long-sailed on b2g34 (v2.1).

Comment 56

[Seth Fowler [:seth] [:s2h]]

Comment on attachment 8592564 [details] [diff] [review]
Snap both the fill and dest rects using UserToDeviceSnapped() when pixel snapping images

I think it's worth requesting B2G 2.2 uplift, because this bug definitely affects B2G 2.2, and it will be much more obvious on a low-resolution mobile screen than on desktop. And this is a pretty low risk patch, so the downsides are pretty small.

Approval Request Comment
[Feature/regressing bug #]: Bug 1043560.
[User impact if declined]: Blurry images in some situations.
[Describe test coverage new/current, TreeHerder]: On m-c. Test landed but will be intermittent until DDD lands, so it's currently disabled.
[Risks and why]: Low risk.
[String/UUID change made/needed]: None.

Comment 57

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8592564 [details] [diff] [review]
Snap both the fill and dest rects using UserToDeviceSnapped() when pixel snapping images

I think we're good to take this fix in Aurora at this point. We have a couple of weeks left to deal with any fallout. Aurora+

Comment 58

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/a0044e233d6a
https://hg.mozilla.org/releases/mozilla-aurora/rev/59db2adb7bee

Comment 59

[Ryan VanderMeulen [:RyanVM]]

Needs rebasing for b2g37 uplift.

Comment 60

[Seth Fowler [:seth] [:s2h]]

I'm guessing it's just that bug 1068881 isn't on b2g37? I'll check, but if that's all it is, I'd prefer we just uplift that one too.

Comment 61

[Seth Fowler [:seth] [:s2h]]

Attachment: Snap both the fill and dest rects using UserToDeviceSnapped() when pixel snapping images. (B2G 37 Backport)

With bug 1068881 uplift (which I've requested) and some trivial rebasing, this should apply cleanly to B2G 37. Here's a pre-rebased version.

Comment 62

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/8b83dbe19870

Comment 63

[Ryan VanderMeulen [:RyanVM]]

The branch patch had bustage. Re-landed with that fixed.
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/79e7065ceefa

Comment 64

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/79e7065ceefa