555863 - Qt widget code assumes 32bpp target surface

Status: RESOLVED FIXED

(Core Graveyard :: Widget: Qt, defect)

Description

[Doug Turner (:dougt)]

Our current drawing code for Qt requires that the target device/surface we paint to is 32bpps.  On the maemo devices, the surface is 16bpps.  We haven't been seeing this regression because we have been running bug 545632.

When the gBufferImage does not patch the device, we should color correct (using pixman not QImage)

Comment 1

[Doug Turner (:dougt)]

Attachment: patch v.1 (WIP - do not review or checkin)

Comment 2

[Oleg Romashin (:romaxa)]

Attachment: Rework for qt graphics backends and rendering types

Comment 3

[Jeff Muizelaar [:jrmuizel]]

It looks like there at least two things going on in this patch:
1. Removing the Xlib render mode
2. Supporting 32 bit Qt drawing

Can we do them in separate patches?

Comment 4

[Oleg Romashin (:romaxa)]

(In reply to comment #3)
> It looks like there at least two things going on in this patch:
> 1. Removing the Xlib render mode
> 2. Supporting 32 bit Qt drawing

It was supported before, but not for qt raster backend

> 
> Can we do them in separate patches?

I think it does not make sense, because 2. will be very small patch in that case, because it just bugfix for raster mode.

Comment 5

[Oleg Romashin (:romaxa)]

Attachment: Remove XLIB render mode

Comment 6

[Oleg Romashin (:romaxa)]

Attachment: Make it works properly with raster backend

Comment 7

[Jeff Muizelaar [:jrmuizel]]

It still seems like there's a couple of things going on in the second patch (splitting them would be nice)..
Why do you switch from pixman to qt for doing the color conversion?

Comment 8

[Oleg Romashin (:romaxa)]

(In reply to comment #7)
> It still seems like there's a couple of things going on in the second patch
> (splitting them would be nice)..
> Why do you switch from pixman to qt for doing the color conversion?
To get rid of direct pixman code, and hardcoded pixman format values...

Comment 9

[Oleg Romashin (:romaxa)]

Also this pixman code looks a bit strange in Qt context which is allowing to do the same things... It would be possible to use gfxImageSurface here... but not know, because gfxImageSurface is not supporting 565 format yet...

Comment 10

[Oleg Romashin (:romaxa)]

Also with pixman I need to create separate function which is choosing pixman format according to depth value.

Comment 11

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 437365 [details] [diff] [review]
Make it works properly with raster backend

>diff -r 1cbd90b9b4c0 gfx/thebes/public/gfxSharedImageSurface.h
>--- a/gfx/thebes/public/gfxSharedImageSurface.h	Tue Apr 06 22:29:42 2010 +0300
>+++ b/gfx/thebes/public/gfxSharedImageSurface.h	Tue Apr 06 22:36:26 2010 +0300
>@@ -114,19 +114,25 @@ public:
>    * @param aShmId Shared Memory ID of data created outside,
>    *                if not specified, then class will allocate own shared memory
>    * @display aDisplay display used for X-Image creation
>    *                if not specified, then used system default
>    *
>    */
>   bool Init(const gfxIntSize& aSize,
>             gfxImageFormat aFormat = ImageFormatUnknown,
>+            int aDepth = 0,
>             int aShmId = -1,
>             Display *aDisplay = NULL);

It looks like a bunch of these default parameters are not used. Can we get rid of them?

>     // Check if system depth has related gfxImage format
>     gfxASurface::gfxImageFormat format =
>-        _depth_to_gfximage_format(gBufferPixmap->x11Info().depth());
>-
>-    gNeedColorConversion = (format == gfxASurface::ImageFormatUnknown);
>+        _depth_to_gfximage_format(aDepth);
>+    PRBool depthFormatInCompatible = (format == gfxASurface::ImageFormatUnknown);

I guess this can be cleaned up when we add 565 support?

>+
>+    // In raster backend we don't care about incompatible mode, and will paint in
>+    // default RB24 format... Raster engine will convert it t otarget color depth

typo here.


>+#if 0
>+    // No idea why this always returning type 0, it should work but it does not
>+    QPaintEngine::Type paintEngineType = GetViewWidget()->paintEngine()->type();
>+#else
>+    if (sCachedPaintEngineType < 0) {
>+        QPixmap temp_pix(1, 1);
>+        sCachedPaintEngineType = temp_pix.handle() ? QPaintEngine::X11 : QPaintEngine::Raster;
>+    }
>+    QPaintEngine::Type paintEngineType = (QPaintEngine::Type)sCachedPaintEngineType;
>+#endif

What's the cost of finding out the paint engine type? Is it worth caching? Also, can you figure out why paintEngine()->type() doesn't work.

Comment 12

[Oleg Romashin (:romaxa)]

> >    */
> >   bool Init(const gfxIntSize& aSize,
> >             gfxImageFormat aFormat = ImageFormatUnknown,
> >+            int aDepth = 0,
> >             int aShmId = -1,
> >             Display *aDisplay = NULL);
> 
> It looks like a bunch of these default parameters are not used. Can we get rid
> of them?

I think we can kill only aDisplay parameter, because aShmId is used for image initialization from remote ID in ipc/glue/XSHM


> >+    PRBool depthFormatInCompatible = (format == gfxASurface::ImageFormatUnknown);
> 
> I guess this can be cleaned up when we add 565 support?

Yes.

> 
> >+#if 0
> >+    // No idea why this always returning type 0, it should work but it does not
> >+    QPaintEngine::Type paintEngineType = GetViewWidget()->paintEngine()->type();
> >+#else
> >+    if (sCachedPaintEngineType < 0) {
> >+        QPixmap temp_pix(1, 1);
> >+        sCachedPaintEngineType = temp_pix.handle() ? QPaintEngine::X11 : QPaintEngine::Raster;
> >+    }
> >+    QPaintEngine::Type paintEngineType = (QPaintEngine::Type)sCachedPaintEngineType;
> >+#endif

> 
> What's the cost of finding out the paint engine type? Is it worth caching?

Probably I can move this engine type detection in function which is creating ViewWidget (::createWidget - toplevel), that will make this finding once, and cache it. is it ok?

> Also, can you figure out why paintEngine()->type() doesn't work.

Not sure that I can quickly understand why it does not work... need to recompile Qt for it... 
Can I create some bu about it, and fix later?

Comment 13

[Oleg Romashin (:romaxa)]

Attachment: Updated version.

Comment 14

[Oleg Romashin (:romaxa)]

Attachment: Fixed comments + solved problem with engine type detection

Comment 15

[Jeff Muizelaar [:jrmuizel]]

> > 
> > What's the cost of finding out the paint engine type? Is it worth caching?
> 
> Probably I can move this engine type detection in function which is creating
> ViewWidget (::createWidget - toplevel), that will make this finding once, and
> cache it. is it ok?

Is it worth caching at all? It looks like it should be very cheap to just check when needed.

Comment 16

[Oleg Romashin (:romaxa)]

Attachment: static variable renamed

Comment 17

[Oleg Romashin (:romaxa)]

Attachment: Removed caching.

Comment 18

[Oleg Romashin (:romaxa)]

Pushed in http://hg.mozilla.org/mozilla-central/rev/d926bc567542

Comment 19

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 438512 [details] [diff] [review]
Fixed comments + solved problem with engine type detection

Already fixed