Closed Bug 1261964 Opened 8 years ago Closed 8 years ago

moz-icon images aren't rendered properly when downscaled (e.g. in the download panel, or downloads library pane, or in URL bar for certain doorhanger notifications)

Categories

(Core :: Graphics: ImageLib, defect)

48 Branch
x86_64
All
defect
Not set
major

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox47 --- unaffected
firefox48 + fixed
firefox49 --- fixed

People

(Reporter: MattN, Assigned: seth)

References

(Blocks 1 open bug)

Details

(Keywords: regression, Whiteboard: [gfx-noted])

Attachments

(6 files, 1 obsolete file)

Build identifier: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.10; rv:48.0) Gecko/20100101 Firefox/48.0

STR A:
1) Download an image file
2) Open the download panel from the toolbar button.

STR B: 
1) Tools > Downloads (or click "Show All Downloads" in the download panel footer) to open the Downloads view in the Places Library.

Expected result:
Icons beside the download are appropriate for the file type. e.g. PNG images should show the icon of Preview.app

Actual result:
Some corrupt/unusual representation of the icon appears.
Loading the moz-icon URL directly in a tab seems to work fine though: moz-icon:///private/tmp/my_photo.png?size=32&state=normal

Last good revision: 102886e9ac63b3fa33a6f1b394aea054abce2dfd (2016-03-11)
First bad revision: 946ed22cad04431c75ab5093989dfedf1bae5a3e (2016-03-12)
Pushlog:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=102886e9ac63b3fa33a6f1b394aea054abce2dfd&tochange=946ed22cad04431c75ab5093989dfedf1bae5a3e

Backing out bug 1255104 fixes the problem for me.

[Tracking Requested - why for this release]: Secondary UI obvious visual regression.
Flags: needinfo?(seth)
Has Regression Range: --- → yes
Has STR: --- → yes
(In reply to Matthew N. [:MattN] from comment #0)
> Loading the moz-icon URL directly in a tab seems to work fine though:
> moz-icon:///private/tmp/my_photo.png?size=32&state=normal

That because the image is not presented at a size smaller than it's intrinsic size. If you change the zoom so it's smaller I bet the problem shows up.

Not having any tests for downscaling icon's makes me sad :(
(In reply to Timothy Nikkel (:tnikkel) from comment #1)
> (In reply to Matthew N. [:MattN] from comment #0)
> > Loading the moz-icon URL directly in a tab seems to work fine though:
> > moz-icon:///private/tmp/my_photo.png?size=32&state=normal
> 
> That because the image is not presented at a size smaller than it's
> intrinsic size. If you change the zoom so it's smaller I bet the problem
> shows up.

Yep, the simplest STR is to simply load moz-icon:///private/tmp/my_photo.png?size=32&state=normal in a tab and zoom out to see the corruption.
It seems this doesn't affect all Mac users so I'm including my about:support Graphics info:
Asynchronous Pan/Zoom:	none
Device ID:	0x0d26
GPU Accelerated Windows:	1/1 OpenGL (OMTC)
Supports Hardware H264 Decoding:	Yes
Vendor ID:	0x8086
WebGL Renderer:	NVIDIA Corporation -- NVIDIA GeForce GT 750M OpenGL Engine
windowLayerManagerRemote:	true
AzureCanvasAccelerated:	1
AzureCanvasBackend:	skia
AzureContentBackend:	skia
AzureFallbackCanvasBackend:	none
I can see the problem on my mac. It might depend on retina vs non-retina (that might change whether the image is downscaled or not).
There is likely a simple coordinate mistake in the new code that will become obvious once the problem is examined in a debugger.
(In reply to Timothy Nikkel (:tnikkel) from comment #4)
> I can see the problem on my mac. It might depend on retina vs non-retina
> (that might change whether the image is downscaled or not).

Yeah, it's only a problem on non-retina. I got confused when comparing with the person who didn't see the problem because I have a dual monitor setup (one retina, one not).
See Also: → 1256198
See Also: 1256198
On Ubuntu, this reproduces periodically with the (default-installed) Ubuntu Modifications add-on, in its "You should restart Nightly now to install updates" doorhanger notification (which triggers periodically for mysterious reasons).

I can reproduce it on-demand by visiting moz-icon://stock/reboot-notifier?size=16 and zooming out (thanks to MattN for that suggestion in comment 2).
OS: Mac OS X → All
Summary: Download icons (moz-icon) aren't rendered properly in the download panel or downloads library pane → moz-icon images aren't rendered properly when downscaled (e.g. in the download panel, or downloads library pane, or in URL bar for certain doorhanger notifications)
Here's a testcase that reproduces the problem for me (which should work on all platforms, I think).

It renders whatever your platform's icon is for a plain-text document, at various scales. The 3rd and 4th images are downscaled, and they render as garbled for me.
(In reply to Matthew N. [:MattN] (behind on reviews) from comment #6)
> Yeah, it's only a problem on non-retina. I got confused when comparing with
> the person who didn't see the problem because I have a dual monitor setup
> (one retina, one not).

This isn't quite true. The problem *does* reproduce on retina -- you just have to downscale extra-much in order to *actually* be downscaling on a retina display.

For example, my just-attached testcase demonstrates the problem on a retina mac -- its 4th example (downscaled ~10x) *should* render a tiny picture of a text-document, but instead it renders as transparent.
Here's what testcase 1 looks like in Linux Nightly 48. As noted in my previous comment, on a retina Mac, only the final image here is broken.
Whiteboard: [gfx-noted]
Recent regression, tracking
How do we get this moving while 48 is still on Aurora?  We don't want to have to uplift all the way to Beta.
Assignee: nobody → seth
Old world: the frame rect was handled by the downscaler or imgFrame itself and specified in output space.
New world: the frame rect is handled by RemoveFrameRectFilter and specified in input space.
Bug: new world code which continues to specify the frame rect in output space.

<smacks forehead>
Attachment #8756537 - Flags: review?(n.nethercote)
Flags: needinfo?(seth)
Comment on attachment 8756537 [details] [diff] [review]
Specify the frame rect in input space, not output space, in nsIconDecoder.

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

The fix seems fine, but can we have a test case? Pretty please?
Attachment #8756537 - Flags: review?(n.nethercote) → review+
A reftest please, not a unit test.
Comment on attachment 8756537 [details] [diff] [review]
Specify the frame rect in input space, not output space, in nsIconDecoder.

Approval Request Comment
[Feature/regressing bug #]: See the blocked bug.
[User impact if declined]: Distorted display of moz-icon images, which we use for example to display file type icons in the Downloads window.
[Describe test coverage new/current, TreeHerder]: I'll add a new test, though it isn't attached to the bug yet.
[Risks and why]: Very low; the change is trivial.
[String/UUID change made/needed]: None.
Attachment #8756537 - Flags: approval-mozilla-aurora?
(In reply to Timothy Nikkel (:tnikkel) from comment #17)
> A reftest please, not a unit test.

As a first pass I'm going to add unit tests for downscaling images during decode. I'd be happy to add reftests as well. We already have DDD reftests for other formats, though, and the specific case of moz-icon is tricky since its output is platform-specific - I'm not sure how to get a platform-independent icon out of moz-icon. We also can't simply include "image.icon" in a reftest since content type sniffing will never return "image/icon" and we have no way of specifying an HTTP content type in a reftest to my knowledge. (That *is* possible in a mochitest, though.) Given the trickiness, I'd say let's revisit this later.
https://hg.mozilla.org/integration/mozilla-inbound/rev/e13e5ddca69f284c514fb46e5225b8f73210e208
Bug 1261964 - Specify the frame rect in input space, not output space, in nsIconDecoder. r=njn
(In reply to Seth Fowler [:seth] [:s2h] from comment #19)
> platform-independent icon out of moz-icon. We also can't simply include
> "image.icon" in a reftest since content type sniffing will never return
> "image/icon" and we have no way of specifying an HTTP content type in a
> reftest to my knowledge. (That *is* possible in a mochitest, though.) Given

Looks like we've had the ability to use sjs files to serve images in reftests for a long time:
http://mxr.mozilla.org/mozilla-central/source/layout/reftests/backgrounds/root-background-1.html?force=1#2
So I've written up some tests in bug 1207830 which I've verified would've caught this bug. The amount of new code is big enough that I thought it'd be better to add the tests in a separate bug, so we'll do that there.

(In reply to Timothy Nikkel (:tnikkel) from comment #21)
> Looks like we've had the ability to use sjs files to serve images in
> reftests for a long time:
> http://mxr.mozilla.org/mozilla-central/source/layout/reftests/backgrounds/
> root-background-1.html?force=1#2

That's useful to know. I had been under the impression we needed a mochitest for that. With this, we may be able to write reftests for icon images. Since we have none at all currently, we probably want to add a few in addition to the downscaling one. I'll file a followup.
Blocks: 1207830
Blocks: 1275827
Attached patch simple reftest patch (obsolete) — Splinter Review
Here's a simple reftest, which just compares a 90%-scaled <img src="moz-icon://goat?size=100"> against the same image, untransformed, with "?size=90" in its URI instead of ?size=100.

They're not expected to be a perfect match -- I think the OS can give us different pixels depending on the provided size -- but they're close enough to be usefully comparable with "fuzzy()", on my machine at least.  (And they *really* mismatch without the fix here applied.)

It sounds like the ones you're working on in the other bug are more sophisticated, but I was already partway through spinning this one up as a simple proof-of-concept, so I figured I might as well attach it here. (and we can use the other bug for better / more comprehensive tests.)

What do you think?

(NOTE: The "fuzzy" annotation needs tuning after I've gotten TryServer results back, to ensure that it passes on all platforms.  Currently the fuzzy() values are from my Linux system, with Seth's fix applied.)
Attachment #8756738 - Flags: review?(seth)
[Hmm, I used "moz-icon://goat" in the reftest because I thought it was something special, based on finding it in the tree...
  http://mxr.mozilla.org/mozilla-central/search?string=moz-icon%3A%2F%2Fgoat%3F
...but it looks like e.g. "moz-icon://bogus-unrecognized-icon" (or any arbitrary unrecognized string) renders just the same -- the icon looks like a sheet of paper with a question mark.  Perhaps "bogus-unrecognized-icon" be more better to use from a self-documenting perspective, as compared to "goat". :)]
(In reply to Daniel Holbert [:dholbert] from comment #23)
> What do you think?

Very nice! The "size" feature of moz-icon URIs is another thing I didn't know about. Presumably the OS is doing some scaling for us there? If so, yeah, that's a great way to test downscale-during-decode for moz-icon URIs - much easier than using the reference images I was afraid were going to be necessary if we couldn't use a non-OS-provided image. I'm guessing "goat" just gives us the "default filetype" icon, which seems fine.
(In reply to Daniel Holbert [:dholbert] from comment #24)
> Perhaps "bogus-unrecognized-icon" be
> more better to use from a self-documenting perspective, as compared to
> "goat". :)]

Yeah, I'd concur. =)
Comment on attachment 8756738 [details] [diff] [review]
simple reftest patch

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

I like it. Thanks for putting this together, Daniel! r+, maybe with a change to the URI as you suggest above.
Attachment #8756738 - Flags: review?(seth) → review+
Thanks! I've applied s/goat/bogus-unrecognized-icon/ to the patch locally, and I'll land after I've gotten Try results back (assuming they're sane) and adjusted the fuzzy() annotation accordingly.

Try run with this bug's fix and the reftest patch, to find out how much fuzz we need:
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=fb00f9f66e33

...and just for fun, here's another Try run without the fix, to show how bad the mismatch is in a buggy unfixed build:
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=a1d6542bcf5f

(Ideally, the "max difference" & "number of mismatching pixels" should be substantially higher in the second Try run, as compared to the first.)
Flags: needinfo?(dholbert)
(My reftest doesn't work at all on Windows; on that platform, we only seem to support two platform-provided icon sizes.  So my ?size=90 and ?size=100 URIs each end up returning a 32px-by-32px image -- not a 90px and a 100px image.  I suspect this is intentional (perhaps because older windows versions don't have scalable icons), and might be due to this hack:
http://mxr.mozilla.org/mozilla-central/source/image/decoders/icon/win/nsIconChannel.cpp?rev=c75f253f4335&mark=316-317#313

In any case, I'll be flagging my reftest as "fails-if(winWidget)" as a result.)
https://hg.mozilla.org/mozilla-central/rev/e13e5ddca69f
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
(In reply to Daniel Holbert [:dholbert] from comment #29)
> (My reftest doesn't work at all on Windows; on that platform, we only seem
> to support two platform-provided icon sizes.  So my ?size=90 and ?size=100
> URIs each end up returning a 32px-by-32px image -- not a 90px and a 100px
> image.  I suspect this is intentional (perhaps because older windows
> versions don't have scalable icons), and might be due to this hack:
> http://mxr.mozilla.org/mozilla-central/source/image/decoders/icon/win/
> nsIconChannel.cpp?rev=c75f253f4335&mark=316-317#313
> 
> In any case, I'll be flagging my reftest as "fails-if(winWidget)" as a
> result.)

Can you drawImage the icon to a canvas at fullsize, and then use a CSS transform to scale down the canvas? That would be a good easy way to do a downscaling reftest of any image without having to have/create a smaller sized reference if it works.
Ooh, that is a great idea -- it should reduce the need for fuzzy matching on non-Windows platforms, too.
Here's the reftest again, now using a <canvas> in the reference case.

Try run (expecting some mismatches & will update fuzzy annotation to accommodate): https://treeherder.mozilla.org/#/jobs?repo=try&revision=1205cd9fd1ea
Attachment #8756738 - Attachment is obsolete: true
Comment on attachment 8757141 [details] [diff] [review]
reftest patch, v2

(Seth, mind re-reviewing for the updated reference case?)
Flags: needinfo?(dholbert)
Attachment #8757141 - Flags: review?(seth)
Comment on attachment 8757141 [details] [diff] [review]
reftest patch, v2

Meh, I'm just going to go ahead and land the reftest with the prior r+ carried forward; I'll soften my updated r? request to feedback?, and I'll be happy to land any tweaks if you've got review feedback.
Attachment #8757141 - Flags: review?(seth) → feedback?(seth)
Flags: in-testsuite+
(Just for fun, here's a Try run with the final version of the reftest, with the fix here backed out, to verify that the test is actually effectively testing something:
 https://treeherder.mozilla.org/#/jobs?repo=try&revision=ada30a9f6da2

Failures on all platforms. Hooray!)
Comment on attachment 8757141 [details] [diff] [review]
reftest patch, v2

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

Looks good!
Attachment #8757141 - Flags: feedback?(seth) → feedback+
Comment 39 & 40 are fuzziness threshold bumps for Win8 and WinXP (respectively), which unfortunately need more fuzziness than other platforms, and which get builds but no testruns on a "-p all -u reftest" try run. :|  (which is apparently expected; they're opt-in, see bug 1276387.)
Comment on attachment 8756537 [details] [diff] [review]
Specify the frame rect in input space, not output space, in nsIconDecoder.

Fix a regression, taking it
Attachment #8756537 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.