Last Comment Bug 749538 - Failure to create GLContext for RGBX windows (i.e. in RGBX_8888 format)
: Failure to create GLContext for RGBX windows (i.e. in RGBX_8888 format)
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: unspecified
: ARM Gonk (Firefox OS)
: -- normal (vote)
: mozilla16
Assigned To: Marshall Culpepper [:marshall_law]
:
:
Mentors:
: 771539 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-04-27 00:25 PDT by Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
Modified: 2012-07-07 11:59 PDT (History)
11 users (show)
ryanvm: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Support RGBX configs (3.09 KB, patch)
2012-04-30 18:01 PDT, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
cjones.bugs: feedback+
b56girard: feedback+
ajuma.bugzilla: feedback+
Details | Diff | Splinter Review
Support 24bpp/RGBX and 32bpp/RGBA configs - v1 (3.26 KB, patch)
2012-07-06 14:22 PDT, Marshall Culpepper [:marshall_law]
no flags Details | Diff | Splinter Review
Support 24bpp/RGBX and 32bpp/RGBA configs - v2 (3.25 KB, patch)
2012-07-06 15:02 PDT, Marshall Culpepper [:marshall_law]
vladimir: review+
Details | Diff | Splinter Review
Support 24bpp/RGBX and 32bpp/RGBA configs - v3 (3.05 KB, patch)
2012-07-06 20:46 PDT, Marshall Culpepper [:marshall_law]
no flags Details | Diff | Splinter Review

Description Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-04-27 00:25:17 PDT
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.
Comment 1 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-04-30 18:01:05 PDT
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 ...
Comment 2 Ali Juma [:ajuma] 2012-05-01 07:29:15 PDT
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.
Comment 3 Marshall Culpepper [:marshall_law] 2012-07-06 11:59:41 PDT
*** Bug 771539 has been marked as a duplicate of this bug. ***
Comment 4 Marshall Culpepper [:marshall_law] 2012-07-06 14:18:37 PDT
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.
Comment 5 Marshall Culpepper [:marshall_law] 2012-07-06 14:22:39 PDT
Created attachment 639796 [details] [diff] [review]
Support 24bpp/RGBX and 32bpp/RGBA configs - v1
Comment 6 Marshall Culpepper [:marshall_law] 2012-07-06 14:24:00 PDT
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
Comment 7 Vladimir Vukicevic [:vlad] [:vladv] 2012-07-06 14:42:48 PDT
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).
Comment 8 Marshall Culpepper [:marshall_law] 2012-07-06 15:02:26 PDT
Created attachment 639820 [details] [diff] [review]
Support 24bpp/RGBX and 32bpp/RGBA configs - v2
Comment 9 Vladimir Vukicevic [:vlad] [:vladv] 2012-07-06 15:49:41 PDT
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.
Comment 10 Marshall Culpepper [:marshall_law] 2012-07-06 20:46:21 PDT
Created attachment 639920 [details] [diff] [review]
Support 24bpp/RGBX and 32bpp/RGBA configs - v3

Removed buffer size checks and query
Comment 11 Ryan VanderMeulen [:RyanVM] 2012-07-07 07:08:27 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/ac99859dce8c
Comment 12 Ryan VanderMeulen [:RyanVM] 2012-07-07 11:59:41 PDT
https://hg.mozilla.org/mozilla-central/rev/ac99859dce8c

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