The default bug view has changed. See this FAQ.

Failure to create GLContext for RGBX windows (i.e. in RGBX_8888 format)

RESOLVED FIXED in mozilla16

Status

()

Core
Graphics
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: cjones, Assigned: marshall_law)

Tracking

unspecified
mozilla16
ARM
Gonk (Firefox OS)
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

The basic problem here is that GLContextProviderEGL always tries to use an RGBA_8888 config when the color depth is 24-bit, but on some systems where the window for which the context is being created is actually RGBX_8888, the context creation fails.

This might be a little bit tricky to fix because I'm not sure we always know that the OS window manager supports translucent windows and we want to paint to the window with opacity.  However, we certainly *do* know that the screen GL context for gonk never needs an alpha channel, so we can reorg things so that at least gonk nsWindow only asks for RGBX.
Created attachment 619790 [details] [diff] [review]
Support RGBX configs

I'm not qualified to drive this through, but the approach looks good.

First, an all-important nit: the |if (depth == ...)| block should be a switch statement.

More importantly, this patch exposes a big mess of assumptions we make in widget-land about color depth, pixel depth, and pixel format.  On some windowing systems, we actually do want to create an RGBA config.  Maybe on android?  I think we do on win7 when this code is used, but no one cares about that path.  We definitely don't need/want RGBA for Gonk's "screen GL context", so this patch definitely fixes that up.

The problem is, all relevant widget backends will report color-depth == 24, but that's not enough info to let us decide whether we really do want RGBA or don't care.  An approach in which we try RGBA first but then fall back on RGBX won't work for the original bug report, because the GL impl supported RGBA *contexts*, just not for the particular *window* we wanted to use it with.

So I think the solution is probably for the widget backends to report RGBA/RGBX correctly.  We can have android say RGBA to avoid breaking current code.  And we can have gonk report RGBX to fix this bug, and fall back on RGBA if RGBX isn't supported.  Some breakage is still possible there, though.

What do you guys think?  Not sure who to touch for review here ...
Attachment #619790 - Flags: feedback+
Attachment #619790 - Flags: feedback?(bgirard)
Attachment #619790 - Flags: feedback?(ajuma)

Comment 2

5 years ago
Comment on attachment 619790 [details] [diff] [review]
Support RGBX configs

(In reply to Chris Jones [:cjones] [:warhammer] from comment #1)
> So I think the solution is probably for the widget backends to report
> RGBA/RGBX correctly.  We can have android say RGBA to avoid breaking current
> code.  And we can have gonk report RGBX to fix this bug, and fall back on
> RGBA if RGBX isn't supported. 

I'd prefer widget backends explicitly reporting RGBA/RGBX rather than doing a depth-to-format conversion in CreateConfig. But the approach overall seems fine.
Attachment #619790 - Flags: feedback?(ajuma) → feedback+

Updated

5 years ago
Attachment #619790 - Flags: feedback?(bgirard) → feedback+
(Assignee)

Updated

5 years ago
Duplicate of this bug: 771539
(Assignee)

Comment 4

5 years ago
The hardware-accelerated Android emulator returns 32bpp for it's depth, and the current logic only looks at RGBA_8888 when then depth is 24bpp, causing a fallback to software rendering.
QA Contact: marshall
(Assignee)

Updated

5 years ago
Assignee: nobody → marshall
QA Contact: marshall
(Assignee)

Comment 5

5 years ago
Created attachment 639796 [details] [diff] [review]
Support 24bpp/RGBX and 32bpp/RGBA configs - v1
Attachment #619790 - Attachment is obsolete: true
Attachment #639796 - Flags: review?
(Assignee)

Comment 6

5 years ago
Comment on attachment 639796 [details] [diff] [review]
Support 24bpp/RGBX and 32bpp/RGBA configs - v1

This is an update of cjones' original patch, along with a few updates
Attachment #639796 - Flags: review? → review?(vladimir)
(Assignee)

Updated

5 years ago
Attachment #639796 - Attachment description: Suport 24bpp/RGBX and 32bpp/RGBA configs - v1 → Support 24bpp/RGBX and 32bpp/RGBA configs - v1
Comment on attachment 639796 [details] [diff] [review]
Support 24bpp/RGBX and 32bpp/RGBA configs - v1

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

::: gfx/gl/GLContextProviderEGL.cpp
@@ +1570,2 @@
>              ((depth == 16 && r == 5 && g == 6 && b == 5) ||
> +             (depth == 24 && d == 24 && r == 8 && g == 8 && b == 8 && a == 0) ||

I don't actually like the check for a == 0 -- there may be some platforms that can only give you an 8888 config and have no 8880 configs.  (Maybe?)  I would just remove the a == 0 portion of this check, since we don't care according to our config query (if we cared, we'd specify alpha max of 0).
(Assignee)

Comment 8

5 years ago
Created attachment 639820 [details] [diff] [review]
Support 24bpp/RGBX and 32bpp/RGBA configs - v2
Attachment #639796 - Attachment is obsolete: true
Attachment #639796 - Flags: review?(vladimir)
Attachment #639820 - Flags: review?(vladimir)
Comment on attachment 639820 [details] [diff] [review]
Support 24bpp/RGBX and 32bpp/RGBA configs - v2

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

::: gfx/gl/GLContextProviderEGL.cpp
@@ +1570,2 @@
>              ((depth == 16 && r == 5 && g == 6 && b == 5) ||
> +             (depth == 24 && d == 24 && r == 8 && g == 8 && b == 8) ||

Oop, I missed this first time around.  This should be d >= 24, though I would just omit it entirely since we check the r/g/b values directly.  Same with getting rid of d == 32 (and querying BUFFER_SIZE at all) -- no need.

r+ with that.
Attachment #639820 - Flags: review?(vladimir) → review+
(Assignee)

Comment 10

5 years ago
Created attachment 639920 [details] [diff] [review]
Support 24bpp/RGBX and 32bpp/RGBA configs - v3

Removed buffer size checks and query
Attachment #639820 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/ac99859dce8c
Flags: in-testsuite-
Keywords: checkin-needed
Target Milestone: --- → mozilla16
https://hg.mozilla.org/mozilla-central/rev/ac99859dce8c
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.