815512 - moz-icon not working on Retina displays

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Justin Dolske [:Dolske]]

I noticed that I wasn't getting per-file icons in the download panel/window, nor was I getting file icons when browsing a directory listing (eg file:///Users/dolske/).

With a debug build I see that there's a NS_ENSURE_TRUE failing at file:///Users/dolske/Desktop/

256   NS_ENSURE_TRUE(![bitmapRep isPlanar] &&
257                  (unsigned int)[bitmapRep bytesPerPlane] == desiredImageSize * desiredImageSize * 4 &&
258                  [bitmapRep bitsPerPixel] == 32 &&
259                  [bitmapRep samplesPerPixel] == 4 &&
260                  [bitmapRep hasAlpha] == YES,
261                  NS_ERROR_UNEXPECTED);

Here desiredImageSize is 32 (ie, it's being very strict about wanting a 32x32 RGBA bitmap). The debugger won't show me everything on bitmapRep, but it's showing enough other things to indicate that we see to actually have a 64x64 image here. (Maybe? Eg _size.width says 32x32, but _pixelsWide is 64 and _bytesPerRow is 256)

Not sure what the best fix is here. We can presumably do some twiddling to actually get a 32x32 icon, but we really actually do want to display a 64x64 pixel icon in the end since this is a Retina display.

Comment 1

[Jonathan Kew [:jfkthame]]

So it looks like creating the NSBitmapImageRep from an NSImage with initWithFocusedViewRect: may, on a Retina machine, give us a 2x-size bitmap, where the width and height of the actual bitmap data is twice the size of the rect that was passed in. Which makes sense, as that rect is interpreted as Cocoa points, but the resulting bitmap represents device pixels for a hi-dpi screen.

We can pretty easily adapt the code here to accept and return that double-size bitmap. However, as there's no resolution data attached to the returned image, code that uses moz-icon will need to be more careful to explictly set the size at which the resulting image is to be displayed, otherwise we'll get oversized images all over the place.

Comment 2

[Jonathan Kew [:jfkthame]]

Attachment: pt 1 - on retina-display Macs, moz-icon gets a bitmap that is 2x the 'expected' size

This patch fixes the Cocoa nsIconChannel implementation so that it will return 2x icons if that's what Cocoa gives us.

Comment 3

[Jonathan Kew [:jfkthame]]

Attachment: pt 2 - explicitly set size of download item icons

With the preceding patch, moz-icon may return an image that's twice the expected size (for HiDPI systems); this fixes the downloads list so that the resulting file icons are displayed at the proper scale.

Comment 4

[Jonathan Kew [:jfkthame]]

Attachment: pt 3 - limit size of file icons in HTML directory listing

And this fixes the HTML directory listing so that file icons are scaled appropriately.

Comment 5

[Jonathan Kew [:jfkthame]]

And are there additional consumers of moz-icon that we'll need to fix up? These are the ones I've noticed so far.

Comment 6

[Joe Drew (not getting mail)]

Comment on attachment 685597 [details] [diff] [review]
pt 1 - on retina-display Macs, moz-icon gets a bitmap that is 2x the 'expected' size

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

::: image/decoders/icon/mac/nsIconChannelCocoa.mm
@@ -222,5 @@
>    // first try to get the icon from the file if it exists
>    if (fileExists) {
>      nsCOMPtr<nsILocalFileMac> localFileMac(do_QueryInterface(fileloc, &rv));
>      NS_ENSURE_SUCCESS(rv, rv);
> -    

i will r+ you just for this

@@ +277,3 @@
>    nsAutoTArray<uint8_t, 3 + 16 * 16 * 5> iconBuffer; // initial size is for 16x16
>    if (!iconBuffer.SetLength(bufferCapacity))
>      return NS_ERROR_OUT_OF_MEMORY;

We don't actually need this since nsAutoTArray is infallible, but I don't care if you remove this code.

@@ +315,4 @@
>    // Now, create a pipe and stuff our data into it
>    nsCOMPtr<nsIInputStream> inStream;
>    nsCOMPtr<nsIOutputStream> outStream;
>    rv = NS_NewPipe(getter_AddRefs(inStream), getter_AddRefs(outStream), bufferCapacity, bufferCapacity, nonBlocking);  

wanna fix this while you're here?

Comment 7

[Jonathan Kew [:jfkthame]]

Meaning wrap the line? Sure, why not.

Comment 8

[Jonathan Kew [:jfkthame]]

Tryserver build with the fixed moz-icons: https://tbpl.mozilla.org/?tree=Try&rev=29fe08cea84e

Comment 9

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 685598 [details] [diff] [review]
pt 2 - explicitly set size of download item icons

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

So I don't actually have a Retina Macbook here to test all of this with, but I think this is a pretty safe change.

Comment 10

[Joe Drew (not getting mail)]

(In reply to Jonathan Kew (:jfkthame) from comment #7)
> Meaning wrap the line? Sure, why not.

There was whitespace at the end, but line wrapping is fine with me too as long as the bad whitespace goes away! :)

Comment 11

[Jason Duell]

Comment on attachment 685599 [details] [diff] [review]
pt 3 - limit size of file icons in HTML directory listing

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

bz: this is just a couple of lines, and looks simple enough, but I don't know this code at all.  You've reviewed changes to this file a lot in the past...

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 685599 [details] [diff] [review]
pt 3 - limit size of file icons in HTML directory listing

r=me

Comment 13

[:Gavin Sharp [email: [e-mail]]]

(In reply to Jonathan Kew (:jfkthame) from comment #1)
> We can pretty easily adapt the code here to accept and return that
> double-size bitmap. However, as there's no resolution data attached to the
> returned image, code that uses moz-icon will need to be more careful to
> explictly set the size at which the resulting image is to be displayed,
> otherwise we'll get oversized images all over the place.

That sounds scary! Aside from the various other users of moz-icon I'm sure we have in-tree, addons also use this, and are going to be a bit harder to get updated...

Comment 14

[Jonathan Kew [:jfkthame]]

True... but such users of moz-icon are *currently* broken on Retina systems in the released version of Firefox anyway, because moz-icon is simply failing and giving them nothing, rather than the images they expect to get. I'm not sure getting a double-sized image is any worse.

FWIW, in the case of the downloads list and the directory listing, the fix to moz-icon (without the corresponding CSS fixes) didn't actually break anything - the icons showed up larger, but the UI elements adjusted themselves accordingly. I'd expect many use cases to be somewhat like that, unless the UI involves too many constrained-size elements.

Comment 15

[Jonathan Kew [:jfkthame]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/72e123c2602d
https://hg.mozilla.org/integration/mozilla-inbound/rev/8ab6b738b686
https://hg.mozilla.org/integration/mozilla-inbound/rev/f1df34df5f3b

Comment 16

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/72e123c2602d
https://hg.mozilla.org/mozilla-central/rev/8ab6b738b686
https://hg.mozilla.org/mozilla-central/rev/f1df34df5f3b

Comment 17

[Justin Dolske [:Dolske]]

Wow, fast turnaround! Thanks! \o/