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.

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

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.

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

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.

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?

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

Comment on attachment 434601 [details] [diff] [review] Fix patch apply - cairo upstream 16bpp patch version This is the same as what's upstream right?

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

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.

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

Tested with try server, and no problems... Pushed again... http://hg.mozilla.org/mozilla-central/rev/fd947d0c3918