| Message ID | 20171123185928.6303-1-imirkin@alum.mit.edu (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Nov 23, 2017 at 01:59:28PM -0500, Ilia Mirkin wrote: > We need to shift the values up, otherwise we'd end up with a negative > shift. This works for up-to 16-bit components, which is fine for now. Shouldn't we actually replicate the high bits in the low bits? > > Signed-off-by: Ilia Mirkin <imirkin@alum.mit.edu> > --- > tests/util/pattern.c | 14 ++++++++++---- > 1 file changed, 10 insertions(+), 4 deletions(-) > > diff --git a/tests/util/pattern.c b/tests/util/pattern.c > index 41fb541b..2f9bb384 100644 > --- a/tests/util/pattern.c > +++ b/tests/util/pattern.c > @@ -64,11 +64,17 @@ struct color_yuv { > .u = MAKE_YUV_601_U(r, g, b), \ > .v = MAKE_YUV_601_V(r, g, b) } > > +static inline uint32_t shiftcolor(const struct util_color_component *comp, > + uint32_t value) > +{ > + return ((value << 8) >> (16 - comp->length)) << comp->offset; > +} > + > #define MAKE_RGBA(rgb, r, g, b, a) \ > - ((((r) >> (8 - (rgb)->red.length)) << (rgb)->red.offset) | \ > - (((g) >> (8 - (rgb)->green.length)) << (rgb)->green.offset) | \ > - (((b) >> (8 - (rgb)->blue.length)) << (rgb)->blue.offset) | \ > - (((a) >> (8 - (rgb)->alpha.length)) << (rgb)->alpha.offset)) > + (shiftcolor(&(rgb)->red, (r)) | \ > + shiftcolor(&(rgb)->green, (g)) | \ > + shiftcolor(&(rgb)->blue, (b)) | \ > + shiftcolor(&(rgb)->alpha, (a))) > > #define MAKE_RGB24(rgb, r, g, b) \ > { .value = MAKE_RGBA(rgb, r, g, b, 0) } > -- > 2.13.6 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
On Thu, Nov 23, 2017 at 2:09 PM, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > On Thu, Nov 23, 2017 at 01:59:28PM -0500, Ilia Mirkin wrote: >> We need to shift the values up, otherwise we'd end up with a negative >> shift. This works for up-to 16-bit components, which is fine for now. > > Shouldn't we actually replicate the high bits in the low bits? Not entirely sure what you're proposing... Ideally we wouldn't be lazy and pass e.g. 16-bit values to MAKE_RGBA which would then shift down as necessary (and even there, you could end up with off-by-1's maybe?). For e.g. 0xff, that should become 0x3ff but with my code will become 0x3fc. But for other values, it's less clear what to do with the low bits. I figured it didn't really matter. Do you have a concrete proposal? -ilia
On Thu, Nov 23, 2017 at 03:14:46PM -0500, Ilia Mirkin wrote: > On Thu, Nov 23, 2017 at 2:09 PM, Ville Syrjälä > <ville.syrjala@linux.intel.com> wrote: > > On Thu, Nov 23, 2017 at 01:59:28PM -0500, Ilia Mirkin wrote: > >> We need to shift the values up, otherwise we'd end up with a negative > >> shift. This works for up-to 16-bit components, which is fine for now. > > > > Shouldn't we actually replicate the high bits in the low bits? > > Not entirely sure what you're proposing... > > Ideally we wouldn't be lazy and pass e.g. 16-bit values to MAKE_RGBA > which would then shift down as necessary (and even there, you could > end up with off-by-1's maybe?). For e.g. 0xff, that should become > 0x3ff but with my code will become 0x3fc. Exactly the issue. > But for other values, it's > less clear what to do with the low bits. I figured it didn't really > matter. > > Do you have a concrete proposal? The usual thing would be just (value) * 0x3ff / 0xff or ((value) << 2) | ((value) >> 6)
On Fri, Nov 24, 2017 at 8:38 AM, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > On Thu, Nov 23, 2017 at 03:14:46PM -0500, Ilia Mirkin wrote: >> On Thu, Nov 23, 2017 at 2:09 PM, Ville Syrjälä >> <ville.syrjala@linux.intel.com> wrote: >> > On Thu, Nov 23, 2017 at 01:59:28PM -0500, Ilia Mirkin wrote: >> >> We need to shift the values up, otherwise we'd end up with a negative >> >> shift. This works for up-to 16-bit components, which is fine for now. >> > >> > Shouldn't we actually replicate the high bits in the low bits? >> >> Not entirely sure what you're proposing... >> >> Ideally we wouldn't be lazy and pass e.g. 16-bit values to MAKE_RGBA >> which would then shift down as necessary (and even there, you could >> end up with off-by-1's maybe?). For e.g. 0xff, that should become >> 0x3ff but with my code will become 0x3fc. > > Exactly the issue. > >> But for other values, it's >> less clear what to do with the low bits. I figured it didn't really >> matter. >> >> Do you have a concrete proposal? > > The usual thing would be just > > (value) * 0x3ff / 0xff > > or > > ((value) << 2) | ((value) >> 6) The latter method is interesting, it slowly biases from rounding down to rounding up as you go through the range. Clever. I guess I could change my function to use (value << 8) | value and then use that to shift around. I'll send a v2. -ilia
diff --git a/tests/util/pattern.c b/tests/util/pattern.c index 41fb541b..2f9bb384 100644 --- a/tests/util/pattern.c +++ b/tests/util/pattern.c @@ -64,11 +64,17 @@ struct color_yuv { .u = MAKE_YUV_601_U(r, g, b), \ .v = MAKE_YUV_601_V(r, g, b) } +static inline uint32_t shiftcolor(const struct util_color_component *comp, + uint32_t value) +{ + return ((value << 8) >> (16 - comp->length)) << comp->offset; +} + #define MAKE_RGBA(rgb, r, g, b, a) \ - ((((r) >> (8 - (rgb)->red.length)) << (rgb)->red.offset) | \ - (((g) >> (8 - (rgb)->green.length)) << (rgb)->green.offset) | \ - (((b) >> (8 - (rgb)->blue.length)) << (rgb)->blue.offset) | \ - (((a) >> (8 - (rgb)->alpha.length)) << (rgb)->alpha.offset)) + (shiftcolor(&(rgb)->red, (r)) | \ + shiftcolor(&(rgb)->green, (g)) | \ + shiftcolor(&(rgb)->blue, (b)) | \ + shiftcolor(&(rgb)->alpha, (a))) #define MAKE_RGB24(rgb, r, g, b) \ { .value = MAKE_RGBA(rgb, r, g, b, 0) }
We need to shift the values up, otherwise we'd end up with a negative shift. This works for up-to 16-bit components, which is fine for now. Signed-off-by: Ilia Mirkin <imirkin@alum.mit.edu> --- tests/util/pattern.c | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-)