If the comparison fails, the returned values are modulus fixed_16_16, rather than clamped. I think that gives a lot less possibilities for the calling code to recover from or simulate the results of the out-of-range value.
On Fri, Apr 29, 2016 at 6:33 AM, Emil Velikov <emil.l.veli...@gmail.com> wrote: > On 29 April 2016 at 11:35, Pekka Paalanen <ppaala...@gmail.com> wrote: > > On Fri, 29 Apr 2016 10:15:37 +0100 > > Emil Velikov <emil.l.veli...@gmail.com> wrote: > > > >> On 27 April 2016 at 18:46, Bill Spitzak <spit...@gmail.com> wrote: > >> > On Wed, Apr 27, 2016 at 2:03 AM, Pekka Paalanen <ppaala...@gmail.com> > wrote: > >> >> > >> >> On Wed, 27 Apr 2016 09:56:44 +0100 > >> >> Emil Velikov <emil.l.veli...@gmail.com> wrote: > >> >> > >> >> > On 26 April 2016 at 19:12, Bill Spitzak <spit...@gmail.com> wrote: > >> >> > > The old code is comparing pixman_fixed_48_16_t values to > >> >> > > pixman_fixed_16_16_t values, thus it is checking for truncation > of > >> >> > > overflow > >> >> > > values. > >> >> > > > >> >> > Indeed it does. I'll grep more before asking silly questions ;-) > >> >> > > >> >> > > It would probably be better to clamp these overflowed values, > like > >> >> > > pixman_transform_point_31_16 is doing to clamp to the > >> >> > > pixman_fixed_48_16 > >> >> > > result. Right now the result is an odd mix of clamping and > modulus. A > >> >> > > rewrite to go directly to clamped pixman_fixed_16_16 values > would be > >> >> > > even > >> >> > > better. > >> >> > > > >> >> > Sounds like a plan. Sadly I doubt I'll get to it any time soon. > >> >> > >> >> Wasn't the point of the boolean return from these functions to tell > the > >> >> caller to drop what it is doing because it cannot be done properly? > >> > > >> > > >> > Dropping a fill is a lot worse than trying to simulate it using the > clamped > >> > path. It will produce a wrong result if one of the edges connected to > a > >> > clamped point passes through the clip, but this often does not > happen, and > >> > the transition to the wrong result is gradual as the point moves > outside the > >> > clamped region. > >> > > >> > More importantly the caller cannot do anything with the return values > right > >> > now, as they are modulus MAX_16_16+1. Even the direction they are in > is > >> > lost. > >> > > >> I think that keeping the user provided memory as-is when the function > >> does not succeed is a good idea. > >> Afaics currently the contents get overwritten regardless of the result. > >> > >> This is what you guys were on about, right ? Or perhaps you're > >> thinking about spinning v2 of the function with different > >> signature/behaviour ? > > > > Hi Emil, > > > > I think the conclusion was that the comparisons are not useless, and > > this patch should be dropped. You noted it yourself that this patch > > causes a regression in the test suite. > > > Fully agree on both points. Just trying to understand what you and > Bill are talking about and suggest that if one changes things, would > be nice to avoid "feeding garbage" back to the user [on error]. > > Thanks > Emil >
_______________________________________________ Pixman mailing list Pixman@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/pixman