Closed
Bug 721230
Opened 12 years ago
Closed 12 years ago
Implement stub compressed texture support
Categories
(Core :: Graphics: CanvasWebGL, defect)
Core
Graphics: CanvasWebGL
Tracking
()
RESOLVED
FIXED
mozilla12
People
(Reporter: bjacob, Assigned: jon)
References
()
Details
Attachments
(3 files, 3 obsolete files)
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•12 years ago
|
||
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•12 years ago
|
||
Wait, changes to qsgen.py??? Did the #jsapi folks ask you to fix it to support typed arrays??
Reporter | ||
Updated•12 years ago
|
Attachment #592031 -
Flags: review?(bjacob) → review+
Assignee | ||
Comment 3•12 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•12 years ago
|
||
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•12 years ago
|
||
Reporter | ||
Comment 6•12 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•12 years ago
|
||
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•12 years ago
|
||
Try server run: https://tbpl.mozilla.org/?tree=Try&rev=018fa973dd91
Assignee | ||
Comment 9•12 years ago
|
||
Attachment #592254 -
Attachment is obsolete: true
Assignee | ||
Comment 10•12 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•12 years ago
|
||
With this patch applied, Nightly passes the just added compressed texture tests.
Reporter | ||
Comment 12•12 years ago
|
||
http://hg.mozilla.org/integration/mozilla-inbound/rev/c84e0881d475
Target Milestone: --- → mozilla12
Comment 13•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/c84e0881d475
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Comment 14•12 years ago
|
||
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.
Reporter | ||
Comment 15•12 years ago
|
||
(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•12 years ago
|
||
(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");
Reporter | ||
Comment 17•12 years ago
|
||
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.
Description
•