handle external raw buffer #776

JerryShih · 2017-01-24T18:21:46Z

Here are the patch sets to support external raw buffer.
What I do here are:

extract texture border updating into utility function[1].
if the imageData is an external raw buffer, pass all parameters used by the utility function[1] to renderer. Then, replay that function at renderer with all passed parameters.
at the renderer side, use ExternalImageHandler[2] to get the external buffer from WR client.

[1]
https://github.com/servo/webrender/compare/master...JerryShih:issue-723-handle-external-raw-buffer-rebase?expand=1#diff-d52ac938b0c681e0d7bb3f946e47489fR603
[2]
https://github.com/servo/webrender/compare/master...JerryShih:issue-723-handle-external-raw-buffer-rebase?expand=1#diff-f5062b694b9fe53fc1757ed483d577d9R959

This change is

bors-servo · 2017-01-26T06:33:12Z

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

kvark

Looks sane, thanks @JerryShih !
Need to clean up the change a bit before merging.

kvark · 2017-01-26T21:09:14Z

webrender/src/internal_types.rs

+    // UpdateExternalBuffer(alloc.x, alloc.y, alloc.width, alloc.height,
+    //                      request.x, request.y, request.width, request.height,
+    //                      external_image_id, bpp, stride)
+    UpdateForExternalBuffer(u32, u32, u32, u32, u32, u32, u32, u32, ExternalImageId, u32, Option<u32>),


Can we use DeviceUintRect here?

kvark · 2017-01-26T21:09:14Z

webrender/src/internal_types.rs

@@ -342,10 +342,18 @@ pub enum RenderTargetMode {
 }

 pub enum TextureUpdateOp {
+    // Create(image.width, image.height, format, filter, mode, buffer)


I believe, a better option than doing comments in this style is to use stronger types and/or the named fields, which are self-descriptive. E.g:

Create { width: u32, height: u32, format: ImageFormat, filter: TextureFilter, mode: RenderTargetMode, buffer: Option<Arc<Vec<u8>>>, }

kvark · 2017-01-26T21:09:14Z

webrender/src/internal_types.rs

+    // UpdateExternalBuffer(alloc.x, alloc.y, alloc.width, alloc.height,
+    //                      request.x, request.y, request.width, request.height,
+    //                      external_image_id, bpp, stride)
+    UpdateForExternalBuffer(u32, u32, u32, u32, u32, u32, u32, u32, ExternalImageId, u32, Option<u32>),


Do we have to pass the bpp with the update? It seems to me that this should be derived from the ImageFormat, which is either taken from the (already created) texture info.

If we try to find the texture info at renderer, we should reference to the texture structure in device[1]. Then, we need to have additional accessor for this texture structure and a bpp calculation function in render or device. Just pass a bpp is the simplest way.

[1]
https://github.com/JerryShih/webrender/blob/23123c85ddfccf4c458b9c3856890869644be667/webrender/src/device.rs#L804

kvark · 2017-01-26T21:09:14Z

webrender/src/internal_types.rs

    Create(u32, u32, ImageFormat, TextureFilter, RenderTargetMode, Option<Arc<Vec<u8>>>),
+    // Create(image.width, image.height, format, filter, mode, external_image_id)
+    CreateForExternalBuffer(u32, u32, ImageFormat, TextureFilter, RenderTargetMode, ExternalImageId),


It looks like the handling of Create versus CreateForExternalBuffer shares a lot of code. I think it would be cleaner to merge those by just having Option<ImageData> as the last parameter.

The Update cases are much different though, so it's reasonable to keep them separate.

kvark · 2017-01-26T21:09:14Z

webrender/src/texture_cache.rs

@@ -594,6 +600,42 @@ impl TextureCache {
        }
    }

+    pub fn insert_image_border_updating_operation(src: &[u8],


I think we can shorten it out to just insert_image_border.

kvark · 2017-01-26T21:09:14Z

webrender/src/texture_cache.rs

+    pub fn insert_image_border_updating_operation(src: &[u8],
+                                                  alloc_x: u32,
+                                                  alloc_y: u32,
+                                                  alloc_width: u32,


let's use rectangles (DeviceUintRect) here for both the allocation and request regions

kvark · 2017-01-26T21:09:14Z

webrender/src/texture_cache.rs

+                                                  request_height: u32,
+                                                  stride: Option<u32>,
+                                                  bpp: u32,
+                                                  op: &mut BorderUpdatingMethod) {


Let's pass this in as a closure. You only use it with temporary structs anyway, and using a closure would remove a lot of boilerplate.

kvark · 2017-01-26T21:09:14Z

webrender/src/texture_cache.rs

+            ImageData::ExternalBuffer(id) => {
+                match result.kind {
+                    AllocationKind::TexturePage => {
+                        let update_op = TextureUpdate {


let's move the assignment and self.pending_updates population to after the match, as in:

let update_op = match result.kind {...}; self.pending_updates(update_op);

Please check whether if it's still a problem after reordering the match expressions in my new patch.

glennw · 2017-01-26T22:14:56Z

These changes look good to me once the comments from @kvark are resolved.

The texture cache in general could do with some refactoring and cleanup, but that can happen later (and include the improved ideas for border updating).

JerryShih · 2017-01-28T08:06:24Z

I address all review comments after the commit [1].
If these patch sets look good, I will fix the conflicts to the latest revision of WR and do squash.

[1]
6baa756

JerryShih · 2017-01-28T08:40:31Z

@glennw @kvark
The new patches are ready.

glennw · 2017-01-31T03:16:36Z

Reviewed 4 of 7 files at r1, 1 of 3 files at r2, 2 of 2 files at r3.
Review status: all files reviewed at latest revision, 7 unresolved discussions, some commit checks failed.

Comments from Reviewable

glennw · 2017-01-31T03:17:37Z

This looks good - let's get it rebased and squashed, then we should be ready for merge (@kvark may want to do a final pass over it too).

JerryShih · 2017-01-31T06:38:39Z

Review status: all files reviewed at latest revision, 7 unresolved discussions, some commit checks failed.

webrender/src/internal_types.rs, line 345 at r1 (raw file):