Handle external image life time in api.delete_image() call #636

JerryShih · 2016-12-14T13:52:01Z

Currently, the delete_image() call just removes the image_key in the hash_map. If the key is associate to an external image, we should release that image using the ExternalImageHandler callbacks.

Since the ExternalImageHandler callbacks are only used in renderer, a pending releasing array is used. Then, pass this array to the renderer for ExternalImageHandler::release() call.

@nical @glennw

This change is

emilio · 2016-12-14T14:02:33Z

webrender/src/renderer.rs

@@ -1451,7 +1472,17 @@ pub struct ExternalImage {
 /// Interface that an application can implement
 /// to support providing external image buffers.
 pub trait ExternalImageHandler {
+    // Get the associate external image from the id. It also implies that the
+    // image is held by WR. The WR client will keep the life time for this
+    // image until the release() call.


nit: Please make these doc coments (/// instead of //)

kvark · 2016-12-14T18:41:01Z

I'm not convinced this handling needs to be integrated into WR. Can't we move this responsibility to the user by doing the following scheme:

the user sets the root display list and pipeline, receiving a frame ID that starts going to the render backend for processing. Whatever external resources are involved, the user is able to associate with this frame ID outside of WR being aware.
the backend finishes the work, sends the frame to the Renderer
the Renderer draws the frame, issues the user callback and passes the frame ID in there.
the user can safely assume the external resources associated with this ID are done, and frees them

In this scheme, the only APIs that WR needs to provide are:

frame ID
user callback

@glennw @JerryShih does it make sense?

glennw

Looks good, just a few minor issues.

glennw · 2016-12-15T00:03:00Z

webrender/src/renderer.rs

                }
                ResultMsg::NewFrame(frame, profile_counters) => {
+                    // When a new frame is ready, we could start to update all pending external
+                    // image request here.
+                    if !self.pending_external_image_updates.is_empty() {


I think it's probably more "correct" to move this loop to the end of the render() call.

@glennw
We could release the unused external image asap here. These external images will not be used because we set a new frame. If we move the deletion to the end of render() call, we just extend the life-time of these images.
What do you think for this reason?

I think what could happen is this sequence:

dl_builder.add_image(ext_image); api.add_dl(dl_builder). api.delete_image(blah)

In this case, it seems possible to me that you could have an image in the current frame's display list, which is also in the pending release list? That's why it seems safer to me to do it at the end of render(). The update() and render() are called directly after each other, so I don't think it will have any major impact on extra lifetimes. Does that seem possible to you? (I might be over-thinking it).

api.add_image(A)
dl.push_image() with A
set_root_dl(frame 1)
update()
render() <= the frame 1 is not ready, draw frame 0 instead
api.delete_image(A)
api.add_image(B)
dl.push_image with B
update()
render() <= the frame 1 is ready now, draw with A

So, we the image A will still be used until frame 2 ready. That's why I put the release() call when we receives the new frame.

dl_builder.add_image(ext_image);
api.add_dl(dl_builder).
api.delete_image(blah)

If we have this usage, it will have a panic in block_until_all_resources_added(). So, that problem is not related to this patch.

glennw · 2016-12-15T00:03:38Z

webrender/src/renderer.rs

+                        let handler = self.external_image_handler
+                                          .as_mut()
+                                          .expect("Found external image updates, but no handler set!");
+                        let mut pending_external_image_updates = mem::replace(&mut self.pending_external_image_updates, Vec::new());


This line shouldn't be necessary - the loop could just be self.pending_external_image_updates.drain(..) I think.

glennw · 2016-12-15T00:04:31Z

webrender/src/resource_cache.rs

+        let value = self.image_templates.remove(&image_key);
+
+        // If the key is associate to an external image, pass the external id to render for cleanup.
+        if let Some(image) = value {


Let's print a error if image is not Some(..) here as well, since that indicates a bug (such as double deletion of an image key).

JerryShih · 2016-12-15T05:29:45Z

@kvark

the user sets the root display list and pipeline, receiving a frame ID that starts going to the render backend for processing. Whatever external resources are involved, the user is able to associate with this frame ID outside of WR being aware.
the backend finishes the work, sends the frame to the Renderer
the Renderer draws the frame, issues the user callback and passes the frame ID in there.
the user can safely assume the external resources associated with this ID are done, and frees them

If we call Render::update() and Render::render() multiple times without update the display_list, when should we release the resources? The user will receive several callback with the same frame id.

glennw · 2016-12-15T06:11:06Z

@kvark I think it's probably fine to have this code in WR. Even though it's probably possible for the user to handle it externally, it seems like we could make it a safer interface by managing this internally. Thoughts?

JerryShih · 2016-12-15T10:05:42Z

@glennw
I address the review comments in patch 3-6.

JerryShih · 2016-12-15T16:01:21Z

@kvark , could you please review my updates? Glenn is on PTO.

kvark · 2016-12-15T16:33:23Z

@JerryShih

If we call Render::update() and Render::render() multiple times without update the display_list, when should we release the resources? The user will receive several callback with the same frame id.

Right, but this is easily workable. You could free the resources associated with frame N when you get a callback of frame N+1.

could you please review my updates? Glenn is on PTO.

Will make sure to review today.

@glennw

I think it's probably fine to have this code in WR. Even though it's probably possible for the user to handle it externally, it seems like we could make it a safer interface by managing this internally.

It's not obvious to me. The external image stuff is not allocated/owned by WR, so we should not be responsible for freeing it either. The less API surface is the better for me. How do you see this being a part of WR to be potentially safer?

kvark · 2016-12-15T16:50:57Z

webrender/src/internal_types.rs

@@ -344,6 +344,22 @@ pub enum TextureUpdateOp {
    Grow(u32, u32, ImageFormat, TextureFilter, RenderTargetMode),
 }

+pub struct ExternalImageUpdateList {


Are you planning to extend this struct? Otherwise, it could be turned into an alias (given that it only has one field and it's public, and no actual logic in the methods):

pub type ExternalImageUpdateList = Vec<ExternalImageId>;

kvark · 2016-12-15T18:13:22Z

webrender/src/renderer.rs

@@ -1409,7 +1423,7 @@ impl Renderer {
                                      &projection);
        }

-        self.release_external_textures();
+        self.unlock_external_images();


I think you missed the corresponding lock() call somewhere

kvark · 2016-12-15T18:53:35Z

Please squash it as well.

JerryShih · 2016-12-16T02:55:16Z

@kvark

Right, but this is easily workable. You could free the resources associated with frame N when you get a callback of frame N+1.

The removing condition will become complicated. You should remember the frame id for each external image. Then, you also need to have a flag to check the external image is be called with api.delete_image(). We should not delete an external image that is still exist in resource cache.
Is it reasonable?

I think you missed the corresponding lock() call somewhere
Oh, I should check all the flows for get(), release(), lock() and unlock().

I will update a squashed version later.

JerryShih · 2016-12-16T07:26:59Z

webrender/src/renderer.rs

@@ -1263,7 +1265,7 @@ impl Renderer {
                let props = &deferred_resolve.image_properties;
                let external_id = props.external_id
                                       .expect("BUG: Deferred resolves must be external images!");
-                let image = handler.get(external_id);
+                let image = handler.lock(external_id);


turn to use lock() here.

JerryShih · 2016-12-16T07:30:51Z

webrender/src/renderer.rs

+                    self.pending_texture_updates.push(texture_update_list);
+
+                    // When a new frame is ready, we could start to update all pending external image requests here.
+                    self.release_external_images(external_image_update_list);


Only release the external images used in previous frames when a new frame is ready.

JerryShih · 2016-12-16T07:33:10Z

@kvark , the updated patch is ready.

kvark · 2016-12-16T18:15:35Z

@JerryShih

The removing condition will become complicated. You should remember the frame id for each external image.

Yes, that's what I meant originally:

Whatever external resources are involved, the user is able to associate with this frame ID outside of WR being aware.

@JerryShih

Then, you also need to have a flag to check the external image is be called with api.delete_image(). We should not delete an external image that is still exist in resource cache.

I don't see which safety guarantees WR can impose on the external resources anyway, so I don't see a reason to complicate WR with an attempt to share any responsibility. The user should be fully responsible for the lifetime of those, and all information it needs to do so is just that frame ID. So, if the Renderer is done with frame N+1 (or later), the caller can free the resources associated with N. WR's delete_image() for the external resources would just clean up internal structures and not do anything dangerous by itself.

I realize that my suggestion is a little hand-wavy, but I believe we should consider this as a way of minimal API to support the externally owned objects. Looking forward to hear what @glennw thinks about the possible safety extras we can get from a fatter API.

glennw · 2016-12-22T02:57:41Z

@kvark I think you're quite right - this could be handled on the client side with a smaller API.

I don't think having this API necessarily makes anything safer (that was bad wording on my part). But I do think if we can handle these details internally, it makes it easier for other clients to use the API correctly and safely (i.e. having the code to manage this in one place in WR is probably better than Servo, Gecko and other API clients having to correctly implement the semantics above).

Thoughts?

nical · 2016-12-22T07:53:10Z

On the gecko side we're about to make some substantial changes how the different WebRender parts are organized and threads, so some of the assumptions that this PR has been designed against will probably change. While it's good to move forward with this and start iterating, I don't think we need to get all of details right just now. We can come back on how much is handled inside or outside of WebRender as the moving pieces come together.

kvark · 2016-12-22T22:27:00Z

Given what @nical said, It looks like a classical textbook case for a fork, given that you need to play with API/internals and you don't know ~~what you are doing~~ what you'll need at the end, by which time you'd want to merge the polished changes upstream...
However, if @glennw is willing to tolerate the experiments happening here (upstream), and this will make GFX team life easier, then let's merge. I don't want to be stalling the team's progress.

bors-servo · 2016-12-23T02:16:14Z

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

JerryShih · 2016-12-23T04:12:29Z

@kvark @glennw
I think I could try to handle the external resource as @kvark 's suggestion(maybe just show up a simple pseudo code and api interface). And we can compare them later.

@nical
Even though we will make render out of ipc-receiver thread, I think it's doesn't affect the design of these external image callbacks. We might just need to handle the thread problem for the TextureSource/TextureHost or compositables in WR's callback. What's your concern for this issue.

glennw · 2016-12-23T04:17:03Z

I'm fine with API experiments happening here in upstream - so I'm happy to either merge this (once rebased and review is finished) or leave it until things settle down more - whatever will make it easiest for you.

Now, the ExternalImageHandler has lock(), unlock() and release() callback functions. Use lock() instead of get() callback in WR render update_deferred_resolves().

Release the external images when a new frame is ready. Merge ResultMsg::UpdateTextureData and NewFrame::NewFrame message into one message.

JerryShih · 2016-12-28T12:24:39Z

I'm trying to update the gecko's new WR model, so I would like to use these patches first. These patches are rebased.
@glennw @kvark

kvark · 2016-12-28T20:26:31Z

@bors-servo r=glennw

bors-servo · 2016-12-28T20:26:32Z

📌 Commit ba33b43 has been approved by glennw

bors-servo · 2016-12-28T20:26:36Z

⚡ Test exempted - status

@nical

Handle external image life time in api.delete_image() call Currently, the delete_image() call just removes the image_key in the hash_map. If the key is associate to an external image, we should release that image using the ExternalImageHandler callbacks. Since the ExternalImageHandler callbacks are only used in renderer, a pending releasing array is used. Then, pass this array to the renderer for ExternalImageHandler::release() call. @nical @glennw  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/webrender/636)

JerryShih mentioned this pull request Dec 14, 2016

extend the external image callback sets #631

Closed

emilio reviewed Dec 14, 2016

View reviewed changes

glennw reviewed Dec 15, 2016

View reviewed changes

kvark self-requested a review December 15, 2016 16:33

kvark requested changes Dec 15, 2016

View reviewed changes

JerryShih force-pushed the handle-external-image-life-time branch from 9df394d to e2bfc7e Compare December 16, 2016 07:17

JerryShih commented Dec 16, 2016

View reviewed changes

JerryShih added 3 commits December 28, 2016 20:11

Extend the ExternalImageHandler callback functions

1cb9ab2

Now, the ExternalImageHandler has lock(), unlock() and release() callback functions. Use lock() instead of get() callback in WR render update_deferred_resolves().

Handle the releasing of external images in api.delete_image().

5606b5a

Release the external images when a new frame is ready. Merge ResultMsg::UpdateTextureData and NewFrame::NewFrame message into one message.

Show panic if we try to call update() for external image.

ba33b43

JerryShih force-pushed the handle-external-image-life-time branch from e2bfc7e to ba33b43 Compare December 28, 2016 12:16

kvark approved these changes Dec 28, 2016

View reviewed changes

bors-servo merged commit ba33b43 into servo:master Dec 28, 2016

Handle external image life time in api.delete_image() call #636

Handle external image life time in api.delete_image() call #636

Conversation

JerryShih commented Dec 14, 2016 • edited by larsbergstrom

Choose a reason for hiding this comment

kvark commented Dec 14, 2016 • edited

glennw 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

api.add_image(A) dl.push_image() with A set_root_dl(frame 1) update() render() <= the frame 1 is not ready, draw frame 0 instead api.delete_image(A) api.add_image(B) dl.push_image with B update() render() <= the frame 1 is ready now, draw with A

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

JerryShih commented Dec 15, 2016

glennw commented Dec 15, 2016

JerryShih commented Dec 15, 2016

JerryShih commented Dec 15, 2016

kvark commented Dec 15, 2016

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

kvark commented Dec 15, 2016

JerryShih commented Dec 16, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JerryShih commented Dec 16, 2016

kvark commented Dec 16, 2016 • edited

glennw commented Dec 22, 2016

nical commented Dec 22, 2016

kvark commented Dec 22, 2016

bors-servo commented Dec 23, 2016

JerryShih commented Dec 23, 2016

glennw commented Dec 23, 2016

JerryShih commented Dec 28, 2016

kvark commented Dec 28, 2016

bors-servo commented Dec 28, 2016

bors-servo commented Dec 28, 2016

JerryShih commented Dec 14, 2016 •

edited by larsbergstrom

kvark commented Dec 14, 2016 •

edited

api.add_image(A)
dl.push_image() with A
set_root_dl(frame 1)
update()
render() <= the frame 1 is not ready, draw frame 0 instead
api.delete_image(A)
api.add_image(B)
dl.push_image with B
update()
render() <= the frame 1 is ready now, draw with A

kvark commented Dec 16, 2016 •

edited