Last Comment Bug 721230 - Implement stub compressed texture support
: Implement stub compressed texture support
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Canvas: WebGL (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla12
Assigned To: Jon Buckley
:
Mentors:
http://www.khronos.org/registry/webgl...
Depends on: 722153 722154
Blocks: webgl-conformance 722082
  Show dependency treegraph
 
Reported: 2012-01-25 15:00 PST by Benoit Jacob [:bjacob] (mostly away)
Modified: 2012-01-29 07:32 PST (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Implement stubs for compressed textures (4.26 KB, patch)
2012-01-26 19:22 PST, Jon Buckley
jacob.benoit.1: review+
Details | Diff | Splinter Review
Now with a custom quickstub (10.47 KB, patch)
2012-01-27 08:02 PST, Jon Buckley
jacob.benoit.1: review+
Details | Diff | Splinter Review
A test page to for the new functions and changed enums (880 bytes, text/html)
2012-01-27 08:03 PST, Jon Buckley
no flags Details
v3 Fix review nits (10.46 KB, patch)
2012-01-27 13:37 PST, Jon Buckley
jon: review+
Details | Diff | Splinter Review
v4 C++ needs return statements (10.53 KB, patch)
2012-01-27 14:16 PST, Jon Buckley
jon: review+
Details | Diff | Splinter Review
Results of /webgl/sdk/tests/conformance/textures/compressed-tex-image.html with patch applied (201.15 KB, image/png)
2012-01-28 12:55 PST, Jon Buckley
no flags Details

Description Benoit Jacob [:bjacob] (mostly away) 2012-01-25 15:00:49 PST
The WebGL spec just got updated and now the compressed texture entry points are present on WebGLRenderingContext.

Implement that! We need that to be WebGL 1.0.1 conformant.

dom/interfaces/canvas/nsIDOMWebGLRenderingContext.idl

content/canvas/src/WebGLContextGL.cpp

Look at the depthMask function for an example; just return ErrorInvalidEnum(... for now.

In getParameter, just return a plain array instead of a Typed Array for now, unless you can get a hold of #jsapi people to tell you how to do it.
Comment 1 Jon Buckley 2012-01-26 19:22:37 PST
Created attachment 592031 [details] [diff] [review]
Implement stubs for compressed textures

This patch does the IDL and C++ changes. A second patch will implement a custom quick stub if I can't figure out the changes needed to qsgen.py...
Comment 2 Benoit Jacob [:bjacob] (mostly away) 2012-01-26 19:28:43 PST
Wait, changes to qsgen.py??? Did the #jsapi folks ask you to fix it to support typed arrays??
Comment 3 Jon Buckley 2012-01-26 19:32:54 PST
qsgen.py needs changes to generate a quickstub for WebGLJSObjectPtr parameters, or a custom quickstub. I haven't tried to figure out how to return a typed array within getParameter yet...
Comment 4 Jon Buckley 2012-01-27 08:02:11 PST
Created attachment 592135 [details] [diff] [review]
Now with a custom quickstub

Hm, I was asleep at the keyboard when I wrote that first patch, I had IDL *and* C++ errors. Here's a new patch with a custom quickstub.

Also, I see a difference in the IDL for compressedTexSubImage2D in 5.14 and 5.14.8. I think 5.14.8 is correct, as it follows textSubImage2D more closely.

Re-requesting review, since my last patch was not working.
Comment 5 Jon Buckley 2012-01-27 08:03:30 PST
Created attachment 592137 [details]
A test page to for the new functions and changed enums
Comment 6 Benoit Jacob [:bjacob] (mostly away) 2012-01-27 10:35:54 PST
Comment on attachment 592135 [details] [diff] [review]
Now with a custom quickstub

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

r=me but some comments below, see especially about the new typed arrays API.

::: content/canvas/src/CustomQS_WebGL.h
@@ +309,5 @@
> +    if (!JSVAL_IS_PRIMITIVE(argv[7])) {
> +        JSObject* argv7 = JSVAL_TO_OBJECT(argv[7]);
> +        if (js_IsTypedArray(argv7)) {
> +            rv = self->CompressedTexSubImage2D_array(argv0, argv1, argv2, argv3, argv4, argv5,
> +                                                     argv6, js::TypedArray::getTypedArray(argv7));

Can you check out this patch:
https://bugzilla.mozilla.org/attachment.cgi?id=588654&action=diff

They're adapting to a new Typed Arrays API so I'd rather have your new code use the new API.

::: content/canvas/src/WebGLContextGL.cpp
@@ +4462,5 @@
> +NS_IMETHODIMP
> +WebGLContext::CompressedTexImage2D(PRInt32)
> +{
> +    if (!IsContextStable())
> +        return NS_OK;

You don't have to implement anything here: this function should never get called. But I can see that this was added automatically to other functions, so not your fault. We should fix that in a separate bug (remove any nontrivial code from those functions that never get called. Or put a NS_RUNTIMEABORT(), even).

@@ +4486,5 @@
> +NS_IMETHODIMP
> +WebGLContext::CompressedTexSubImage2D(PRInt32)
> +{
> +    if (!IsContextStable())
> +        return NS_OK;

Same.
Comment 7 Jon Buckley 2012-01-27 13:37:40 PST
Created attachment 592254 [details] [diff] [review]
v3 Fix review nits

Carrying forward r+ from previous patch

This patch only addresses the NS_RUNTIMEABORT change; it's not possible to use the new jsfriend typed arrays api (bug 711843), because it hasn't landed on m-c yet.
Comment 8 Jon Buckley 2012-01-27 13:48:07 PST
Try server run: https://tbpl.mozilla.org/?tree=Try&rev=018fa973dd91
Comment 9 Jon Buckley 2012-01-27 14:16:42 PST
Created attachment 592268 [details] [diff] [review]
v4 C++ needs return statements
Comment 10 Jon Buckley 2012-01-27 14:17:28 PST
Comment on attachment 592268 [details] [diff] [review]
v4 C++ needs return statements

Carrying r+ from v2.

Try: https://tbpl.mozilla.org/?tree=Try&rev=f7e348ed22f4
Comment 11 Jon Buckley 2012-01-28 12:55:47 PST
Created attachment 592434 [details]
Results of /webgl/sdk/tests/conformance/textures/compressed-tex-image.html with patch applied

With this patch applied, Nightly passes the just added compressed texture tests.
Comment 12 Benoit Jacob [:bjacob] (mostly away) 2012-01-28 13:17:52 PST
http://hg.mozilla.org/integration/mozilla-inbound/rev/c84e0881d475
Comment 13 Joe Drew (not getting mail) 2012-01-28 18:47:39 PST
https://hg.mozilla.org/mozilla-central/rev/c84e0881d475
Comment 14 :Ms2ger (⌚ UTC+1/+2) 2012-01-29 05:32:00 PST
Comment on attachment 592268 [details] [diff] [review]
v4 C++ needs return statements

>+    if (argc != 7)
>+        return xpc_qsThrow(cx, NS_ERROR_XPC_NOT_ENOUGH_ARGS);

This is wrong, passing more than 7 arguments isn't allowed to throw.

I also see no reason that this should be a custom quickstub.
Comment 15 Benoit Jacob [:bjacob] (mostly away) 2012-01-29 05:42:25 PST
(In reply to Ms2ger from comment #14)
> Comment on attachment 592268 [details] [diff] [review]
> v4 C++ needs return statements
> 
> >+    if (argc != 7)
> >+        return xpc_qsThrow(cx, NS_ERROR_XPC_NOT_ENOUGH_ARGS);
> 
> This is wrong, passing more than 7 arguments isn't allowed to throw.

What should be the behavior then?

> 
> I also see no reason that this should be a custom quickstub.

How would you do it then?
 - how do you write the IDL for a function that takes a ArrayBufferView argument?
 - what should the corresponding C++ impl look like?
Comment 16 :Ms2ger (⌚ UTC+1/+2) 2012-01-29 07:13:43 PST
(In reply to Benoit Jacob [:bjacob] from comment #15)
> (In reply to Ms2ger from comment #14)
> > Comment on attachment 592268 [details] [diff] [review]
> > v4 C++ needs return statements
> > 
> > >+    if (argc != 7)
> > >+        return xpc_qsThrow(cx, NS_ERROR_XPC_NOT_ENOUGH_ARGS);
> > 
> > This is wrong, passing more than 7 arguments isn't allowed to throw.
> 
> What should be the behavior then?

Act as if the extra arguments weren't there

> > 
> > I also see no reason that this should be a custom quickstub.
> 
> How would you do it then?
>  - how do you write the IDL for a function that takes a ArrayBufferView
> argument?
>  - what should the corresponding C++ impl look like?

[implicit_jscontext]
CompressedTexImage2D(in unsigned long target,
                     in long level,
                     in unsigned long internalformat,
                     in long width,
                     in long height,
                     in long border,
                     in jsval data);

if (!data.isObject()) {
  return NS_ERROR_FAILURE;
}

JSObject& dataObject = data.toObject();
if (!js_IsTypedArray(&dataObject)) {
  return NS_ERROR_FAILURE;
}

JSObject* array = js::TypedArray::getTypedArray(&dataObject);

WebGLTexture *tex = activeBoundTextureForTarget(target);
if (!tex) {
  return ErrorInvalidOperation("compressedTexImage2D: no texture is bound to this target");
}

return ErrorInvalidEnum("compressedTexImage2D: compressed textures are not supported");
Comment 17 Benoit Jacob [:bjacob] (mostly away) 2012-01-29 07:32:57 PST
Hah! pass a jsval. ok. Jon, sorry I didn't know that, would have saved you the work of writing a custom QS.

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