Closed Bug 545632 Opened 11 years ago Closed 10 years ago

Add 16bpp format support for gfx/Cairo Image surface

Categories

(Core :: Graphics, defect)

x86
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: romaxa, Assigned: romaxa)

References

Details

Attachments

(2 files, 9 obsolete files)

Due to slow memory speed, 16bpp color depth is still very popular on mobile devices.

Maemo5 also using 16bpp color depth.
It would be nice to have 16bpp thebes/cairo rendering support.
Attached patch gfx 16bpp enabler Qt Part. (obsolete) — Splinter Review
What's the purpose of CAIRO_CONTENT_COLOR16? Do we need to distinguish between CAIRO_CONTENT_COLOR and CAIRO_CONTENT_COLOR16?
No sure that it is needed, I just tried to modify all RGBA24 places, and make sure that it works with 16bpp cairo_surfaces rendering...

Also this patch does not work when you try to composite 16bpp xlib_surface to 16bpp image surface... or another way, don't remember exactly...

Also I had some Idea to not provide RGB16 format only, but make public API cairo_image_surface_with_pixman_format, and allow to create cairo_image surface with any format supported by pixman.

If you that some parts of this patch wrong or just not needed, comment about it..
Duplicate of this bug: 491357
Attachment #428772 - Flags: review?(jmuizelaar)
romaxa,
what sort of perf numbers are/were you seeing?
I have not measured that in some numbers, but with this patch are not doing this: http://mxr.mozilla.org/mozilla-central/source/widget/src/qt/nsWindow.cpp#1056

Also we having better memory bandwidth because we are pushing 2x less data in graphics pipeline 

Also we are using all neon optimizations which has been done for fremantle 16bpp.
Better version, removed CAIRO_CONTENT_16, and fixed rendering from 16bpp images surface to XLIB surface.
Attachment #428772 - Attachment is obsolete: true
Attachment #432353 - Flags: review?(jmuizelaar)
Attachment #428772 - Flags: review?(jmuizelaar)
Can we try to do this upstream first? I have no problem taking the patch early if it's already upstream.
(In reply to comment #9)
> Can we try to do this upstream first? I have no problem taking the patch early
> if it's already upstream.

https://bugs.freedesktop.org/show_bug.cgi?id=10208#c4
I don't see the relevance of that bug. That seems to be about performance. This patch is about adding support.
Yes, that was wrong bug...
Here is the new bug:
https://bugs.freedesktop.org/show_bug.cgi?id=27094
I'd suggest sending a mail to the cairo list. If no one has any reasonable complaints I can push it upstream.
Patch in mailing list already, but it seems hang for some time...
Attachment #432353 - Attachment is obsolete: true
Attachment #434464 - Flags: review?(jmuizelaar)
Attachment #432353 - Flags: review?(jmuizelaar)
Attachment #434464 - Attachment is obsolete: true
Attachment #434601 - Flags: review?(jmuizelaar)
Attachment #434464 - Flags: review?(jmuizelaar)
Comment on attachment 434601 [details] [diff] [review]
Fix patch apply - cairo upstream 16bpp patch version

>diff -r 66d31d382c6c gfx/cairo/cairo/src/cairo-deprecated.h
>--- a/gfx/cairo/cairo/src/cairo-deprecated.h	Wed Mar 24 10:51:17 2010 -0700
>+++ b/gfx/cairo/cairo/src/cairo-deprecated.h	Wed Mar 24 19:30:06 2010 -0400
>@@ -43,17 +43,17 @@
>  * backend can work fine with 16-bit visuals in the same way it works
>  * with BGR visuals without any BGR formats in
>  * #cairo_format_t).
>  *
>  * Additionally, the support for the RGB16_565 format was never
>  * completely implemented. So while this format value is currently
>  * deprecated, it may eventually acquire complete support in the future.
>  */
>-#define CAIRO_FORMAT_RGB16_565 4
>+/* #define CAIRO_FORMAT_RGB16_565 4 */

Let's fix up the comment properly here.
Attached patch RGB16_565 format enabler. (obsolete) — Splinter Review
Assignee: nobody → romaxa
Attachment #428773 - Attachment is obsolete: true
Attachment #434601 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #438831 - Flags: review?(jmuizelaar)
Attachment #434601 - Flags: review?(jmuizelaar)
Depends on: 562746
Attached patch 16bpp cairo patch (obsolete) — Splinter Review
Assignee: romaxa → doug.turner
Attachment #450378 - Flags: review?(jmuizelaar)
Attached patch enable 16bpp image surface (obsolete) — Splinter Review
Attachment #450379 - Flags: review?(jmuizelaar)
Attachment #450379 - Attachment is obsolete: true
Attachment #450379 - Flags: review?(jmuizelaar)
Attached patch enable 16bpp image surface (obsolete) — Splinter Review
Attachment #450380 - Flags: review?(jmuizelaar)
Comment on attachment 450378 [details] [diff] [review]
16bpp cairo patch

I don't think the CAIRO_CONTENT_COLOR16 stuff is good. cairo_content_t is meant to describe the contents of a surface not the format. CAIRO_CONTENT_COLOR should be sufficient. What do you need CONTENT_COLOR16 for?
Attachment #450378 - Flags: review?(jmuizelaar) → review-
Comment on attachment 438831 [details] [diff] [review]
RGB16_565 format enabler.

Not sure if this is being done, but we should try to keep 16bpp images still have a 4-byte-aligned stride.

Also:

gfxSharedImageSurface::ComputeFormat()
> {
>     if (mDepth == 32)
>         mFormat = ImageFormatARGB32;
>     if (mDepth == 24)
>         mFormat = ImageFormatRGB24;
>+    if (mDepth == 16)
>+        mFormat = ImageFormatRGB16_565;
>     else {
>         NS_WARNING("Unknown depth specified to gfxSharedImageSurface!");
>         mFormat = ImageFormatUnknown;
>         return false;
>     }

Wasn't introduced in this patch, but these all need to be "else if" -- otherwise that else clause will fire for every depth that's not 16.
Comment on attachment 438831 [details] [diff] [review]
RGB16_565 format enabler.

>diff -r 7d5962d3a808 gfx/layers/basic/BasicImages.cpp
>--- a/gfx/layers/basic/BasicImages.cpp	Sat Apr 10 13:03:40 2010 -0500
>+++ b/gfx/layers/basic/BasicImages.cpp	Tue Apr 13 23:15:21 2010 +0300
>@@ -159,11 +159,11 @@ BasicPlanarYCbCrImage::GetAsSurface()
>   if (!mBuffer) {
>     return nsnull;
>   }
>   nsRefPtr<gfxImageSurface> imgSurface =
>       new gfxImageSurface(mBuffer, mSize,
>-                          mSize.width * 4,
>+                          mSize.width * gfxASurface::BytePerPixelFromFormat(gfxASurface::ImageFormatRGB24),
>                           gfxASurface::ImageFormatRGB24);
>   if (!imgSurface) {
>     return nsnull;
>   }
> 
>diff -r 7d5962d3a808 gfx/thebes/public/gfxASurface.h
>--- a/gfx/thebes/public/gfxASurface.h	Sat Apr 10 13:03:40 2010 -0500
>+++ b/gfx/thebes/public/gfxASurface.h	Tue Apr 13 23:15:21 2010 +0300
>@@ -66,10 +66,11 @@ public:
>     typedef enum {
>         ImageFormatARGB32, ///< ARGB data in native endianness, using premultiplied alpha
>         ImageFormatRGB24,  ///< xRGB data in native endianness
>         ImageFormatA8,     ///< Only an alpha channel
>         ImageFormatA1,     ///< Packed transparency information (one byte refers to 8 pixels)
>+        ImageFormatRGB16_565,  ///< RGB_5656 data in native endianness

typo: an extra 6 in RGB_5656

>         ImageFormatUnknown
>     } gfxImageFormat;
> 
>     typedef enum {
>         SurfaceTypeImage,
>@@ -145,10 +146,12 @@ public:
>      */
>     virtual PRInt32 GetDefaultContextFlags() const { return 0; }
> 
>     static gfxContentType ContentFromFormat(gfxImageFormat format);
> 
>+    static PRInt16 BytePerPixelFromFormat(gfxImageFormat format);
>+

Can't see a reason for using PRInt16...

> protected:
>     gfxASurface() : mSurface(nsnull), mFloatingRefs(0), mSurfaceValid(PR_FALSE) { }
> 
>     static gfxASurface* GetSurfaceWrapper(cairo_surface_t *csurf);
>     static void SetSurfaceWrapper(cairo_surface_t *csurf, gfxASurface *asurf);

>diff -r 7d5962d3a808 gfx/thebes/src/gfxPlatformGtk.cpp
>--- a/gfx/thebes/src/gfxPlatformGtk.cpp	Sat Apr 10 13:03:40 2010 -0500
>+++ b/gfx/thebes/src/gfxPlatformGtk.cpp	Tue Apr 13 23:15:21 2010 +0300
>@@ -169,27 +169,24 @@ gfxPlatformGtk::CreateOffscreenSurface(c
>     if (size.width >= GDK_PIXMAP_SIZE_MAX ||
>         size.height >= GDK_PIXMAP_SIZE_MAX)
>         sizeOk = PR_FALSE;
> 
> #ifdef MOZ_X11
>-    int glitzf;
>-    int xrenderFormatID;
>+    int xrenderFormatID = -1;
>     switch (imageFormat) {
>         case gfxASurface::ImageFormatARGB32:
>-            glitzf = 0; // GLITZ_STANDARD_ARGB32;
>             xrenderFormatID = PictStandardARGB32;
>             break;
>         case gfxASurface::ImageFormatRGB24:
>-            glitzf = 1; // GLITZ_STANDARD_RGB24;
>             xrenderFormatID = PictStandardRGB24;
>             break;
>+        case gfxASurface::ImageFormatRGB16_565:
>+            break;
>         case gfxASurface::ImageFormatA8:
>-            glitzf = 2; // GLITZ_STANDARD_A8;
>             xrenderFormatID = PictStandardA8;
>             break;
>         case gfxASurface::ImageFormatA1:
>-            glitzf = 3; // GLITZ_STANDARD_A1;
>             xrenderFormatID = PictStandardA1;
>             break;
>         default:
>             return nsnull;
>     }
>@@ -200,12 +197,17 @@ gfxPlatformGtk::CreateOffscreenSurface(c
>     Display* display = GDK_DISPLAY();
>     if (!display)
>         return nsnull;
> 
>     GdkPixmap* pixmap = nsnull;
>-    XRenderPictFormat* xrenderFormat =
>-        XRenderFindStandardFormat(display, xrenderFormatID);
>+    XRenderPictFormat* xrenderFormat = nsnull;
>+    if ((xrenderFormatID == PictStandardRGB24 && gdk_visual_get_system()->depth == 16) || xrenderFormatID == -1) {
>+        xrenderFormat =
>+            XRenderFindVisualFormat(display,
>+                                    GDK_VISUAL_XVISUAL(gdk_visual_get_best_with_depth(16)));

This could use a comment explaining why we can't use FindVisualFormat() for all depths.

>+    } else
>+        xrenderFormat = XRenderFindStandardFormat(display, xrenderFormatID);
> 
>     if (xrenderFormat && sizeOk) {
>         pixmap = gdk_pixmap_new(nsnull, size.width, size.height,
>                                 xrenderFormat->depth);
> 
>diff -r 7d5962d3a808 gfx/thebes/src/gfxQtPlatform.cpp
>--- a/gfx/thebes/src/gfxQtPlatform.cpp	Sat Apr 10 13:03:40 2010 -0500
>+++ b/gfx/thebes/src/gfxQtPlatform.cpp	Tue Apr 13 23:15:21 2010 +0300
>@@ -193,10 +193,12 @@ gfxQtPlatform::CreateOffscreenSurface(co
>       newSurface = new gfxQPainterSurface(size, gfxASurface::ContentFromFormat(imageFormat));
>       return newSurface.forget();
>     }
> 
>     if (mRenderMode == RENDER_SHARED_IMAGE) {
>+      if (imageFormat == gfxASurface::ImageFormatRGB24 && QX11Info().depth() == 16)
>+          imageFormat = gfxASurface::ImageFormatRGB16_565;

use FormatForDepth()?

>       newSurface = new gfxImageSurface(size, imageFormat);
>       return newSurface.forget();
>     }
> 
> #ifdef MOZ_X11
>@@ -219,12 +223,15 @@ gfxQtPlatform::CreateOffscreenSurface(co
>     }
> 
>     // XXX we really need a different interface here, something that passes
>     // in more context, including the display and/or target surface type that
>     // we should try to match
>-    XRenderPictFormat* xrenderFormat =
>-        XRenderFindStandardFormat(QX11Info().display(), xrenderFormatID);
>+    XRenderPictFormat* xrenderFormat = nsnull;
>+    if ((xrenderFormatID == PictStandardRGB24 && QX11Info().depth() == 16) || xrenderFormatID == -1)
>+        xrenderFormat = XRenderFindVisualFormat(QX11Info().display(), (Visual*)QX11Info().visual());

This could use a comment explaining why we can't use FindVisualFormat() for all depths.

>+    else
>+        xrenderFormat = XRenderFindStandardFormat(QX11Info().display(), xrenderFormatID);
> 
>     newSurface = new gfxXlibSurface((Display*)QX11Info().display(),
>                                     xrenderFormat,
>                                     size);
> #endif
>diff -r 7d5962d3a808 gfx/thebes/src/gfxSharedImageSurface.cpp
>--- a/gfx/thebes/src/gfxSharedImageSurface.cpp	Sat Apr 10 13:03:40 2010 -0500
>+++ b/gfx/thebes/src/gfxSharedImageSurface.cpp	Tue Apr 13 23:15:21 2010 +0300
>@@ -76,10 +76,13 @@ gfxSharedImageSurface::getASurface(void)
> 
>     gfxASurface::gfxImageFormat imageFormat = gfxASurface::ImageFormatRGB24;
>     if (mDepth == 32)
>         imageFormat = gfxASurface::ImageFormatARGB32;
> 
>+    if (mDepth == 16)
>+        imageFormat = gfxASurface::ImageFormatRGB16_565;
>+

Should this use ComputeFormat? Also ComputeFormat is a poor name... (FormatForDepth?)

>diff -r 7d5962d3a808 gfx/thebes/src/gfxXlibSurface.cpp
>--- a/gfx/thebes/src/gfxXlibSurface.cpp	Sat Apr 10 13:03:40 2010 -0500
>+++ b/gfx/thebes/src/gfxXlibSurface.cpp	Tue Apr 13 23:15:21 2010 +0300
>@@ -156,10 +156,26 @@ gfxXlibSurface::FindRenderFormat(Display
>             return XRenderFindStandardFormat (dpy, PictStandardARGB32);
>             break;
>         case ImageFormatRGB24:
>             return XRenderFindStandardFormat (dpy, PictStandardRGB24);
>             break;
>+        case ImageFormatRGB16_565: {
>+            Visual *visual = NULL;
>+            Screen *screen = DefaultScreenOfDisplay(dpy);
>+            int j;

This code could use a comment describing what it's doing and why.

>+            for (j = 0; j < screen->ndepths; j++) {
>+                Depth *d = &screen->depths[j];
>+                if (d->depth == 16 && d->nvisuals && &d->visuals[0]) {
>+                    visual = &d->visuals[0];
>+                    break;
>+                }
>+            }
>+            if (!visual)
>+                return NULL;
>+            return XRenderFindVisualFormat(dpy, visual);
>+            break;
>+        }
>         case ImageFormatA8:
>             return XRenderFindStandardFormat (dpy, PictStandardA8);
>             break;
>         case ImageFormatA1:
>             return XRenderFindStandardFormat (dpy, PictStandardA1);
Attachment #438831 - Flags: review?(jmuizelaar) → review-
Attachment #434601 - Attachment description: Fix patch apply → Fix patch apply - cairo upstream 16bpp patch version
Attachment #434601 - Attachment is obsolete: false
Comment on attachment 434601 [details] [diff] [review]
Fix patch apply - cairo upstream 16bpp patch version

This is the same as what's upstream right?
Attachment #434601 - Flags: review+
Assignee: doug.turner → romaxa
Attachment #434601 - Attachment is obsolete: true
Attachment #438831 - Attachment is obsolete: true
Attachment #450378 - Attachment is obsolete: true
Attachment #450380 - Attachment is obsolete: true
Attachment #450405 - Flags: review?(jmuizelaar)
Attachment #450380 - Flags: review?(jmuizelaar)
Attachment #450405 - Flags: review?(jmuizelaar) → review+
Attachment #450407 - Flags: review?(jmuizelaar)
(In reply to comment #27)
> Created an attachment (id=450407) [details]
> gfx 16bpp enabler

Ah, forgot.
-+PRInt16
-+gfxASurface::BytePerPixelFromFormat(gfxImageFormat format)

++PRInt32
++gfxASurface::BytePerPixelFromFormat(gfxImageFormat format)
Attachment #450407 - Flags: review?(jmuizelaar) → review+
Summary: Add support for 16bpp or custom pixman image format image surface type → Add 16bpp format support for gfx/Cairo Image surface
Two nits about: ComputeStride()
1. This would be readable as a switch/case:
2. Are 16bpp images also dword aligned per row?
this was pushed:

http://hg.mozilla.org/mozilla-central/rev/2959eddcd018
http://hg.mozilla.org/mozilla-central/rev/85335f212ac3

Given that, Alfred, please file new bugs with your concerns.
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
> 
> http://hg.mozilla.org/mozilla-central/rev/2959eddcd018
> http://hg.mozilla.org/mozilla-central/rev/85335f212ac3

These changes was reverted back, and only cairo part pushed in.

We still need to figure out what is causing Linux MD oranges, and why 16bpp enabler patch making it...
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Tested with try server, and no problems...

Pushed again...
http://hg.mozilla.org/mozilla-central/rev/fd947d0c3918
Status: REOPENED → RESOLVED
Closed: 10 years ago10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.