Last Comment Bug 697757 - context-attributes-alpha-depth-stencil-antialias.html test failures
: context-attributes-alpha-depth-stencil-antialias.html test failures
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Canvas: WebGL (show other bugs)
: Trunk
: All Mac OS X
: -- normal (vote)
: mozilla10
Assigned To: Doug Sherk (:drs) (inactive)
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-10-27 10:59 PDT by Doug Sherk (:drs) (inactive)
Modified: 2011-11-01 07:40 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v1.0, fixed stencil alpha test failures. (2.38 KB, patch)
2011-10-27 11:04 PDT, Doug Sherk (:drs) (inactive)
jgilbert: review-
Details | Diff | Splinter Review
Patch v1.0, fix for alpha being improperly set. (2.15 KB, patch)
2011-10-27 12:29 PDT, Doug Sherk (:drs) (inactive)
no flags Details | Diff | Splinter Review
Patch v1.0, fix for alpha being improperly set. (2.90 KB, patch)
2011-10-27 12:31 PDT, Doug Sherk (:drs) (inactive)
jgilbert: review+
Details | Diff | Splinter Review
Patch v1.1, fixed stencil alpha test failures. (3.32 KB, patch)
2011-10-28 15:37 PDT, Doug Sherk (:drs) (inactive)
bugzilla: review+
Details | Diff | Splinter Review

Description Doug Sherk (:drs) (inactive) 2011-10-27 10:59:50 PDT
https://cvs.khronos.org/svn/repos/registry/trunk/public/webgl/sdk/tests/conformance/context/context-attributes-alpha-depth-stencil-antialias.html

This page fails because we're not giving the user back the info they inputted and instead getting an internal format.
Comment 1 Doug Sherk (:drs) (inactive) 2011-10-27 11:04:58 PDT
Created attachment 570035 [details] [diff] [review]
Patch v1.0, fixed stencil alpha test failures.

Proposed patch. Ran mochitests locally on OSX 10.7.
Comment 2 Doug Sherk (:drs) (inactive) 2011-10-27 11:25:34 PDT
So this patch is wrong and we're going to hold off on fixing it for now. jgilbert tells me that it's probably due to this block of code in gfx/thebes/GLContext.cpp:

1104    // Allocate texture
1105     if (aUseReadFBO) {
1106         fBindTexture(LOCAL_GL_TEXTURE_2D, newOffscreenTexture);
1107         fTexParameteri(LOCAL_GL_TEXTURE_2D, LOCAL_GL_TEXTURE_MIN_FILTER, LOCAL_GL_LINEAR);
1108         fTexParameteri(LOCAL_GL_TEXTURE_2D, LOCAL_GL_TEXTURE_MAG_FILTER, LOCAL_GL_LINEAR);
1109 
1110         if (alpha) {
1111             fTexImage2D(LOCAL_GL_TEXTURE_2D,
1112                         0,
1113                         LOCAL_GL_RGBA,
1114                         aSize.width, aSize.height,
1115                         0,
1116                         LOCAL_GL_RGBA,
1117                         LOCAL_GL_UNSIGNED_BYTE,
1118                         NULL);
1119 
1120             cf.red = cf.green = cf.blue = cf.alpha = 8;
1121         } else {
1122             fTexImage2D(LOCAL_GL_TEXTURE_2D,
1123                         0,
1124                         LOCAL_GL_RGB,
1125                         aSize.width, aSize.height,
1126                         0,
1127                         LOCAL_GL_RGB,
1128 #ifdef XP_WIN
1129                         LOCAL_GL_UNSIGNED_BYTE,
1130 #else
1131                         mIsGLES2 ? LOCAL_GL_UNSIGNED_SHORT_5_6_5
1132                                  : LOCAL_GL_UNSIGNED_BYTE,
1133 #endif
1134                         NULL);
1135 
1136 #ifdef XP_WIN
1137             cf.red = cf.green = cf.blue = 8;
1138 #else
1139             cf.red = 5;
1140             cf.green = 6;
1141             cf.blue = 5;
1142 #endif
1143             cf.alpha = 0;
1144         }
1145     }
Comment 3 Jeff Gilbert [:jgilbert] 2011-10-27 11:51:24 PDT
Comment on attachment 570035 [details] [diff] [review]
Patch v1.0, fixed stencil alpha test failures.

After offline discussion, the proper approach seems to be that we need to check whether our requests for the presence/absence of certain buffers is respected during offscreen context creation.
Comment 4 Doug Sherk (:drs) (inactive) 2011-10-27 12:29:19 PDT
Created attachment 570050 [details] [diff] [review]
Patch v1.0, fix for alpha being improperly set.

If we request alpha and it isn't enabled, or we request it off and it gets enabled, we now fail the pbuffer creation and fall back to FBOs. This is bad for performance but allows us to pass the conformance test in this case. Ran mochitests locally.
Comment 5 Doug Sherk (:drs) (inactive) 2011-10-27 12:31:04 PDT
Created attachment 570051 [details] [diff] [review]
Patch v1.0, fix for alpha being improperly set.

Whoops, forgot to remove this test from failing_tests_mac.txt.
Comment 6 Jeff Gilbert [:jgilbert] 2011-10-27 15:56:29 PDT
Comment on attachment 570051 [details] [diff] [review]
Patch v1.0, fix for alpha being improperly set.

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

Looks good.

I'll just add that since we are able to fail out of PBuffer creation before we allocate the PBuffer, it should have a negligible effect on context start-up performance. Also it lets us provide a proper context for a given request.
Comment 7 Doug Sherk (:drs) (inactive) 2011-10-27 21:42:14 PDT
Running on Try: https://tbpl.mozilla.org/?tree=Try&rev=20a50709f71c
Comment 8 Doug Sherk (:drs) (inactive) 2011-10-28 15:37:26 PDT
Created attachment 570382 [details] [diff] [review]
Patch v1.1, fixed stencil alpha test failures.

Didn't notice at first, but this also fixes tex-input-validation.html. I removed this from failing_tests_mac.txt.

+r carried
Comment 9 Jeff Gilbert [:jgilbert] 2011-10-31 17:42:22 PDT
More recent try: https://tbpl.mozilla.org/?tree=Try&rev=69bb43191112
Comment 11 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-11-01 07:40:38 PDT
https://hg.mozilla.org/mozilla-central/rev/4f67677970f2

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