Merge lp://qastaging/~mc-return/compiz/compiz.merge-fix1173914-low-res-non-power-of-2-icon-texture-res into lp://qastaging/compiz/0.9.10

Proposed by MC Return
Status: Merged
Approved by: Sam Spilsbury
Approved revision: 3691
Merged at revision: 3695
Proposed branch: lp://qastaging/~mc-return/compiz/compiz.merge-fix1173914-low-res-non-power-of-2-icon-texture-res
Merge into: lp://qastaging/compiz/0.9.10
Diff against target: 135 lines (+13/-11)
9 files modified
plugins/compiztoolbox/include/compiztoolbox/compiztoolbox.h (+1/-1)
plugins/compiztoolbox/src/compiztoolbox.cpp (+2/-2)
plugins/kdecompat/src/kdecompat.cpp (+1/-1)
plugins/ring/src/ring.cpp (+2/-2)
plugins/scale/src/scale.cpp (+1/-1)
plugins/shift/src/shift.cpp (+1/-1)
plugins/stackswitch/src/stackswitch.cpp (+2/-2)
plugins/staticswitcher/src/staticswitcher.cpp (+2/-0)
plugins/staticswitcher/src/staticswitcher.h (+1/-1)
To merge this branch: bzr merge lp://qastaging/~mc-return/compiz/compiz.merge-fix1173914-low-res-non-power-of-2-icon-texture-res
Reviewer Review Type Date Requested Status
PS Jenkins bot (community) continuous-integration Approve
Sam Spilsbury Approve
MC Return Needs Information
Review via email: mp+161324@code.qastaging.launchpad.net

Commit message

Ring, Shift and Stack Window Switcher:

Increased the non-power-of-2 icon texture resolution from 96x96 to 512x512 pixels,
in the case of the ring switcher from 64 to 512.

Static Switcher:

Replaced #define of MAX_ICON_SIZE with type-safe extern const unsigned short and
set the icon resolution to 512 as well.

Compiztoolbox:

Increased the non-power-of-2 icon texture resolution const unsigned short ICON_SIZE
from 48 to 512 and const unsigned int MAX_ICON_SIZE from 256 to 512 as well.
Changed the type of MAX_ICON_SIZE from const unsigned int to const unsigned short.

Scale:

Increased the non-power-of-2 icon texture resolution from 96x96 to 512x512 pixels.

KDE Compatibility:

Increased the icon texture resolution from 256x256 to 512x512 pixels.

(LP: #1173914)

Description of the change

1. Texture sizes should *always* be power-of-2, otherwise they will be internally upscaled to the next higher power-of-2 resolution, which means this comes with a loss of quality (with all the known scaling issues).

Non-power-of-2 resolutions should *never* be used for textures.

Example: 96x96 becomes 128x128 internally, so the 96 pixels have to be upscaled to 128, which creates scaling artifacts.

2. 96x96 or even lower sizes are not good sizes for icons, especially if they get upscaled to 50% of the screen width in the case of "Big" overlay icon modes of our Switchers (Shift, Stack, Ring) - please see bug #1173694 for details on this issue.

[Solution]

We should increase the icon texture sizes to at least 256x256, which is quite the standard in 2013.
We should investigate if we could force Compiz to load the .svg version of the icon, if available, because vectorgraphics could be scaled without any artifacts.

* IMPORTANT NOTE *:
===================

This MP does unfortunately *not* fix bug #1173916 and bug #1173694 (which are also icon related), but it is a first step.

To post a comment you must log in.
Revision history for this message
Sam Spilsbury (smspillaz) wrote :

"Non-power-of-2 resolutions should *never* be used for textures.

Example: 96x96 becomes 128x128 internally, so the 96 pixels have to be upscaled to 128, which creates scaling artifacts."

Most hardware supports non-power-of-two textures. I have not heard of any hardware that does internal upscaling.

In any case, there is no difference between MAX_ICON_SIZE and ICON_SIZE. The constant can be the same - 512 everywhere. CompWindow::getIcon () will go for the largest sized icon first and then smaller ones after that.

Revision history for this message
MC Return (mc-return) wrote :

>
> Most hardware supports non-power-of-two textures. I have not heard of any
> hardware that does internal upscaling.
>
Well, *every* graphics hardware I know of does internal upscaling of non-power-of-2 textures.
Of course almost every hardware supports those, but upscaling creates artifacts and makes troubles with mipmapping as well.

> In any case, there is no difference between MAX_ICON_SIZE and ICON_SIZE. The
> constant can be the same - 512 everywhere. CompWindow::getIcon () will go for
> the largest sized icon first and then smaller ones after that.

Well, somewhere around the getIcon () code something's definitely wrong -> see bug #1173916 and bug #1173694.

Revision history for this message
Sam Spilsbury (smspillaz) wrote :

> Well, somewhere around the getIcon () code something's definitely wrong -> see
> bug #1173916 and bug #1173694.

I'm confused by what you mean by this.

Using the svg icon data from the current desktop icon theme is a very large undertaking, and is often quite a messy procedure because the process used to match applications to windows is at best inexact on the EWMH standard. This is the entire reason why lp:bamf exists.

As for the icons themselves, the EWMH says that the applications can specify an arbitrary array of arbitrarily sized icons in their window properties[1]. It is up to the application to specify a high resolution icon. Note that the application is actually specifying the raw bitmap data that makes up the icon in each item of the array of icon properties as opposed to a filename.

The parameters to getIcon () specify the _maximum_ allowed icon dimensions (both width and height) to be used. So, for example if an application uploads icons sizes 32x32, 64x64, 128x128, 256x256 and 512x512 and we call getIcon with (240), then it will pick the largest icon below that size, that being the 128x128 icon size.

Calling getIcon () with a certain parameter doesn't guarantee that you'll get an icon that size, but it does guarantee that you won't get an icon larger than it.

In any case, setting maximum of 512 should more than cover any icon size bottlenecks on the compiz side - its just up to the individual applications to provide larger icons.

[1] http://standards.freedesktop.org/wm-spec/1.3/ar01s05.html

Revision history for this message
MC Return (mc-return) wrote :

> > Well, somewhere around the getIcon () code something's definitely wrong ->
> see
> > bug #1173916 and bug #1173694.
>
> I'm confused by what you mean by this.
>
Well, I just meant our switchers should act more intelligent and use the best-quality icons available, not low-res ones.

> Using the svg icon data from the current desktop icon theme is a very large
> undertaking, and is often quite a messy procedure because the process used to
> match applications to windows is at best inexact on the EWMH standard. This is
> the entire reason why lp:bamf exists.
>
Ah -> this explains why the Unity Switcher is doing this thing right (had no time to look at the code yet).
We should, in this case, check if bamf is available and mimic the icon selection behaviour of the Unity Switcher in all other Compiz Switchers then...

> As for the icons themselves, the EWMH says that the applications can specify
> an arbitrary array of arbitrarily sized icons in their window properties[1].
> It is up to the application to specify a high resolution icon. Note that the
> application is actually specifying the raw bitmap data that makes up the icon
> in each item of the array of icon properties as opposed to a filename.
>
A very interesting document. Will study that in detail later.

> The parameters to getIcon () specify the _maximum_ allowed icon dimensions
> (both width and height) to be used. So, for example if an application uploads
> icons sizes 32x32, 64x64, 128x128, 256x256 and 512x512 and we call getIcon
> with (240), then it will pick the largest icon below that size, that being the
> 128x128 icon size.
>
I could not find the problem in getIcon (), so my guess was it is "somewhere around that code"...

> Calling getIcon () with a certain parameter doesn't guarantee that you'll get
> an icon that size, but it does guarantee that you won't get an icon larger
> than it.
>
Ok.

> In any case, setting maximum of 512 should more than cover any icon size
> bottlenecks on the compiz side - its just up to the individual applications to
> provide larger icons.
>
> [1] http://standards.freedesktop.org/wm-spec/1.3/ar01s05.html

I agree.
Nevertheless the Compiz Switchers should be tuned to be able to deliver high quality icons like the Unity Switcher already does...

Revision history for this message
MC Return (mc-return) wrote :

All adjusted to 512 now...

review: Needs Resubmitting
Revision history for this message
MC Return (mc-return) wrote :

This here is a good example for bug #1178237 - "Commonly used constants should be defined just once globally and be static" by the way...

Revision history for this message
Sam Spilsbury (smspillaz) :
review: Approve
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
MC Return (mc-return) wrote :

Was a stupid typo. Fixed.

review: Needs Resubmitting
Revision history for this message
Sam Spilsbury (smspillaz) :
review: Approve
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
MC Return (mc-return) wrote :

My fault -> fixing.

Revision history for this message
MC Return (mc-return) wrote :

Fixed (and recompiled to make sure).

review: Needs Resubmitting
Revision history for this message
MC Return (mc-return) wrote :

Improved this a little bit, when I'm already on it.

review: Needs Resubmitting
Revision history for this message
MC Return (mc-return) wrote :

Question:

How about simply adding

+static const unsigned short MAX_ICON_SIZE = 512;

to composite.h and using this variable everywhere, instead of defining 512 several times throughout the code ? We could keep this variable static in memory and use it everywhere appropriately.

If okay, should this be done in this MP or some follow-up ?

review: Needs Information
Revision history for this message
Sam Spilsbury (smspillaz) wrote :

> Question:
>
> How about simply adding
>
> +static const unsigned short MAX_ICON_SIZE = 512;
>
> to composite.h and using this variable everywhere, instead of defining 512
> several times throughout the code ? We could keep this variable static in
> memory and use it everywhere appropriately.
>
> If okay, should this be done in this MP or some follow-up ?

It might be better to change the default parameters of getIcon () to be int w = 512, int h = 512 and making plugins call the function with the default params (eg, as screen->getIcon ()).

In any case, this is fine as is.

review: Approve
Revision history for this message
PS Jenkins bot (ps-jenkins) :
review: Approve (continuous-integration)
Revision history for this message
MC Return (mc-return) wrote :

It might be better to change the default parameters of getIcon () to be int w = 512, int h = 512 and making plugins call the function with the default params (eg, as screen->getIcon ()).

Sounds good. I'll try to look into that in some follow-up.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches

to all changes: