593175 - Use 16bpp when MOZ_GFX_OPTIMIZE_MOBILE is set.

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Doug Turner (:dougt)]

right now we use 32/24 bit images in our basic canvas layer.  we should default these to 16 bit when MOZ_GFX_OPTIMIZE_MOBILE is set.

Comment 1

[Doug Turner (:dougt)]

Attachment: patch v.1

this just hard codes, we probably can do something smarter at some point.. maybe move this into a inline.

Comment 2

[Oleg Romashin (:romaxa)]

Comment on attachment 471657 [details] [diff] [review]
patch v.1


>   if (mGLContext) {
>     nsRefPtr<gfxImageSurface> isurf =
>       new gfxImageSurface(gfxIntSize(mBounds.width, mBounds.height),
>+#ifdef MOZ_GFX_OPTIMIZE_MOBILE
>+                          gfxASurface::ImageFormatRGB16_565);
>+#else
>                           IsOpaqueContent()
>                             ? gfxASurface::ImageFormatRGB24
>                             : gfxASurface::ImageFormatARGB32);
>+#endif
>+

I think it is not really good to drop ARGB surface for mobile in general...
I think we should do switch to 565 only when content is opaque && defaultSystemDepth == 16...

Comment 3

[Doug Turner (:dougt)]

indeed.  this patch forces 16bpp with a hammer which is wrong.

Comment 4

[Oleg Romashin (:romaxa)]

Also it would be probably nice to use gfxPlatform->CreateOffscreenSurface, that function will create for us platform preferred  surface with automatic 16bpp switch

Comment 5

[Oleg Romashin (:romaxa)]

Also I think this bug should cooperate with bug 576437

Comment 6

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Note

[17:35]	<karl>	cjones: i don't know what the plan is for the 16/24 choice
[17:36]	<cjones>	hm
[17:36]	<karl>	cjones: there is a pref somewhere, but if it is going to be composited with an ARGB32, i'd choose 24

Comment 7

[Karl Tomlinson (:karlt)]

Currently Fennec sets mozilla.widget.force-24bpp:
http://hg.mozilla.org/mobile-browser/file/bf6750e6e92a/app/mobile.js#l110

If that pref is unset, then it would probably make sense to use the root window with its default visual for gfxPlatform::ScreenReferenceSurface().

Comment 8

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

(In reply to comment #6)
> Note
> 
> [17:35]    <karl>    cjones: i don't know what the plan is for the 16/24 choice
> [17:36]    <cjones>    hm
> [17:36]    <karl>    cjones: there is a pref somewhere, but if it is going to
> be composited with an ARGB32, i'd choose 24

so this was always my argument against 16bpp, but based on romaxa's testing, it sounds like halving the memory bandwidth needed has a far greater positive impact on performance than the extra computation required to composite 32-bit argb and 16-bit 5:6:5 rgb.  a standalone benchmark with some numbers would be great to confirm this, though; can probably do it with pixman/cairo directly.

Comment 9

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Hey Doug, mind if I steal this bug from you?  I ported your original patch to what gfx/layers will be after bug 570625.

Comment 10

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: part 0: Allow forcing use of Shmem-backed cross-process layers in DEBUG builds.

Doesn't really fit here, but I thought I'd sneak it in ;).

Comment 11

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: part 1: Request backing buffers with gfxContentType rather than gfxImageFormat

Comment 12

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: part 2: Hand out 5-6-5 opaque buffers on mobile platforms

Comment 13

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Requesting blocking based on comment 8.  This should hopefully also be a win with GL compositing for similar reasons.

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Why is OptimalFormatFor duplicated?

Comment 15

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Might be different for different backends.  I'd be happy to keep one impl and split as needed, if you prefer.

Comment 16

[Oleg Romashin (:romaxa)]

Chris does it make sense fix bug 576437? will it help for fixing this bug?

Comment 17

[Stuart Parmenter]

blocking b1 on this -- makes a pretty big difference on Maemo.

Comment 18

[Karl Tomlinson (:karlt)]

Comment on attachment 475002 [details] [diff] [review]
part 2: Hand out 5-6-5 opaque buffers on mobile platforms

>--- a/gfx/layers/ipc/ShadowLayerUtilsX11.cpp
>+++ b/gfx/layers/ipc/ShadowLayerUtilsX11.cpp

I don't think it's good to expect a 565 pict format to exist because
MOZ_GFX_OPTIMIZE_MOBILE was on (and my comment in bug 570625 comment 53 about
the ABORT_IF_FALSE wasn't addressed).

What I suggest for a quick simple solution here is to use
XRenderFindVisualFormat(Display*, DefaultVisualOfScreen(Screen*)) in
CreateSimilar.  I don't mind whether or not that is conditional on
MOZ_GFX_OPTIMIZE_MOBILE.

Might as well forget ImageFormats in (the X11) CreateSimilar and use
XRenderFindStandardFormat(Display*, PictStandardARGB32) for COLOR_ALPHA.

>--- a/gfx/layers/ipc/ShadowLayers.cpp
>+++ b/gfx/layers/ipc/ShadowLayers.cpp
>@@ -340,18 +340,21 @@ ShadowLayerForwarder::EndTransaction(nsT
>   return PR_TRUE;
> }
> 
> static gfxASurface::gfxImageFormat
> OptimalFormatFor(gfxASurface::gfxContentType aContent)
> {
>   switch (aContent) {
>   case gfxASurface::CONTENT_COLOR:
>-    // FIXME/bug 593175: investigate 16bpp
>+#ifdef MOZ_GFX_OPTIMIZE_MOBILE
>+    return gfxASurface::ImageFormatRGB16_565;
>+#else
>     return gfxASurface::ImageFormatRGB24;
>+#endif

This gfxASurface::ImageFormatRGB16_565 for MOZ_GFX_OPTIMIZE_MOBILE in
ShadowLayers.cpp is fine because image surfaces are known to support image formats.

Please turn off mozilla.widget.force-24bpp in Fennec if Gecko is not going to be
consistent in honoring it.

Comment 19

[Oleg Romashin (:romaxa)]

Ok, I'm about to push bug 576437 (checking it on try server)..

I think it make sense create this fixes based on fix from bug 576437

Comment 20

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: part 1: Request backing buffers with gfxContentType rather than gfxImageFormat

Also fixed missed review comment.

Comment 21

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: part 2: Use 5-6-5 opaque gfxSharedImageSurfaces on mobile platforms

Mobile/X11 can be a separate patch when dep is fixed.

Comment 22

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: WIP rollup with 16bpp for X11

Comment 23

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: part 3: Let CreateOffscreenSurface() decide which image format to use on X11 platforms

Comment 24

[Oleg Romashin (:romaxa)]

http://hg.mozilla.org/mozilla-central/rev/bb22acd87a02
http://hg.mozilla.org/mozilla-central/rev/266e981a31bd
http://hg.mozilla.org/mozilla-central/rev/4746a5bf6662
http://hg.mozilla.org/mozilla-central/rev/abc747783d43