Hi, On 6 April 2017 at 15:14, Pekka Paalanen <[email protected]> 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 _______________________________________________ wayland-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/wayland-devel
