Closed Bug 743182 Opened 12 years ago Closed 12 years ago

Not every mobile device uses RGB565

Categories

(Core :: Graphics, defect)

Product:
Core
Component:
Graphics
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla15

People

(Reporter: kanru, Assigned: kanru)

References

Details

Attachments

(1 file, 1 obsolete file)

Use gfxPlatform::OptimalFormatForContent everywhere.
23.26 KB, patch
joe
: review+
Details | Diff | Splinter Review 
For example using RGB565 will cause color banding on galaxy s2.
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.
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.
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
That's definitely not the intended behavior.  Whatever code was being "fixed" should have been using gfxPlatform::CreateOffscreenSurface(), most likely.
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.
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).
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.
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
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.
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.
(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)
It's wrong to assume that CONTENT_COLOR => (some particular surface format), that's correct.  I'd be happy with removing that.
https://tbpl.mozilla.org/?tree=Try&rev=31cc3333c7ec

Try push fixing the missing GdkScreen issue.
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!
Attachment #622101 - Flags: review?(jones.chris.g)
Attachment #622101 - Flags: review?(joe)
Attachment #622101 - Flags: feedback+
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?
Attachment #622101 - Flags: review?(joe) → review-
(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!
(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?
(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.
try run: https://tbpl.mozilla.org/?tree=Try&rev=624a2d7ab50f
Assignee: nobody → kchen
Attachment #622101 - Attachment is obsolete: true
Attachment #626745 - Flags: review?(joe)
Comment on attachment 626745 [details] [diff] [review]
Use gfxPlatform::OptimalFormatForContent everywhere.

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

Looks great!
Attachment #626745 - Flags: review?(joe) → review+
https://hg.mozilla.org/mozilla-central/rev/2225460277a3
Status: NEW → RESOLVED
Closed: 12 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Depends on: 759653
Blocks: 764752
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: