Closed Bug 593175 Opened 12 years ago Closed 12 years ago

Use 16bpp when MOZ_GFX_OPTIMIZE_MOBILE is set.

Categories

(Core :: Graphics, defect)

All
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Tracking Status
fennec 2.0b1+ ---

People

(Reporter: dougt, Assigned: cjones)

References

Details

Attachments

(5 files, 3 obsolete files)

1.44 KB, patch
karlt
: review+
Details | Diff | Splinter Review
7.02 KB, patch
karlt
: review+
Details | Diff | Splinter Review
1.10 KB, patch
karlt
: review+
Details | Diff | Splinter Review
49.60 KB, patch
Details | Diff | Splinter Review
4.41 KB, patch
karlt
: review+
Details | Diff | Splinter Review
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.
Attached patch patch v.1 (obsolete) — Splinter Review
this just hard codes, we probably can do something smarter at some point.. maybe move this into a inline.
Assignee: nobody → doug.turner
Attachment #471657 - Flags: review?(jones.chris.g)
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...
indeed.  this patch forces 16bpp with a hammer which is wrong.
Also it would be probably nice to use gfxPlatform->CreateOffscreenSurface, that function will create for us platform preferred  surface with automatic 16bpp switch
Also I think this bug should cooperate with bug 576437
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
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().
(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.
Hey Doug, mind if I steal this bug from you?  I ported your original patch to what gfx/layers will be after bug 570625.
Assignee: doug.turner → jones.chris.g
Attachment #471657 - Flags: review?(jones.chris.g) → review-
Doesn't really fit here, but I thought I'd sneak it in ;).
Attachment #475000 - Flags: review?(karlt)
Requesting blocking based on comment 8.  This should hopefully also be a win with GL compositing for similar reasons.
tracking-fennec: --- → ?
tracking-fennec: ? → 2.0+
Why is OptimalFormatFor duplicated?
Might be different for different backends.  I'd be happy to keep one impl and split as needed, if you prefer.
Attachment #475000 - Flags: review?(karlt) → review+
Chris does it make sense fix bug 576437? will it help for fixing this bug?
blocking b1 on this -- makes a pretty big difference on Maemo.
tracking-fennec: 2.0+ → 2.0b1+
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.
Attachment #475002 - Flags: review?(karlt) → review-
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
Depends on: 576437
Also fixed missed review comment.
Attachment #475001 - Attachment is obsolete: true
Attachment #475002 - Attachment is obsolete: true
Attachment #476018 - Flags: review?(karlt)
Attachment #475001 - Flags: review?(roc)
Mobile/X11 can be a separate patch when dep is fixed.
Attachment #476019 - Flags: review?(karlt)
Attachment #476018 - Flags: review?(karlt) → review+
Attachment #476019 - Flags: review?(karlt) → review+
Attachment #476337 - Flags: review?(karlt) → review+
You need to log in before you can comment on or make changes to this bug.