Bug 355684

Summary: xembedsniproxy system tray icons are blank
Product: [Plasma] plasmashell Reporter: Benedikt Gollatz <benedikt>
Component: XembedSNIProxyAssignee: Plasma Bugs List <plasma-bugs>
Severity: normal CC: ad1rie3, darkbasic, demm, fademind, hioeribhr, javier.paya, jeremy9856, kde, koras, lukas.schneiderbauer, rapiteanu.catalin, rdieter, vasyl.demin, w01dnick, zellox
Priority: NOR    
Version: 5.4.3   
Target Milestone: 1.0   
Platform: Fedora RPMs   
OS: Linux   
Latest Commit: Version Fixed In:
Attachments: Patch for xembed-sni-proxy-0~git20151104-ded1538
Patch for xembed-sni-proxy-0~git20151104-ded1538
Try naive conversion, only then use alternative copying method

Description Benedikt Gollatz 2015-11-21 11:06:11 UTC
On one of my machines, all system tray icons placed by xembedsniproxy are blank. This is probably the same issue as <https://github.com/davidedmundson/xembed-sni-proxy/issues/28>. I believe I have some additional information (see below), but since the github repository is closed, I am filing the bug here.

Reproducible: Always

Steps to Reproduce:
1. Start an X session with plasmashell and xembedsniproxy.
2. Run an application using a legacy tray icon, such as pidgin or hexchat.
3. Look at the system tray.

Actual Results:  
For all applications with legacy system tray icons, the icons displayed are blank. They still react to mouse events, such as toggling visibility on left click and showing a context menu on right click.

Expected Results:  
There should be non-transparent icons in the system tray for these applications.

I have two fairly similarly set-up machines running Plasma 5 (both running Fedora 23), but only one exhibits this problem. The biggest difference that comes to mind is that the unproblematic machine uses the i915 graphics driver, whereas the problematic one uses an nvidia graphics chip (though the problem occurs both using the nouveau and the proprietary drivers).

On the problematic machine, xembedsniproxy will print debug messages such as

    kde.xembedsniproxy: Skip transparent xembed icon for 60819945 "Pidgin"

Other than this, I don't observe any significant differences in log output.

Replacing the (in any case fine looking) transparency detection code in SNIProxy::update() with a simple "isTransparentImage = false;" doesn't alleviate the problem; the icon received through XCB actually appears to be transparent.
Comment 1 David Edmundson 2015-11-22 00:40:33 UTC

I still don't have any new info to go on. I'm kinda stuck
Comment 2 Benedikt Gollatz 2015-11-22 09:15:38 UTC
Inspecting the image data received through XCB, I can see that the image is there, only the alpha channel is set to transparent for each pixel.

If I add

    for (int i = 0; i < image->width * image->height; i++)
        image->data[i * image->bpp / 8 - 1] = 0xFF;

before the conversion to a QImage in SNIProxy::getImageNonComposite(), the icons become visible (though of course with a transparent background).

I suppose this can function as a workaround for those affected and willing to compile their own version of xembedsniproxy.
Comment 3 Benedikt Gollatz 2015-11-22 09:19:01 UTC
For clarification, I meant to say the icons become visible with a black background, not with a transparent one.
Comment 4 Rakyn Barker 2015-12-07 04:52:10 UTC
Created attachment 95921 [details]
Patch for xembed-sni-proxy-0~git20151104-ded1538

Found a nice image load function in another kde commit which works for me
Comment 5 Benedikt Gollatz 2015-12-07 06:04:18 UTC
Using Rakyn's patch, I get visible icons with a black background on both of my systems, i.e. including the one where icon display worked well previously. This is because for the icons grabbed those two systems, xcbImage->depth is 24, whereas xcbImage->bpp is 32. This is in line with the comments in the xcb-util-image source code:

  uint8_t            depth;   /**< Depth in bits. Valid depths
                   *   are 1, 4, 8, 16, 24 for z format,
                   *   1 for xy-bitmap-format, anything
                   *   for xy-pixmap-format.
  uint8_t            bpp;   /**< Storage per pixel in bits.
                 *   Must be >= depth. Valid bpp
                 *   are 1, 4, 8, 16, 24, 32 for z
                 *   format, 1 for xy-bitmap format,
                 *   anything for xy-pixmap-format.

Effectively, what Rakyn's code does in this case, is simply use QImage::Format_RGB32 always and ignore the alpha channel completely (basically, what I did with my hacky workaround above). Once one changes xcbImage->depth to xcbImage->bpp in the switch statement, the old behaviour of the unpatched version returns.
Comment 6 David Edmundson 2015-12-11 11:20:11 UTC
*** Bug 355418 has been marked as a duplicate of this bug. ***
Comment 7 David Edmundson 2015-12-11 11:20:34 UTC
*** Bug 356430 has been marked as a duplicate of this bug. ***
Comment 8 David Edmundson 2015-12-11 11:31:01 UTC
Patch looks good. 

In theory I specific what format embedded clients should be using, but they also seem to just ignore it.

In future, if you can put patches on git.reviewboard.kde.org it's a bit easier for me to comment especially if we have some back and forth.  (like to say that method should be const)

and otherwise they get lost.
Comment 9 David Edmundson 2015-12-11 11:39:13 UTC
Rakyn, what's your surname. I need it for the commit author.
Comment 10 David Edmundson 2015-12-12 21:46:19 UTC
*** Bug 356514 has been marked as a duplicate of this bug. ***
Comment 11 koras 2015-12-12 22:54:38 UTC
Sorry for a duplicate I submitted. The same problem here. Arch Linux, Nvidia driver. I will be glad to provide all the info needed to resolve it completely.
Comment 12 Rakyn Barker 2015-12-13 17:36:03 UTC
Created attachment 96044 [details]
Patch for xembed-sni-proxy-0~git20151104-ded1538
Comment 13 Rakyn Barker 2015-12-13 17:38:03 UTC
(In reply to David Edmundson from comment #9)
> Rakyn, what's your surname. I need it for the commit author.
Rakyn Barker

Added sort of artificial alpha mask to the icons,
posting the patch at this place one last time.
Comment 14 Benedikt Gollatz 2015-12-13 17:48:43 UTC
This patch is still a regression on machines where transparency worked fine in the first place. It is visible e.g. in the hexchat icon, where createHeuristicMap will leave the black spots in the middle of the icon unchanged and not detect the semitransparency at the edges of the icon (leaving a black seam).
Comment 15 Rakyn Barker 2015-12-13 18:15:39 UTC
Can you name a few applications which worked fine before please?
I would like to check.
Comment 16 David Edmundson 2015-12-13 18:23:01 UTC
I don't see how it's a regression? your new section only applies to images where depth == 24, so they wouldn't have had any transparency anyway?
Comment 17 David Edmundson 2015-12-13 18:27:25 UTC
My usual test suite is

xchat, tuxguitar, spotify

as that's:
gtk, java, qt

then I get someone else to test Steam via Wine as that normally behaves weirdly.

I've just run the 3 above. First two are fine, spotify isn't showing up at all for me, but that's not related to your changes.
Comment 18 Benedikt Gollatz 2015-12-13 18:32:34 UTC
Like I said in comment #5, depth is 24 even for transparent images (bpp is 32), both in practice and according to xcb-util-image documentation.

I'm currently preparing a patch that only uses Rakyn's conversion routine when the previous method didn't work, which should give us the best of both worlds.
Comment 19 Benedikt Gollatz 2015-12-13 18:44:08 UTC
Created attachment 96045 [details]
Try naive conversion, only then use alternative copying method

This patch doesn't regress on my machine where the current version works already, though I suppose it should be checked whether it reintroduces the WINE bug mentioned in the source code. The Battle.net launcher icon works fine on my machine though.

One could probably also use createMaskFromColor(qRgba(0, 0, 0, 0)) instead of createHeuristicMask() in Rakyn's copying routine, which gives somewhat better results with the hexchat icon, but might of course be worse for other icons that use the black colour intentionally.
Comment 20 David Edmundson 2015-12-13 19:03:33 UTC
Seems OK to me.

Code review: 
   - I don't like how update() has a call to isTransparentImage even when we potentially we've already done that.

   can we make getImageNonComposite  have a second isTransparentImage  after convertFromNative, and if applicable return  a null QImage. Then remove the check from update(). It groups the logic together

  - don't leave commented out code
+    //QImage image(xcbImage->data, xcbImage->width, xcbImage->height, format);

 - why did you remove the braces in isTransparentImage  ? 

  - make methods const
Comment 21 Rakyn Barker 2015-12-13 19:28:01 UTC
Works fine here too.
There is a typo in na(t)ive.
Comment 22 Rakyn Barker 2015-12-13 19:38:08 UTC
Sorry no typo, i think it is just my poor english.
Comment 23 Benedikt Gollatz 2015-12-13 20:06:23 UTC
David, I believe I've addressed your points, and I've posted the new version of the patch as review request #126336: https://git.reviewboard.kde.org/r/126336/

Rakyn, yes, I mean naive as opposed to sophisticated/elaborate. It should be clearer in the latest version of the patch. :)
Comment 24 David Edmundson 2015-12-13 20:49:35 UTC
Git commit 41df1bdb8b478eb27fb424d201c075c76ec0ed5a by David Edmundson, on behalf of Benedikt Gollatz.
Committed on 13/12/2015 at 20:49.
Pushed by davidedmundson into branch 'Plasma/5.5'.

Mitigate failed icon grabbing in xembed-sni-proxy

If grabbed icons are blank, try to salvage the copied data as well as
possible while leaving setups where image grabbing works fine alone.

Based on a patch by Rakyn Barker.
REVIEW: 126336

M  +110  -33   xembed-sni-proxy/sniproxy.cpp
M  +4    -1    xembed-sni-proxy/sniproxy.h