Re: Review Request 112755: Reimplement KXUtils::createPixmapFromHandle with XCB

[Martin Gräßlin]

---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/112755/
---

(Updated Nov. 18, 2013, 8:58 a.m.)


Status
--

This change has been marked as submitted.


Review request for KDE Frameworks.


Repository: kdelibs


Description
---

Implements the createPixmapFromHandle by getting the image for the pixmaps and 
using it as either the Pixmap or the bitmap mask.


Diffs
-

  tier1/kwindowsystem/src/kxutils.cpp 33bd678 
  tier1/kwindowsystem/src/kxutils_p.h 84d639b 
  tier1/kwindowsystem/tests/CMakeLists.txt 0060903 
  tier1/kwindowsystem/tests/createpixmapfromhandletest.cpp PRE-CREATION 

Diff: http://git.reviewboard.kde.org/r/112755/diff/


Testing
---

Adjusted KWin to take this codepath and say thanks to Iceweasel for having a 
mask


Thanks,

Martin Gräßlin

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 112755: Reimplement KXUtils::createPixmapFromHandle with XCB

[Commit Hook]

---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/112755/#review43873
---


This review has been submitted with commit 
b025c4be103313b59a9c37ec9cc907a245df293b by Martin Gräßlin to branch frameworks.

- Commit Hook


On Nov. 6, 2013, 6:28 a.m., Martin Gräßlin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/112755/
> ---
> 
> (Updated Nov. 6, 2013, 6:28 a.m.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Repository: kdelibs
> 
> 
> Description
> ---
> 
> Implements the createPixmapFromHandle by getting the image for the pixmaps 
> and using it as either the Pixmap or the bitmap mask.
> 
> 
> Diffs
> -
> 
>   tier1/kwindowsystem/src/kxutils.cpp 33bd678 
>   tier1/kwindowsystem/src/kxutils_p.h 84d639b 
>   tier1/kwindowsystem/tests/CMakeLists.txt 0060903 
>   tier1/kwindowsystem/tests/createpixmapfromhandletest.cpp PRE-CREATION 
> 
> Diff: http://git.reviewboard.kde.org/r/112755/diff/
> 
> 
> Testing
> ---
> 
> Adjusted KWin to take this codepath and say thanks to Iceweasel for having a 
> mask
> 
> 
> Thanks,
> 
> Martin Gräßlin
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 112755: Reimplement KXUtils::createPixmapFromHandle with XCB

[Martin Gräßlin]

---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/112755/
---

(Updated Nov. 6, 2013, 7:28 a.m.)


Review request for KDE Frameworks.


Changes
---

support for depth 30


Repository: kdelibs


Description
---

Implements the createPixmapFromHandle by getting the image for the pixmaps and 
using it as either the Pixmap or the bitmap mask.


Diffs (updated)
-

  tier1/kwindowsystem/src/kxutils.cpp 33bd678 
  tier1/kwindowsystem/src/kxutils_p.h 84d639b 
  tier1/kwindowsystem/tests/CMakeLists.txt 0060903 
  tier1/kwindowsystem/tests/createpixmapfromhandletest.cpp PRE-CREATION 

Diff: http://git.reviewboard.kde.org/r/112755/diff/


Testing
---

Adjusted KWin to take this codepath and say thanks to Iceweasel for having a 
mask


Thanks,

Martin Gräßlin

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 112755: Reimplement KXUtils::createPixmapFromHandle with XCB

[Fredrik Höglund]

> On Oct. 29, 2013, 2:42 a.m., Fredrik Höglund wrote:
> > Looks much better, but it doesn't handle depth 30 pixmaps.
> 
> Martin Gräßlin wrote:
> I'm lacking ideas on how to test this. Do you know any application which 
> uses 30 bit pixmaps?
> 
> Fredrik Höglund wrote:
> The default depth is 30 when you're using the NVIDIA driver and the 
> monitor
> supports 30 bits. This is the reason why it's important.
> 
> The X server can be told to use a 30 bit visual even when the monitor 
> only supports
> 24 bits by starting it with -depth 30. But this isn't supported by all 
> drivers.
> 
> Note that the raster graphics system is completely broken when the 
> default depth
> is 30 since Qt assumes that any depth >= 24 means 8 bits per channel.
>
> 
> Martin Gräßlin wrote:
> thanks, that did work. I had tried Xephyr yesterday which didn't work and 
> didn't think of running X with -depth 30.
> 
> I have some code now, but have no idea whether that's correct (bit 
> manipulation is not what I'm very used to ;-)
> 
> case 30: {
> // Qt doesn't have a matching image format. We need to convert 
> manually
> uint8_t *origData = xcb_get_image_data(xImage.data());
> uint8_t *converted = new 
> uint8_t[xcb_get_image_data_length(xImage.data())];
> for (int i = 0; i < xcb_get_image_data_length(xImage.data()); i += 4) 
> {
> uint32_t origWord = 0x;
> for (int j = 0; j < 4; ++j) {
> if (j > 0) {
> origWord = origWord << 8;
> }
> origWord |= origData[i+j];
> }
> converted[i]   = uint8_t((float(origWord & 0xc000) / 
> 0xc000) * 0xff);
> converted[i+1] = uint8_t((float(origWord & 0x3ff0) / 
> 0x3ff0) * 0xff);
> converted[i+2] = uint8_t((float(origWord & 0x000ffc00) / 
> 0x000ffc00) * 0xff);
> converted[i+3] = uint8_t((float(origWord & 0x03ff) / 
> 0x03ff) * 0xff);
> }
> QImage image(converted, geo->width, geo->height,
>  xcb_get_image_data_length(xImage.data())/geo->height, 
> QImage::Format_ARGB32_Premultiplied);
> if (image.isNull()) {
> return T();
> }
> T result = T::fromImage(image);
> delete[] converted;
> return result;
> }
> 
> Testing is rather difficult. For Qt applications I'm rather sceptic that 
> the icon has a correct color in the first place given the very visible 
> brokeness and all GTK apps I tried just had an empty icon and looked equally 
> broken as the Qt apps.

I would simplify this to:

uint32_t *pixels = reinterpret_cast(xcb_get_image_data(xImage.data()));
for (int i = 0; i < xImage.data()->length; i++) {
int r = (pixels[i] >> 22) & 0xff;
int g = (pixels[i] >> 12) & 0xff;
int b = (pixels[i] >> 2)  & 0xff;

pixels[i] = qRgba(r, g, b, 0xff);
}

The conversion can be done in-place, and the alpha bits are likely zero.

This algorithm obviously just truncates the values, but it's probably
good enough for things like icons. For extra credit you would use an
ordered dither map.


- Fredrik


---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/112755/#review42568
---


On Nov. 4, 2013, 8:14 a.m., Martin Gräßlin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/112755/
> ---
> 
> (Updated Nov. 4, 2013, 8:14 a.m.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Repository: kdelibs
> 
> 
> Description
> ---
> 
> Implements the createPixmapFromHandle by getting the image for the pixmaps 
> and using it as either the Pixmap or the bitmap mask.
> 
> 
> Diffs
> -
> 
>   tier1/kwindowsystem/src/kxutils.cpp 33bd678 
>   tier1/kwindowsystem/src/kxutils_p.h 84d639b 
>   tier1/kwindowsystem/tests/CMakeLists.txt 0060903 
>   tier1/kwindowsystem/tests/createpixmapfromhandletest.cpp PRE-CREATION 
> 
> Diff: http://git.reviewboard.kde.org/r/112755/diff/
> 
> 
> Testing
> ---
> 
> Adjusted KWin to take this codepath and say thanks to Iceweasel for having a 
> mask
> 
> 
> Thanks,
> 
> Martin Gräßlin
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 112755: Reimplement KXUtils::createPixmapFromHandle with XCB

[Martin Gräßlin]

> On Oct. 29, 2013, 3:42 a.m., Fredrik Höglund wrote:
> > Looks much better, but it doesn't handle depth 30 pixmaps.
> 
> Martin Gräßlin wrote:
> I'm lacking ideas on how to test this. Do you know any application which 
> uses 30 bit pixmaps?
> 
> Fredrik Höglund wrote:
> The default depth is 30 when you're using the NVIDIA driver and the 
> monitor
> supports 30 bits. This is the reason why it's important.
> 
> The X server can be told to use a 30 bit visual even when the monitor 
> only supports
> 24 bits by starting it with -depth 30. But this isn't supported by all 
> drivers.
> 
> Note that the raster graphics system is completely broken when the 
> default depth
> is 30 since Qt assumes that any depth >= 24 means 8 bits per channel.
>

thanks, that did work. I had tried Xephyr yesterday which didn't work and 
didn't think of running X with -depth 30.

I have some code now, but have no idea whether that's correct (bit manipulation 
is not what I'm very used to ;-)

case 30: {
// Qt doesn't have a matching image format. We need to convert manually
uint8_t *origData = xcb_get_image_data(xImage.data());
uint8_t *converted = new uint8_t[xcb_get_image_data_length(xImage.data())];
for (int i = 0; i < xcb_get_image_data_length(xImage.data()); i += 4) {
uint32_t origWord = 0x;
for (int j = 0; j < 4; ++j) {
if (j > 0) {
origWord = origWord << 8;
}
origWord |= origData[i+j];
}
converted[i]   = uint8_t((float(origWord & 0xc000) / 0xc000) * 
0xff);
converted[i+1] = uint8_t((float(origWord & 0x3ff0) / 0x3ff0) * 
0xff);
converted[i+2] = uint8_t((float(origWord & 0x000ffc00) / 0x000ffc00) * 
0xff);
converted[i+3] = uint8_t((float(origWord & 0x03ff) / 0x03ff) * 
0xff);
}
QImage image(converted, geo->width, geo->height,
 xcb_get_image_data_length(xImage.data())/geo->height, 
QImage::Format_ARGB32_Premultiplied);
if (image.isNull()) {
return T();
}
T result = T::fromImage(image);
delete[] converted;
return result;
}

Testing is rather difficult. For Qt applications I'm rather sceptic that the 
icon has a correct color in the first place given the very visible brokeness 
and all GTK apps I tried just had an empty icon and looked equally broken as 
the Qt apps.


- Martin


---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/112755/#review42568
---


On Nov. 4, 2013, 9:14 a.m., Martin Gräßlin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/112755/
> ---
> 
> (Updated Nov. 4, 2013, 9:14 a.m.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Repository: kdelibs
> 
> 
> Description
> ---
> 
> Implements the createPixmapFromHandle by getting the image for the pixmaps 
> and using it as either the Pixmap or the bitmap mask.
> 
> 
> Diffs
> -
> 
>   tier1/kwindowsystem/src/kxutils.cpp 33bd678 
>   tier1/kwindowsystem/src/kxutils_p.h 84d639b 
>   tier1/kwindowsystem/tests/CMakeLists.txt 0060903 
>   tier1/kwindowsystem/tests/createpixmapfromhandletest.cpp PRE-CREATION 
> 
> Diff: http://git.reviewboard.kde.org/r/112755/diff/
> 
> 
> Testing
> ---
> 
> Adjusted KWin to take this codepath and say thanks to Iceweasel for having a 
> mask
> 
> 
> Thanks,
> 
> Martin Gräßlin
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 112755: Reimplement KXUtils::createPixmapFromHandle with XCB

[Fredrik Höglund]

> On Oct. 29, 2013, 2:42 a.m., Fredrik Höglund wrote:
> > Looks much better, but it doesn't handle depth 30 pixmaps.
> 
> Martin Gräßlin wrote:
> I'm lacking ideas on how to test this. Do you know any application which 
> uses 30 bit pixmaps?

The default depth is 30 when you're using the NVIDIA driver and the monitor
supports 30 bits. This is the reason why it's important.

The X server can be told to use a 30 bit visual even when the monitor only 
supports
24 bits by starting it with -depth 30. But this isn't supported by all drivers.

Note that the raster graphics system is completely broken when the default depth
is 30 since Qt assumes that any depth >= 24 means 8 bits per channel.


- Fredrik


---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/112755/#review42568
---


On Nov. 4, 2013, 8:14 a.m., Martin Gräßlin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/112755/
> ---
> 
> (Updated Nov. 4, 2013, 8:14 a.m.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Repository: kdelibs
> 
> 
> Description
> ---
> 
> Implements the createPixmapFromHandle by getting the image for the pixmaps 
> and using it as either the Pixmap or the bitmap mask.
> 
> 
> Diffs
> -
> 
>   tier1/kwindowsystem/src/kxutils.cpp 33bd678 
>   tier1/kwindowsystem/src/kxutils_p.h 84d639b 
>   tier1/kwindowsystem/tests/CMakeLists.txt 0060903 
>   tier1/kwindowsystem/tests/createpixmapfromhandletest.cpp PRE-CREATION 
> 
> Diff: http://git.reviewboard.kde.org/r/112755/diff/
> 
> 
> Testing
> ---
> 
> Adjusted KWin to take this codepath and say thanks to Iceweasel for having a 
> mask
> 
> 
> Thanks,
> 
> Martin Gräßlin
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 112755: Reimplement KXUtils::createPixmapFromHandle with XCB

[Martin Gräßlin]

---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/112755/
---

(Updated Nov. 4, 2013, 9:14 a.m.)


Review request for KDE Frameworks.


Changes
---

* added the missing static to the two helper methods
* added a check to early quit if big endian system. If someone has a big endian 
system I would appreciate if he could look into properly supporting this.


Repository: kdelibs


Description
---

Implements the createPixmapFromHandle by getting the image for the pixmaps and 
using it as either the Pixmap or the bitmap mask.


Diffs (updated)
-

  tier1/kwindowsystem/src/kxutils.cpp 33bd678 
  tier1/kwindowsystem/src/kxutils_p.h 84d639b 
  tier1/kwindowsystem/tests/CMakeLists.txt 0060903 
  tier1/kwindowsystem/tests/createpixmapfromhandletest.cpp PRE-CREATION 

Diff: http://git.reviewboard.kde.org/r/112755/diff/


Testing
---

Adjusted KWin to take this codepath and say thanks to Iceweasel for having a 
mask


Thanks,

Martin Gräßlin

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 112755: Reimplement KXUtils::createPixmapFromHandle with XCB

[Martin Gräßlin]

> On Oct. 29, 2013, 3:42 a.m., Fredrik Höglund wrote:
> > Looks much better, but it doesn't handle depth 30 pixmaps.

I'm lacking ideas on how to test this. Do you know any application which uses 
30 bit pixmaps?


- Martin


---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/112755/#review42568
---


On Oct. 24, 2013, 12:22 p.m., Martin Gräßlin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/112755/
> ---
> 
> (Updated Oct. 24, 2013, 12:22 p.m.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Repository: kdelibs
> 
> 
> Description
> ---
> 
> Implements the createPixmapFromHandle by getting the image for the pixmaps 
> and using it as either the Pixmap or the bitmap mask.
> 
> 
> Diffs
> -
> 
>   tier1/kwindowsystem/src/kxutils.cpp 33bd678 
>   tier1/kwindowsystem/src/kxutils_p.h 84d639b 
>   tier1/kwindowsystem/tests/CMakeLists.txt 5cf5868 
>   tier1/kwindowsystem/tests/createpixmapfromhandletest.cpp PRE-CREATION 
> 
> Diff: http://git.reviewboard.kde.org/r/112755/diff/
> 
> 
> Testing
> ---
> 
> Adjusted KWin to take this codepath and say thanks to Iceweasel for having a 
> mask
> 
> 
> Thanks,
> 
> Martin Gräßlin
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 112755: Reimplement KXUtils::createPixmapFromHandle with XCB

[Fredrik Höglund]

---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/112755/#review42568
---


Looks much better, but it doesn't handle depth 30 pixmaps.


tier1/kwindowsystem/src/kxutils.cpp


static



tier1/kwindowsystem/src/kxutils.cpp


static



tier1/kwindowsystem/src/kxutils.cpp


What if the host byte order is not little endian?


- Fredrik Höglund


On Oct. 24, 2013, 10:22 a.m., Martin Gräßlin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/112755/
> ---
> 
> (Updated Oct. 24, 2013, 10:22 a.m.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Repository: kdelibs
> 
> 
> Description
> ---
> 
> Implements the createPixmapFromHandle by getting the image for the pixmaps 
> and using it as either the Pixmap or the bitmap mask.
> 
> 
> Diffs
> -
> 
>   tier1/kwindowsystem/src/kxutils.cpp 33bd678 
>   tier1/kwindowsystem/src/kxutils_p.h 84d639b 
>   tier1/kwindowsystem/tests/CMakeLists.txt 5cf5868 
>   tier1/kwindowsystem/tests/createpixmapfromhandletest.cpp PRE-CREATION 
> 
> Diff: http://git.reviewboard.kde.org/r/112755/diff/
> 
> 
> Testing
> ---
> 
> Adjusted KWin to take this codepath and say thanks to Iceweasel for having a 
> mask
> 
> 
> Thanks,
> 
> Martin Gräßlin
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 112755: Reimplement KXUtils::createPixmapFromHandle with XCB

[Martin Gräßlin]

---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/112755/
---

(Updated Oct. 24, 2013, 12:22 p.m.)


Review request for KDE Frameworks.


Changes
---

* added a small test app which is able to render the icon of a passed in window 
id into a QLabel
* added a byte order check, if not LSB no image conversion is done
* added better depth and visual conversion. Only a small subset is checked with 
inspiration from kde-workspace/ksmserver/logouteffect.cpp

For testing I used iceweasel and xclock. Both work fine.


Repository: kdelibs


Description
---

Implements the createPixmapFromHandle by getting the image for the pixmaps and 
using it as either the Pixmap or the bitmap mask.


Diffs (updated)
-

  tier1/kwindowsystem/src/kxutils.cpp 33bd678 
  tier1/kwindowsystem/src/kxutils_p.h 84d639b 
  tier1/kwindowsystem/tests/CMakeLists.txt 5cf5868 
  tier1/kwindowsystem/tests/createpixmapfromhandletest.cpp PRE-CREATION 

Diff: http://git.reviewboard.kde.org/r/112755/diff/


Testing
---

Adjusted KWin to take this codepath and say thanks to Iceweasel for having a 
mask


Thanks,

Martin Gräßlin

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 112755: Reimplement KXUtils::createPixmapFromHandle with XCB

[Martin Gräßlin]

> On Oct. 9, 2013, 6:39 p.m., Kevin Ottens wrote:
> > Any news about that patch? It hasn't seen activity lately.
> 
> Kevin Ottens wrote:
> Anybody home?
> 
> FYI I'll close it as discarded if it doesn't have any activity by next 
> monday.

hey, I'm going to pick it up again! Just had been busy on kde-workspace.


- Martin


---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/112755/#review41439
---


On Sept. 17, 2013, 7:42 a.m., Martin Gräßlin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/112755/
> ---
> 
> (Updated Sept. 17, 2013, 7:42 a.m.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Repository: kdelibs
> 
> 
> Description
> ---
> 
> Implements the createPixmapFromHandle by getting the image for the pixmaps 
> and using it as either the Pixmap or the bitmap mask.
> 
> 
> Diffs
> -
> 
>   tier1/kwindowsystem/src/kxutils.cpp 33bd678 
>   tier1/kwindowsystem/src/kxutils_p.h 84d639b 
> 
> Diff: http://git.reviewboard.kde.org/r/112755/diff/
> 
> 
> Testing
> ---
> 
> Adjusted KWin to take this codepath and say thanks to Iceweasel for having a 
> mask
> 
> 
> Thanks,
> 
> Martin Gräßlin
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 112755: Reimplement KXUtils::createPixmapFromHandle with XCB

[Kevin Ottens]

> On Oct. 9, 2013, 4:39 p.m., Kevin Ottens wrote:
> > Any news about that patch? It hasn't seen activity lately.

Anybody home?

FYI I'll close it as discarded if it doesn't have any activity by next monday.


- Kevin


---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/112755/#review41439
---


On Sept. 17, 2013, 5:42 a.m., Martin Gräßlin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/112755/
> ---
> 
> (Updated Sept. 17, 2013, 5:42 a.m.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Repository: kdelibs
> 
> 
> Description
> ---
> 
> Implements the createPixmapFromHandle by getting the image for the pixmaps 
> and using it as either the Pixmap or the bitmap mask.
> 
> 
> Diffs
> -
> 
>   tier1/kwindowsystem/src/kxutils.cpp 33bd678 
>   tier1/kwindowsystem/src/kxutils_p.h 84d639b 
> 
> Diff: http://git.reviewboard.kde.org/r/112755/diff/
> 
> 
> Testing
> ---
> 
> Adjusted KWin to take this codepath and say thanks to Iceweasel for having a 
> mask
> 
> 
> Thanks,
> 
> Martin Gräßlin
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 112755: Reimplement KXUtils::createPixmapFromHandle with XCB

[Kevin Ottens]

---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/112755/#review41439
---


Any news about that patch? It hasn't seen activity lately.

- Kevin Ottens


On Sept. 17, 2013, 5:42 a.m., Martin Gräßlin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/112755/
> ---
> 
> (Updated Sept. 17, 2013, 5:42 a.m.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Repository: kdelibs
> 
> 
> Description
> ---
> 
> Implements the createPixmapFromHandle by getting the image for the pixmaps 
> and using it as either the Pixmap or the bitmap mask.
> 
> 
> Diffs
> -
> 
>   tier1/kwindowsystem/src/kxutils.cpp 33bd678 
>   tier1/kwindowsystem/src/kxutils_p.h 84d639b 
> 
> Diff: http://git.reviewboard.kde.org/r/112755/diff/
> 
> 
> Testing
> ---
> 
> Adjusted KWin to take this codepath and say thanks to Iceweasel for having a 
> mask
> 
> 
> Thanks,
> 
> Martin Gräßlin
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 112755: Reimplement KXUtils::createPixmapFromHandle with XCB

[Kevin Ottens]

> On Sept. 23, 2013, 10:27 a.m., Kevin Ottens wrote:
> > Not that I'm really qualified with xcb code, but it looks ok to me.
> 
> Martin Gräßlin wrote:
> no, no. The issues mentioned by Fredrik should be fixed.
> 
> Fredrik Höglund wrote:
> Kevin, please don't take this the wrong way, but it would be enormously 
> helpful if you didn't approve patches
> you yourself claim to NOT be qualified to review, when others who ARE 
> qualified have already reviewed them and
> voiced concerns.
> 
> This is not the first time you have done this either.
>

Yeah, that's one of the things I don't like with reviewboard... I've no way to 
vote that properly (I'm kind of used to the +1 vs +2 distinction in gerrit). 
There's more than one aspect to a patch, so although I'm not qualified for the 
XCB specific code I looked at the other aspects.

That's why I put a ship it, but adding the comment so that others with know how 
look at it... I looks heavy handed in reviewboard lingo indeed. Hm, I guess 
with reviewboard I should simply post a comment in those cases.


- Kevin


---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/112755/#review40515
---


On Sept. 17, 2013, 5:42 a.m., Martin Gräßlin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/112755/
> ---
> 
> (Updated Sept. 17, 2013, 5:42 a.m.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Description
> ---
> 
> Implements the createPixmapFromHandle by getting the image for the pixmaps 
> and using it as either the Pixmap or the bitmap mask.
> 
> 
> Diffs
> -
> 
>   tier1/kwindowsystem/src/kxutils.cpp 33bd678 
>   tier1/kwindowsystem/src/kxutils_p.h 84d639b 
> 
> Diff: http://git.reviewboard.kde.org/r/112755/diff/
> 
> 
> Testing
> ---
> 
> Adjusted KWin to take this codepath and say thanks to Iceweasel for having a 
> mask
> 
> 
> Thanks,
> 
> Martin Gräßlin
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 112755: Reimplement KXUtils::createPixmapFromHandle with XCB

[Fredrik Höglund]

> On Sept. 23, 2013, 10:27 a.m., Kevin Ottens wrote:
> > Not that I'm really qualified with xcb code, but it looks ok to me.
> 
> Martin Gräßlin wrote:
> no, no. The issues mentioned by Fredrik should be fixed.

Kevin, please don't take this the wrong way, but it would be enormously helpful 
if you didn't approve patches
you yourself claim to NOT be qualified to review, when others who ARE qualified 
have already reviewed them and
voiced concerns.

This is not the first time you have done this either.


- Fredrik


---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/112755/#review40515
---


On Sept. 17, 2013, 5:42 a.m., Martin Gräßlin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/112755/
> ---
> 
> (Updated Sept. 17, 2013, 5:42 a.m.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Description
> ---
> 
> Implements the createPixmapFromHandle by getting the image for the pixmaps 
> and using it as either the Pixmap or the bitmap mask.
> 
> 
> Diffs
> -
> 
>   tier1/kwindowsystem/src/kxutils.cpp 33bd678 
>   tier1/kwindowsystem/src/kxutils_p.h 84d639b 
> 
> Diff: http://git.reviewboard.kde.org/r/112755/diff/
> 
> 
> Testing
> ---
> 
> Adjusted KWin to take this codepath and say thanks to Iceweasel for having a 
> mask
> 
> 
> Thanks,
> 
> Martin Gräßlin
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 112755: Reimplement KXUtils::createPixmapFromHandle with XCB

[Martin Gräßlin]

> On Sept. 23, 2013, 12:27 p.m., Kevin Ottens wrote:
> > Not that I'm really qualified with xcb code, but it looks ok to me.

no, no. The issues mentioned by Fredrik should be fixed.


- Martin


---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/112755/#review40515
---


On Sept. 17, 2013, 7:42 a.m., Martin Gräßlin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/112755/
> ---
> 
> (Updated Sept. 17, 2013, 7:42 a.m.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Description
> ---
> 
> Implements the createPixmapFromHandle by getting the image for the pixmaps 
> and using it as either the Pixmap or the bitmap mask.
> 
> 
> Diffs
> -
> 
>   tier1/kwindowsystem/src/kxutils.cpp 33bd678 
>   tier1/kwindowsystem/src/kxutils_p.h 84d639b 
> 
> Diff: http://git.reviewboard.kde.org/r/112755/diff/
> 
> 
> Testing
> ---
> 
> Adjusted KWin to take this codepath and say thanks to Iceweasel for having a 
> mask
> 
> 
> Thanks,
> 
> Martin Gräßlin
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 112755: Reimplement KXUtils::createPixmapFromHandle with XCB

[Kevin Ottens]

---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/112755/#review40515
---

Ship it!


Not that I'm really qualified with xcb code, but it looks ok to me.

- Kevin Ottens


On Sept. 17, 2013, 5:42 a.m., Martin Gräßlin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/112755/
> ---
> 
> (Updated Sept. 17, 2013, 5:42 a.m.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Description
> ---
> 
> Implements the createPixmapFromHandle by getting the image for the pixmaps 
> and using it as either the Pixmap or the bitmap mask.
> 
> 
> Diffs
> -
> 
>   tier1/kwindowsystem/src/kxutils.cpp 33bd678 
>   tier1/kwindowsystem/src/kxutils_p.h 84d639b 
> 
> Diff: http://git.reviewboard.kde.org/r/112755/diff/
> 
> 
> Testing
> ---
> 
> Adjusted KWin to take this codepath and say thanks to Iceweasel for having a 
> mask
> 
> 
> Thanks,
> 
> Martin Gräßlin
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 112755: Reimplement KXUtils::createPixmapFromHandle with XCB

[Fredrik Höglund]

> On Sept. 16, 2013, 12:16 p.m., Fredrik Höglund wrote:
> > tier1/kwindowsystem/src/kxutils_p.h, line 49
> > 
> >
> > This statement is no longer true. If the pixmap is not an ARGB32 
> > pixmap, the function will crash. It will also fail if the image byte order 
> > doesn't match the CPU byte order, if the mask has the wrong size, or the 
> > mask isn't a bitmap.
> >
> 
> Martin Gräßlin wrote:
> I added some further checks to handle these problems except for byte 
> order. I'm lacking ideas on how to do it and even more on how to test it.

I think the logic needs to be the following:

* If the depth is 32, assume that the format is ARGB32_Premultiplied.
* If the depth is the default depth, examine the red/green/blue masks in the 
default visual
  and try to find a matching QImage format. If there is no matching format, 
it's necessary
  to do format conversion. x2rgb10 in particular is becoming increasingly 
likely,
  and it's not supported by Qt.
* If the depth is not 32, the default depth, or 1, we simply don't know.

The byte order is found in xcb_setup_t::image_byte_order.


- Fredrik


---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/112755/#review40111
---


On Sept. 17, 2013, 5:42 a.m., Martin Gräßlin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/112755/
> ---
> 
> (Updated Sept. 17, 2013, 5:42 a.m.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Description
> ---
> 
> Implements the createPixmapFromHandle by getting the image for the pixmaps 
> and using it as either the Pixmap or the bitmap mask.
> 
> 
> Diffs
> -
> 
>   tier1/kwindowsystem/src/kxutils.cpp 33bd678 
>   tier1/kwindowsystem/src/kxutils_p.h 84d639b 
> 
> Diff: http://git.reviewboard.kde.org/r/112755/diff/
> 
> 
> Testing
> ---
> 
> Adjusted KWin to take this codepath and say thanks to Iceweasel for having a 
> mask
> 
> 
> Thanks,
> 
> Martin Gräßlin
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 112755: Reimplement KXUtils::createPixmapFromHandle with XCB

[Martin Gräßlin]

---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/112755/
---

(Updated Sept. 17, 2013, 7:42 a.m.)


Review request for KDE Frameworks.


Changes
---

* verify mask is a bitmap
* verify mask has same size as the pixmap
* specify bytes per line when creating the image
* try to get the format from the ximage depth (tested with 1 and 24 - xclock 
used for the bitmap case, looks strange to be honest)


Description
---

Implements the createPixmapFromHandle by getting the image for the pixmaps and 
using it as either the Pixmap or the bitmap mask.


Diffs (updated)
-

  tier1/kwindowsystem/src/kxutils.cpp 33bd678 
  tier1/kwindowsystem/src/kxutils_p.h 84d639b 

Diff: http://git.reviewboard.kde.org/r/112755/diff/


Testing
---

Adjusted KWin to take this codepath and say thanks to Iceweasel for having a 
mask


Thanks,

Martin Gräßlin

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 112755: Reimplement KXUtils::createPixmapFromHandle with XCB

[Martin Gräßlin]

> On Sept. 16, 2013, 2:16 p.m., Fredrik Höglund wrote:
> > tier1/kwindowsystem/src/kxutils_p.h, line 49
> > 
> >
> > This statement is no longer true. If the pixmap is not an ARGB32 
> > pixmap, the function will crash. It will also fail if the image byte order 
> > doesn't match the CPU byte order, if the mask has the wrong size, or the 
> > mask isn't a bitmap.
> >

I added some further checks to handle these problems except for byte order. I'm 
lacking ideas on how to do it and even more on how to test it.


- Martin


---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/112755/#review40111
---


On Sept. 16, 2013, 9:27 a.m., Martin Gräßlin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/112755/
> ---
> 
> (Updated Sept. 16, 2013, 9:27 a.m.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Description
> ---
> 
> Implements the createPixmapFromHandle by getting the image for the pixmaps 
> and using it as either the Pixmap or the bitmap mask.
> 
> 
> Diffs
> -
> 
>   tier1/kwindowsystem/src/kxutils.cpp 33bd678 
>   tier1/kwindowsystem/src/kxutils_p.h 84d639b 
> 
> Diff: http://git.reviewboard.kde.org/r/112755/diff/
> 
> 
> Testing
> ---
> 
> Adjusted KWin to take this codepath and say thanks to Iceweasel for having a 
> mask
> 
> 
> Thanks,
> 
> Martin Gräßlin
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 112755: Reimplement KXUtils::createPixmapFromHandle with XCB

[Fredrik Höglund]

---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/112755/#review40111
---



tier1/kwindowsystem/src/kxutils.cpp


X pixmaps use premultiplied alpha.



tier1/kwindowsystem/src/kxutils.cpp


A bitmap is typically fetched as an XY_PIXMAP.




tier1/kwindowsystem/src/kxutils_p.h


This statement is no longer true. If the pixmap is not an ARGB32 pixmap, 
the function will crash. It will also fail if the image byte order doesn't 
match the CPU byte order, if the mask has the wrong size, or the mask isn't a 
bitmap.



- Fredrik Höglund


On Sept. 16, 2013, 7:27 a.m., Martin Gräßlin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/112755/
> ---
> 
> (Updated Sept. 16, 2013, 7:27 a.m.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Description
> ---
> 
> Implements the createPixmapFromHandle by getting the image for the pixmaps 
> and using it as either the Pixmap or the bitmap mask.
> 
> 
> Diffs
> -
> 
>   tier1/kwindowsystem/src/kxutils.cpp 33bd678 
>   tier1/kwindowsystem/src/kxutils_p.h 84d639b 
> 
> Diff: http://git.reviewboard.kde.org/r/112755/diff/
> 
> 
> Testing
> ---
> 
> Adjusted KWin to take this codepath and say thanks to Iceweasel for having a 
> mask
> 
> 
> Thanks,
> 
> Martin Gräßlin
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Review Request 112755: Reimplement KXUtils::createPixmapFromHandle with XCB

[Martin Gräßlin]

---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/112755/
---

Review request for KDE Frameworks.


Description
---

Implements the createPixmapFromHandle by getting the image for the pixmaps and 
using it as either the Pixmap or the bitmap mask.


Diffs
-

  tier1/kwindowsystem/src/kxutils.cpp 33bd678 
  tier1/kwindowsystem/src/kxutils_p.h 84d639b 

Diff: http://git.reviewboard.kde.org/r/112755/diff/


Testing
---

Adjusted KWin to take this codepath and say thanks to Iceweasel for having a 
mask


Thanks,

Martin Gräßlin

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel