[WIP] Add support for externally allocated images in the render backend #548
Conversation
|
☔ The latest upstream changes (presumably #546) made this pull request unmergeable. Please resolve the merge conflicts. |
|
@nical This looks mostly good to me. It seems unfortunate to add a new primitive type / api though (add_external_image). Could we make it just work through the normal image primitive path? In terms of getting the custom UVs, how about we make that part of what the external image callback returns? (That seems like it might be useful to allow deferring the UV calculation until then too?). I've been thinking a bit about this the last couple of days, and I think long term we should remove the knowledge of GL texture IDs from the render backend altogether, and even native images should go through a resolve step in the renderer to get the native texture ID. But that can be done later on as a follow up anyway - just thought I'd mention it now. |
|
I would also prefer to not add an extra display item in the API, and now that you mention it, it makes a lot of sense to defer the knowledge of UV calculation to the renderer thread as well. I'll look into making these changes. At least as a first step I'll keep the external and non-external code paths distinct in the primitive store to avoid resource cache allocations. |
nical
force-pushed
the
external-texture
branch
from
958f72a to
f417f58
Compare
|
The last commit is a hacky attempt at deferring the resolution of external image UVs to the renderer so that it can be specified in the callback instead of a display item. I am not very happy about how it turns out because patching up the UVs in the frame's gpu_data32 in the renderer requires the primitive store to track a list of key+offset pairs and ship that in the frame, and have the renderer unsafely poke at the buffer's internals. The renderer needs to resolve the UV and the texture id at two different times which is also not great if we are going for a callback API to pull the texture and UV from gecko (if we do that I'd rather have gecko hand off the UV and textureId to the renderer using a simple method instead of a callback, and let the renderer store them in a HashMap. In this commit, ExternalImageKey is just an alias of ImageKey using the highest bit of the namespace to specify whether the image is external, but it would be better to just use an enum (I don't know how I would call it). @glennw: you mentioned resolving all texture ids in the renderer, maybe the changes you have in mind would also simplify deferring some of the GpuStore data? |
| @@ -347,6 +346,9 @@ pub struct PrimitiveStore { | |||
| pub cpu_gradients: Vec<GradientPrimitiveCpu>, | |||
| pub cpu_metadata: Vec<PrimitiveMetadata>, | |||
| pub cpu_borders: Vec<BorderPrimitiveCpu>, | |||
| // the indices witihin gpu_data_32 of the image primitives to be resolved by | |||
| // the renderer instead of the render backend. | |||
| pub deferred_image_primitives: Vec<(ExternalImageKey, GpuStoreAddress)>, | |||
There was a problem hiding this comment.
I don't think we need to have primitives in the name, since technically an external texture can be a part of anything, not necessarily it's own Image type primitive. Perhaps, external_images?
| if let Some(key) = image_cpu.color_texture_id.to_external() { | ||
| // If the image primitive uses externally allocated textures, | ||
| // we resolve it on the renderer. | ||
| self.deferred_image_primitives.push((key, metadata.gpu_prim_index)); |
There was a problem hiding this comment.
Do you ever force the primitive to appear in the prims_to_resolve list? Perhaps, it would be easier to straight push to deferred_image_primitives from prepare_prim_for_render().
| @@ -20,6 +20,7 @@ use internal_types::{TextureUpdateDetails, TextureUpdateList, PackedVertex, Rend | |||
| use internal_types::{ORTHO_NEAR_PLANE, ORTHO_FAR_PLANE, DevicePoint}; | |||
| use internal_types::{SourceTexture, BatchTextures, TextureSampler, GLContextHandleWrapper}; | |||
| use ipc_channel::ipc; | |||
| use prim_store::{ImagePrimitiveGpu}; | |||
There was a problem hiding this comment.
Yeah, this is a red flag for me. Renderer shouldn't need to know anything about the primitive store, at least directly.
| @@ -107,9 +107,23 @@ impl FontKey { | |||
| } | |||
| } | |||
|
|
|||
| // hijack the last highest bit of the namespace to store whether or not the key | |||
|
Do we absolutely have to call something for the texture id and UV coords on the renderer thread? Is there an option to listen to a channel inside |
|
☔ The latest upstream changes (presumably #552) made this pull request unmergeable. Please resolve the merge conflicts. |
|
PR #554 contains most of this one and is in a better shape. |
Although there is some overlap with PR #547, the latter currently focuses on the API aspect while the work I have here is a proof-of-concept implementation of how to get the indirection layer set up in the various bits that happen in the render backend thread. I'd love to have the two PRs on the same ticket to group the discussions and reviews but it looks like github doesn't work that way).
Since this is rebased on top of PR #534, you currently see a lot more change than you need to in the diff view, so for now just ignore the commit "Refactor texture names and support more than two textures in the shaders."
The basic idea here is to add an image item which does not use the resource cache to generate the texture id and uv coordinates. UVs are specified upfront and the texture id is resolved at the last moment by the renderer from an ExternalmageKey (introdoced in PR #547). The render backend thread only sees the ExternalImageKey so there is some plumbing done, in particular in the primitive store, in order to not use the resource cache in this particular case.
UVs are provided up front because I want to be able to store several images in the same texture (basically have something that would work like webrender's texture cache for content that is not supported by webrender, and is rendered from a different thread). Also we sometimes have to fiddle with the texture size because the texture is too small or the driver is too buggy, so we can't assume that the UVs are going to cover the entire image.
It's currently in a rough and unfinished state (UVs are passed through without considering the tiling, etc.) but the basic infrastructure is here (for the render backend side of things, refer to PR #547 for the renderer and API aspects).
cc @glennw @JerryShih
This change is