Bug 355684

Thanks.

I still don't have any new info to go on. I'm kinda stuck

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.

For clarification, I meant to say the icons become visible with a black background, not with a transparent one.

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
    
https://quickgit.kde.org/?p=spectacle.git&a=commit&h=7e30d1d6f73803a00042e6267b077b60cd80ac15

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.

*** Bug 355418 has been marked as a duplicate of this bug. ***

*** Bug 356430 has been marked as a duplicate of this bug. ***

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.

Rakyn, what's your surname. I need it for the commit author.

*** Bug 356514 has been marked as a duplicate of this bug. ***

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.

Created attachment 96044 [details]
Patch for xembed-sni-proxy-0~git20151104-ded1538

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

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

Can you name a few applications which worked fine before please?
I would like to check.

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?

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.

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.

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.

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  ? 
https://techbase.kde.org/Policies/Kdelibs_Coding_Style

  - make methods const

Works fine here too.
There is a typo in na(t)ive.

Sorry no typo, i think it is just my poor english.

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

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

http://commits.kde.org/plasma-workspace/41df1bdb8b478eb27fb424d201c075c76ec0ed5a