Implement DOM to texture #18592

MortimerGoro · 2017-09-21T14:33:47Z

This is a prototype of the WebGL DOMToTexture feature. The API should be fine as a privileged extension for now due to security concerns. The working group has been investigating the viability of unprivileged usage but the spec is not ready yet.

Demo video: https://www.youtube.com/watch?v=hpZqEM5hPao

./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-09-21T14:33:51Z

Heads up! This PR modifies the following files:

@asajeffrey: components/constellation/constellation.rs
@KiChjang: components/script/dom/webglrenderingcontext.rs, components/script/dom/webidls/WebGLRenderingContext.webidl, components/script/dom/webgltexture.rs, components/script_traits/script_msg.rs, components/script_traits/script_msg.rs
@paulrouget: components/constellation/constellation.rs, components/servo/lib.rs
@fitzgen: components/script/dom/webglrenderingcontext.rs, components/script/dom/webidls/WebGLRenderingContext.webidl, components/script/dom/webgltexture.rs, components/script_traits/script_msg.rs, components/script_traits/script_msg.rs
@emilio: components/canvas/webgl_mode/inprocess.rs, components/script/dom/webglrenderingcontext.rs, components/script/dom/webgltexture.rs, components/canvas/webgl_thread.rs

highfive · 2017-09-21T14:33:53Z

Warning

These commits modify script code, but no tests are modified. Please consider adding a test!

MortimerGoro · 2017-09-21T14:35:33Z

Related PR in WR: servo/webrender#1687

bors-servo · 2017-09-24T22:21:33Z

☔ The latest upstream changes (presumably #18598) made this pull request unmergeable. Please resolve the merge conflicts.

jdm · 2017-09-27T16:06:11Z

components/canvas/webgl_thread.rs

 /// WebGL Commands Implementation
 pub struct WebGLImpl;

 impl WebGLImpl {
    pub fn apply<Native: NativeGLContextMethods>(ctx: &GLContext<Native>, command: WebGLCommand) {
+        //println!("WebGL::{:?}", command);


nit: remove this.

jdm · 2017-09-27T16:06:11Z

components/canvas/webgl_thread.rs

-            WebGLCommand::BindTexture(target, id) =>
-                ctx.gl().bind_texture(target, id.map_or(0, WebGLTextureId::get)),
+            WebGLCommand::BindTexture(target, id) => {
+                ctx.gl().bind_texture(target, id.map_or(0, WebGLTextureId::get)) },


nit: revert these changes.

jdm · 2017-09-27T16:06:11Z

components/canvas_traits/webgl.rs

 }

+


nit: revert this change.

jdm · 2017-09-27T16:06:11Z

components/canvas_traits/webgl.rs

@@ -379,6 +391,18 @@ pub trait WebVRRenderHandler: Send {
    fn handle(&mut self, command: WebVRCommand, texture: Option<(u32, Size2D<i32>)>);
 }

+


nit: remove this extra newline.

jdm · 2017-09-27T16:06:11Z

components/constellation/constellation.rs

@@ -1234,6 +1234,12 @@ impl<Message, LTF, STF> Constellation<Message, LTF, STF>
                    warn!("Sending reply to get browsing context failed ({:?}).", e);
                }
            }
+            FromScriptMsg::GetDocumentId(_pipeline_id, sender) => {


Why is the pipeline id included in the message if it's never used?

jdm · 2017-09-27T16:06:11Z

components/script/dom/webglrenderingcontext.rs

+        let (sender, receiver) = ipc::channel().unwrap();
+        let global = self.global();
+        let script_to_constellation_chan = global.script_to_constellation_chan();
+        script_to_constellation_chan.send(ScriptMsg::GetDocumentId(pipeline_id, sender)).unwrap();


Since the document id is constant after the constellation is created, we could send that information as part of creating a script thread. This would avoid the need for sync IPC.

jdm · 2017-09-27T16:06:11Z

components/script/dom/webglrenderingcontext.rs

+        };
+
+        // Get PipelineId from the source
+        let pipeline_id = match source.pipeline_id() {


let pipeline_id = source.pipeline_id().or_or(Error::InvalidState)?;

jdm · 2017-09-27T16:06:11Z

components/script/dom/webglrenderingcontext.rs

+            }
+        };
+
+        // mark that the texture is attached to DOM.


I'm not sure that this comment adds anything to the code.

MortimerGoro · 2017-09-28T14:46:23Z

Done!

jdm

Remaining comments are all minor.

jdm · 2017-09-28T15:58:09Z

components/script_traits/script_msg.rs

@@ -27,7 +27,7 @@ use servo_url::ServoUrl;
 use style_traits::CSSPixel;
 use style_traits::cursor::Cursor;
 use style_traits::viewport::ViewportConstraints;
-use webrender_api::ClipId;
+use webrender_api::{ClipId, DocumentId};


We can remove this now.

jdm · 2017-09-28T15:58:09Z

components/script/dom/webgltexture.rs

@@ -45,6 +46,8 @@ pub struct WebGLTexture {
    mag_filter: Cell<Option<u32>>,
    #[ignore_heap_size_of = "Defined in ipc-channel"]
    renderer: WebGLMsgSender,
+    // True if this texture is used for the DOMToTexture feature.


jdm · 2017-09-28T15:58:09Z

components/script/dom/webglrenderingcontext.rs

+        };
+
+        let pipeline_id = source.pipeline_id().ok_or(Error::InvalidState)?;
+        let document_id  = self.global().as_window().webrender_document();


We should return an error if this is invoked from a context which does not have a Window global.

jdm · 2017-09-28T15:58:09Z

components/script/dom/webidls/WebGLRenderingContext.webidl

@@ -654,6 +654,9 @@ interface WebGLRenderingContextBase
    [Throws]
    void texImage2D(GLenum target, GLint level, GLenum internalformat,
                    GLenum format, GLenum type, TexImageSource? source); // May throw DOMException
+    [Throws, Pref="dom.webgl.dom_to_texture.enabled"]


This does not actually throw any exceptions, so we can remove the annotation.

It still throws the let pipeline_id = source.pipeline_id().or_or(Error::InvalidState)?;exception

jdm · 2017-09-28T15:58:09Z

components/canvas_traits/webgl.rs

@@ -86,6 +88,11 @@ impl WebGLMsgSender {
        }
    }

+    // Returns the WebGLContextId associated to this sender
+    pub fn ctx_id(&self) -> WebGLContextId {


Let's call this context_id.

jdm · 2017-09-28T15:58:09Z

components/canvas_traits/webgl.rs

@@ -86,6 +88,11 @@ impl WebGLMsgSender {
        }
    }

+    // Returns the WebGLContextId associated to this sender


bors-servo · 2017-10-13T01:05:16Z

☔ The latest upstream changes (presumably #18840) made this pull request unmergeable. Please resolve the merge conflicts.

MortimerGoro · 2017-10-16T13:52:40Z

Done. Sorry for the delay, I was on a trip ;)

jdm · 2017-10-16T14:34:58Z

@bors-servo: r+

bors-servo · 2017-10-16T14:34:59Z

📌 Commit 9093c42 has been approved by jdm

bors-servo · 2017-10-16T17:53:56Z

🔒 Merge conflict

bors-servo · 2017-10-16T17:55:22Z

☔ The latest upstream changes (presumably #18900) made this pull request unmergeable. Please resolve the merge conflicts.


        Implement DOM to texture

MortimerGoro · 2017-10-16T19:38:49Z

Merge conflict fixed

jdm · 2017-10-16T19:53:14Z

@bors-servo: r+

bors-servo · 2017-10-16T19:53:15Z

📌 Commit 8ae0739 has been approved by jdm

bors-servo · 2017-10-16T20:36:44Z

⌛ Testing commit 8ae0739 with merge 3209d22...


        Auto merge of #18592 - MortimerGoro:dom_texture, r=jdm

Implement DOM to texture  This is a prototype of the WebGL DOMToTexture feature. The API should be fine as a privileged extension for now due to security concerns. The working group has been investigating the viability of unprivileged usage but the spec is not ready yet. Demo video: https://www.youtube.com/watch?v=hpZqEM5hPao ---  - [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/18592)

bors-servo · 2017-10-16T22:07:47Z

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css1, mac-rel-css2, mac-rel-wpt1, mac-rel-wpt2, mac-rel-wpt3, mac-rel-wpt4, windows-msvc-dev
Approved by: jdm
Pushing 3209d22 to master...

highfive assigned jdm Sep 21, 2017

highfive added the S-awaiting-review label Sep 21, 2017

highfive added the S-needs-rebase label Sep 24, 2017

jdm requested changes Sep 27, 2017

View changes

jdm added S-needs-code-changes and removed S-awaiting-review labels Sep 27, 2017

MortimerGoro force-pushed the MortimerGoro:dom_texture branch from 5352ac8 to f116561 Sep 28, 2017

highfive added S-awaiting-review and removed S-needs-code-changes labels Sep 28, 2017

jdm requested changes Sep 28, 2017

View changes

jdm added S-needs-code-changes and removed S-awaiting-review S-needs-rebase labels Sep 28, 2017

MortimerGoro force-pushed the MortimerGoro:dom_texture branch from f116561 to 26236d8 Sep 29, 2017

highfive added S-awaiting-review and removed S-needs-code-changes labels Sep 29, 2017

jdm added S-needs-code-changes and removed S-awaiting-review labels Oct 2, 2017

highfive added the S-needs-rebase label Oct 13, 2017

MortimerGoro force-pushed the MortimerGoro:dom_texture branch from 26236d8 to 9093c42 Oct 16, 2017

highfive added S-awaiting-review and removed S-needs-code-changes labels Oct 16, 2017

highfive added S-awaiting-merge and removed S-awaiting-review S-needs-rebase labels Oct 16, 2017

jdm approved these changes Oct 16, 2017

View changes

highfive added S-needs-rebase and removed S-awaiting-merge labels Oct 16, 2017

Implement DOM to texture

Loading status checks…

8ae0739

MortimerGoro force-pushed the MortimerGoro:dom_texture branch from 9093c42 to 8ae0739 Oct 16, 2017

highfive added the S-awaiting-review label Oct 16, 2017

highfive added S-awaiting-merge and removed S-awaiting-review S-needs-rebase labels Oct 16, 2017

bors-servo merged commit 8ae0739 into servo:master Oct 16, 2017
3 checks passed

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

continuous-integration/travis-ci/pr The Travis CI build passed
Details

homu Test successful
Details

highfive removed the S-awaiting-merge label Oct 16, 2017

mbrubeck mentioned this pull request Apr 4, 2018

Image created with texImageDom (DOM to texture) is mirrored vertically #20540

Closed

servo / servo

Implement DOM to texture #18592

Implement DOM to texture #18592

MortimerGoro commented Sep 21, 2017 •

edited by SimonSapin

highfive commented Sep 21, 2017

highfive commented Sep 21, 2017

MortimerGoro commented Sep 21, 2017

bors-servo commented Sep 24, 2017

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

MortimerGoro commented Sep 28, 2017

jdm left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

bors-servo commented Oct 13, 2017

MortimerGoro commented Oct 16, 2017

jdm commented Oct 16, 2017

bors-servo commented Oct 16, 2017

bors-servo commented Oct 16, 2017

bors-servo commented Oct 16, 2017

MortimerGoro commented Oct 16, 2017

jdm commented Oct 16, 2017

bors-servo commented Oct 16, 2017

bors-servo commented Oct 16, 2017

bors-servo commented Oct 16, 2017

		@@ -379,6 +391,18 @@ pub trait WebVRRenderHandler: Send {
		fn handle(&mut self, command: WebVRCommand, texture: Option<(u32, Size2D<i32>)>);
		}

servo / servo

Join GitHub today

Implement DOM to texture #18592

Implement DOM to texture #18592

Conversation

MortimerGoro commented Sep 21, 2017 • edited by SimonSapin

highfive commented Sep 21, 2017

highfive commented Sep 21, 2017

MortimerGoro commented Sep 21, 2017

bors-servo commented Sep 24, 2017

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

MortimerGoro commented Sep 28, 2017

jdm left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

bors-servo commented Oct 13, 2017

MortimerGoro commented Oct 16, 2017

jdm commented Oct 16, 2017

bors-servo commented Oct 16, 2017

bors-servo commented Oct 16, 2017

bors-servo commented Oct 16, 2017

MortimerGoro commented Oct 16, 2017

jdm commented Oct 16, 2017

bors-servo commented Oct 16, 2017

bors-servo commented Oct 16, 2017

bors-servo commented Oct 16, 2017

MortimerGoro commented Sep 21, 2017 •

edited by SimonSapin