Implement stub compressed texture support

RESOLVED FIXED in mozilla12

Status

()

Core
Canvas: WebGL
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: bjacob, Assigned: Jon Buckley)

Tracking

unspecified
mozilla12
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(3 attachments, 3 obsolete attachments)

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.
(Assignee)

Comment 1

6 years ago
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...
Attachment #592031 - Flags: review?(bjacob)
(Reporter)

Comment 2

6 years ago
Wait, changes to qsgen.py??? Did the #jsapi folks ask you to fix it to support typed arrays??
(Reporter)

Updated

6 years ago
Attachment #592031 - Flags: review?(bjacob) → review+
(Assignee)

Comment 3

6 years ago
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...
(Assignee)

Comment 4

6 years ago
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.
Attachment #592031 - Attachment is obsolete: true
Attachment #592135 - Flags: review?(bjacob)
(Assignee)

Comment 5

6 years ago
Created attachment 592137 [details]
A test page to for the new functions and changed enums
(Reporter)

Comment 6

6 years ago
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.
Attachment #592135 - Flags: review?(bjacob) → review+
(Assignee)

Comment 7

6 years ago
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.
Attachment #592135 - Attachment is obsolete: true
Attachment #592254 - Flags: review+
(Assignee)

Comment 8

6 years ago
Try server run: https://tbpl.mozilla.org/?tree=Try&rev=018fa973dd91
(Assignee)

Comment 9

6 years ago
Created attachment 592268 [details] [diff] [review]
v4 C++ needs return statements
Attachment #592254 - Attachment is obsolete: true
(Assignee)

Comment 10

6 years ago
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
Attachment #592268 - Flags: review+
(Assignee)

Comment 11

6 years ago
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.
http://hg.mozilla.org/integration/mozilla-inbound/rev/c84e0881d475
Target Milestone: --- → mozilla12
(Assignee)

Updated

6 years ago
Blocks: 722082
https://hg.mozilla.org/mozilla-central/rev/c84e0881d475
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
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.

Updated

6 years ago
Depends on: 722153

Updated

6 years ago
Depends on: 722154
(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?
(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");
Hah! pass a jsval. ok. Jon, sorry I didn't know that, would have saved you the work of writing a custom QS.
You need to log in before you can comment on or make changes to this bug.