[PATCH weston v10 01/61] Clip co-ordinates in surface->buffer translation

[Daniel Stone]

Hi,

On 6 April 2017 at 15:14, Pekka Paalanen <ppaalanen at gmail.com> wrote:
> weston_surface_to_buffer_rect() does flooring and ceil'ing after
> calling viewport_surface_to_buffer() and seems to be aimed at damage
> tracking where not clamping is not a problem. It intentionally rounds
> the rect to be never smaller than the unrounded result.
>
> weston_surface_to_buffer_rect() is used by gl-renderer to compute the
> rectangles for glTexSubImage2D(), where it is crucial to not exceed the
> buffer boundaries. That was arguably buggy and needs the clamping
> *after* the call to weston_transformed_rect().

Arguably the fact that we're ever asking the renderer to upload
partial pixels, tells us that we've already lost too much information
in an earlier stage, and should be preserving information in the
original (buffer) co-ordinate space. But as you say, not fixing this
does get us uploading pixels outside the viewport source, which
contradicts the second paragraph of wp_viewport: '... and content
outside the source rectangle is ignored'.

> weston_surface_to_buffer_float() is used in gl-renderer's
> texture_region() where it is used to produce texture coordinates. The
> lack of clamping was not serious here, and clamping shouldn't change
> anything.

I fail to see how this is any different from the glTexSubImage2D case.
Either the renderers cannot feed surface co-ordinates which generate
out-of-bounds (per the viewport) texture co-ords, in which case no-one
needs to do any clamping anywhere, or they can, in which case both
cases involve constructing a rendering pipeline which specifically
samples from out-of-bounds co-ordinates.

> Given all that, this patch fails to explain why to clamp and why exactly
> here.

/* Here we clamp to the source co-ordinates, in order to avoid
sampling from outside the source co-ordinates. */

I am having trouble understanding why it would be good to configure a
sampler to access pixels that the client has specifically instructed
us not to. I understand that filtering makes it possible for the
hardware to sample neighbouring but out-of-bounds co-ordinates, but
you seem to be arguing to instruct the sampler, that out-of-bounds
co-ordinates are in fact in bounds.

I also don't understand how this doesn't violate the assertion we made
in wp_viewport, which doesn't say anything about sometimes considering
neighbouring pixels in-bounds, which may in turn cause filtering
samplers, to sample neighbours _of neighbours_. IOW, in order to use
wp_viewport with no clipping, clients would have to maintain a border
of a few pixels outside the source area, not just one.

> I see "[PATCH weston v10 38/61] compositor-drm: Extract buffer->plane
> co-ord translation" is the reason for this. I would prefer the clamping
> to happen in drm_plane_state_coords_for_view() as the final step, not
> in the middle of floating point computations like this patch is doing.
>
> gl-renderer would probably need its own clamping fix in
> gl_renderer_flush_damage().
>
> I recommend keeping the clamping in the DRM backend where the final
> parameters are computed, so we don't have to start shaving this yak.
>
> If we had an end-to-end transformation matrix, we would need to clamp
> in the DRM backend anyway, so it's also better for future refactorings.

Calling a helper (which in turn calls two helpers) to get the buffer
transformation, and then following that up by punching through to get
one very particular part of the transformation, seems a bit backwards
to me. It also makes transformation handling more fragile, which is
not what we need.

Anyway, since the argument seems to be that specifically instructing
rendering to sample from outside the wp_viewport source region is OK,
and I don't see a path towards changing that, I've just dropped the
patch entirely, and compositor-drm can join all the other rendering
paths in being broken. If someone else wants to pick this up then I'll
happily review what I can.

Cheers,
Daniel