Implement sub-image updates. #1003

nical · 2017-03-22T16:30:38Z

Add an optional dirty rect to the update_image command (None means invalidate all), and only upload the required part of the image if the image was already in the texture cache. This will become more important when used with blob images since the blob image renderer will use this information to avoid repainting the entire image when possible.

This change is

nical · 2017-03-22T16:31:23Z

r? @glennw

glennw

One nit and a question about the purpose of the union() and how that works.

glennw · 2017-03-22T23:41:41Z

webrender/src/resource_cache.rs

@@ -317,6 +320,11 @@ impl ResourceCache {
            data: ImageData::new(bytes),
            epoch: next_epoch,
            tiling: None,
+            dirty_rect: match (dirty_rect, prev_dirty_rect) {


I don't really understand this part. I'm assuming the union() here is for the case where you update different parts of the same image multiple times in one frame? But then, the second update loses the bytes from the original update doesn't it?

The case I'm trying to support is updating an image several times (either several times in the same frame or across several frames if the image isn't requested (not in the viewport?)). In this situation the idea is to keep a dirty rectangle containing all dirty rectangles since the last time the texture_cache was updated.

I'm not sure what you mean by losing the bytes of the original update.

Edit: Ah! After talking with @kvark on irc I realize you probably meant by losing the bytes. The image data sent in update image still contains the whole image, the optional dirty rect only gives a hint that only a portion of the new image is different from the previous version.
So at any time the ImageData that we have can fully describe the whole image (only sending the damaged part would make things more complicated because the image might not be in the texture cache, in which case the dirty rect isn't useful since we need to upload the whole thing).

Same thing for blob images, the new blob is the whole "blob display list" for that image.

glennw · 2017-03-22T23:42:18Z

webrender/src/texture_cache.rs

-                    data: bytes,
-                    stride: descriptor.stride,
-                    offset: descriptor.offset,
+                if let Some(dirty) = dirty_rect {


This if let is better as a match since there is an else clause anyway.

kvark · 2017-03-23T12:44:31Z

webrender/src/render_backend.rs

@@ -147,8 +147,8 @@ impl RenderBackend {
                            }
                            self.resource_cache.add_image_template(id, descriptor, data, tiling);
                        }
-                        ApiMsg::UpdateImage(id, descriptor, bytes) => {
-                            self.resource_cache.update_image_template(id, descriptor, bytes);
+                        ApiMsg::UpdateImage(id, descriptor, bytes, dirty_rect) => {


naming nit: this is not a "dirty" rectangle, just the one to update. "dirty" brings on more semantics than the interface really provides.

I can rename that updated_rect if you prefer.

kvark · 2017-03-23T12:47:44Z

webrender/src/texture_cache.rs

-                    stride: descriptor.stride,
-                    offset: descriptor.offset,
+                if let Some(dirty) = dirty_rect {
+                    let stride = descriptor.compute_stride();


that's a bit confusing that we have both descriptor.stride and descriptor.compute_stride()

What you mean is that ImageDescriptor::stride being an option is confusing, and that's true (but it's convenient because in a lot of cases we don't have a stride larger than width * bpp).
You can fix that by making the stride not optional, and computing it in a construction function if you want.

right. It's not related to this PR though

kvark · 2017-03-23T12:51:39Z

webrender/src/resource_cache.rs

@@ -317,6 +320,11 @@ impl ResourceCache {
            data: ImageData::new(bytes),


where does the old image data go? Looks like it gets lost

The old image data is replaced by the new one, yes. The underlying buffer is reference counted, not sure what you mean by lost.

kvark · 2017-03-23T12:52:22Z

webrender/src/texture_cache.rs

-                    offset: descriptor.offset,
+                if let Some(dirty) = dirty_rect {
+                    let stride = descriptor.compute_stride();
+                    let offset = descriptor.offset + dirty.origin.y * stride + dirty.origin.x;


gotta double-check this line, but it depends on the answers to previous questions

kvark

After talking to @nical on IRC, the questions and details were clarified. My only concern would be the need to re-allocate the whole image on each sub-image update, which seems a bit wasteful.

nical · 2017-03-23T13:38:25Z

Yes, I considered only passing the updated rect's worth of pixel data in the update message, but that makes the implementation a lot more complicated, and isn't necessarily more efficient because we currently save copies thanks to the image data being immutable and atomically ref counted.

I'm not against doing it this way but I would prefer to have a compelling use case to motivate the extra complexity.

glennw · 2017-03-23T20:16:38Z

@nical OK, that makes sense. Let's get this merged - but it would be good to change that if let to a map as a follow up, which is a bit tidier.

glennw · 2017-03-23T20:16:44Z

@bors-servo r+

bors-servo · 2017-03-23T20:16:45Z

📌 Commit 4a6703d has been approved by glennw

bors-servo · 2017-03-23T20:16:48Z

⌛ Testing commit 4a6703d with merge a24673b...

Implement sub-image updates. Add an optional dirty rect to the update_image command (None means invalidate all), and only upload the required part of the image if the image was already in the texture cache. This will become more important when used with blob images since the blob image renderer will use this information to avoid repainting the entire image when possible.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/webrender/1003)

bors-servo · 2017-03-23T20:48:50Z

☀️ Test successful - status-travis
Approved by: glennw
Pushing a24673b to master...

Implement sub-image updates.

4a6703d

nical mentioned this pull request Mar 22, 2017

Allow updating images that are not raw buffers #1004

Merged

glennw requested changes Mar 22, 2017

View reviewed changes

kvark reviewed Mar 23, 2017

View reviewed changes

kvark approved these changes Mar 23, 2017

View reviewed changes

glennw approved these changes Mar 23, 2017

View reviewed changes

bors-servo merged commit 4a6703d into servo:master Mar 23, 2017

bors-servo mentioned this pull request Mar 23, 2017

GL_TEXTURE_RECTANGLE texture target support #997

Merged

nical mentioned this pull request Mar 24, 2017

Sub-image updates #993

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement sub-image updates. #1003

Implement sub-image updates. #1003

nical commented Mar 22, 2017 •

edited by larsbergstrom

Loading

nical commented Mar 22, 2017

glennw left a comment

glennw Mar 22, 2017

nical Mar 23, 2017 •

edited

Loading

glennw Mar 22, 2017

kvark Mar 23, 2017

nical Mar 23, 2017

kvark Mar 23, 2017

nical Mar 23, 2017

kvark Mar 23, 2017

kvark Mar 23, 2017

nical Mar 23, 2017

kvark Mar 23, 2017

kvark left a comment

nical commented Mar 23, 2017 •

edited

Loading

glennw commented Mar 23, 2017

glennw commented Mar 23, 2017

bors-servo commented Mar 23, 2017

bors-servo commented Mar 23, 2017

bors-servo commented Mar 23, 2017

		@@ -317,6 +320,11 @@ impl ResourceCache {
		data: ImageData::new(bytes),

Implement sub-image updates. #1003

Implement sub-image updates. #1003

Conversation

nical commented Mar 22, 2017 • edited by larsbergstrom Loading

nical commented Mar 22, 2017

glennw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nical Mar 23, 2017 • edited Loading

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

kvark left a comment

Choose a reason for hiding this comment

nical commented Mar 23, 2017 • edited Loading

glennw commented Mar 23, 2017

glennw commented Mar 23, 2017

bors-servo commented Mar 23, 2017

bors-servo commented Mar 23, 2017

bors-servo commented Mar 23, 2017

nical commented Mar 22, 2017 •

edited by larsbergstrom

Loading

nical Mar 23, 2017 •

edited

Loading

nical commented Mar 23, 2017 •

edited

Loading