721230 - Implement stub compressed texture support

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Description

[Benoit Jacob [:bjacob] (mostly away)]

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]

Attachment: 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)]

Wait, changes to qsgen.py??? Did the #jsapi folks ask you to fix it to support typed arrays??

Comment 3

[Jon Buckley]

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]

Attachment: 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]

Attachment: A test page to for the new functions and changed enums

Comment 6

[Benoit Jacob [:bjacob] (mostly away)]

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]

Attachment: 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]

Try server run: https://tbpl.mozilla.org/?tree=Try&rev=018fa973dd91

Comment 9

[Jon Buckley]

Attachment: v4 C++ needs return statements

Comment 10

[Jon Buckley]

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]

Attachment: 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)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/c84e0881d475

Comment 13

[Joe Drew (not getting mail)]

https://hg.mozilla.org/mozilla-central/rev/c84e0881d475

Comment 14

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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)]

(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 (he/him; ⌚ UTC+1/+2)]

(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)]

Hah! pass a jsval. ok. Jon, sorry I didn't know that, would have saved you the work of writing a custom QS.