| Message ID | 20221205080339.12801-5-tomi.valkeinen+renesas@ideasonboard.com (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Kieran Bingham |
| Headers | show |
| Series | Support Y210, Y212, Y216 | expand |
Hi Tomi, Thank you for the patch. On Mon, Dec 05, 2022 at 10:03:39AM +0200, Tomi Valkeinen wrote: > Add support for drawing Y210, Y212, Y216 pixels. > > Signed-off-by: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com> > --- > kms++util/src/drawing.cpp | 63 +++++++++++++++++++++++++++++++++++++++ > 1 file changed, 63 insertions(+) > > diff --git a/kms++util/src/drawing.cpp b/kms++util/src/drawing.cpp > index 79e0d90..5764b08 100644 > --- a/kms++util/src/drawing.cpp > +++ b/kms++util/src/drawing.cpp > @@ -3,6 +3,7 @@ > > #include <kms++/kms++.h> > #include <kms++util/kms++util.h> > +#include <kms++util/endian.h> > > using namespace std; > > @@ -179,6 +180,62 @@ static void draw_yuv422_packed_macropixel(IFramebuffer& buf, unsigned x, unsigne > } > } > > +static void draw_y2xx_packed_macropixel(IFramebuffer& buf, unsigned x, unsigned y, > + YUV yuv1, YUV yuv2) > +{ > + const uint32_t macro_size = 4; > + uint16_t* p = (uint16_t*)(buf.map(0) + buf.stride(0) * y + x * macro_size); > + > + switch (buf.format()) { > + case PixelFormat::Y210: { > + // XXX naive expansion to 10 bits, similar to 10-bit funcs in class RGB > + uint16_t y0 = yuv1.y << 2; > + uint16_t y1 = yuv2.y << 2; > + uint16_t cb = ((yuv1.u << 2) + (yuv2.u << 2)) / 2; > + uint16_t cr = ((yuv1.v << 2) + (yuv2.v << 2)) / 2; > + > + // The 10 bits occupy the msb, so we shift left by 16-10 = 6 > + write16le(&p[0], y0 << 6); > + write16le(&p[1], cb << 6); > + write16le(&p[2], y1 << 6); > + write16le(&p[3], cr << 6); > + break; > + } > + > + case PixelFormat::Y212: { > + // XXX naive expansion to 12 bits > + uint16_t y0 = yuv1.y << 4; > + uint16_t y1 = yuv2.y << 4; > + uint16_t cb = ((yuv1.u << 4) + (yuv2.u << 4)) / 2; > + uint16_t cr = ((yuv1.v << 4) + (yuv2.v << 4)) / 2; > + > + // The 10 bits occupy the msb, so we shift left by 16-12 = 4 > + write16le(&p[0], y0 << 4); > + write16le(&p[1], cb << 4); > + write16le(&p[2], y1 << 4); > + write16le(&p[3], cr << 4); > + break; > + } > + > + case PixelFormat::Y216: { > + // XXX naive expansion to 16 bits > + uint16_t y0 = yuv1.y << 8; > + uint16_t y1 = yuv2.y << 8; > + uint16_t cb = ((yuv1.u << 8) + (yuv2.u << 8)) / 8; > + uint16_t cr = ((yuv1.v << 8) + (yuv2.v << 8)) / 8; > + > + write16le(&p[0], y0); > + write16le(&p[1], cb); > + write16le(&p[2], y1); > + write16le(&p[3], cr); > + break; These three cases end up all shifting left by 8 bits. It looks like you could simplify the code by merging all implementations into one. Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > + } > + > + default: > + throw std::invalid_argument("invalid pixelformat"); > + } > +} > + > static void draw_yuv422_semiplanar_macropixel(IFramebuffer& buf, unsigned x, unsigned y, > YUV yuv1, YUV yuv2) > { > @@ -257,6 +314,12 @@ void draw_yuv422_macropixel(IFramebuffer& buf, unsigned x, unsigned y, YUV yuv1, > draw_yuv422_packed_macropixel(buf, x, y, yuv1, yuv2); > break; > > + case PixelFormat::Y210: > + case PixelFormat::Y212: > + case PixelFormat::Y216: > + draw_y2xx_packed_macropixel(buf, x, y, yuv1, yuv2); > + break; > + > case PixelFormat::NV16: > case PixelFormat::NV61: > draw_yuv422_semiplanar_macropixel(buf, x, y, yuv1, yuv2);
On 05/12/2022 10:17, Laurent Pinchart wrote: > Hi Tomi, > > Thank you for the patch. > > On Mon, Dec 05, 2022 at 10:03:39AM +0200, Tomi Valkeinen wrote: >> Add support for drawing Y210, Y212, Y216 pixels. >> >> Signed-off-by: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com> >> --- >> kms++util/src/drawing.cpp | 63 +++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 63 insertions(+) >> >> diff --git a/kms++util/src/drawing.cpp b/kms++util/src/drawing.cpp >> index 79e0d90..5764b08 100644 >> --- a/kms++util/src/drawing.cpp >> +++ b/kms++util/src/drawing.cpp >> @@ -3,6 +3,7 @@ >> >> #include <kms++/kms++.h> >> #include <kms++util/kms++util.h> >> +#include <kms++util/endian.h> >> >> using namespace std; >> >> @@ -179,6 +180,62 @@ static void draw_yuv422_packed_macropixel(IFramebuffer& buf, unsigned x, unsigne >> } >> } >> >> +static void draw_y2xx_packed_macropixel(IFramebuffer& buf, unsigned x, unsigned y, >> + YUV yuv1, YUV yuv2) >> +{ >> + const uint32_t macro_size = 4; >> + uint16_t* p = (uint16_t*)(buf.map(0) + buf.stride(0) * y + x * macro_size); >> + >> + switch (buf.format()) { >> + case PixelFormat::Y210: { >> + // XXX naive expansion to 10 bits, similar to 10-bit funcs in class RGB >> + uint16_t y0 = yuv1.y << 2; >> + uint16_t y1 = yuv2.y << 2; >> + uint16_t cb = ((yuv1.u << 2) + (yuv2.u << 2)) / 2; >> + uint16_t cr = ((yuv1.v << 2) + (yuv2.v << 2)) / 2; >> + >> + // The 10 bits occupy the msb, so we shift left by 16-10 = 6 >> + write16le(&p[0], y0 << 6); >> + write16le(&p[1], cb << 6); >> + write16le(&p[2], y1 << 6); >> + write16le(&p[3], cr << 6); >> + break; >> + } >> + >> + case PixelFormat::Y212: { >> + // XXX naive expansion to 12 bits >> + uint16_t y0 = yuv1.y << 4; >> + uint16_t y1 = yuv2.y << 4; >> + uint16_t cb = ((yuv1.u << 4) + (yuv2.u << 4)) / 2; >> + uint16_t cr = ((yuv1.v << 4) + (yuv2.v << 4)) / 2; >> + >> + // The 10 bits occupy the msb, so we shift left by 16-12 = 4 >> + write16le(&p[0], y0 << 4); >> + write16le(&p[1], cb << 4); >> + write16le(&p[2], y1 << 4); >> + write16le(&p[3], cr << 4); >> + break; >> + } >> + >> + case PixelFormat::Y216: { >> + // XXX naive expansion to 16 bits >> + uint16_t y0 = yuv1.y << 8; >> + uint16_t y1 = yuv2.y << 8; >> + uint16_t cb = ((yuv1.u << 8) + (yuv2.u << 8)) / 8; >> + uint16_t cr = ((yuv1.v << 8) + (yuv2.v << 8)) / 8; >> + >> + write16le(&p[0], y0); >> + write16le(&p[1], cb); >> + write16le(&p[2], y1); >> + write16le(&p[3], cr); >> + break; > > These three cases end up all shifting left by 8 bits. It looks like you > could simplify the code by merging all implementations into one. That's true. I'd like to expand the RGB and YUV classes to contain 16-bits per component pixels. Then this code will change a bit, as instead of shifting we'll need to zero-out the lsbs in Y210 and Y212. Tomi
Hi Tomi, On Mon, Dec 5, 2022 at 9:07 AM Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com> wrote: > Add support for drawing Y210, Y212, Y216 pixels. > > Signed-off-by: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com> Thanks for your patch! > --- a/kms++util/src/drawing.cpp > +++ b/kms++util/src/drawing.cpp > @@ -179,6 +180,62 @@ static void draw_yuv422_packed_macropixel(IFramebuffer& buf, unsigned x, unsigne > } > } > > +static void draw_y2xx_packed_macropixel(IFramebuffer& buf, unsigned x, unsigned y, > + YUV yuv1, YUV yuv2) > +{ > + const uint32_t macro_size = 4; > + uint16_t* p = (uint16_t*)(buf.map(0) + buf.stride(0) * y + x * macro_size); > + > + switch (buf.format()) { > + case PixelFormat::Y210: { > + // XXX naive expansion to 10 bits, similar to 10-bit funcs in class RGB > + uint16_t y0 = yuv1.y << 2; "yuv1.y << 2 | yuv1.y >> 6" etc... > + uint16_t y1 = yuv2.y << 2; > + uint16_t cb = ((yuv1.u << 2) + (yuv2.u << 2)) / 2; > + uint16_t cr = ((yuv1.v << 2) + (yuv2.v << 2)) / 2; > + > + // The 10 bits occupy the msb, so we shift left by 16-10 = 6 > + write16le(&p[0], y0 << 6); > + write16le(&p[1], cb << 6); > + write16le(&p[2], y1 << 6); > + write16le(&p[3], cr << 6); > + break; > + } > + > + case PixelFormat::Y212: { > + // XXX naive expansion to 12 bits > + uint16_t y0 = yuv1.y << 4; "yuv1.y << 4 | yuv1.y >> 4" etc. > + uint16_t y1 = yuv2.y << 4; > + uint16_t cb = ((yuv1.u << 4) + (yuv2.u << 4)) / 2; > + uint16_t cr = ((yuv1.v << 4) + (yuv2.v << 4)) / 2; > + > + // The 10 bits occupy the msb, so we shift left by 16-12 = 4 > + write16le(&p[0], y0 << 4); > + write16le(&p[1], cb << 4); > + write16le(&p[2], y1 << 4); > + write16le(&p[3], cr << 4); > + break; > + } > + > + case PixelFormat::Y216: { > + // XXX naive expansion to 16 bits > + uint16_t y0 = yuv1.y << 8; "yuv1.y << 8 | yuv1.y" etc. > + uint16_t y1 = yuv2.y << 8; > + uint16_t cb = ((yuv1.u << 8) + (yuv2.u << 8)) / 8; Why divide by 8 instead of 2? > + uint16_t cr = ((yuv1.v << 8) + (yuv2.v << 8)) / 8; > + > + write16le(&p[0], y0); > + write16le(&p[1], cb); > + write16le(&p[2], y1); > + write16le(&p[3], cr); > + break; > + } > + > + default: > + throw std::invalid_argument("invalid pixelformat"); > + } > +} > + Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
Hi, On 05/12/2022 10:34, Geert Uytterhoeven wrote: > Hi Tomi, > > On Mon, Dec 5, 2022 at 9:07 AM Tomi Valkeinen > <tomi.valkeinen+renesas@ideasonboard.com> wrote: >> Add support for drawing Y210, Y212, Y216 pixels. >> >> Signed-off-by: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com> > > Thanks for your patch! > >> --- a/kms++util/src/drawing.cpp >> +++ b/kms++util/src/drawing.cpp > >> @@ -179,6 +180,62 @@ static void draw_yuv422_packed_macropixel(IFramebuffer& buf, unsigned x, unsigne >> } >> } >> >> +static void draw_y2xx_packed_macropixel(IFramebuffer& buf, unsigned x, unsigned y, >> + YUV yuv1, YUV yuv2) >> +{ >> + const uint32_t macro_size = 4; >> + uint16_t* p = (uint16_t*)(buf.map(0) + buf.stride(0) * y + x * macro_size); >> + >> + switch (buf.format()) { >> + case PixelFormat::Y210: { >> + // XXX naive expansion to 10 bits, similar to 10-bit funcs in class RGB >> + uint16_t y0 = yuv1.y << 2; > > "yuv1.y << 2 | yuv1.y >> 6" etc... Yes, the current code leaves the lsbs zero. That's done in a few other places too. I want to do the proper expansion in some separate class, or rather, I want the RGB and YUV classes to contain 16-bit components so that we'll just downshift instead of expand. But I have not gotten there yet. It's been on my todo-list only for years now... >> + uint16_t y1 = yuv2.y << 2; >> + uint16_t cb = ((yuv1.u << 2) + (yuv2.u << 2)) / 2; >> + uint16_t cr = ((yuv1.v << 2) + (yuv2.v << 2)) / 2; >> + >> + // The 10 bits occupy the msb, so we shift left by 16-10 = 6 >> + write16le(&p[0], y0 << 6); >> + write16le(&p[1], cb << 6); >> + write16le(&p[2], y1 << 6); >> + write16le(&p[3], cr << 6); >> + break; >> + } >> + >> + case PixelFormat::Y212: { >> + // XXX naive expansion to 12 bits >> + uint16_t y0 = yuv1.y << 4; > > "yuv1.y << 4 | yuv1.y >> 4" etc. > >> + uint16_t y1 = yuv2.y << 4; >> + uint16_t cb = ((yuv1.u << 4) + (yuv2.u << 4)) / 2; >> + uint16_t cr = ((yuv1.v << 4) + (yuv2.v << 4)) / 2; >> + >> + // The 10 bits occupy the msb, so we shift left by 16-12 = 4 >> + write16le(&p[0], y0 << 4); >> + write16le(&p[1], cb << 4); >> + write16le(&p[2], y1 << 4); >> + write16le(&p[3], cr << 4); >> + break; >> + } >> + >> + case PixelFormat::Y216: { >> + // XXX naive expansion to 16 bits >> + uint16_t y0 = yuv1.y << 8; > > "yuv1.y << 8 | yuv1.y" etc. > > >> + uint16_t y1 = yuv2.y << 8; >> + uint16_t cb = ((yuv1.u << 8) + (yuv2.u << 8)) / 8; > > Why divide by 8 instead of 2? Oops, good that someone is awake! That was a mistake. Tomi
diff --git a/kms++util/src/drawing.cpp b/kms++util/src/drawing.cpp index 79e0d90..5764b08 100644 --- a/kms++util/src/drawing.cpp +++ b/kms++util/src/drawing.cpp @@ -3,6 +3,7 @@ #include <kms++/kms++.h> #include <kms++util/kms++util.h> +#include <kms++util/endian.h> using namespace std; @@ -179,6 +180,62 @@ static void draw_yuv422_packed_macropixel(IFramebuffer& buf, unsigned x, unsigne } } +static void draw_y2xx_packed_macropixel(IFramebuffer& buf, unsigned x, unsigned y, + YUV yuv1, YUV yuv2) +{ + const uint32_t macro_size = 4; + uint16_t* p = (uint16_t*)(buf.map(0) + buf.stride(0) * y + x * macro_size); + + switch (buf.format()) { + case PixelFormat::Y210: { + // XXX naive expansion to 10 bits, similar to 10-bit funcs in class RGB + uint16_t y0 = yuv1.y << 2; + uint16_t y1 = yuv2.y << 2; + uint16_t cb = ((yuv1.u << 2) + (yuv2.u << 2)) / 2; + uint16_t cr = ((yuv1.v << 2) + (yuv2.v << 2)) / 2; + + // The 10 bits occupy the msb, so we shift left by 16-10 = 6 + write16le(&p[0], y0 << 6); + write16le(&p[1], cb << 6); + write16le(&p[2], y1 << 6); + write16le(&p[3], cr << 6); + break; + } + + case PixelFormat::Y212: { + // XXX naive expansion to 12 bits + uint16_t y0 = yuv1.y << 4; + uint16_t y1 = yuv2.y << 4; + uint16_t cb = ((yuv1.u << 4) + (yuv2.u << 4)) / 2; + uint16_t cr = ((yuv1.v << 4) + (yuv2.v << 4)) / 2; + + // The 10 bits occupy the msb, so we shift left by 16-12 = 4 + write16le(&p[0], y0 << 4); + write16le(&p[1], cb << 4); + write16le(&p[2], y1 << 4); + write16le(&p[3], cr << 4); + break; + } + + case PixelFormat::Y216: { + // XXX naive expansion to 16 bits + uint16_t y0 = yuv1.y << 8; + uint16_t y1 = yuv2.y << 8; + uint16_t cb = ((yuv1.u << 8) + (yuv2.u << 8)) / 8; + uint16_t cr = ((yuv1.v << 8) + (yuv2.v << 8)) / 8; + + write16le(&p[0], y0); + write16le(&p[1], cb); + write16le(&p[2], y1); + write16le(&p[3], cr); + break; + } + + default: + throw std::invalid_argument("invalid pixelformat"); + } +} + static void draw_yuv422_semiplanar_macropixel(IFramebuffer& buf, unsigned x, unsigned y, YUV yuv1, YUV yuv2) { @@ -257,6 +314,12 @@ void draw_yuv422_macropixel(IFramebuffer& buf, unsigned x, unsigned y, YUV yuv1, draw_yuv422_packed_macropixel(buf, x, y, yuv1, yuv2); break; + case PixelFormat::Y210: + case PixelFormat::Y212: + case PixelFormat::Y216: + draw_y2xx_packed_macropixel(buf, x, y, yuv1, yuv2); + break; + case PixelFormat::NV16: case PixelFormat::NV61: draw_yuv422_semiplanar_macropixel(buf, x, y, yuv1, yuv2);
Add support for drawing Y210, Y212, Y216 pixels. Signed-off-by: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com> --- kms++util/src/drawing.cpp | 63 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 63 insertions(+)