Repository navigation
Conversation
LaurenzV
left a comment
LaurenzV
left a comment
There was a problem hiding this comment.
Just some drive-by comments, overall this is awesome. 😍 Thanks so much! Two questions:
- Is there a reason why we don't have any tests in
vello_tests? We could add wgpu-only tests to make sure the ycbcr conversion logic looks properly, no? - More importantly, the second question: Have we considered introducing a separate shader program just for videos? While the code for the whole logic isn't that much, we've seen that low-tier devices are very sensitive to shader complexity. WebGL is luckily unaffected, but on native we might see the same slowdowns we've seen on web before, because the shader there is more complex. Which is a bit unfortunate, because only a low percentage of people actually use videos, but everyone is going to feel the slowdown for non-video situations. Having a separate program for videos could avoid that, although the downside is that we will have more draw calls + some duplicated shader logic (for anti-aliasing). Not saying this should block this PR or anything, just maybe something to consider for the future?
Other than that, two implementation notes from AI, I don't have the knowledge to judge whether they are accurate or not. 😄
| let s10 = textureLoad(external_texture, vec2<i32>(clamp(coords + vec2(-0.5, -1.5), image_offset, image_max)), 0); | ||
| let s20 = textureLoad(external_texture, vec2<i32>(clamp(coords + vec2(0.5, -1.5), image_offset, image_max)), 0); | ||
| let s30 = textureLoad(external_texture, vec2<i32>(clamp(coords + vec2(1.5, -1.5), image_offset, image_max)), 0); | ||
| let s00 = textureLoad( |
There was a problem hiding this comment.
Why did the formatting change here?
There was a problem hiding this comment.
Same also further below and in image.wesl
| if quality == 0u { | ||
| y = textureLoad(y_texture, vec2<i32>(clamped_coords), 0).r; | ||
| } else { | ||
| let sample_coords = clamped_coords - vec2(0.5); | ||
| let coord_quad = vec4(floor(sample_coords), ceil(sample_coords)); | ||
| let coord_frac = fract(sample_coords); | ||
| let a = textureLoad(y_texture, vec2<i32>(clamp(coord_quad.xy, image_offset, image_max)), 0) | ||
| .r; | ||
| let b = textureLoad(y_texture, vec2<i32>(clamp(coord_quad.xw, image_offset, image_max)), 0) | ||
| .r; | ||
| let c = textureLoad(y_texture, vec2<i32>(clamp(coord_quad.zy, image_offset, image_max)), 0) | ||
| .r; | ||
| let d = textureLoad(y_texture, vec2<i32>(clamp(coord_quad.zw, image_offset, image_max)), 0) | ||
| .r; | ||
| y = mix(mix(a, b, coord_frac.y), mix(c, d, coord_frac.y), coord_frac.x); | ||
| } |
There was a problem hiding this comment.
Up to you, but since we are likely going to pull in that direction (#1964), how about introducing a sampler for the ycbcr texture and using textureSampleLevel, so the filtering happpens automatically? Should reduce the complexity in the shader, unless I'm missing something!
There was a problem hiding this comment.
Ah, but I guess that only works once the Full texture rect enum variant is added. So feel free to disregard, I can just change it afterwards.
There was a problem hiding this comment.
Nice! It's so awesome to play with the example and see zero copy NV12 sampling.
I think we should sync on the strategy because, of the Android devices I tested, N12 wasn't output from hardware decoders directly.
On some Androids, you don't even get a texture with a known format - instead, you get a vendor specific layout embedded in a AHardwareBuffer/opaque Vulkan texture. This requires using a vendor specific sampler (essentially, you can ask the GPU to give you its Ycbcr sampler) - so we can't sample Y / UV textures directly.
I can take a look at the blit cost separately (to go from opaque texture -> RGBA) but I'm wondering whether using WGPU's ExternalTexture feature might be a better route. This would allow us to bind an external texture with its own sampler configuration and leave all the NV12 / NV21 / ... conversion work out of Vello (and support a better long term direction).
I.e., wgpu::ExternalTexture already supports conversion metadata + YUV->RGB matrix and WGSL supports sampling an RGBA from a bound external texture. This should reduce Vello GPU and shader complexity significantly.
The trouble with ExternalTexture is that:
- It's lacking a Vulkan implementation
- It's lacking external sampler support
I think it's worth discussing / thinking about. But, a strategy could be:
- Land the
ExternalTexturework in Vello here (enabling it in Vulkan simply doesn't work, but everything else will). - Land the vulkan impl and external sampler support in WGPU behind a fork
- Notify Vello consumers that if they want
ExternalTextureto work with Vulkan and a custom sampler, they need to use the fork via a[patch]in Cargo.toml.
To be clear, this means we can land ExternalTexture into Vello GPU without depending on the WGPU fork in linebender/vello while at the same time allowing consumers to zero copy Video frames into Vello if they adopt the WGPU fork (or consumers can implement their own fork themselves).
Any reason that work can't be upstreamed? Or is this only for the short term? |
It definitely should be upstreamed 👍😊. Until it hasn't hit a release, we would want to point to the fork. |
This branch has not been deployed
3 participants
Summary
Hardware video decoders usually output NV12: a full-resolution luma plane plus a half-resolution interleaved chroma plane. Right now those frames have to be converted to RGBA before Vello can draw them. This PR lets the wgpu backend sample NV12 textures directly as external image sources, converting them to RGB in the shader.
nv12_ycbcr_external_texture_support.mp4
New API:
TextureBindings::insert_ycbcr_nv12(id, y_plane, uv_plane, YCbCrInfo).YCbCrInfosets the matrix (BT.601, BT.709 or BT.2020 NCL) and the range (limited or full). Image paints don't change, since the binding decides how an external texture is sampled. Transfer function and color primaries aren't handled, which matches the existing RGBA external-texture path.The feature is behind a new
external_texture_ycbcrflag, which is on by default. WebGL doesn't support it. The shader build now links GLSL separately with this feature turned off, because a second sampler that WebGL never binds would fall back to texture unit 0 and clash with the alphas sampler. A new test checks that the generated GLSL declares only the samplers the WebGL backend binds.Commits
The PR has two commits, and they can be reviewed separately:
vello_gpuandvello_gpu_shaders.Created with assistance from Claude Opus 5.5.