[PATCH v14 19/41] compositor-drm: Extract buffer->plane co-ord translation

[Pekka Paalanen]

On Fri, 2 Mar 2018 16:32:20 +0000
Daniel Stone <daniel at fooishbar.org> wrote:

> Hi Pekka,
> Thanks for the explanation; I'm not sure why it's requiring so much
> mental contortion on my end. :\

Hi Daniel,

no worries, the issue both easy to dismiss as simply rounding, and hard
to trigger in the first place.

> On 26 February 2018 at 08:30, Pekka Paalanen <ppaalanen at gmail.com> wrote:
> > On Fri, 23 Feb 2018 17:25:49 +0000 Daniel Stone <daniel at fooishbar.org> wrote:  
> >> Hm ... I am feeling particularly dense today, but I can't get my head
> >> around this. Why/how would we need to translate the dest region?  
> >
> > The function currently works like this:
> >
> > 1. Starting from the view parameters, compute the destination rectangle
> >    in the framebuffer.
> >
> > 2. Starting from the view parameters, compute the source rectangle in
> >    the buffer.
> >
> > 3. Source rectangle is clamped to the accessible buffer area.
> >
> > If step 3 changes the source rectangle position in the buffer area, and
> > we do not take that into account in the destination rectangle, it will
> > look like as if the image has been shifted slightly. E.g. if computed
> > src_x was -1, and it gets clamped to 0, then the whole resulting image
> > will get shifted by one source pixel.
> >
> > One possible solution would be to compute the source rectangle first,
> > clamp it, and then starting from the source rectangle compute the
> > destination rectangle. Whether the destination rectangle ends up any
> > different depends on the actual values in the transformation.
> >
> > I think one could also take this further to take advantage of the
> > fractional source coordinates of KMS, by first computing the
> > destination coordinates from the view parameters, then computing the
> > source coordinates from the destination coordinates (which may produce
> > fractional values), clamp, and then re-compute the destination
> > coordinates from the final source coordinates.  
> 
> I think that sounds like the best approach. I wonder if, for now,
> we're best off just punting back to the renderer and refusing to
> promote the surface, if we generate OOB co-ordinates.

I believe the case is only hittable with some combination of output
scale > 1, buffer scale > 1, and having a view transformation that does
*not* result in integer coordinates in the global space. Since we do
our window management in global space and surface space which share the
units, I can't think of any way to actually hit this case. It would
need to be a plugin installing its own view transformation. Or a bug.
Or an inaccuracy when computing with doubles in which case the error
magnitude should be so small that the clamping will produce exactly
correct coordinates.

I'm very much ok with refusing to promote, or even ignoring the issue
altogether.

The important thing is that the clamps are in place to avoid OOB access.

> > However, as I gave my R-b already, I don't think this is important
> > enough to work on right now. Testing it would require carefully
> > constructing special test cases. We also seem to still lack an
> > automatable way to test this, so it would be a manual test. And it
> > seems like it would be easy to break this by accident.
> >
> > It would be a lot of work for what seems to be a "rounding effect"
> > leading to the clamps even changing anything.
> >
> > Also, this patch is just extracting code into a new function, not
> > intending to fix it, so it's fine.  
> 
> Thanks for this. I think we'd very definitely need a test with a few
> cases on either side of the line to both illustrate what's supposed to
> happen, and make sure it doesn't break. Unfortunately it doesn't seem
> at all easy to plumb it through, so would likely be a lot of annoying
> mechanical work. On the other hand, it would at least provide a good
> example for making more parts unit-testable, so would be great if
> someone felt like stepping up to do it.

I think it would be possible to trigger the case in an integration
test, but I agree that a unit test would be awesome.

IIRS we have a (single?) existing example of unit testing, the matrix
test, which unfortunately was never refined to be non-interactive.


Thanks,
pq
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/wayland-devel/attachments/20180305/caf8d7ff/attachment-0001.sig>