Add an API for providing external images. #561

glennw · 2016-11-15T04:27:48Z

This allows applications to provide images without supplying
the image bytes up front. This can be useful if the application
doesn't have the bytes available immediately (e.g. video decoding)
or wants to manage the buffer allocation and reduce memory
copies.

This change is

glennw · 2016-11-15T04:28:18Z

cc @nical @kvark @JerryShih

How does this look as an API?

glennw · 2016-11-15T04:28:27Z

Also CC @jrmuizel

JerryShih · 2016-11-15T05:31:01Z

@glennw

How about the external "gl texture"?
I think we need both "external buffer" and "external texture".

Here is my patch to resolve the external texture using the callback function:
JerryShih/gecko-dev@56ab6b7#diff-8f929d6c300729f5810b7f5ae7d3233fR790

glennw · 2016-11-15T07:08:16Z

@JerryShih Added basic support for using a native texture as an external image. I haven't added support for the release callback of native textures yet (it should work for cpu external images though). If we're happy in general with the API, I think it's fine to get this merged, and I'll do the native texture release code in a follow up PR.

glennw · 2016-11-15T07:16:42Z

The semantics of the get/release calls are a bit confusing probably. Perhaps there should be a separate callback to just get the timestamp instead of calling get() each frame to check if the timestamp has changed? Thoughts?

JerryShih · 2016-11-15T07:35:14Z

@glennw
When should we use the timestamp?
If there is a video playback case, I think we could just call update_image() with the new "external key" for the new buffer and the exist image key which we want to update.
I think the timestamp logic(e.g. skip video frame) could put at c++ side. It makes WR much clear. When we call update_image() or add_image(), WR should just do the texture uploading with no condition.

glennw · 2016-11-15T07:44:44Z

@JerryShih The reason for the timestamp was based on a discussion with @nical.

He suggested that we'd want to be able to just invoke the render thread to re-draw the frame in the case of a video where nothing else in the layout has changed, but the video is advancing (for performance and power reasons).

If we go via the update_image() API, that involves waking up the backend thread, processing the frame and pushing that to the render thread.

Personally, I'm happy with either solution - but hopefully that explains the logic of having the timestamp field, and we can discuss here the preferred option.

nical · 2016-11-15T17:47:08Z

The timestamped video frames logic lives in the gecko/compositor side. I don't think we need to expose it to webrender, as long as the latter invokes the the callback every frame. The gecko implementation of the callback can query the composition timestamp from the Compositor and select the texture accordingly.

As far as gecko is concerned we don't want to use update_image() for video frames because it wakes up the backend thread and rebuilds the frame, which we don't want to happen every frame during video playback unless the geometry actually changed. We only want to swap out the the texture id and uv coordinates and submit the draw calls again.

Edit: to be clear, with video we don't want to call add_image every frame, but we can use it in the first frame to to create the video source, passing it an external id instead of actual image data. Then, every time we render a new frame, the callback resolves that external id (which does not change) into a texture id.

kvark

Got a few questions, but it looks good.

kvark · 2016-11-15T17:11:47Z

webrender/res/prim_shared.glsl

@@ -55,6 +55,7 @@ uniform sampler2D sData16;
 uniform sampler2D sData32;
 uniform sampler2D sData64;
 uniform sampler2D sData128;
+uniform sampler2D sResourceRects;


why does it have to be a separate texture instead of re-using sData16?

In the future, I expect this will be an array managed by the texture cache on a thread that is running async to the backend thread - so I've made it a separate array because of that.

kvark · 2016-11-15T17:17:27Z

webrender/src/prim_store.rs

@@ -16,6 +16,39 @@ use webrender_traits::{AuxiliaryLists, ColorF, ImageKey, ImageRendering};
 use webrender_traits::{FontRenderMode, WebGLContextId};
 use webrender_traits::{ClipRegion, FontKey, ItemRange, ComplexClipRegion, GlyphKey};

+/// Stores two coordinates in texel space. The coordinates
+/// are stored in texel coordinates because the texture atlas
+/// may grow. Storing them as texel coords and normalizing


nit: if the atlas grows, means it's re-allocated and all the contents need to be copied over anyways, in which case we might want to re-arrange the placement of textures inside it, meaning that UV rects change anyways.

Right now, we don't do that - we resize via FBO. But in the future that's possible, yes.

kvark · 2016-11-15T17:26:40Z

webrender/src/prim_store.rs

                        }
                        ImagePrimitiveKind::WebGL(context_id) => {
-                            resource_cache.get_webgl_texture(&context_id)
+                            let cache_item = resource_cache.get_webgl_texture(&context_id);
+                            let resource_rect = self.gpu_resource_rects.get_mut(image_cpu.resource_address);


nit: given that these 3 lines are copy-pasted from above, we might want to return Option<CacheItem> from the match and do them afterwards

kvark · 2016-11-15T17:27:35Z

webrender/src/prim_store.rs

@@ -747,7 +805,7 @@ impl PrimitiveStore {
                    debug_assert!(metadata.gpu_data_count == text.glyph_range.length as i32);
                    debug_assert!(text.glyph_indices.is_empty());
                    let src_glyphs = auxiliary_lists.glyph_instances(&text.glyph_range);
-                    let dest_glyphs = self.gpu_data32.get_slice_mut(metadata.gpu_data_address,
+                    let dest_glyphs = self.gpu_data16.get_slice_mut(metadata.gpu_data_address,


future: in order to catch this kind of errors later, we might consider templating the GpuStoreAddress<T> to only work with GpuStore<T>

kvark · 2016-11-15T17:28:25Z

webrender/src/render_backend.rs

+                        ApiMsg::AddImage(id, width, height, stride, format, data) => {
+                            match data {
+                                ImageData::Raw(ref bytes) => {
+                                    profile_counters.image_templates.inc(bytes.len());


nit: perhaps do if let instead

kvark · 2016-11-15T17:35:33Z

webrender/src/renderer.rs

+                        let texture_id = match image.data {
+                            ExternalImageData::Buffer(..) => {
+                                // For a custom user buffer, allocate a native texture.
+                                let texture_id = self.device


when is this texture deleted, if ever?

kvark · 2016-11-15T17:36:45Z

webrender/src/renderer.rs

@@ -1267,6 +1299,100 @@ impl Renderer {
    fn draw_tile_frame(&mut self,
                       frame: &mut Frame,
                       framebuffer_size: &Size2D<u32>) {
+        // The first thing we do is run through any pending deferred


voting to move this out into a separate method. You'd be able to do return from there if the timestamps match, simplifying the logic a bit

kvark · 2016-11-15T17:42:01Z

webrender/src/renderer.rs

@@ -1394,6 +1522,36 @@ impl Renderer {
    }
 }

+pub enum ExternalImageData {


ExternalImageData::Native doesn't sound right to me. Perhaps, ExternalImageSource? with RawData and NativeTexture variants

kvark · 2016-11-15T17:45:42Z

webrender/src/resource_cache.rs

+                                Occupied(entry) => {
+                                    let image_id = entry.get().texture_cache_id;
+
+                                    if entry.get().epoch != image_template.epoch {


is the semantic of timespamps (above) different from the epoch here?

kvark · 2016-11-15T18:05:25Z

webrender/src/renderer.rs

+                              .as_mut()
+                              .expect("Found external image, but no handler set!");
+
+            for deferred_resolve in &frame.deferred_resolves {


is this guaranteed to run once only? and if not, will it behave properly for multiple invocations (scanning the deferred_resolves list)?

It can run once per scroll, but this is expected behaviour.

bors-servo · 2016-11-16T00:12:42Z

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

JerryShih · 2016-11-16T03:50:46Z

@nical
I think it's fine for "external texture id", but how about the "external buffer" ?
For external "texture id", the flow will be:

call add_image() with the external key at the first time
update the corresponding texture id for the external key
call wr_render() to do the rendering without any layout task

For external "buffer":

call add_image() with the external key at the first time
update the corresponding texture id for the external key
call wr_render(). At this time, Should we upload the buffer into gl texture here? What's the uploading condition for the external buffer? In this PR, @glennw uses a timestamp id to make sure WR only uploads the external buffer once.

I think we still need to have a function to notify WR that here comes a new image. Then, WR could do the texture uploading if the image is an "external buffer".

Could update_image() only update the TextureSource, TextureId in the frame? It will still wake up the backend thread as other rendering api does, but just update the TextureSource or TextureId with for the new external key. Frame structure building should be skip in this case.

glennw · 2016-11-16T05:37:46Z

Rebased over recent changes and addressed most of the review comments.

I think @nical mentioned that a first test of this case would be using native textures? So I have removed the external buffer (and timestamp related code) until we work out a suitable API for them.

Hopefully that means we can land this part of the external image support (once any further API changes / review comments are sorted), which will give us external texture support, and then work on the external buffer support as a follow up API.

It now requests the external texture ID at the start of the frame, and releases after issuing all the draw calls. The calling application doesn't have to free the texture at that point (e.g. it may know it's going to use it again next frame), but it knows that WR will not issue another draw call referencing that texture without calling get() again first.

glennw · 2016-11-16T05:38:07Z

@JerryShih @nical @kvark How does the above sound?

glennw · 2016-11-16T06:26:35Z

I pushed a simple test case / sample of using the external image function to draw an image which is the output of an FBO clear - glennw@481a6b1

nical · 2016-11-16T09:28:14Z

Sounds good to me.
As far as gecko is concerned, external texture ids are enough to get the functionality we need. If we need external buffers in the future (or for the sake of completeness), we can add them later. There may be more efficient ways to upload textures if it is handled by webrender so we may want to revisit that. In the mean time, texture ids are simple to express at the ffi boundary and I think that getting it to work in this common scenario and iterating on the lifecycles and synchronization of external textures is what's most important at the moment.

@JerryShih perhaps I misunderstand what you mean by update_image(). This API currently just sends a message to the backend thread which rebuilds the frame with the updated information. For externally allocated textures all we need to happen is on the renderer thread, so it would make sense to have a separate API which interacts with the renderer directly rather than the backend.
Sorry for the confusion about timestamps in the PR, I thought you were referring to something else.

pcwalton · 2016-11-16T18:57:49Z

r=me, squash if you want

glennw · 2016-11-16T20:31:17Z

@bors-servo r=pcwalton

bors-servo · 2016-11-16T20:31:17Z

📌 Commit 003dbeb has been approved by pcwalton

larsbergstrom · 2016-11-16T21:06:59Z

@bors-servo clean retry

larsbergstrom · 2016-11-16T21:08:05Z

@bors-servo force clean retry

This allows applications to provide images without supplying the image bytes up front. This can be useful if the application doesn't have the bytes available immediately (e.g. video decoding) or wants to manage the buffer allocation and reduce memory copies.

glennw · 2016-11-16T21:09:50Z

@bors-servo r=pcwalton

bors-servo · 2016-11-16T21:09:51Z

📌 Commit afb0f06 has been approved by pcwalton

bors-servo · 2016-11-16T21:09:53Z

⌛ Testing commit afb0f06 with merge bcb5968...

Add an API for providing external images. This allows applications to provide images without supplying the image bytes up front. This can be useful if the application doesn't have the bytes available immediately (e.g. video decoding) or wants to manage the buffer allocation and reduce memory copies.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/webrender/561)

bors-servo · 2016-11-16T22:52:48Z

☀️ 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 15, 2016

View reviewed changes

glennw force-pushed the external-images branch from 373a7a9 to 003dbeb Compare November 16, 2016 05:32

glennw force-pushed the external-images branch from 003dbeb to aa8aafc Compare November 16, 2016 19:00

glennw force-pushed the external-images branch from aa8aafc to afb0f06 Compare November 16, 2016 21:09

bors-servo merged commit afb0f06 into servo:master Nov 16, 2016

glennw deleted the external-images branch December 12, 2016 20:43

glennw mentioned this pull request Dec 13, 2016

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

Merged

Add an API for providing external images. #561

Add an API for providing external images. #561

Conversation

glennw commented Nov 15, 2016 • edited by larsbergstrom Loading

glennw commented Nov 15, 2016

glennw commented Nov 15, 2016

JerryShih commented Nov 15, 2016

glennw commented Nov 15, 2016

glennw commented Nov 15, 2016

JerryShih commented Nov 15, 2016

glennw commented Nov 15, 2016

nical commented Nov 15, 2016 • edited Loading

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

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

bors-servo commented Nov 16, 2016

JerryShih commented Nov 16, 2016

glennw commented Nov 16, 2016

glennw commented Nov 16, 2016

glennw commented Nov 16, 2016

nical commented Nov 16, 2016

pcwalton commented Nov 16, 2016

glennw commented Nov 16, 2016

bors-servo commented Nov 16, 2016

larsbergstrom commented Nov 16, 2016

larsbergstrom commented Nov 16, 2016

glennw commented Nov 16, 2016

bors-servo commented Nov 16, 2016

bors-servo commented Nov 16, 2016

bors-servo commented Nov 16, 2016

glennw commented Nov 15, 2016 •

edited by larsbergstrom

Loading

nical commented Nov 15, 2016 •

edited

Loading