implement new TexImage2D pixel store params and signature

RESOLVED FIXED

Status

()

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

People

(Reporter: vlad, Assigned: bjacob)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

Glipy etc. were removed as arguments and moved to webgl-specific pixelstore args; Also the TexImage2D prototype that takes a DOM element signature was changed to take the full range of TexImage2D args.

We need to update our impl for this.

https://cvs.khronos.org/svn/repos/registry/trunk/public/webgl/doc/spec/WebGL-spec.html#5.13.8
https://cvs.khronos.org/svn/repos/registry/trunk/public/webgl/doc/spec/WebGL-spec.html#PIXEL_STORAGE_PARAMETERS
(Assignee)

Comment 1

8 years ago
Created attachment 450725 [details] [diff] [review]
TexImage2D API change

This implements the API change in TexImage2D and TexSubImage2D.
Also improves the quickstubs so we honor the difference between signed and unsigned parameters.
Attachment #450725 - Flags: review?(vladimir)
(Assignee)

Comment 2

8 years ago
Note: to test that, I have local changes to the webgl tests.
(Assignee)

Comment 3

8 years ago
Created attachment 450736 [details] [diff] [review]
Fix PixelStorei

And here is the second part, implementing the new PixelStorei parameters. It also incorporates the fix from bug 566698 and implements PACK_ALIGNMENT in GetParameter.
Attachment #450736 - Flags: review?(vladimir)
(Assignee)

Comment 4

8 years ago
Note that the API change breaks 90% of existing WebGL demos, so I am going to update it with a new patch that supports both the old and new APIs.
(Assignee)

Comment 5

8 years ago
Created attachment 450746 [details] [diff] [review]
TexImage2D API change

This new version supports both the old and new API! And logs a useful message when the old API is used, in the hope that web developers sometimes read the error console.
Attachment #450725 - Attachment is obsolete: true
Attachment #450746 - Flags: review?(vladimir)
Attachment #450725 - Flags: review?(vladimir)
(Assignee)

Comment 6

8 years ago
Note: the TexImage2D API patch should be applied first, the PixelStore patch second.
(Assignee)

Updated

8 years ago
Duplicate of this bug: 566698
Comment on attachment 450746 [details] [diff] [review]
TexImage2D API change

Just a few comments:

>@@ -203,93 +203,98 @@ nsICanvasRenderingContextWebGL_TexImage2
>     nsresult rv;
> 
>     nsICanvasRenderingContextWebGL *self;
>     xpc_qsSelfRef selfref;
>     js::AutoValueRooter tvr(cx);
>     if (!xpc_qsUnwrapThis(cx, obj, nsnull, &self, &selfref.ptr, tvr.addr(), nsnull))
>         return JS_FALSE;
> 
>+    // XXX we currently allow passing only 3 args to support the API. Eventually drop that.
>+    // if (argc < 6 || argc == 7 || argc == 8)
>     if (argc < 3)
>         return xpc_qsThrow(cx, NS_ERROR_XPC_NOT_ENOUGH_ARGS);
> 
>     jsval *argv = JS_ARGV(cx, vp);
> 
>-    int32 intargs[8];
>-    JSObject *arg9 = nsnull;
>+    // arguments common to all cases
>+    uint32 argv0; if (!JS_ValueToECMAUint32(cx, argv[0], &argv0)) return JS_FALSE;
>+    int32  argv1; if (!JS_ValueToECMAInt32 (cx, argv[1], &argv1)) return JS_FALSE;

^ Don't munge these all on one line; split them up into three lines each, though see below.

>-    // convert the first two args, they must be ints
>-    for (jsuint i = 0; i < 2; ++i) {
>-        if (!JS_ValueToECMAInt32(cx, argv[i], &intargs[i]))
>-            return JS_FALSE;
>-    }
>+    if (argc > 2 && JSVAL_IS_OBJECT(argv[2])) {
>+        // the old API. Eventually drop that. also, we don't support the optional bool params
>+        // here anymore, so programs passing nondefault values there will be broken.

^ Why not?  If we're going to retain compat with the old API then we need to support the boolean args, otherwise we're going to break content anyway (upside down textures everywhere, at the very least).  Fetching the optional args shouldn't be hard, and we have code to handle them anyway in the implementation.

>-    if (JSVAL_IS_OBJECT(argv[2])) {
>-        // try to make this a nsIDOMElement
>         nsIDOMElement *elt;
>         xpc_qsSelfRef eltRef;
>         rv = xpc_qsUnwrapArg<nsIDOMElement>(cx, argv[2], &elt, &eltRef.ptr, &argv[2]);
>-        if (NS_SUCCEEDED(rv)) {
>-            intargs[3] = 0;
>-            intargs[4] = 0;
>+        if (NS_FAILED(rv)) return JS_FALSE;
> 
>-            // convert args 4 and 5 if present, default to 0
>-            for (jsuint i = 3; i < 5 && i < argc; ++i) {
>-                if (!JS_ValueToECMAInt32(cx, argv[i], &intargs[i]))
>-                    return JS_FALSE;
>-            }
>+        uint32 argv3 = 0, argv4 = 0; // 0 is the default value for these optional params
>+        if (argc > 3) if (!JS_ValueToECMAUint32(cx, argv[3], &argv3)) return JS_FALSE;
>+        if (argc > 4) if (!JS_ValueToECMAUint32(cx, argv[4], &argv4)) return JS_FALSE;

^ Not on one line pls.  This and similar patterns seem common enough, so I'd suggest creating some macros, something like:

#define FETCH_INT32_ARG(val, index) \
  do { \
    if (!JS_ValueToECMAInt32(cx, argv[index], &(val))) \
      return JS_FALSE; \
  } while (0)

#define SAFE_FETCH_INT32_ARG(val, index) \
  do { \
    if (argc > index) \
      if (!JS_ValueToECMAInt32(cx, argv[index], &(val))) \
        return JS_FALSE; \
  } while (0)

And then using

SAFE_FETCH_INT32_ARG(argv3, 3);
FETCH_INT32_ARG(argv3, 3);

etc.


> NS_IMETHODIMP
> WebGLContext::TexSubImage2D_dom(WebGLenum target, WebGLint level,
>                                 WebGLint xoffset, WebGLint yoffset,
>-                                WebGLsizei width, WebGLsizei height,
>-                                nsIDOMElement *elt,
>-                                WebGLboolean flipY, WebGLboolean premultiplyAlpha)
>+                                WebGLenum format, WebGLenum type,
>+                                nsIDOMElement *elt)
> {
>     nsRefPtr<gfxImageSurface> isurf;
> 
>     nsresult rv = DOMElementToImageSurface(elt, getter_AddRefs(isurf),
>-                                           flipY, premultiplyAlpha);
>+                                           PR_FALSE/*flipY*/, PR_FALSE/*premultiplyAlpha*/);
>     if (NS_FAILED(rv))
>         return rv;
> 
>+    PRUint32 byteLength = isurf->Stride() * isurf->Height();

While you're here, can you add a NS_ASSERTION(isurf->Stride() == isurf->Width() * 4, "Bad stride!") ?
Also, this change will need the tests in the test suite updated...
Comment on attachment 450736 [details] [diff] [review]
Fix PixelStorei

This looks fine, but also needs tests.
Attachment #450736 - Flags: review?(vladimir) → review+
OK, applying your comments. About the tests, I have updated them locally, so I've been able to test, it's just that I haven't committed that, because I had a private discussion with Ken where he told me that he was already doing it upstream in the webkit tests (indeed these tests come from webkit).
>>+        // the old API. Eventually drop that. also, we don't support
>>                       // the optional bool params
>+        // here anymore, so programs passing nondefault values there
>>                       //will be broken.
>
> ^ Why not?  If we're going to retain compat with the old API then we need to
> support the boolean args, otherwise we're going to break content anyway 

This comment was outdated, sorry. As you can see, the patch actually is supporting the optional params.
... except that indeed the code in TexImage2D_dom was also outdated, so the optional args were not passed. oops.
ah no, nevermind comment 13... re-oops.
Created attachment 451114 [details] [diff] [review]
TexImage2D API change, updated

Here's the updated patch. I let the macro take care of declaring the argv variable, but I resisted the temptation of removing that argument and just doing argv##i, because that would have made it mysterious where those argv variables come from. Since the macro declares the variable, 'FETCH' didn't sound so appropriate anymore so I went for 'GET'.
Attachment #450746 - Attachment is obsolete: true
Attachment #451114 - Flags: review?(vladimir)
Attachment #450746 - Flags: review?(vladimir)
Comment on attachment 451114 [details] [diff] [review]
TexImage2D API change, updated

Looks good, though no need to pass an arg for the default value to the optional macros -- it will always be 0 by definition, because you can't pass anything else as a 'default' optional value.
Attachment #451114 - Flags: review?(vladimir) → review+
http://hg.mozilla.org/mozilla-central/rev/52d263717b01
http://hg.mozilla.org/mozilla-central/rev/0c750bd14d54
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
Didn't the uiid need to be changed?
You need to log in before you can comment on or make changes to this bug.