Last Comment Bug 743182 - Not every mobile device uses RGB565
: Not every mobile device uses RGB565
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla15
Assigned To: Kan-Ru Chen [:kanru] (UTC+8)
:
Mentors:
Depends on: 759653
Blocks: 740372 741745 764752
  Show dependency treegraph
 
Reported: 2012-04-06 01:32 PDT by Kan-Ru Chen [:kanru] (UTC+8)
Modified: 2012-06-14 03:16 PDT (History)
7 users (show)
ryanvm: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Use gfxPlatform::OptimalFormatForContent everywhere. (25.25 KB, patch)
2012-05-08 13:01 PDT, Kan-Ru Chen [:kanru] (UTC+8)
joe: review-
cjones.bugs: feedback+
Details | Diff | Splinter Review
Use gfxPlatform::OptimalFormatForContent everywhere. (23.26 KB, patch)
2012-05-24 03:08 PDT, Kan-Ru Chen [:kanru] (UTC+8)
joe: review+
Details | Diff | Splinter Review

Description Kan-Ru Chen [:kanru] (UTC+8) 2012-04-06 01:32:20 PDT
For example using RGB565 will cause color banding on galaxy s2.
Comment 1 Kan-Ru Chen [:kanru] (UTC+8) 2012-04-10 20:49:34 PDT
To be more precise, I think this a regression from bug 740372 which causes FormatFromContent always returns RGB565 unconditionally. I can observe color banding on device which use native 565 framebuffer too.
Comment 2 Michael Wu [:mwu] 2012-04-24 17:20:10 PDT
Yeah bug 740372 looks wrong to me. It's not acceptable to use MOZ_GFX_OPTIMIZE_MOBILE to determine whether 565 should be used. Code should now query nsIScreen or use some other runtime mechanism to determine whether 565 or 888 should be used.
Comment 3 George Wright (:gw280) (:gwright) 2012-04-25 07:29:24 PDT
bug 740372 didn't actually change the intended behaviour; the intended behaviour for this codebase was always to use RGB565 for MOZ_GFX_OPTIMIZE_MOBILE. bug 740372 simply fixed a few places where this assumption wasn't being held (it was being held in other parts of the codebase previously).

I suspect the bug you want to be commenting on is https://bugzilla.mozilla.org/show_bug.cgi?id=593175
Comment 4 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-04-26 17:05:20 PDT
That's definitely not the intended behavior.  Whatever code was being "fixed" should have been using gfxPlatform::CreateOffscreenSurface(), most likely.
Comment 5 George Wright (:gw280) (:gwright) 2012-04-26 17:13:50 PDT
Maybe I'm misunderstanding then; why are some of the conversion functions therefore assuming CONTENT_COLOR == RGB565 and others aren't? The fix I did was simply to make them all consistent.
Comment 6 George Wright (:gw280) (:gwright) 2012-04-26 17:22:41 PDT
Notably: it seems wrong to me that this diff:

https://bugzilla.mozilla.org/attachment.cgi?id=476019&action=diff

Makes it implied that CONTENT_COLOR == ImageFormatRGB16_565, but that our conversion functions in gfx2DGlue.h weren't handling that case. Is there another method we should be using to work out the true colour format for a pixel buffer (this is particularly important if we're doing stuff like allocating a gfxImageSurface to back our content and wrapping an Azure DrawTarget around its pixel data).
Comment 7 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-04-26 18:07:38 PDT
As mwu said, image-allocating code needs to ask nsIScreen or something else that knows about the color depth.  gfxPlatform is one of those things, and gfxPlatform::CreateOffscreenSurface() is usually what you want.

In general, all MOZ_GFX_OPTIMIZE_MOBILE ifdefs are turds, and seeing one should make you vomit in your mouth a little.  We're in the process of removing them.
Comment 8 George Wright (:gw280) (:gwright) 2012-04-26 18:10:59 PDT
OK this makes sense, but I'm still worried then that we have a conversion function that'll take CONTENT_COLOR and return back a format that may or may not be correct... If there's no way to determine what the format is then the function really shouldn't even exist as it makes no sense
Comment 9 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-04-26 18:13:28 PDT
There's no problem with querying the actual format of the surface, or for asking what the preferred format is for CONTENT_COLOR.  The problem is assuming GFX_MOBILE_TURD => r5g6b5, anywhere.
Comment 10 George Wright (:gw280) (:gwright) 2012-04-26 18:21:23 PDT
So it sounds like the patch that was previously landed was wrong, and we should also look to remove the conversion function for content to format? The existence of such a function is dangerous if this is the case, as it'll never be guaranteed to return something that's correct.
Comment 11 George Wright (:gw280) (:gwright) 2012-04-26 18:22:28 PDT
(unless it's possible to guarantee it by querying nsIScreen for the pixel depth? I don't know enough of when CONTENT_COLOR => RGB565 and when it doesn't)
Comment 12 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-04-26 18:24:06 PDT
It's wrong to assume that CONTENT_COLOR => (some particular surface format), that's correct.  I'd be happy with removing that.
Comment 13 Kan-Ru Chen [:kanru] (UTC+8) 2012-05-08 13:01:50 PDT
Created attachment 622101 [details] [diff] [review]
Use gfxPlatform::OptimalFormatForContent everywhere.

https://tbpl.mozilla.org/?tree=Try&rev=f271950214b4
Comment 14 Kan-Ru Chen [:kanru] (UTC+8) 2012-05-08 14:09:19 PDT
https://tbpl.mozilla.org/?tree=Try&rev=31cc3333c7ec

Try push fixing the missing GdkScreen issue.
Comment 15 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-05-23 14:59:49 PDT
Comment on attachment 622101 [details] [diff] [review]
Use gfxPlatform::OptimalFormatForContent everywhere.


Sorry for the review lag :(.

>diff --git a/gfx/layers/ipc/ShadowLayers.cpp b/gfx/layers/ipc/ShadowLayers.cpp

There was a reason I didn't use gfxPlatform here but I don't remember
what it was.  Please make sure this passes try on fennec-XUL.

>diff --git a/gfx/thebes/gfxAndroidPlatform.h b/gfx/thebes/gfxAndroidPlatform.h

>+    virtual mozilla::gfx::SurfaceFormat Optimal2DFormatForContent(gfxASurface::gfxContentType aContent)

Please move this definition into gfxAndroidPlatform.cpp.

>+    virtual gfxImageFormat OptimalFormatForContent(gfxASurface::gfxContentType aContent)

And here.

>diff --git a/gfx/thebes/gfxPlatform.h b/gfx/thebes/gfxPlatform.h
>--- a/gfx/thebes/gfxPlatform.h
>+++ b/gfx/thebes/gfxPlatform.h
>@@ -442,18 +442,45 @@ public:
>     PRInt32 GetBidiNumeralOption();
> 
>     /**
>      * Returns a 1x1 surface that can be used to create graphics contexts
>      * for measuring text etc as if they will be rendered to the screen
>      */
>     gfxASurface* ScreenReferenceSurface() { return mScreenReferenceSurface; }
> 
>+    virtual mozilla::gfx::SurfaceFormat Optimal2DFormatForContent(gfxASurface::gfxContentType aContent)

>+    virtual gfxImageFormat OptimalFormatForContent(gfxASurface::gfxContentType aContent)

And here, please move into gfxPlatform.cpp.

This looks OK to me, but I'm not a gfx peer so I can't review this.
Handing off.

Nice work!
Comment 16 Joe Drew (not getting mail) 2012-05-23 15:19:18 PDT
Comment on attachment 622101 [details] [diff] [review]
Use gfxPlatform::OptimalFormatForContent everywhere.

Review of attachment 622101 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/thebes/gfxAndroidPlatform.h
@@ +71,5 @@
> +    virtual mozilla::gfx::SurfaceFormat Optimal2DFormatForContent(gfxASurface::gfxContentType aContent)
> +    {
> +        if (aContent == gfxASurface::CONTENT_COLOR) {
> +            if (mOffscreenFormat == gfxImageFormat::ImageFormatRGB24) {
> +                return mozilla::gfx::FORMAT_R5G6B5;

This looks exactly backwards. Am I just misreading?

::: gfx/thebes/gfxPlatform.h
@@ +454,5 @@
> +            return mozilla::gfx::FORMAT_B8G8R8X8;
> +        case gfxASurface::CONTENT_ALPHA:
> +            return mozilla::gfx::FORMAT_A8;
> +        default:
> +            return mozilla::gfx::FORMAT_B8G8R8A8;

Maybe throw in a handling of CONTENT_COLOR_ALPHA plus the default case with an NS_NOTREACHED() as below.

::: gfx/thebes/gfxPlatformGtk.cpp
@@ +516,5 @@
>  gfxPlatformGtk::GetOffscreenFormat()
>  {
> +    // Make sure there is a screen
> +    GdkScreen *screen = gdk_screen_get_default();
> +    if (screen && gdk_visual_get_system()->depth == 16) {

This seems unrelated, and should probably be moved to a different bug.

::: gfx/thebes/gfxQtPlatform.cpp
@@ +639,5 @@
> +    if (aContent == gfxASurface::CONTENT_COLOR)
> +        return GetOffscreenFormat();
> +    else
> +        return gfxPlatform::OptimalFormatForContent(aContent);
> +}

There is a lot of duplication of this exact code. Why can't it be put into gfxPlatform, the base class?
Comment 17 Kan-Ru Chen [:kanru] (UTC+8) 2012-05-23 19:15:00 PDT
(In reply to Joe Drew (:JOEDREW!) from comment #16)
> Comment on attachment 622101 [details] [diff] [review]
> Use gfxPlatform::OptimalFormatForContent everywhere.
> 
> Review of attachment 622101 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/thebes/gfxAndroidPlatform.h
> @@ +71,5 @@
> > +    virtual mozilla::gfx::SurfaceFormat Optimal2DFormatForContent(gfxASurface::gfxContentType aContent)
> > +    {
> > +        if (aContent == gfxASurface::CONTENT_COLOR) {
> > +            if (mOffscreenFormat == gfxImageFormat::ImageFormatRGB24) {
> > +                return mozilla::gfx::FORMAT_R5G6B5;
> 
> This looks exactly backwards. Am I just misreading?

Good catch! For Android we probably want to always return FORMAT_R5G6B5 for now. I will kill the condition.
 
> ::: gfx/thebes/gfxPlatform.h
> @@ +454,5 @@
> > +            return mozilla::gfx::FORMAT_B8G8R8X8;
> > +        case gfxASurface::CONTENT_ALPHA:
> > +            return mozilla::gfx::FORMAT_A8;
> > +        default:
> > +            return mozilla::gfx::FORMAT_B8G8R8A8;
> 
> Maybe throw in a handling of CONTENT_COLOR_ALPHA plus the default case with
> an NS_NOTREACHED() as below.

This is copied from the old FormatForContent and I thought I shouldn't touch the logic too much. Will try that. 

> ::: gfx/thebes/gfxPlatformGtk.cpp
> @@ +516,5 @@
> >  gfxPlatformGtk::GetOffscreenFormat()
> >  {
> > +    // Make sure there is a screen
> > +    GdkScreen *screen = gdk_screen_get_default();
> > +    if (screen && gdk_visual_get_system()->depth == 16) {
> 
> This seems unrelated, and should probably be moved to a different bug.

I have to fix that otherwise some gtk xpcshell tests will break.
 
> ::: gfx/thebes/gfxQtPlatform.cpp
> @@ +639,5 @@
> > +    if (aContent == gfxASurface::CONTENT_COLOR)
> > +        return GetOffscreenFormat();
> > +    else
> > +        return gfxPlatform::OptimalFormatForContent(aContent);
> > +}
> 
> There is a lot of duplication of this exact code. Why can't it be put into
> gfxPlatform, the base class?

Good idea!
Comment 18 Joe Drew (not getting mail) 2012-05-23 19:30:55 PDT
(In reply to Kan-Ru Chen [:kanru] from comment #17)
> > ::: gfx/thebes/gfxPlatformGtk.cpp
> > @@ +516,5 @@
> > >  gfxPlatformGtk::GetOffscreenFormat()
> > >  {
> > > +    // Make sure there is a screen
> > > +    GdkScreen *screen = gdk_screen_get_default();
> > > +    if (screen && gdk_visual_get_system()->depth == 16) {
> > 
> > This seems unrelated, and should probably be moved to a different bug.
> 
> I have to fix that otherwise some gtk xpcshell tests will break.

That implies you've changed behaviour somehow; can you (or have you) track(ed) down how?
Comment 19 Kan-Ru Chen [:kanru] (UTC+8) 2012-05-23 19:39:37 PDT
(In reply to Joe Drew (:JOEDREW!) from comment #18)
> (In reply to Kan-Ru Chen [:kanru] from comment #17)
> > > ::: gfx/thebes/gfxPlatformGtk.cpp
> > > @@ +516,5 @@
> > > >  gfxPlatformGtk::GetOffscreenFormat()
> > > >  {
> > > > +    // Make sure there is a screen
> > > > +    GdkScreen *screen = gdk_screen_get_default();
> > > > +    if (screen && gdk_visual_get_system()->depth == 16) {
> > > 
> > > This seems unrelated, and should probably be moved to a different bug.
> > 
> > I have to fix that otherwise some gtk xpcshell tests will break.
> 
> That implies you've changed behaviour somehow; can you (or have you)
> track(ed) down how?

Yes. Because previously GetOffscreenFormat() was guarded:

>     GdkScreen *gdkScreen = gdk_screen_get_default();
>     if (gdkScreen) {
>-        // try to optimize it for 16bpp default screen
>-        if (gfxASurface::CONTENT_COLOR == contentType) {
>-            imageFormat = GetOffscreenFormat();
>-        }

But now it might be called elsewhere so I moved that check into GetOffscreenFormat() itself.
Comment 20 Kan-Ru Chen [:kanru] (UTC+8) 2012-05-24 03:08:54 PDT
Created attachment 626745 [details] [diff] [review]
Use gfxPlatform::OptimalFormatForContent everywhere.

try run: https://tbpl.mozilla.org/?tree=Try&rev=624a2d7ab50f
Comment 21 Joe Drew (not getting mail) 2012-05-25 11:24:46 PDT
Comment on attachment 626745 [details] [diff] [review]
Use gfxPlatform::OptimalFormatForContent everywhere.

Review of attachment 626745 [details] [diff] [review]:
-----------------------------------------------------------------

Looks great!
Comment 22 Kan-Ru Chen [:kanru] (UTC+8) 2012-05-25 21:41:33 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/2225460277a3
Comment 23 Ryan VanderMeulen [:RyanVM] 2012-05-26 05:35:05 PDT
https://hg.mozilla.org/mozilla-central/rev/2225460277a3

Note You need to log in before you can comment on or make changes to this bug.