Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] External image handlers #1676

Closed
wants to merge 1 commit into from
Closed

[WIP] External image handlers #1676

wants to merge 1 commit into from

Conversation

@kvark
Copy link
Member

kvark commented Sep 7, 2017

WebRender does not have a single "client". It can be used by multiple independent actors, which may, for example, manage their respective namespaces, or populate/present different documents. Therefore, WR needs to support multiple external image handlers, where each external image is strongly associated with one.

WIP, because TODO: Gecko patch and try push
Edit: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a1fa7748d33f862bb25dc86fad8e091631328484

@glennw comments and r?


This change is Reviewable

@kvark
Copy link
Member Author

kvark commented Sep 7, 2017

I think Servo would prefer the API to be more in line with generate_external_image_namespace followed by set_external_image_handler(namespace, handler), so I'm going to rework it a bit.

@glennw
Copy link
Member

glennw commented Sep 7, 2017

The general idea here seems fine, and the implementation looks good. I'll take another look after your suggested changes for Servo.

@kvark
Copy link
Member Author

kvark commented Sep 7, 2017

@glennw thanks for taking a look! I figured the simplest way here would be if a handler could be assigned per IdNamespace (without the need for an extra ExternalImageNamespace type).

@glennw
Copy link
Member

glennw commented Sep 7, 2017

Right, that would probably be ideal :)

@kvark kvark force-pushed the kvark:external branch 2 times, most recently from 1dbf91f to 66b0400 Sep 8, 2017
@kvark
Copy link
Member Author

kvark commented Sep 8, 2017

app_units crate fails to compile on nightly - requires feature(clamp) to be enabled

@kvark kvark changed the title [WIP] External image handlers External image handlers Sep 8, 2017
@kvark
Copy link
Member Author

kvark commented Sep 8, 2017

@glennw AppVeyor failures aside (unrelated), and while Gecko push is pending, please take another look as the approach is changed a bit.

@glennw glennw self-requested a review Sep 8, 2017
@glennw
glennw approved these changes Sep 8, 2017
Copy link
Member

glennw left a comment

Looks good. If this covers the use case that Gecko needs, then r=me.

@glennw
Copy link
Member

glennw commented Sep 8, 2017

@kvark I guess we'll want to remove the handler if the api namespace goes out of scope. Not a big deal though - we can work that out as a follow up.

@kvark kvark force-pushed the kvark:external branch from 66b0400 to 4d1d731 Sep 8, 2017
@kvark
Copy link
Member Author

kvark commented Sep 8, 2017

we'll want to remove the handler if the api namespace goes out of scope

Sure, good point. Fixed now :)

If this covers the use case that Gecko needs, then r=me.

It's not covering Gecko needs at this very moment as much as it's needed for smooth WebGPU prototyping (WebGL and WebGPU don't want to share the same handler).

@kvark kvark changed the title External image handlers [WIP] External image handlers Sep 8, 2017
@kvark kvark force-pushed the kvark:external branch from 4d1d731 to 4c3bf84 Sep 8, 2017
@kvark
Copy link
Member Author

kvark commented Sep 8, 2017

@glennw I switched back to new-typed external hander IDs after talking to @jrmuizel about possible future use in Gecko. However, producing an actual patch/try-push for Gecko seems to require a ton of plumbing in this case, so that's where I am now.

@glennw
Copy link
Member

glennw commented Sep 10, 2017

@kvark This looks fine to me. Is it ready to go from your perspective?

@kvark
Copy link
Member Author

kvark commented Sep 11, 2017

@glennw thanks!
I haven't got the time to produce a Gecko patch yet (as noted earlier, it's not trivial). I don't think we should be merging this without a patch and try-push.

@bors-servo
Copy link
Contributor

bors-servo commented Sep 13, 2017

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

@glennw
Copy link
Member

glennw commented Oct 5, 2017

Closing for now - feel free to re-open when you have time to work on this again :)

@glennw glennw closed this Oct 5, 2017
@kvark kvark deleted the kvark:external branch Jul 26, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.