Provide UV rectangle for the image primitive/shader

[kvark]

There is a problem in Gecko showing images when the system UI scaling is above 100%: https://bugzilla.mozilla.org/show_bug.cgi?id=1447093

Gecko needs to provide us with UV clamping rectangle per image primitive, so that the ps_image shader uses it (instead of the one derived from image descriptor) and doesn't sample outside of the designated region if UI scaling is high.

[glennw]

There's a bit of concept overlap between this, image sub-rects, image tiling, and render tasks with UV bounds. Perhaps in the future we can unify some of these concepts, but the fix proposed here and in bugzilla seems reasonable, I think.

[glennw]

@kvark Actually, an alternative could be that if Gecko supplies one of these UV rects, WR only uploads that portion of the image - and doesn't even copy / upload the rest of the image. That might be slightly more efficient?

[mstange]

At the DisplayListBuilder::push_image API level, maybe all we need is an extra sampling rect parameter in image pixel space.

[glennw]

@mstange WR does support this in internal APIs (the sub-rect parameter when adding an image to frame builder), so perhaps we just need to expose this?

[kvark]

WR only uploads that portion of the image - and doesn't even copy / upload the rest of the image

That's an option, but it seems to require more effort to explore, and we aren't concerned about upload size here (in this particular issue).

WR does support this in internal APIs (the sub-rect parameter when adding an image to frame builder), so perhaps we just need to expose this?

I don't think WR supports it on the frame builder level, given the ps_image shader logic: it only clamps the UV coordinates according to the full image UV rect. So exposing the rect has to start with the shader, all the way up to the push_image API.

[mstange]

On the other hand, if we move all the snapping into WebRender and have Gecko provide the raw, unsnapped rectangles, then we can compute the sampling restriction rectangle on the WebRender side as well and don't need an extra parameter. The code that does this in Gecko is here: https://searchfox.org/mozilla-central/rev/a0665934fa05158a5a943d4c8b277465910c029c/layout/base/nsLayoutUtils.cpp#6828-6837

[kvark]

@mstange but this is not about snapping as much as it's about UI scaling.

[mstange]

Let's say you have an atlas image of size 165x165, and you want to draw the middle 55x55 piece of it at location 21,21 on the page. So you set a clip to 21,21 55x55 and draw the image with a destination rectangle of -34,-34 165x165. Without scaling, everything works and you don't sample any unexpected pixels.

Now you apply a UI scale of 1.1. All the coordinates get scaled up; the clip is now at 23.1,23.1 60.5x60.5 and the dest rect is at -37.4,-37.4 181.5x181.5. If these rectangles are snapped inside Gecko, WebRender will see a clip of 23,23 61x61 and a dest rect of -37,-37 181x181. In order to determine which pixels to sample, WebRender will translate the clip into image space:

imageSpaceClip.x = (23.0 - -37.0) / 181.0 * 165.0 = 54.69613259668508
imageSpaceClip.y = (23.0 - -37.0) / 181.0 * 165.0 = 54.69613259668508
imageSpaceClip.width = 61.0 / 181.0 * 165.0 = 55.607734806629836
imageSpaceClip.height = 61.0 / 181.0 * 165.0 = 55.607734806629836

imageSpaceClip.XMost() == 54.69613259668508 + 55.607734806629836 == 110.30386740331491
imageSpaceClip.YMost() == 54.69613259668508 + 55.607734806629836 == 110.30386740331491

So snapping has expanded the per-dimension sampling range from [55, 110] to [54.7, 110.3]. From the snapped values that it gets from Gecko, WebRender cannot know that it's not supposed to sample outside the range [55, 110] in the image.

However, if Gecko gave WebRender the raw values 23.1,23.1 60.5x60.5 and -37.4,-37.4 181.5x181.5, then WebRender would be able to map these values back into image space precisely, and would end up with an exact sampling range of [55, 110].

[glennw]

I don't think WR supports it on the frame builder level, given the ps_image shader logic: it only clamps the UV coordinates according to the full image UV rect. So exposing the rect has to start with the shader, all the way up to the push_image API.

It's handled before the shader (which has no knowledge of it). If the frame builder has a sub-rect specified, it requests a render task that blits the sub-rect to a (cached) render task item. This means that the shader sees the sub-rect as a complete image, and therefore I think the clamping applied in the image shader would do what is needed here.

I think that would solve the problem here - whether it's an appropriate fix is another thing altogether :)

[mstange]

However, if Gecko gave WebRender the raw values 23.1,23.1 60.5x60.5 and -37.4,-37.4 181.5x181.5, then WebRender would be able to map these values back into image space precisely, and would end up with an exact sampling range of [55, 110].

@kvark convinced me that this isn't really viable: Many different clips can apply to an image item, and they can come from different sources and different coordinate spaces. There's only one rectangle that we want to take into account for the purposes of computing the image's source clip in image space, and that's the image's "fill rect". Gecko happens to apply the fill rect as a clip, but it doesn't mark that particular clip in any special way.

So in a world where the sampling rect is computed by WebRender, Gecko would still need to supply the "fill rect" separately to WebRender. But at that point there's no real advantage over just having Gecko supply the "sampling rect" to WebRender. So let's stick to the original plan of adding a "sampling rect" parameter to push_image.

[kvark]

@gw3583

If the frame builder has a sub-rect specified, it requests a render task that blits the sub-rect to a (cached) render task item.

Ok, I see what you mean now. But can we (or should we) afford this extra work? It is a waste if we do a sub-image blit (to the render task cache) just because we need a proper clamp in the shader.

[dralley]

The bugzilla is marked resolved, can this be closed?