| Message ID | 20200617164442.2643-1-david.plowman@raspberrypi.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi David, On Wed, Jun 17, 2020 at 05:44:42PM +0100, David Plowman wrote: > These formats can be helpful when downstream applications or libraries > support them natively (avoiding a costly conversion). But only if cameras can provide them :-) Does the Raspberry Pi ISP support both V4L2_PIX_FMT_YUV420 and V4L2_PIX_FMT_YUV422P ? I see mentions of the former in the driver, but not of the latter. > Signed-off-by: David Plowman <david.plowman@raspberrypi.com> > --- > src/libcamera/formats.cpp | 14 ++++++++++++++ > src/libcamera/v4l2_pixelformat.cpp | 2 ++ > src/v4l2/v4l2_camera_proxy.cpp | 2 ++ > 3 files changed, 18 insertions(+) > > diff --git a/src/libcamera/formats.cpp b/src/libcamera/formats.cpp > index 2ac3b41..dcd1dcf 100644 > --- a/src/libcamera/formats.cpp > +++ b/src/libcamera/formats.cpp > @@ -268,6 +268,20 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{ > .colourEncoding = PixelFormatInfo::ColourEncodingYUV, > .packed = false, > } }, > + { PixelFormat(DRM_FORMAT_YUV422), { > + .format = PixelFormat(DRM_FORMAT_YUV422), > + .v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_YUV422P), > + .bitsPerPixel = 16, > + .colourEncoding = PixelFormatInfo::ColourEncodingYUV, > + .packed = false, > + } }, > + { PixelFormat(DRM_FORMAT_YUV420), { > + .format = PixelFormat(DRM_FORMAT_YUV420), > + .v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_YUV420), > + .bitsPerPixel = 12, > + .colourEncoding = PixelFormatInfo::ColourEncodingYUV, > + .packed = false, > + } }, > > /* Greyscale formats. */ > { PixelFormat(DRM_FORMAT_R8), { > diff --git a/src/libcamera/v4l2_pixelformat.cpp b/src/libcamera/v4l2_pixelformat.cpp > index 94fae47..01778c0 100644 > --- a/src/libcamera/v4l2_pixelformat.cpp > +++ b/src/libcamera/v4l2_pixelformat.cpp > @@ -64,6 +64,8 @@ const std::map<V4L2PixelFormat, PixelFormat> vpf2pf{ > { V4L2PixelFormat(V4L2_PIX_FMT_NV61), PixelFormat(DRM_FORMAT_NV61) }, > { V4L2PixelFormat(V4L2_PIX_FMT_NV12), PixelFormat(DRM_FORMAT_NV12) }, > { V4L2PixelFormat(V4L2_PIX_FMT_NV21), PixelFormat(DRM_FORMAT_NV21) }, > + { V4L2PixelFormat(V4L2_PIX_FMT_YUV422P), PixelFormat(DRM_FORMAT_YUV422) }, > + { V4L2PixelFormat(V4L2_PIX_FMT_YUV420), PixelFormat(DRM_FORMAT_YUV420) }, > > /* Greyscale formats. */ > { V4L2PixelFormat(V4L2_PIX_FMT_GREY), PixelFormat(DRM_FORMAT_R8) }, > diff --git a/src/v4l2/v4l2_camera_proxy.cpp b/src/v4l2/v4l2_camera_proxy.cpp > index d7f14e6..a54d47e 100644 > --- a/src/v4l2/v4l2_camera_proxy.cpp > +++ b/src/v4l2/v4l2_camera_proxy.cpp > @@ -576,6 +576,8 @@ static const std::array<PixelFormatInfo, 14> pixelFormatInfo = {{ > { PixelFormat(DRM_FORMAT_NV61), V4L2_PIX_FMT_NV61, 2, {{ { 8, 1, 1 }, { 16, 2, 1 }, { 0, 0, 0 } }} }, > { PixelFormat(DRM_FORMAT_NV24), V4L2_PIX_FMT_NV24, 2, {{ { 8, 1, 1 }, { 16, 1, 1 }, { 0, 0, 0 } }} }, > { PixelFormat(DRM_FORMAT_NV42), V4L2_PIX_FMT_NV42, 2, {{ { 8, 1, 1 }, { 16, 1, 1 }, { 0, 0, 0 } }} }, > + { PixelFormat(DRM_FORMAT_YUV422), V4L2_PIX_FMT_YUV422P, 2, {{ { 8, 1, 1 }, { 8, 2, 1 }, { 8, 2, 1 } }} }, > + { PixelFormat(DRM_FORMAT_YUV420), V4L2_PIX_FMT_YUV420, 2, {{ { 8, 1, 1 }, { 8, 2, 2 }, { 8, 2, 2 } }} }, > /* Compressed formats. */ > /* > * \todo Get a better image size estimate for MJPEG, via
Hi David, On Thu, Jun 18, 2020 at 01:18:21AM +0300, Laurent Pinchart wrote: > On Wed, Jun 17, 2020 at 05:44:42PM +0100, David Plowman wrote: > > These formats can be helpful when downstream applications or libraries > > support them natively (avoiding a costly conversion). > > But only if cameras can provide them :-) > > Does the Raspberry Pi ISP support both V4L2_PIX_FMT_YUV420 and > V4L2_PIX_FMT_YUV422P ? I see mentions of the former in the driver, but > not of the latter. > > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com> > > --- > > src/libcamera/formats.cpp | 14 ++++++++++++++ > > src/libcamera/v4l2_pixelformat.cpp | 2 ++ > > src/v4l2/v4l2_camera_proxy.cpp | 2 ++ > > 3 files changed, 18 insertions(+) > > > > diff --git a/src/libcamera/formats.cpp b/src/libcamera/formats.cpp > > index 2ac3b41..dcd1dcf 100644 > > --- a/src/libcamera/formats.cpp > > +++ b/src/libcamera/formats.cpp > > @@ -268,6 +268,20 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{ > > .colourEncoding = PixelFormatInfo::ColourEncodingYUV, > > .packed = false, > > } }, > > + { PixelFormat(DRM_FORMAT_YUV422), { > > + .format = PixelFormat(DRM_FORMAT_YUV422), > > + .v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_YUV422P), > > + .bitsPerPixel = 16, > > + .colourEncoding = PixelFormatInfo::ColourEncodingYUV, > > + .packed = false, > > + } }, > > + { PixelFormat(DRM_FORMAT_YUV420), { > > + .format = PixelFormat(DRM_FORMAT_YUV420), > > + .v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_YUV420), > > + .bitsPerPixel = 12, > > + .colourEncoding = PixelFormatInfo::ColourEncodingYUV, > > + .packed = false, > > + } }, I may merge in the morning a patch series that rework format handling and would conflict with this patch. If that's the case, I'll rebase this patch and send the rebased version to the list. > > > > /* Greyscale formats. */ > > { PixelFormat(DRM_FORMAT_R8), { > > diff --git a/src/libcamera/v4l2_pixelformat.cpp b/src/libcamera/v4l2_pixelformat.cpp > > index 94fae47..01778c0 100644 > > --- a/src/libcamera/v4l2_pixelformat.cpp > > +++ b/src/libcamera/v4l2_pixelformat.cpp > > @@ -64,6 +64,8 @@ const std::map<V4L2PixelFormat, PixelFormat> vpf2pf{ > > { V4L2PixelFormat(V4L2_PIX_FMT_NV61), PixelFormat(DRM_FORMAT_NV61) }, > > { V4L2PixelFormat(V4L2_PIX_FMT_NV12), PixelFormat(DRM_FORMAT_NV12) }, > > { V4L2PixelFormat(V4L2_PIX_FMT_NV21), PixelFormat(DRM_FORMAT_NV21) }, > > + { V4L2PixelFormat(V4L2_PIX_FMT_YUV422P), PixelFormat(DRM_FORMAT_YUV422) }, > > + { V4L2PixelFormat(V4L2_PIX_FMT_YUV420), PixelFormat(DRM_FORMAT_YUV420) }, > > > > /* Greyscale formats. */ > > { V4L2PixelFormat(V4L2_PIX_FMT_GREY), PixelFormat(DRM_FORMAT_R8) }, > > diff --git a/src/v4l2/v4l2_camera_proxy.cpp b/src/v4l2/v4l2_camera_proxy.cpp > > index d7f14e6..a54d47e 100644 > > --- a/src/v4l2/v4l2_camera_proxy.cpp > > +++ b/src/v4l2/v4l2_camera_proxy.cpp > > @@ -576,6 +576,8 @@ static const std::array<PixelFormatInfo, 14> pixelFormatInfo = {{ > > { PixelFormat(DRM_FORMAT_NV61), V4L2_PIX_FMT_NV61, 2, {{ { 8, 1, 1 }, { 16, 2, 1 }, { 0, 0, 0 } }} }, > > { PixelFormat(DRM_FORMAT_NV24), V4L2_PIX_FMT_NV24, 2, {{ { 8, 1, 1 }, { 16, 1, 1 }, { 0, 0, 0 } }} }, > > { PixelFormat(DRM_FORMAT_NV42), V4L2_PIX_FMT_NV42, 2, {{ { 8, 1, 1 }, { 16, 1, 1 }, { 0, 0, 0 } }} }, > > + { PixelFormat(DRM_FORMAT_YUV422), V4L2_PIX_FMT_YUV422P, 2, {{ { 8, 1, 1 }, { 8, 2, 1 }, { 8, 2, 1 } }} }, > > + { PixelFormat(DRM_FORMAT_YUV420), V4L2_PIX_FMT_YUV420, 2, {{ { 8, 1, 1 }, { 8, 2, 2 }, { 8, 2, 2 } }} }, Additionally, don't those formats have 3 planes, not two ? > > /* Compressed formats. */ > > /* > > * \todo Get a better image size estimate for MJPEG, via
Hi Laurent Thanks for the review and the correction. Just to add some background, I've found that libjpeg works _way_ faster if you can feed it directly with planar YUV420 (what it calls "raw" data), hence the motivation for this patch. I added YUV422 because, although we don't support it yet, it's common for folks to prefer 422 rather than 420 JPEGs, so I expect we'll need to do it at some point. I'm happy to re-submit the patch if you like, though it sounds like the work you're doing means this isn't worth the trouble. Just let me know! Thanks again and best regards David On Thu, 18 Jun 2020 at 01:46, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi David, > > On Thu, Jun 18, 2020 at 01:18:21AM +0300, Laurent Pinchart wrote: > > On Wed, Jun 17, 2020 at 05:44:42PM +0100, David Plowman wrote: > > > These formats can be helpful when downstream applications or libraries > > > support them natively (avoiding a costly conversion). > > > > But only if cameras can provide them :-) > > > > Does the Raspberry Pi ISP support both V4L2_PIX_FMT_YUV420 and > > V4L2_PIX_FMT_YUV422P ? I see mentions of the former in the driver, but > > not of the latter. > > > > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com> > > > --- > > > src/libcamera/formats.cpp | 14 ++++++++++++++ > > > src/libcamera/v4l2_pixelformat.cpp | 2 ++ > > > src/v4l2/v4l2_camera_proxy.cpp | 2 ++ > > > 3 files changed, 18 insertions(+) > > > > > > diff --git a/src/libcamera/formats.cpp b/src/libcamera/formats.cpp > > > index 2ac3b41..dcd1dcf 100644 > > > --- a/src/libcamera/formats.cpp > > > +++ b/src/libcamera/formats.cpp > > > @@ -268,6 +268,20 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{ > > > .colourEncoding = PixelFormatInfo::ColourEncodingYUV, > > > .packed = false, > > > } }, > > > + { PixelFormat(DRM_FORMAT_YUV422), { > > > + .format = PixelFormat(DRM_FORMAT_YUV422), > > > + .v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_YUV422P), > > > + .bitsPerPixel = 16, > > > + .colourEncoding = PixelFormatInfo::ColourEncodingYUV, > > > + .packed = false, > > > + } }, > > > + { PixelFormat(DRM_FORMAT_YUV420), { > > > + .format = PixelFormat(DRM_FORMAT_YUV420), > > > + .v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_YUV420), > > > + .bitsPerPixel = 12, > > > + .colourEncoding = PixelFormatInfo::ColourEncodingYUV, > > > + .packed = false, > > > + } }, > > I may merge in the morning a patch series that rework format handling > and would conflict with this patch. If that's the case, I'll rebase this > patch and send the rebased version to the list. > > > > > > > /* Greyscale formats. */ > > > { PixelFormat(DRM_FORMAT_R8), { > > > diff --git a/src/libcamera/v4l2_pixelformat.cpp b/src/libcamera/v4l2_pixelformat.cpp > > > index 94fae47..01778c0 100644 > > > --- a/src/libcamera/v4l2_pixelformat.cpp > > > +++ b/src/libcamera/v4l2_pixelformat.cpp > > > @@ -64,6 +64,8 @@ const std::map<V4L2PixelFormat, PixelFormat> vpf2pf{ > > > { V4L2PixelFormat(V4L2_PIX_FMT_NV61), PixelFormat(DRM_FORMAT_NV61) }, > > > { V4L2PixelFormat(V4L2_PIX_FMT_NV12), PixelFormat(DRM_FORMAT_NV12) }, > > > { V4L2PixelFormat(V4L2_PIX_FMT_NV21), PixelFormat(DRM_FORMAT_NV21) }, > > > + { V4L2PixelFormat(V4L2_PIX_FMT_YUV422P), PixelFormat(DRM_FORMAT_YUV422) }, > > > + { V4L2PixelFormat(V4L2_PIX_FMT_YUV420), PixelFormat(DRM_FORMAT_YUV420) }, > > > > > > /* Greyscale formats. */ > > > { V4L2PixelFormat(V4L2_PIX_FMT_GREY), PixelFormat(DRM_FORMAT_R8) }, > > > diff --git a/src/v4l2/v4l2_camera_proxy.cpp b/src/v4l2/v4l2_camera_proxy.cpp > > > index d7f14e6..a54d47e 100644 > > > --- a/src/v4l2/v4l2_camera_proxy.cpp > > > +++ b/src/v4l2/v4l2_camera_proxy.cpp > > > @@ -576,6 +576,8 @@ static const std::array<PixelFormatInfo, 14> pixelFormatInfo = {{ > > > { PixelFormat(DRM_FORMAT_NV61), V4L2_PIX_FMT_NV61, 2, {{ { 8, 1, 1 }, { 16, 2, 1 }, { 0, 0, 0 } }} }, > > > { PixelFormat(DRM_FORMAT_NV24), V4L2_PIX_FMT_NV24, 2, {{ { 8, 1, 1 }, { 16, 1, 1 }, { 0, 0, 0 } }} }, > > > { PixelFormat(DRM_FORMAT_NV42), V4L2_PIX_FMT_NV42, 2, {{ { 8, 1, 1 }, { 16, 1, 1 }, { 0, 0, 0 } }} }, > > > + { PixelFormat(DRM_FORMAT_YUV422), V4L2_PIX_FMT_YUV422P, 2, {{ { 8, 1, 1 }, { 8, 2, 1 }, { 8, 2, 1 } }} }, > > > + { PixelFormat(DRM_FORMAT_YUV420), V4L2_PIX_FMT_YUV420, 2, {{ { 8, 1, 1 }, { 8, 2, 2 }, { 8, 2, 2 } }} }, > > Additionally, don't those formats have 3 planes, not two ? You are quite right, my mistake. It seems to work both ways...! > > > > /* Compressed formats. */ > > > /* > > > * \todo Get a better image size estimate for MJPEG, via > > -- > Regards, > > Laurent Pinchart
Hi David, On Thu, Jun 18, 2020 at 09:19:55AM +0100, David Plowman wrote: > Hi Laurent > > Thanks for the review and the correction. > > Just to add some background, I've found that libjpeg works _way_ > faster if you can feed > it directly with planar YUV420 (what it calls "raw" data), hence the > motivation for this > patch. I added YUV422 because, although we don't support it yet, it's > common for folks > to prefer 422 rather than 420 JPEGs, so I expect we'll need to do it > at some point. Does your ISP hardware support YUV422 ? > I'm happy to re-submit the patch if you like, though it sounds like > the work you're doing > means this isn't worth the trouble. Just let me know! I've pushed the format rework, and will send the rebased version of your patch shortly. > On Thu, 18 Jun 2020 at 01:46, Laurent Pinchart wrote: > > On Thu, Jun 18, 2020 at 01:18:21AM +0300, Laurent Pinchart wrote: > > > On Wed, Jun 17, 2020 at 05:44:42PM +0100, David Plowman wrote: > > > > These formats can be helpful when downstream applications or libraries > > > > support them natively (avoiding a costly conversion). > > > > > > But only if cameras can provide them :-) > > > > > > Does the Raspberry Pi ISP support both V4L2_PIX_FMT_YUV420 and > > > V4L2_PIX_FMT_YUV422P ? I see mentions of the former in the driver, but > > > not of the latter. > > > > > > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com> > > > > --- > > > > src/libcamera/formats.cpp | 14 ++++++++++++++ > > > > src/libcamera/v4l2_pixelformat.cpp | 2 ++ > > > > src/v4l2/v4l2_camera_proxy.cpp | 2 ++ > > > > 3 files changed, 18 insertions(+) > > > > > > > > diff --git a/src/libcamera/formats.cpp b/src/libcamera/formats.cpp > > > > index 2ac3b41..dcd1dcf 100644 > > > > --- a/src/libcamera/formats.cpp > > > > +++ b/src/libcamera/formats.cpp > > > > @@ -268,6 +268,20 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{ > > > > .colourEncoding = PixelFormatInfo::ColourEncodingYUV, > > > > .packed = false, > > > > } }, > > > > + { PixelFormat(DRM_FORMAT_YUV422), { > > > > + .format = PixelFormat(DRM_FORMAT_YUV422), > > > > + .v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_YUV422P), > > > > + .bitsPerPixel = 16, > > > > + .colourEncoding = PixelFormatInfo::ColourEncodingYUV, > > > > + .packed = false, > > > > + } }, > > > > + { PixelFormat(DRM_FORMAT_YUV420), { > > > > + .format = PixelFormat(DRM_FORMAT_YUV420), > > > > + .v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_YUV420), > > > > + .bitsPerPixel = 12, > > > > + .colourEncoding = PixelFormatInfo::ColourEncodingYUV, > > > > + .packed = false, > > > > + } }, > > > > I may merge in the morning a patch series that rework format handling > > and would conflict with this patch. If that's the case, I'll rebase this > > patch and send the rebased version to the list. > > > > > > > > > > /* Greyscale formats. */ > > > > { PixelFormat(DRM_FORMAT_R8), { > > > > diff --git a/src/libcamera/v4l2_pixelformat.cpp b/src/libcamera/v4l2_pixelformat.cpp > > > > index 94fae47..01778c0 100644 > > > > --- a/src/libcamera/v4l2_pixelformat.cpp > > > > +++ b/src/libcamera/v4l2_pixelformat.cpp > > > > @@ -64,6 +64,8 @@ const std::map<V4L2PixelFormat, PixelFormat> vpf2pf{ > > > > { V4L2PixelFormat(V4L2_PIX_FMT_NV61), PixelFormat(DRM_FORMAT_NV61) }, > > > > { V4L2PixelFormat(V4L2_PIX_FMT_NV12), PixelFormat(DRM_FORMAT_NV12) }, > > > > { V4L2PixelFormat(V4L2_PIX_FMT_NV21), PixelFormat(DRM_FORMAT_NV21) }, > > > > + { V4L2PixelFormat(V4L2_PIX_FMT_YUV422P), PixelFormat(DRM_FORMAT_YUV422) }, > > > > + { V4L2PixelFormat(V4L2_PIX_FMT_YUV420), PixelFormat(DRM_FORMAT_YUV420) }, > > > > > > > > /* Greyscale formats. */ > > > > { V4L2PixelFormat(V4L2_PIX_FMT_GREY), PixelFormat(DRM_FORMAT_R8) }, > > > > diff --git a/src/v4l2/v4l2_camera_proxy.cpp b/src/v4l2/v4l2_camera_proxy.cpp > > > > index d7f14e6..a54d47e 100644 > > > > --- a/src/v4l2/v4l2_camera_proxy.cpp > > > > +++ b/src/v4l2/v4l2_camera_proxy.cpp > > > > @@ -576,6 +576,8 @@ static const std::array<PixelFormatInfo, 14> pixelFormatInfo = {{ > > > > { PixelFormat(DRM_FORMAT_NV61), V4L2_PIX_FMT_NV61, 2, {{ { 8, 1, 1 }, { 16, 2, 1 }, { 0, 0, 0 } }} }, > > > > { PixelFormat(DRM_FORMAT_NV24), V4L2_PIX_FMT_NV24, 2, {{ { 8, 1, 1 }, { 16, 1, 1 }, { 0, 0, 0 } }} }, > > > > { PixelFormat(DRM_FORMAT_NV42), V4L2_PIX_FMT_NV42, 2, {{ { 8, 1, 1 }, { 16, 1, 1 }, { 0, 0, 0 } }} }, > > > > + { PixelFormat(DRM_FORMAT_YUV422), V4L2_PIX_FMT_YUV422P, 2, {{ { 8, 1, 1 }, { 8, 2, 1 }, { 8, 2, 1 } }} }, > > > > + { PixelFormat(DRM_FORMAT_YUV420), V4L2_PIX_FMT_YUV420, 2, {{ { 8, 1, 1 }, { 8, 2, 2 }, { 8, 2, 2 } }} }, > > > > Additionally, don't those formats have 3 planes, not two ? > > You are quite right, my mistake. It seems to work both ways...! > > > > > /* Compressed formats. */ > > > > /* > > > > * \todo Get a better image size estimate for MJPEG, via
Hi David, On Wed, Jun 17, 2020 at 05:44:42PM +0100, David Plowman wrote: > These formats can be helpful when downstream applications or libraries > support them natively (avoiding a costly conversion). > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com> > --- > src/libcamera/formats.cpp | 14 ++++++++++++++ > src/libcamera/v4l2_pixelformat.cpp | 2 ++ > src/v4l2/v4l2_camera_proxy.cpp | 2 ++ > 3 files changed, 18 insertions(+) > > diff --git a/src/libcamera/formats.cpp b/src/libcamera/formats.cpp > index 2ac3b41..dcd1dcf 100644 > --- a/src/libcamera/formats.cpp > +++ b/src/libcamera/formats.cpp > @@ -268,6 +268,20 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{ > .colourEncoding = PixelFormatInfo::ColourEncodingYUV, > .packed = false, > } }, > + { PixelFormat(DRM_FORMAT_YUV422), { > + .format = PixelFormat(DRM_FORMAT_YUV422), > + .v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_YUV422P), > + .bitsPerPixel = 16, > + .colourEncoding = PixelFormatInfo::ColourEncodingYUV, > + .packed = false, > + } }, > + { PixelFormat(DRM_FORMAT_YUV420), { > + .format = PixelFormat(DRM_FORMAT_YUV420), > + .v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_YUV420), > + .bitsPerPixel = 12, > + .colourEncoding = PixelFormatInfo::ColourEncodingYUV, > + .packed = false, > + } }, > > /* Greyscale formats. */ > { PixelFormat(DRM_FORMAT_R8), { > diff --git a/src/libcamera/v4l2_pixelformat.cpp b/src/libcamera/v4l2_pixelformat.cpp > index 94fae47..01778c0 100644 > --- a/src/libcamera/v4l2_pixelformat.cpp > +++ b/src/libcamera/v4l2_pixelformat.cpp > @@ -64,6 +64,8 @@ const std::map<V4L2PixelFormat, PixelFormat> vpf2pf{ > { V4L2PixelFormat(V4L2_PIX_FMT_NV61), PixelFormat(DRM_FORMAT_NV61) }, > { V4L2PixelFormat(V4L2_PIX_FMT_NV12), PixelFormat(DRM_FORMAT_NV12) }, > { V4L2PixelFormat(V4L2_PIX_FMT_NV21), PixelFormat(DRM_FORMAT_NV21) }, > + { V4L2PixelFormat(V4L2_PIX_FMT_YUV422P), PixelFormat(DRM_FORMAT_YUV422) }, > + { V4L2PixelFormat(V4L2_PIX_FMT_YUV420), PixelFormat(DRM_FORMAT_YUV420) }, > > /* Greyscale formats. */ > { V4L2PixelFormat(V4L2_PIX_FMT_GREY), PixelFormat(DRM_FORMAT_R8) }, > diff --git a/src/v4l2/v4l2_camera_proxy.cpp b/src/v4l2/v4l2_camera_proxy.cpp > index d7f14e6..a54d47e 100644 > --- a/src/v4l2/v4l2_camera_proxy.cpp > +++ b/src/v4l2/v4l2_camera_proxy.cpp > @@ -576,6 +576,8 @@ static const std::array<PixelFormatInfo, 14> pixelFormatInfo = {{ > { PixelFormat(DRM_FORMAT_NV61), V4L2_PIX_FMT_NV61, 2, {{ { 8, 1, 1 }, { 16, 2, 1 }, { 0, 0, 0 } }} }, > { PixelFormat(DRM_FORMAT_NV24), V4L2_PIX_FMT_NV24, 2, {{ { 8, 1, 1 }, { 16, 1, 1 }, { 0, 0, 0 } }} }, > { PixelFormat(DRM_FORMAT_NV42), V4L2_PIX_FMT_NV42, 2, {{ { 8, 1, 1 }, { 16, 1, 1 }, { 0, 0, 0 } }} }, > + { PixelFormat(DRM_FORMAT_YUV422), V4L2_PIX_FMT_YUV422P, 2, {{ { 8, 1, 1 }, { 8, 2, 1 }, { 8, 2, 1 } }} }, > + { PixelFormat(DRM_FORMAT_YUV420), V4L2_PIX_FMT_YUV420, 2, {{ { 8, 1, 1 }, { 8, 2, 2 }, { 8, 2, 2 } }} }, This causes a compilation error, as the array is defined with a fixed number of elements. Could you make sure to enable compilation of the V4L2 compatibility layer to develop patches that touches it ? It's disabled by default, and you can enable it by running meson configure -Dv4l2=true in the build directory. Same for the Android compatibility layer, with -Dandroid=true if you author patches that impact it. I enable both in all my builds, just in case. > /* Compressed formats. */ > /* > * \todo Get a better image size estimate for MJPEG, via
Hi Laurent On Mon, 22 Jun 2020 at 05:31, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi David, > > On Thu, Jun 18, 2020 at 09:19:55AM +0100, David Plowman wrote: > > Hi Laurent > > > > Thanks for the review and the correction. > > > > Just to add some background, I've found that libjpeg works _way_ > > faster if you can feed > > it directly with planar YUV420 (what it calls "raw" data), hence the > > motivation for this > > patch. I added YUV422 because, although we don't support it yet, it's > > common for folks > > to prefer 422 rather than 420 JPEGs, so I expect we'll need to do it > > at some point. > > Does your ISP hardware support YUV422 ? Yes it does! In fact I think it was our stills format of choice in the distant past... Best regards David > > > I'm happy to re-submit the patch if you like, though it sounds like > > the work you're doing > > means this isn't worth the trouble. Just let me know! > > I've pushed the format rework, and will send the rebased version of your > patch shortly. > > > On Thu, 18 Jun 2020 at 01:46, Laurent Pinchart wrote: > > > On Thu, Jun 18, 2020 at 01:18:21AM +0300, Laurent Pinchart wrote: > > > > On Wed, Jun 17, 2020 at 05:44:42PM +0100, David Plowman wrote: > > > > > These formats can be helpful when downstream applications or libraries > > > > > support them natively (avoiding a costly conversion). > > > > > > > > But only if cameras can provide them :-) > > > > > > > > Does the Raspberry Pi ISP support both V4L2_PIX_FMT_YUV420 and > > > > V4L2_PIX_FMT_YUV422P ? I see mentions of the former in the driver, but > > > > not of the latter. > > > > > > > > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com> > > > > > --- > > > > > src/libcamera/formats.cpp | 14 ++++++++++++++ > > > > > src/libcamera/v4l2_pixelformat.cpp | 2 ++ > > > > > src/v4l2/v4l2_camera_proxy.cpp | 2 ++ > > > > > 3 files changed, 18 insertions(+) > > > > > > > > > > diff --git a/src/libcamera/formats.cpp b/src/libcamera/formats.cpp > > > > > index 2ac3b41..dcd1dcf 100644 > > > > > --- a/src/libcamera/formats.cpp > > > > > +++ b/src/libcamera/formats.cpp > > > > > @@ -268,6 +268,20 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{ > > > > > .colourEncoding = PixelFormatInfo::ColourEncodingYUV, > > > > > .packed = false, > > > > > } }, > > > > > + { PixelFormat(DRM_FORMAT_YUV422), { > > > > > + .format = PixelFormat(DRM_FORMAT_YUV422), > > > > > + .v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_YUV422P), > > > > > + .bitsPerPixel = 16, > > > > > + .colourEncoding = PixelFormatInfo::ColourEncodingYUV, > > > > > + .packed = false, > > > > > + } }, > > > > > + { PixelFormat(DRM_FORMAT_YUV420), { > > > > > + .format = PixelFormat(DRM_FORMAT_YUV420), > > > > > + .v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_YUV420), > > > > > + .bitsPerPixel = 12, > > > > > + .colourEncoding = PixelFormatInfo::ColourEncodingYUV, > > > > > + .packed = false, > > > > > + } }, > > > > > > I may merge in the morning a patch series that rework format handling > > > and would conflict with this patch. If that's the case, I'll rebase this > > > patch and send the rebased version to the list. > > > > > > > > > > > > > /* Greyscale formats. */ > > > > > { PixelFormat(DRM_FORMAT_R8), { > > > > > diff --git a/src/libcamera/v4l2_pixelformat.cpp b/src/libcamera/v4l2_pixelformat.cpp > > > > > index 94fae47..01778c0 100644 > > > > > --- a/src/libcamera/v4l2_pixelformat.cpp > > > > > +++ b/src/libcamera/v4l2_pixelformat.cpp > > > > > @@ -64,6 +64,8 @@ const std::map<V4L2PixelFormat, PixelFormat> vpf2pf{ > > > > > { V4L2PixelFormat(V4L2_PIX_FMT_NV61), PixelFormat(DRM_FORMAT_NV61) }, > > > > > { V4L2PixelFormat(V4L2_PIX_FMT_NV12), PixelFormat(DRM_FORMAT_NV12) }, > > > > > { V4L2PixelFormat(V4L2_PIX_FMT_NV21), PixelFormat(DRM_FORMAT_NV21) }, > > > > > + { V4L2PixelFormat(V4L2_PIX_FMT_YUV422P), PixelFormat(DRM_FORMAT_YUV422) }, > > > > > + { V4L2PixelFormat(V4L2_PIX_FMT_YUV420), PixelFormat(DRM_FORMAT_YUV420) }, > > > > > > > > > > /* Greyscale formats. */ > > > > > { V4L2PixelFormat(V4L2_PIX_FMT_GREY), PixelFormat(DRM_FORMAT_R8) }, > > > > > diff --git a/src/v4l2/v4l2_camera_proxy.cpp b/src/v4l2/v4l2_camera_proxy.cpp > > > > > index d7f14e6..a54d47e 100644 > > > > > --- a/src/v4l2/v4l2_camera_proxy.cpp > > > > > +++ b/src/v4l2/v4l2_camera_proxy.cpp > > > > > @@ -576,6 +576,8 @@ static const std::array<PixelFormatInfo, 14> pixelFormatInfo = {{ > > > > > { PixelFormat(DRM_FORMAT_NV61), V4L2_PIX_FMT_NV61, 2, {{ { 8, 1, 1 }, { 16, 2, 1 }, { 0, 0, 0 } }} }, > > > > > { PixelFormat(DRM_FORMAT_NV24), V4L2_PIX_FMT_NV24, 2, {{ { 8, 1, 1 }, { 16, 1, 1 }, { 0, 0, 0 } }} }, > > > > > { PixelFormat(DRM_FORMAT_NV42), V4L2_PIX_FMT_NV42, 2, {{ { 8, 1, 1 }, { 16, 1, 1 }, { 0, 0, 0 } }} }, > > > > > + { PixelFormat(DRM_FORMAT_YUV422), V4L2_PIX_FMT_YUV422P, 2, {{ { 8, 1, 1 }, { 8, 2, 1 }, { 8, 2, 1 } }} }, > > > > > + { PixelFormat(DRM_FORMAT_YUV420), V4L2_PIX_FMT_YUV420, 2, {{ { 8, 1, 1 }, { 8, 2, 2 }, { 8, 2, 2 } }} }, > > > > > > Additionally, don't those formats have 3 planes, not two ? > > > > You are quite right, my mistake. It seems to work both ways...! > > > > > > > /* Compressed formats. */ > > > > > /* > > > > > * \todo Get a better image size estimate for MJPEG, via > > -- > Regards, > > Laurent Pinchart
diff --git a/src/libcamera/formats.cpp b/src/libcamera/formats.cpp index 2ac3b41..dcd1dcf 100644 --- a/src/libcamera/formats.cpp +++ b/src/libcamera/formats.cpp @@ -268,6 +268,20 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{ .colourEncoding = PixelFormatInfo::ColourEncodingYUV, .packed = false, } }, + { PixelFormat(DRM_FORMAT_YUV422), { + .format = PixelFormat(DRM_FORMAT_YUV422), + .v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_YUV422P), + .bitsPerPixel = 16, + .colourEncoding = PixelFormatInfo::ColourEncodingYUV, + .packed = false, + } }, + { PixelFormat(DRM_FORMAT_YUV420), { + .format = PixelFormat(DRM_FORMAT_YUV420), + .v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_YUV420), + .bitsPerPixel = 12, + .colourEncoding = PixelFormatInfo::ColourEncodingYUV, + .packed = false, + } }, /* Greyscale formats. */ { PixelFormat(DRM_FORMAT_R8), { diff --git a/src/libcamera/v4l2_pixelformat.cpp b/src/libcamera/v4l2_pixelformat.cpp index 94fae47..01778c0 100644 --- a/src/libcamera/v4l2_pixelformat.cpp +++ b/src/libcamera/v4l2_pixelformat.cpp @@ -64,6 +64,8 @@ const std::map<V4L2PixelFormat, PixelFormat> vpf2pf{ { V4L2PixelFormat(V4L2_PIX_FMT_NV61), PixelFormat(DRM_FORMAT_NV61) }, { V4L2PixelFormat(V4L2_PIX_FMT_NV12), PixelFormat(DRM_FORMAT_NV12) }, { V4L2PixelFormat(V4L2_PIX_FMT_NV21), PixelFormat(DRM_FORMAT_NV21) }, + { V4L2PixelFormat(V4L2_PIX_FMT_YUV422P), PixelFormat(DRM_FORMAT_YUV422) }, + { V4L2PixelFormat(V4L2_PIX_FMT_YUV420), PixelFormat(DRM_FORMAT_YUV420) }, /* Greyscale formats. */ { V4L2PixelFormat(V4L2_PIX_FMT_GREY), PixelFormat(DRM_FORMAT_R8) }, diff --git a/src/v4l2/v4l2_camera_proxy.cpp b/src/v4l2/v4l2_camera_proxy.cpp index d7f14e6..a54d47e 100644 --- a/src/v4l2/v4l2_camera_proxy.cpp +++ b/src/v4l2/v4l2_camera_proxy.cpp @@ -576,6 +576,8 @@ static const std::array<PixelFormatInfo, 14> pixelFormatInfo = {{ { PixelFormat(DRM_FORMAT_NV61), V4L2_PIX_FMT_NV61, 2, {{ { 8, 1, 1 }, { 16, 2, 1 }, { 0, 0, 0 } }} }, { PixelFormat(DRM_FORMAT_NV24), V4L2_PIX_FMT_NV24, 2, {{ { 8, 1, 1 }, { 16, 1, 1 }, { 0, 0, 0 } }} }, { PixelFormat(DRM_FORMAT_NV42), V4L2_PIX_FMT_NV42, 2, {{ { 8, 1, 1 }, { 16, 1, 1 }, { 0, 0, 0 } }} }, + { PixelFormat(DRM_FORMAT_YUV422), V4L2_PIX_FMT_YUV422P, 2, {{ { 8, 1, 1 }, { 8, 2, 1 }, { 8, 2, 1 } }} }, + { PixelFormat(DRM_FORMAT_YUV420), V4L2_PIX_FMT_YUV420, 2, {{ { 8, 1, 1 }, { 8, 2, 2 }, { 8, 2, 2 } }} }, /* Compressed formats. */ /* * \todo Get a better image size estimate for MJPEG, via
These formats can be helpful when downstream applications or libraries support them natively (avoiding a costly conversion). Signed-off-by: David Plowman <david.plowman@raspberrypi.com> --- src/libcamera/formats.cpp | 14 ++++++++++++++ src/libcamera/v4l2_pixelformat.cpp | 2 ++ src/v4l2/v4l2_camera_proxy.cpp | 2 ++ 3 files changed, 18 insertions(+)