webgl: premultiplication, y flipping, and format conversion #15122

anholt · 2017-01-20T00:07:39Z

This series implements a bunch of the texture unpack path features for WebGL. There are known issues with big-endian systems noted in the comments, but I don't know of a clean way to cast from Vec<u16> to Vec<u8> (which, if we had one, would make this code much cleaner anyway).

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix #__ (github issue number if applicable).

There are tests for these changes OR
These changes do not require tests because _____

This change is

highfive · 2017-01-20T00:07:43Z

Heads up! This PR modifies the following files:

@KiChjang: components/script/dom/webglrenderingcontext.rs
@fitzgen: components/script/dom/webglrenderingcontext.rs
@emilio: components/script/dom/webglrenderingcontext.rs

emilio

Looks great! Only minor comments, thanks for doing this!

emilio · 2017-01-20T00:56:56Z

components/script/dom/webglrenderingcontext.rs

+    //      implementation. If a packed pixel format is specified
+    //      which would imply loss of bits of precision from the image
+    //      data, this loss of precision must occur."
+    fn rgba8_image_to_tex_image_data(&self,


nit: Consider making this a doc comment.

emilio · 2017-01-20T00:57:19Z

components/script/dom/webglrenderingcontext.rs

+    fn rgba8_image_to_tex_image_data(&self,
+                                     format: TexFormat,
+                                     data_type: TexDataType,
+                                     pixels: Vec<u8> ) -> Vec<u8> {


nit: Remove the extra space after Vec<u8>.

emilio · 2017-01-20T00:58:17Z

components/script/dom/webglrenderingcontext.rs

+            // into an enum yet so there's a default case here.  All
+            // WebGL types are 4 bytes per pixel or smaller, so just
+            // return the incoming RGBA8 pixels in this path.
+            _ => pixels


Maybe asserting (at least in debug builds), or marking as unreachable! would help catch errors?

OK, looks like unreachable!() should always panic. I thought I'd seen something in my other debugging that suggested that it didn't panic.

emilio · 2017-01-20T00:59:43Z

components/script/dom/webglrenderingcontext.rs

+                       internal_format: TexFormat,
+                       data_type: TexDataType,
+                       width: usize,
+                       height: usize) -> Vec<u8> {


Since this only creates a vector of the same capacity as pixels, seems like we could flip in place? the logic would be a bit more complicated though, maybe mentioning it with a comment?

We're passed in a non-mut Vec at the top of the tex_image()s since we shouldn't be rewriting the JS's array. We could potentially see that we're going to be chaining more than one of these fixups and use a single mut Vec that we operate on, but that sounded messy.

emilio · 2017-01-20T00:59:58Z

components/script/dom/webglrenderingcontext.rs

+                          format: TexFormat,
+                          data_type: TexDataType,
+                          pixels: Vec<u8>) -> Vec<u8> {
+        if !(self.texture_unpacking_settings.get().contains(PREMULTIPLY_ALPHA)) {


nit: Unnecessary parentheses.

emilio · 2017-01-20T01:01:49Z

components/script/dom/webglrenderingcontext.rs

+    fn premultiply_pixels(&self,
+                          format: TexFormat,
+                          data_type: TexDataType,
+                          pixels: Vec<u8>) -> Vec<u8> {


Just as above, this only returns a vector of the same size as pixels, right?

If that's the case, probably we should also premultiply in-place, either using &mut [u8], or mut pixels: Vec<u8>.

Mind at least mentioning it here (and filing a followup maybe?)

emilio · 2017-01-20T01:02:29Z

...rmance-1.0.3/conformance/textures/tex-image-and-sub-image-2d-with-array-buffer-view.html.ini

-  [WebGL test #1095: at (0, 4) expected: 0,0,0,255 was 0,0,255,255]
-    expected: FAIL
-
-  [WebGL test #1097: at (0, 8) expected: 0,0,0,255 w


Wow, poor test, I was wondering where the diffstat came from, awesome! :)

emilio · 2017-01-20T01:04:36Z

components/script/dom/webglrenderingcontext.rs

@@ -112,6 +112,11 @@ fn has_invalid_blend_constants(arg1: u32, arg2: u32) -> bool {
        (_, _) => false
    }
 }
+
+fn multiply_u8_pixel(a: u8, b: u8) -> u8 {


nit: We have code for premultiplication in components/canvas_traits/lib.rs. They contain code similar to this, you may consider putting it there (or even all the premultiplication subroutines? Though that's not great because they aren't used anywhere else).

Anyway, your choice :)

I had noticed that, but I didn't imagine that that code would want to grow some sort of format enum for handling different GL formats.

emilio · 2017-01-20T01:06:51Z

components/script/dom/webglrenderingcontext.rs

-                // is coherent with the image.
-                //
-                // RGB8 should be easy to support too
+                // TODO(anholt): What other incoming formats do we


I think for now Servo stores all its images internally as RGBA8, so we don't need to worry about it I think.

emilio · 2017-01-20T01:07:46Z

components/script/dom/webglrenderingcontext.rs

+                premul
+            }
+
+            _ => pixels


Is this also unreachable? If not, consider commenting which cases do we expect? Otherwise, mark as such?

emilio · 2017-01-20T01:10:18Z

Regarding casting Vecs, you probably can do something using slice::from_raw_parts{_mut}, but I think we should only introduce unsafe code if totally necessary. Probably the conversion is not going to be more costly than the IPC message, but if we want to optimize it we could use that I think.

Ms2ger · 2017-01-20T12:38:50Z

Perhaps https://docs.rs/byteorder/1.0.0/byteorder/ would help.

emilio

r=me, thanks Eric :)

emilio · 2017-01-21T20:29:50Z

@bors-servo r+

bors-servo · 2017-01-21T20:29:51Z

📌 Commit 41a4f63 has been approved by emilio

bors-servo · 2017-01-21T20:29:54Z

⌛ Testing commit 41a4f63 with merge 82e854a...

webgl: premultiplication, y flipping, and format conversion  This series implements a bunch of the texture unpack path features for WebGL. There are known issues with big-endian systems noted in the comments, but I don't know of a clean way to cast from `Vec<u16>` to `Vec<u8>` (which, if we had one, would make this code much cleaner anyway). ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [ ] These changes fix #__ (github issue number if applicable).  - [x] There are tests for these changes OR - [ ] These changes do not require tests because _____   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/15122)

bors-servo · 2017-01-21T21:14:03Z

💔 Test failed - mac-rel-wpt2

anholt · 2017-01-23T17:36:05Z

OK, I think I know the cause of that Mac failure: not having unpack alignment support.

emilio · 2017-01-28T20:06:34Z

We could mark those as failing on mac if you want, I think this is worth landing

This fixes a couple of tests doing RGBA/ubyte image uploads with flipping. Other tests with flipping get their expectations changed, because some other feature is missing (premultiplication or ImageData/canvas format conversion)

The code was returning RGBA8 data from the non-raw sources (HTML canvas elements, JS ImageData, etc.), but we then validated and passed that rgba8 data as if it was whatever format/datatype was specified in TexImage2D/TexSubImage2D, so the pixels would come out as garbage. It would seem like we could just rewrite the passed in format/datatype for the TexImage call to be RGBA/UNSIGNED_BYTE, but that would leave incorrect levels of precision if the internalformat didn't match the format/datatype (and older desktop implementations often ignore the internalformat in choosing their internal format, anyway).

Now the affected testcase only fails due to unpack alignment.

anholt · 2017-01-28T23:53:20Z

I'm actually testing the unpack fix now.

We were setting it to whatever value from {1,2,4,8} the user requested and otherwise ignoring it. There were two problems there: 1) Validation ignored it, so GL could read outside of the user's array in TexImage() or TexSubImage() if the aligment was greater than cpp. 2) TexImage()/TexSubImage() from image/canvas sources wasn't packing its data according to the unpack alignment. To fix this, start tracking the user-requested alignment in the DOM side of the context. Set the GL's alignment to 1 for image/canvas sources or the user's value for array sources, and pass the user's alignment in to validation so that it can figure out the correct size of image that the GL will ready.

emilio

As a uber-nit, s/ready.$/read./ in the last commit message, but I don't really want to block landing this on it.

emilio · 2017-02-03T14:09:35Z

components/script/dom/webglrenderingcontext.rs

-        let expected_byte_length = width * height * element_size * components / components_per_element;
-        return Ok(expected_byte_length);
+        if height == 0 {
+            return Ok(0);


nit: Since you early-return, there's no need for the else branch, but not a big deal.

emilio · 2017-02-03T14:20:16Z

@bors-servo r+

Thanks!

bors-servo · 2017-02-03T14:20:16Z

📌 Commit fcef92f has been approved by emilio

bors-servo · 2017-02-03T14:59:14Z

⌛ Testing commit fcef92f with merge 536c0d7...

webgl: premultiplication, y flipping, and format conversion  This series implements a bunch of the texture unpack path features for WebGL. There are known issues with big-endian systems noted in the comments, but I don't know of a clean way to cast from `Vec<u16>` to `Vec<u8>` (which, if we had one, would make this code much cleaner anyway). ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [ ] These changes fix #__ (github issue number if applicable).  - [x] There are tests for these changes OR - [ ] These changes do not require tests because _____   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/15122)

bors-servo · 2017-02-03T15:53:12Z

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css, mac-rel-wpt1, mac-rel-wpt2, windows-gnu-dev, windows-msvc-dev
Approved by: emilio
Pushing 536c0d7 to master...

highfive assigned emilio Jan 20, 2017

highfive added the S-awaiting-review There is new code that needs to be reviewed. label Jan 20, 2017

emilio approved these changes Jan 20, 2017

View reviewed changes

anholt force-pushed the webgl-premultiply branch from ec7880a to 41a4f63 Compare January 21, 2017 17:59

emilio approved these changes Jan 21, 2017

View reviewed changes

highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-awaiting-review There is new code that needs to be reviewed. labels Jan 21, 2017

highfive added S-tests-failed The changes caused existing tests to fail. and removed S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. labels Jan 21, 2017

anholt added 3 commits January 28, 2017 14:21

webgl: Add support for premultiplying alpha.

dfc4de0

Now the affected testcase only fails due to unpack alignment.

anholt force-pushed the webgl-premultiply branch from 41a4f63 to fcef92f Compare January 29, 2017 03:33

highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-tests-failed The changes caused existing tests to fail. labels Jan 29, 2017

emilio reviewed Feb 3, 2017

View reviewed changes

highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-awaiting-review There is new code that needs to be reviewed. labels Feb 3, 2017

bors-servo merged commit fcef92f into servo:master Feb 3, 2017

highfive removed the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Feb 3, 2017

bors-servo mentioned this pull request Feb 3, 2017

Rewrite WebGLRenderingContext to use typed array APIs #15352

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

webgl: premultiplication, y flipping, and format conversion #15122

webgl: premultiplication, y flipping, and format conversion #15122

anholt commented Jan 20, 2017 •

edited by larsbergstrom

highfive commented Jan 20, 2017

emilio left a comment

emilio Jan 20, 2017

emilio Jan 20, 2017

emilio Jan 20, 2017

anholt Jan 20, 2017

emilio Jan 20, 2017

anholt Jan 20, 2017

emilio Jan 20, 2017

emilio Jan 20, 2017

emilio Jan 20, 2017

emilio Jan 20, 2017

anholt Jan 20, 2017

emilio Jan 20, 2017

emilio Jan 20, 2017

emilio commented Jan 20, 2017

Ms2ger commented Jan 20, 2017

emilio left a comment

emilio commented Jan 21, 2017

bors-servo commented Jan 21, 2017

bors-servo commented Jan 21, 2017

bors-servo commented Jan 21, 2017

anholt commented Jan 23, 2017

emilio commented Jan 28, 2017

anholt commented Jan 28, 2017

emilio left a comment

emilio Feb 3, 2017

emilio commented Feb 3, 2017

bors-servo commented Feb 3, 2017

bors-servo commented Feb 3, 2017

bors-servo commented Feb 3, 2017

webgl: premultiplication, y flipping, and format conversion #15122

webgl: premultiplication, y flipping, and format conversion #15122

Conversation

anholt commented Jan 20, 2017 • edited by larsbergstrom

highfive commented Jan 20, 2017

emilio left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emilio commented Jan 20, 2017

Ms2ger commented Jan 20, 2017

emilio left a comment

Choose a reason for hiding this comment

emilio commented Jan 21, 2017

bors-servo commented Jan 21, 2017

bors-servo commented Jan 21, 2017

bors-servo commented Jan 21, 2017

anholt commented Jan 23, 2017

emilio commented Jan 28, 2017

anholt commented Jan 28, 2017

emilio left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emilio commented Feb 3, 2017

bors-servo commented Feb 3, 2017

bors-servo commented Feb 3, 2017

bors-servo commented Feb 3, 2017

anholt commented Jan 20, 2017 •

edited by larsbergstrom