Introduce SourceTexture, an abstraction for location of source textures. #554

glennw · 2016-11-11T02:52:41Z

This allows the backend thread to work with textures that have
been allocation by the texture cache, via webgl or an external
source transparently. The native texture ID is resolved by the
rendering thread as requited.

Specifically:

Add SourceTexture, an abstraction for the location of a source texture.
Add CacheTextureID, an ID for a texture cache managed texture.
- This removes the need to allocate a pool of textures up front.
Remove unused batch.rs file.
Remove the concept of "raw textures" from device.rs
- The native texture ID is stored inside the SourceTexture enum now.
Remove some unused functions in device.rs.

This change is

glennw · 2016-11-11T02:56:10Z

I figured I'd prototype adding the source texture -> resolving idea I had today, and it worked out quite well I think. Thoughts @nical @kvark @JerryShih ?

This removes the use of TextureId from the backend thread, using SourceTexture instead, which is resolved to TextureId in the rendering thread. I haven't implemented the external texture support, but I've left a couple of TODO notes where I would expect it to fit in.

This also doesn't handle the idea of getting UV rects in the renderer thread, but that can be sorted out when we do the actual external image support. Does this look reasonable to you guys?

glennw · 2016-11-11T03:00:44Z

This is basically taking @nical 's external texture branch, and extending it to handle webgl textures, and fixing up the texture cache ID pool at the same time.

glennw · 2016-11-11T06:10:38Z

@nical @JerryShih @kvark I have pushed a WIP follow up commit to this 3fe9214 here.

This commit is a messy, untested, but mostly complete implementation of how I think external images + custom UV rects could work.

The first part of the idea is that primitives no longer store their UV rect directly - there is a GpuDataStore for all UV rectangles, just like there is for many other GPU types. This means that vertex shaders fetch their UV rect from here, and primitives allocate a slot inside this array when they are first added.

Next, the add_image() API gets extended to support adding different types of images (raw bytes, or external, with some arbitrary key).

Now, when we resolve images from the texture cache to get their UV rect, if it finds an external texture, instead of storing a UV rect directly, it creates a DeferredResolve item, noting the external source texture and the index in the UV rects array where the UV rect actually belongs.

Finally, that array of DeferredResolves are passed to the render thread with each frame. The first thing the renderer does is iterate that list, invoking the user callback to get the uv rect and / or texture id. Once that is done, it can patch the resource rects array, just before it uploads to the GPU.

The part I haven't implemented is the actual callbacks (there are two panics where the callbacks would be invoked).

Apart from that, I think the ideal should work. Any thoughts on whether this seems like a reasonable API / implementation from your perspective?

glennw · 2016-11-11T06:13:41Z

The nice thing about this approach is that the render thread doesn't really have to know much more about the backend thread than it already does. The only real new functionality on the render thread is to run through a list of deferred resolve structures, run a callback, and patch an array with the result.

kvark

Looks great! I don't find it rough, just got a few minor points.

kvark · 2016-11-11T15:39:56Z

webrender/src/internal_types.rs

@@ -18,11 +18,46 @@ use std::hash::BuildHasherDefault;
 use std::i32;
 use std::path::PathBuf;
 use std::sync::Arc;
+use std::usize;


nit: could combine with std::i32

kvark · 2016-11-11T15:45:04Z

webrender/src/internal_types.rs

+pub struct CacheTextureId(pub usize);
+
+impl CacheTextureId {
+    pub fn invalid() -> CacheTextureId {


I think we are abusing the invalid() concept a bit too much. Rust way is Option, and we already have SourceTexture::invalid variant. Perhaps, we can get away without CacheTextureId::invalid?

kvark · 2016-11-11T15:45:52Z

webrender/src/internal_types.rs

@@ -337,26 +372,17 @@ pub enum RenderTargetMode {

 #[derive(Debug)]
 pub enum TextureUpdateDetails {


as this enum becomes trivial, maybe we can turn it into a struct?

kvark · 2016-11-11T15:47:24Z

webrender/src/internal_types.rs

+// map from cache texture ID to native texture.
+
+#[derive(Copy, Clone, Debug, Eq, PartialEq, Hash)]
+pub struct CacheTextureId(pub usize);


in the long term, I'd like to have types like this opaque (non-public internals), but this can (and should) be done later on as a separate change

kvark · 2016-11-11T15:49:48Z

webrender/src/renderer.rs

+
+    // A vectors for fast resolves of texture cache IDs to
+    // native texture IDs.
+    cache_texture_id_map: Vec<Option<TextureId>>,


would be great to have a few words in the comment saying why it's an Option<> inside

kvark · 2016-11-11T15:51:39Z

webrender/src/renderer.rs

@@ -373,6 +373,10 @@ pub struct Renderer {
    /// Used to dispatch functions to the main thread's event loop.
    /// Required to allow GLContext sharing in some implementations like WGL.
    main_thread_dispatcher: Arc<Mutex<Option<Box<RenderDispatcher>>>>,
+
+    // A vectors for fast resolves of texture cache IDs to


please use real doc comments ///

kvark · 2016-11-11T15:54:27Z

webrender/src/renderer.rs

+            &SourceTexture::Invalid => TextureId::invalid(),
+            &SourceTexture::WebGL(id) => TextureId::new(id),
+            &SourceTexture::TextureCache(index) => {
+                self.cache_texture_id_map[index.0].unwrap()


probably better to switch to expect()

nical · 2016-11-12T20:15:08Z

Looks good! (both this PR and the draft deferred UV implementation).

glennw · 2016-11-14T00:10:15Z

@kvark Thanks for the review, comments addressed and pushed.

r? @pcwalton

bors-servo · 2016-11-14T08:02:57Z

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

This allows the backend thread to work with textures that have been allocation by the texture cache, via webgl or an external source transparently. The native texture ID is resolved by the rendering thread as requited. Specifically: * Add SourceTexture, an abstraction for the location of a source texture. * Add CacheTextureID, an ID for a texture cache managed texture. * This removes the need to allocate a pool of textures up front. * Remove unused batch.rs file. * Remove the concept of "raw textures" from device.rs * The native texture ID is stored inside the SourceTexture enum now. * Remove some unused functions in device.rs.

pcwalton

r=me with that if you want

pcwalton · 2016-11-14T20:55:59Z

webrender/src/renderer.rs

+                            // the new ID there.
+                            debug_assert!(self.cache_texture_id_map[cache_texture_index].is_none());
+                            self.cache_texture_id_map[cache_texture_index] = Some(texture_id);
+                        }


This could use Vec::grow_set() to make this a one-liner if you wanted, but up to you

It looks like grow_set was deprecated (rust-lang/rust#17029) - it doesn't seem to be available in stable rust.

glennw · 2016-11-14T21:11:59Z

@bors-servo r=pcwalton

bors-servo · 2016-11-14T21:12:01Z

📌 Commit 3345d0b has been approved by pcwalton

bors-servo · 2016-11-14T21:12:06Z

⌛ Testing commit 3345d0b with merge 916f46e...

Introduce SourceTexture, an abstraction for location of source textures. This allows the backend thread to work with textures that have been allocation by the texture cache, via webgl or an external source transparently. The native texture ID is resolved by the rendering thread as requited. Specifically: * Add SourceTexture, an abstraction for the location of a source texture. * Add CacheTextureID, an ID for a texture cache managed texture. * This removes the need to allocate a pool of textures up front. * Remove unused batch.rs file. * Remove the concept of "raw textures" from device.rs * The native texture ID is stored inside the SourceTexture enum now. * Remove some unused functions in device.rs.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/webrender/554)

bors-servo · 2016-11-14T22:43:49Z

☀️ Test successful - status-travis

kvark has make several important contributions to webrender, both as a reviewer and committer. He also has significant Rust experience previously as the author of gfx-rs. Some examples of recent reviews: servo/webrender#546 servo/webrender#523 servo/webrender#561 servo/webrender#554

@kvark

[Proposal] Give @kvark reviewer privileges. kvark has make several important contributions to webrender, both as a reviewer and committer. He also has significant Rust experience previously as the author of gfx-rs. Some examples of recent reviews: servo/webrender#546 servo/webrender#523 servo/webrender#561 servo/webrender#554  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/saltfs/551)

kvark has make several important contributions to webrender, both as a reviewer and committer. He also has significant Rust experience previously as the author of gfx-rs. Some examples of recent reviews: servo/webrender#546 servo/webrender#523 servo/webrender#561 servo/webrender#554

kvark approved these changes Nov 11, 2016

View reviewed changes

nical mentioned this pull request Nov 14, 2016

[WIP] Add support for externally allocated images in the render backend #548

Closed

gw3583 added 2 commits November 15, 2016 05:26

Address review comments.

3345d0b

glennw force-pushed the ext branch from 44b07f6 to 3345d0b Compare November 14, 2016 20:26

pcwalton reviewed Nov 14, 2016

View reviewed changes

bors-servo merged commit 3345d0b into servo:master Nov 14, 2016

This was referenced Nov 14, 2016

Support external image texture/buffer #547

Closed

Off-screen clip mask generation #556

Merged

glennw deleted the ext branch November 14, 2016 22:45

glennw mentioned this pull request Dec 13, 2016

[Proposal] Give @kvark reviewer privileges. servo/saltfs#551

Merged

kvark mentioned this pull request Dec 14, 2016

Support external image buffer for add_image() #524

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce SourceTexture, an abstraction for location of source textures. #554

Introduce SourceTexture, an abstraction for location of source textures. #554

glennw commented Nov 11, 2016 •

edited by larsbergstrom

Loading

glennw commented Nov 11, 2016

glennw commented Nov 11, 2016

glennw commented Nov 11, 2016

glennw commented Nov 11, 2016

kvark left a comment

kvark Nov 11, 2016

kvark Nov 11, 2016

kvark Nov 11, 2016

kvark Nov 11, 2016

kvark Nov 11, 2016

kvark Nov 11, 2016

kvark Nov 11, 2016

nical commented Nov 12, 2016

glennw commented Nov 14, 2016

bors-servo commented Nov 14, 2016

pcwalton left a comment

pcwalton Nov 14, 2016

glennw Nov 14, 2016

glennw commented Nov 14, 2016

bors-servo commented Nov 14, 2016

bors-servo commented Nov 14, 2016

bors-servo commented Nov 14, 2016

		@@ -337,26 +372,17 @@ pub enum RenderTargetMode {

		#[derive(Debug)]
		pub enum TextureUpdateDetails {

Introduce SourceTexture, an abstraction for location of source textures. #554

Introduce SourceTexture, an abstraction for location of source textures. #554

Conversation

glennw commented Nov 11, 2016 • edited by larsbergstrom Loading

glennw commented Nov 11, 2016

glennw commented Nov 11, 2016

glennw commented Nov 11, 2016

glennw commented Nov 11, 2016

kvark 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

nical commented Nov 12, 2016

glennw commented Nov 14, 2016

bors-servo commented Nov 14, 2016

pcwalton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glennw commented Nov 14, 2016

bors-servo commented Nov 14, 2016

bors-servo commented Nov 14, 2016

bors-servo commented Nov 14, 2016

glennw commented Nov 11, 2016 •

edited by larsbergstrom

Loading