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

[MC Return]

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)

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.

[Sam Spilsbury (smspillaz)]

"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.

[MC Return (mc-return)]

>
> 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.

[Sam Spilsbury (smspillaz)]

> 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

[MC Return (mc-return)]

> > 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...

[MC Return (mc-return)]

All adjusted to 512 now...

review: Needs Resubmitting

[MC Return (mc-return)]

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

[Sam Spilsbury (smspillaz)]

review: Approve

[PS Jenkins bot (ps-jenkins)]

FAILED: Autolanding.
More details in the following jenkins job:
http://jenkins.qa.ubuntu.com/job/compiz-autolanding/43/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/compiz-raring-amd64-autolanding/43/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/compiz-raring-armhf-autolanding/43/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/compiz-raring-i386-autolanding/43/console

review: Needs Fixing (continuous-integration)

[MC Return (mc-return)]

Was a stupid typo. Fixed.

review: Needs Resubmitting

[Sam Spilsbury (smspillaz)]

review: Approve

[PS Jenkins bot (ps-jenkins)]

FAILED: Autolanding.
More details in the following jenkins job:
http://jenkins.qa.ubuntu.com/job/compiz-autolanding/45/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/compiz-raring-amd64-autolanding/45/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/compiz-raring-armhf-autolanding/45/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/compiz-raring-i386-autolanding/45/console

review: Needs Fixing (continuous-integration)

[MC Return (mc-return)]

My fault -> fixing.

[MC Return (mc-return)]

Fixed (and recompiled to make sure).

review: Needs Resubmitting

[MC Return (mc-return)]

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

review: Needs Resubmitting

[MC Return (mc-return)]

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

[Sam Spilsbury (smspillaz)]

> 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

[PS Jenkins bot (ps-jenkins)]

review: Approve (continuous-integration)

[MC Return (mc-return)]

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.

=== modified file 'plugins/compiztoolbox/include/compiztoolbox/compiztoolbox.h'
--- plugins/compiztoolbox/include/compiztoolbox/compiztoolbox.h	2012-09-07 22:37:20 +0000
+++ plugins/compiztoolbox/include/compiztoolbox/compiztoolbox.h	2013-05-12 08:26:24 +0000
@@ -162,6 +162,6 @@
 };
 
 extern const unsigned short ICON_SIZE;
-extern const unsigned int MAX_ICON_SIZE;
+extern const unsigned short MAX_ICON_SIZE;
 
 #endif

=== modified file 'plugins/compiztoolbox/src/compiztoolbox.cpp'
--- plugins/compiztoolbox/src/compiztoolbox.cpp	2013-05-09 13:43:07 +0000
+++ plugins/compiztoolbox/src/compiztoolbox.cpp	2013-05-12 08:26:24 +0000
@@ -29,8 +29,8 @@
 #include <core/abiversion.h>
 #include <core/propertywriter.h>
 
-const unsigned short ICON_SIZE = 48;
-const unsigned int MAX_ICON_SIZE = 256;
+const unsigned short ICON_SIZE     = 512;
+const unsigned short MAX_ICON_SIZE = 512;
 
 bool openGLAvailable;
 

=== modified file 'plugins/kdecompat/src/kdecompat.cpp'
--- plugins/kdecompat/src/kdecompat.cpp	2013-05-09 13:43:07 +0000
+++ plugins/kdecompat/src/kdecompat.cpp	2013-05-12 08:26:24 +0000
@@ -288,7 +288,7 @@
 	}
 	else
 	{
-	    icon = gWindow->getIcon (256, 256);
+	    icon = gWindow->getIcon (512, 512);
 	    if (!icon)
 		icon = ks->gScreen->defaultIcon ();
 

=== modified file 'plugins/ring/src/ring.cpp'
--- plugins/ring/src/ring.cpp	2013-05-09 13:43:07 +0000
+++ plugins/ring/src/ring.cpp	2013-05-12 08:26:24 +0000
@@ -33,7 +33,7 @@
 
 const double PI = 3.14159265359f;
 
-const unsigned short ICON_SIZE = 64;
+const unsigned short ICON_SIZE = 512;
 
 bool textAvailable;
 
@@ -296,7 +296,7 @@
 	{
 	    GLTexture *icon;
 
-	    icon = gWindow->getIcon (256, 256);
+	    icon = gWindow->getIcon (512, 512);
 	    if (!icon)
 		icon = rs->gScreen->defaultIcon ();
 

=== modified file 'plugins/scale/src/scale.cpp'
--- plugins/scale/src/scale.cpp	2013-05-09 13:43:07 +0000
+++ plugins/scale/src/scale.cpp	2013-05-12 08:26:24 +0000
@@ -137,7 +137,7 @@
 	GLWindowPaintAttrib sAttrib (attrib);
 	GLTexture           *icon;
 
-	icon = priv->gWindow->getIcon (96, 96);
+	icon = priv->gWindow->getIcon (512, 512);
 	if (!icon)
 	    icon = priv->spScreen->gScreen->defaultIcon ();
 

=== modified file 'plugins/shift/src/shift.cpp'
--- plugins/shift/src/shift.cpp	2013-05-09 13:43:07 +0000
+++ plugins/shift/src/shift.cpp	2013-05-12 08:26:24 +0000
@@ -322,7 +322,7 @@
 	{
 	    GLTexture *icon;
 
-	    icon = gWindow->getIcon (256, 256);
+	    icon = gWindow->getIcon (512, 512);
 
 	    if (!icon)
 		icon = ss->gScreen->defaultIcon ();

=== modified file 'plugins/stackswitch/src/stackswitch.cpp'
--- plugins/stackswitch/src/stackswitch.cpp	2013-05-09 13:43:07 +0000
+++ plugins/stackswitch/src/stackswitch.cpp	2013-05-12 08:26:24 +0000
@@ -212,7 +212,7 @@
     glPushMatrix ();
     glLoadMatrixf (wTransform.getMatrix ());
 
-    icon = sw->gWindow->getIcon (96, 96);
+    icon = sw->gWindow->getIcon (512, 512);
 
     if (!icon)
 	icon = gScreen->defaultIcon ();
@@ -352,7 +352,7 @@
 
 	if (scaled && !gWindow->textures ().size ())
 	{
-	    GLTexture *icon = gWindow->getIcon (96, 96);
+	    GLTexture *icon = gWindow->getIcon (512, 512);
 
 	    if (!icon)
 		icon = ss->gScreen->defaultIcon ();

=== modified file 'plugins/staticswitcher/src/staticswitcher.cpp'
--- plugins/staticswitcher/src/staticswitcher.cpp	2013-05-09 13:43:07 +0000
+++ plugins/staticswitcher/src/staticswitcher.cpp	2013-05-12 08:26:24 +0000
@@ -27,6 +27,8 @@
 
 COMPIZ_PLUGIN_20090315 (staticswitcher, StaticSwitchPluginVTable)
 
+const unsigned short MAX_ICON_SIZE = 512;
+
 const unsigned short PREVIEWSIZE = 150;
 const unsigned short BORDER = 10;
 

=== modified file 'plugins/staticswitcher/src/staticswitcher.h'
--- plugins/staticswitcher/src/staticswitcher.h	2012-09-07 23:56:21 +0000
+++ plugins/staticswitcher/src/staticswitcher.h	2013-05-12 08:26:24 +0000
@@ -157,7 +157,7 @@
 	StaticSwitchScreen    *sScreen;
 };
 
-#define MAX_ICON_SIZE 256
+extern const unsigned short MAX_ICON_SIZE;
 
 extern const unsigned short PREVIEWSIZE;
 extern const unsigned short BORDER;