Simplify Qt rendering modes and remove dependency from gfxSharedImageSurface

RESOLVED FIXED

Status

Core Graveyard
Widget: Qt
RESOLVED FIXED
8 years ago
a year ago

People

(Reporter: romaxa, Assigned: romaxa)

Tracking

Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 3 obsolete attachments)

(Assignee)

Description

8 years ago
Created attachment 441503 [details] [diff] [review]
Rework Qt backend, gfxImageSurface.

1) remove temporary buffer converters
2) remove XShmImage stuff from gfxSharedImageSurface, and make it public from gfxImageSurface.
3) Make Qt rendering system initialization more simple.

Provide 2 Qt rendering modes:
1) QPainter mode, (non-default due to missing glyphs implementation)
2) Buffered mode, which is using temporary surface and using stable cairo/gfx backends, here is possible to modes according to graphicssystem type:
   a) QPaintEngine::X11 (cmdline -graphicssystem native) - optimized for rendering with XPixmaps, and implemented internally with XRender backend.
   b) QPaintEngine::Raster (cmdline -graphicssystem raster) - optimized for rendering with local image surfaces, all QPixmaps by default initialized as QImage in this mode.
(Assignee)

Comment 1

8 years ago
Also with gfxImageSurface->GetData(&gfxSharedImageSurface::SHMID_KEY); call we can get shmID pointer also it could be used for gfxSharedImageSurface type detection...
(Assignee)

Comment 2

8 years ago
Also we should create this class with SHMMem initialization using IPC-Shmem when MOZ_IPC defined, and make this class usable in content process configuration.
And then replace direct Shmmem usage in plugins/canvas class
(Assignee)

Updated

8 years ago
Attachment #441503 - Flags: review?(joe)
(Assignee)

Comment 3

8 years ago
Created attachment 442703 [details] [diff] [review]
Remove XShm stuff from Qt port, and simplify rendering path

Same fix but without gfxSharedImageSurface change.
This fix is blocking bug 562285
Assignee: nobody → romaxa
Attachment #441503 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #442703 - Flags: review?(dougt)
Attachment #441503 - Flags: review?(joe)
(Assignee)

Updated

8 years ago
Blocks: 562285
(Assignee)

Updated

8 years ago
Summary: Simplify Qt rendering modes, and fix bad design of gfxSharedImageSurface class → Simplify Qt rendering modes and remove dependency from gfxSharedImageSurface
(Assignee)

Comment 4

8 years ago
Created attachment 443063 [details] [diff] [review]
Remove XShm stuff from Qt port, and simplify rendering path, style, include spell fixes
Attachment #442703 - Attachment is obsolete: true
Attachment #443063 - Flags: review?(dougt)
Attachment #442703 - Flags: review?(dougt)
(Assignee)

Comment 5

8 years ago
Created attachment 443073 [details] [diff] [review]
Remove XShm stuff from Qt port, and simplify rendering path
Attachment #443063 - Attachment is obsolete: true
Attachment #443073 - Flags: review?(dougt)
Attachment #443063 - Flags: review?(dougt)

Updated

8 years ago
Attachment #443073 - Flags: review?(dougt) → review?(joe)
(Assignee)

Updated

8 years ago
Attachment #443073 - Flags: review?(jmuizelaar)
Comment on attachment 443073 [details] [diff] [review]
Remove XShm stuff from Qt port, and simplify rendering path

General comment: can you add showfunc = 1 and unified = 8 to your [diff] and [qdiff] sections of your .hgrc? That makes reviewing easier.

>diff -r 288aeb67ac37 gfx/thebes/src/gfxQtPlatform.cpp

>+// Because of QPainter backend has some problems with glyphs rendering

"Because the QPainter backend..."

>+// it is better to use image or xlib cairo backends by default
>+#define DEFAULT_RENDER_MODE RENDER_BUFFERED

>+    // Qt is not providing public API for detection graphicssystem type
>+    // We can get it only with test QPixmap creation

"Qt doesn't provide a public API to detect the graphicssystem type. We hack around this by checking what type of graphicssystem a test QPixmap uses."

General question: Do we want to support both graphicssystem raster and graphicssystem native? I thought native was going to be the default.

>diff -r 288aeb67ac37 widget/src/qt/nsWindow.cpp

> static inline QImage::Format
>-_depth_to_qformat(PRInt32 aDepth)
>+_gfximage_to_qformat(gfxASurface::gfxImageFormat aFormat)
> {
>-    switch (aDepth) {
>-    case 32:
>+    switch (aFormat) {
>+    case gfxASurface::ImageFormatARGB32:
>+    case gfxASurface::ImageFormatRGB24:
>         return QImage::Format_ARGB32;
>-    case 24:
>-        return QImage::Format_RGB32;
>-    case 16:
>-        return QImage::Format_RGB16;
>     default:
>         return QImage::Format_Invalid;
>     }

Why no 16-bit? Or will restoring that be a separate patch?

>+    if (format == gfxASurface::ImageFormatUnknown)
>         format = gfxASurface::ImageFormatRGB24;
>-    }

Should we just error out in this case? Otherwise you might want to add a comment saying that Cairo or Qt will do the conversion for us.

>+    // For Qt Raster engine better to use image surface as offscreen buffer
>+    gBufferImage = new gfxImageSurface(gBufferMaxSize, format);

This isn't conditionalized on anything, but its comment makes it seem like it is.

Also - maybe as a separate patch/bug - can you remove all checks for new failing? New is infallible now.

>+        else if (paintEngineType == QPaintEngine::Raster) {
>+            NS_ADDREF(targetSurface = gBufferImage);

No NS_ADDREF needed here.
Attachment #443073 - Flags: review?(joe) → review+
(Assignee)

Comment 7

8 years ago
(In reply to comment #6)
> (From update of attachment 443073 [details] [diff] [review])
> General comment: can you add showfunc = 1 and unified = 8 to your [diff] and
> [qdiff] sections of your .hgrc? That makes reviewing easier.

Ye, I have it , but seems I took patch directly from patches folder again

> >-    case 24:
> >-        return QImage::Format_RGB32;
> >-    case 16:
> >-        return QImage::Format_RGB16;
> >     default:
> >         return QImage::Format_Invalid;
> >     }
> 
> Why no 16-bit? Or will restoring that be a separate patch?

Yes it will come with 16bpp patch when next cairo upstream landed

> 
> >+    if (format == gfxASurface::ImageFormatUnknown)
> >         format = gfxASurface::ImageFormatRGB24;
> >-    }
> 
> Should we just error out in this case? Otherwise you might want to add a
No, otherwise we fail on maemo devices immediately.
 
> comment saying that Cairo or Qt will do the conversion for us.
Yep, Qt will do conversation, I'll add comment

> 
> Also - maybe as a separate patch/bug - can you remove all checks for new
> failing? New is infallible now.
> 
> >+        else if (paintEngineType == QPaintEngine::Raster) {
> >+            NS_ADDREF(targetSurface = gBufferImage);
> 
> No NS_ADDREF needed here.

It is needed, otherwise it crash because targetSurface is nsRefPtr
(Assignee)

Comment 8

8 years ago
> General question: Do we want to support both graphicssystem raster and
> graphicssystem native? I thought native was going to be the default.

Linux desktop Qt use native, maemo5 use raster... right now for us raster is working better.
(Assignee)

Comment 9

8 years ago
Created attachment 444247 [details] [diff] [review]
Patch for check-in
(Assignee)

Updated

8 years ago
Attachment #443073 - Flags: review?(jmuizelaar)
(Assignee)

Comment 10

8 years ago
http://hg.mozilla.org/mozilla-central/rev/17cccfe2132d
Fixed
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
(Assignee)

Updated

8 years ago
Depends on: 565893
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.