GTK common dialog icons (e.g. "Start Private Browsing" dialog) are pixelated

RESOLVED FIXED in mozilla1.9.1b3

Status

()

Core
Graphics
RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: dholbert, Assigned: Michael Ventnor)

Tracking

({fixed1.9.1})

Trunk
mozilla1.9.1b3
x86
Linux
fixed1.9.1
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9.1 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(7 attachments)

(Reporter)

Description

9 years ago
Created attachment 347194 [details]
screenshot 1: ugly question mark used in Firefox dialog

STR:
 1. Open "Tools" menu, and select "Private Browsing
 2. Observe icon used in the dialog that appears.

ACTUAL RESULTS:
 Dialog uses crappy-looking pixelated question mark icon. (see upcoming "screenshot 1")

EXPECTED RESULTS:
 Dialog should use clean-looking question mark icon. (see upcoming "screenshot 2")

(Note: This probably isn't a private-browsing-specific bug, AFAIK -- that's just where I've noticed this dialog recently.)

OS: Ubuntu 8.10
Browser: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1b2pre) Gecko/20081109 Minefield/3.1b2pre
(Reporter)

Updated

9 years ago
Component: Build Config → General
QA Contact: build.config → general
(Reporter)

Comment 1

9 years ago
Created attachment 347195 [details]
screenshot 2: pretty question mark, used in Gnome wallpaper app

Here's a screenshot of the clean-looking question mark, as used in the Gnome wallpaper app's file-picker.

This icon is slightly larger than the broken one in the Firefox dialog -- perhaps we're using this same icon, but we're scaling it down slightly, making it look broken...?
(Reporter)

Comment 2

9 years ago
Gavin says in IRC the issue is probably that my icons are 48x48, but Firefox's theme has them hardcoded to 40x40.

e.g. if I load  moz-icon://stock/gtk-dialog-question?size=dialog or  moz-icon://stock/gtk-dialog-info?size=dialog and then right-click and select 'properties', it says "Image Dimensions: 48px x 48px"
http://hg.mozilla.org/mozilla-central/annotate/d8295ec827fa/toolkit/themes/gnomestripe/global/global.css#l91

We hardcode the size to 40x40, but apparently they're actually 48x48 in dholbert's theme.
Component: General → Themes
Product: Firefox → Toolkit
QA Contact: general → themes
Summary: "Start Private Browsing" dialog uses pixelated GTK question mark icon → GTK common dialog icons (e.g. "Start Private Browsing" dialog) are pixelated
I wonder if they're a different size in 8.10 than they were in 8.04?
40x40 is a very bad idea, gtk/freedesktop only support 32x32 or 48x48. Note that GTK themes can change this size so what Firefox should do is get the actual GTK_STOCK_DIALOG (?) size and use this size.
Are we doing this correctly with the quit dialog?
(Reporter)

Comment 7

9 years ago
Created attachment 347473 [details]
screenshot 3: ugly question mark in 'quit' dialog

(In reply to comment #6)
> Are we doing this correctly with the quit dialog?

No -- it's ugly there, as well.  (see newly-attached screenshot)
My Ubuntu 8.04 says they were 48x48 then, too.
Created attachment 347718 [details]
screenshot

I've the ugly icon as well, but this is on 8.04 with some updates.
Looks like the justification for 40x40 is in bug 402839 comment 7. Does removing the explicit size really cause problems with the text?
pretty icon: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1b2pre) Gecko/20081104
Minefield/3.1b2pre

bad icon: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1b2pre) Gecko/20081105
Minefield/3.1b2pre
Perhaps this is a regression from bug 458487, then?
(Reporter)

Comment 13

9 years ago
(In reply to comment #12)
> Perhaps this is a regression from bug 458487, then?

Confirmed regression from that bug.
hg revert --all -r 2d35868314cf --> Pretty icon on Private Browsing dialog
hg revert --all -r 49dd935efff3 --> Bad icon on on Private Browsing dialog

(that second change ID is bug 458487's checkin, and the first is its parent)
Blocks: 458487
Maybe bug 458487 triggered it but the underlying problem is the rescaling of the icon from 48x48 to 40x40, right?
(Reporter)

Comment 15

9 years ago
Right -- perhaps before that, we were *supposed* to rescale, but we were failing at doing it (and thus inadvertently succeeding at keeping pretty icons).
(Reporter)

Comment 16

9 years ago
(In reply to comment #3)
> http://hg.mozilla.org/mozilla-central/annotate/d8295ec827fa/toolkit/themes/gnomestripe/global/global.css#l91
> 
> We hardcode the size to 40x40, but apparently they're actually 48x48 in
> dholbert's theme.

The change to 40x40 was made recently, in bug Bug 402839. (before that, it was 32x32) --> Adding dependency.
Blocks: 402839
(Assignee)

Comment 17

9 years ago
Why did we regress the good image downscaling?

The decision for 40x40 was made ages ago. We chose that size because we can't detect actual size so that if a theme has 32x32 dialog icons (and they do exist) the upscaling doesnt look as bad in 40x40 as 48x48. But GTK generally has larger icons for dialogs and 32x32 really didn't look good as a dialog icon in the environment.
(Assignee)

Comment 18

9 years ago
We need to fix bug 458487 rather than the image size. Fixing the dialog image size doesn't fix the look of every other downscaled image on the web that now looks revolting on Linux.
(Assignee)

Comment 19

9 years ago
What happened before was that downscaling images used the GOOD filter and upscaling images used the FAST filter, which worked perfectly fine. I can't find anything in nsThebesImage::Draw that measures the scale anymore, though...
(Assignee)

Comment 20

9 years ago
Created attachment 347954 [details] [diff] [review]
Fix downscaling

This re-re-re-fixes the smooth downscaling on Linux. Honestly, why do the gfx guys always regress this? Don't they ever test their patches on Linux?

If only I could think of a way of doing so, I would LOVE to write a test for this to light a fire under them if they regress it again.
Attachment #347954 - Flags: superreview?(roc)
Attachment #347954 - Flags: review?(roc)
(Assignee)

Comment 21

9 years ago
Did I mention this is a regression from FF3? Just an incentive for the blocker flag, is all...
Just because we're downscaling doesn't mean we won't sample outside the image, AFAIK.
Comment on attachment 347954 [details] [diff] [review]
Fix downscaling

However, restoring that dubious old code probably is the right thing to do for now.
Attachment #347954 - Flags: superreview?(roc)
Attachment #347954 - Flags: review?(vladimir)
Attachment #347954 - Flags: review?(roc)
(Assignee)

Comment 24

9 years ago
(In reply to comment #22)
> Just because we're downscaling doesn't mean we won't sample outside the image,
> AFAIK.

I've never actually seen a case where sampling outside the image on downscaling was a problem for a web page. But, it didn't take me 3 sites to always find a page where images now looked unreasonably and unintentionally pixellated because we got rid of this downscaling filtering.
(Assignee)

Updated

9 years ago
Flags: blocking1.9.1?
Component: Themes → GFX: Thebes
Product: Toolkit → Core
QA Contact: themes → thebes
(In reply to comment #20)
> Created an attachment (id=347954) [details]
> Fix downscaling
> 
> This re-re-re-fixes the smooth downscaling on Linux. Honestly, why do the gfx
> guys always regress this? Don't they ever test their patches on Linux?
>
> If only I could think of a way of doing so, I would LOVE to write a test for
> this to light a fire under them if they regress it again.

Yes, because bluster and vitriol are exactly what's needed here.  Or something.  Perhaps you can redirect it towards fixing the broken crap upstream so that we can stop having to do workarounds and workarounds for said linux-only broken crap.
Duplicate of this bug: 464798
Also, it was me, not the "gfx guys", who regressed it.
(Assignee)

Comment 28

9 years ago
Sorry, I guess I could have worded that post better as I didn't mean for it to sound rude. But this has been regressed many times in the past and am still wondering why nearest neighbour should be applied to downscaling whenever this code is changed.
(Assignee)

Comment 29

9 years ago
Also, what is needed for EXTEND_PAD to be possible on X11? Is it an XRender deficiency?
(Assignee)

Comment 30

9 years ago
Comment on attachment 347954 [details] [diff] [review]
Fix downscaling

One last thing, would it still be possible to get this in beta 2? If we don't we could probably expect a lot of duplicates about ugly icons and images.
Attachment #347954 - Flags: approval1.9.1b2?
Flags: blocking1.9.1? → blocking1.9.1+
Target Milestone: --- → mozilla1.9.1
Comment on attachment 347954 [details] [diff] [review]
Fix downscaling

a1.9.1b2=beltzner, but we won't hold b2 for this; it does block final release.
Attachment #347954 - Flags: approval1.9.1b2? → approval1.9.1b2+
(Assignee)

Updated

9 years ago
Keywords: checkin-needed
(Reporter)

Comment 32

9 years ago
I'd be happy to land this, with/after the other a1.9b2 patch that I'm landing on https://wiki.mozilla.org/1.9.1_beta_2_checkins .
Assignee: nobody → ventnor.bugzilla
(Reporter)

Comment 33

9 years ago
Pushed:
http://hg.mozilla.org/mozilla-central/rev/e4690fcf6f7c
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
(In reply to comment #29)
> Also, what is needed for EXTEND_PAD to be possible on X11? Is it an XRender
> deficiency?

Yes.

Updated

9 years ago
Keywords: checkin-needed
Target Milestone: mozilla1.9.1 → mozilla1.9.1b2
(Reporter)

Comment 35

9 years ago
Created attachment 348241 [details]
reftest failures (including screenshots of tests)

This patch made these two reftests failed on Linux -- I may back out soon, unless it's determined that the tests are broken.
(Reporter)

Comment 36

9 years ago
The reftests are:
http://mxr.mozilla.org/mozilla-central/source/layout/reftests/bugs/403181-1.xml
http://mxr.mozilla.org/mozilla-central/source/layout/reftests/bugs/421885-1.xml

Note that these tests just use *up* scaling (one by 2.5, one by 2)... which is bad, because in theory this bug's patch was supposed to only change down-scaling, I think.

Backing out & reopening.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: mozilla1.9.1b2 → ---
(Reporter)

Updated

9 years ago
Attachment #348241 - Attachment description: reftest failures → reftest failures (including screenshots of tests)
(Reporter)

Comment 37

9 years ago
In the reftest failures (in attachment 348241 [details]), it looks like the reference cases have solid black shapes, whereas the testcases have shapes with a 1px fuzzy gray border.
(Assignee)

Comment 38

9 years ago
Roc, does aFill or aSubimage represent the source/destination sizes? What the function used to do was calculate the scale itself, which seemed to work well.

I could also be misunderstanding what the values in the transform matrix mean (see, this is why I want to avoid the gfx code :) )
(Assignee)

Comment 39

9 years ago
aFill may be what I want, but that seems to be transformed somehow since using that still doesn't fix the reftests. What I need is the untransformed source image and destination rect sizes and so far I haven't found a way of doing that without adding parameters to Draw() which I want to avoid. roc, are these values available in the Draw() function?
I'm dumb. Let me write the patch for this.
Created attachment 348464 [details] [diff] [review]
fix

When we're downscaling, the transform from device space to image space is actually scaling *up* coordinates. E.g. if you're scaling down by a factor of 2 and no translation is needed, then device coordinate (1,1) maps to image pixel (2,2).

Michael, please test this. If it works, and passes reftests, get review from vlad.
(Assignee)

Comment 42

9 years ago
The 403181 reftest still doesn't pass. 421885 does though. I'm wondering why it fails, 2.5 scale using foreignObject is supposed to scale up, isn't it?
Yes.

We can work on this when you get here next week if you like.
Attachment #347954 - Flags: approval1.9.1b2+
(Assignee)

Comment 44

9 years ago
Robert, it seems 403181 fails on my machine anyway even without your patch. In fact, when comparing the unexpected fails on my machine with your patch vs w/o they are exactly the same.

We should take your fix. r=me (ok, not really)
(Assignee)

Updated

9 years ago
Attachment #348464 - Flags: review?(vladimir)
(Assignee)

Comment 45

9 years ago
Comment on attachment 348464 [details] [diff] [review]
fix

vlad: roc asked me to set r? for him.
Pushed to trunk as 8e22374a4c8a
Status: REOPENED → RESOLVED
Last Resolved: 9 years ago9 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [needs landing] → [needs 191 landing]
Keywords: fixed1.9.1
(sorry, that fixed1.9.1 was added in error - as per comments this has yet to land on 1.9.1)
Keywords: fixed1.9.1
Pushed e54a9f162ee1 to 1.9.1
Keywords: fixed1.9.1
Whiteboard: [needs 191 landing]
Target Milestone: --- → mozilla1.9.1b3
You need to log in before you can comment on or make changes to this bug.