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

[Daniel Stone]

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

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.

> 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.

Cheers,
Daniel