| Message ID | 20221202131658.434114-5-tomi.valkeinen+renesas@ideasonboard.com (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Kieran Bingham |
| Headers | show |
| Series | Support Y210 | expand |
Hi Tomi, Thank you for the patch. On Fri, Dec 02, 2022 at 03:16:58PM +0200, Tomi Valkeinen wrote: > Add support for drawing Y210 pixels. > > Signed-off-by: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com> > --- > kms++util/src/drawing.cpp | 31 +++++++++++++++++++++++++++++++ > 1 file changed, 31 insertions(+) > > diff --git a/kms++util/src/drawing.cpp b/kms++util/src/drawing.cpp > index 79e0d90..7e1b40b 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,32 @@ 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 shift left, similar to 10-bit funcs in class RGB As mentioned in replies to the cover letter, values should be shifted by 6 bits. > + 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; > + > + write16le(y0, &p[0]); > + write16le(cb, &p[1]); > + write16le(y1, &p[2]); > + write16le(cr, &p[3]); If x is odd, won't this swap cb and cr ? draw_yuv422_packed_macropixel() seems to have the same possible issue, so I assume callers always pass an even x value. If so, Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> And if not, draw_yuv422_packed_macropixel() will need to be fixed too, so I'm fine with this patch and an additional fix to both functions on top. > + 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 +284,10 @@ 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: > + 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 03/12/2022 02:01, Laurent Pinchart wrote: > Hi Tomi, > > Thank you for the patch. > > On Fri, Dec 02, 2022 at 03:16:58PM +0200, Tomi Valkeinen wrote: >> Add support for drawing Y210 pixels. >> >> Signed-off-by: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com> >> --- >> kms++util/src/drawing.cpp | 31 +++++++++++++++++++++++++++++++ >> 1 file changed, 31 insertions(+) >> >> diff --git a/kms++util/src/drawing.cpp b/kms++util/src/drawing.cpp >> index 79e0d90..7e1b40b 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,32 @@ 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 shift left, similar to 10-bit funcs in class RGB > > As mentioned in replies to the cover letter, values should be shifted by > 6 bits. > >> + 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; >> + >> + write16le(y0, &p[0]); >> + write16le(cb, &p[1]); >> + write16le(y1, &p[2]); >> + write16le(cr, &p[3]); > > If x is odd, won't this swap cb and cr ? draw_yuv422_packed_macropixel() > seems to have the same possible issue, so I assume callers always pass > an even x value. If so, Yes, a macro pixel always needs to be correctly aligned. With YUYV style macropixels it's even aligned. The test pattern drawing handles this correctly, but I'm not sure if all the funcs available to the users check it correctly. Tomi
diff --git a/kms++util/src/drawing.cpp b/kms++util/src/drawing.cpp index 79e0d90..7e1b40b 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,32 @@ 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 shift left, 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; + + write16le(y0, &p[0]); + write16le(cb, &p[1]); + write16le(y1, &p[2]); + write16le(cr, &p[3]); + 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 +284,10 @@ 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: + 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 pixels. Signed-off-by: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com> --- kms++util/src/drawing.cpp | 31 +++++++++++++++++++++++++++++++ 1 file changed, 31 insertions(+)