[libcamera-devel,1/2] libcamera: formats: Fix colour encoding for "R" raw greyscale formats
diff mbox series

Message ID 20220804104550.4600-2-david.plowman@raspberrypi.com
State New
Headers show
Series
  • Fixes for raw monochrome stream configurations
Related show

Commit Message

David Plowman Aug. 4, 2022, 10:45 a.m. UTC
These are being used for raw monochrome sensors and so the colour
encoding should be "raw".

Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
---
 src/libcamera/formats.cpp | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

Comments

Laurent Pinchart Aug. 4, 2022, 6:25 p.m. UTC | #1
Hi David,

Thank you for the patch.

On Thu, Aug 04, 2022 at 11:45:49AM +0100, David Plowman via libcamera-devel wrote:
> These are being used for raw monochrome sensors and so the colour
> encoding should be "raw".

Ouch. This is something I didn't foresee, but we have a problem here.
These can refer to both raw data from monochrome sensors, but also
greyscale data processed by an ISP. We currently don't consider as RAW
formats, which means that

- The CameraConfiguration::validateColorSpaces() will accept other color
  spaces that ColorSpace::Raw.
- The IPU3 and Raspberry Pi pipeline handlers, and the Android HAL, will
  consider them as processed streams, not raw streams.

This patch will change that, quite likely introducing breakages.

Now, the interesting question is how to support both raw monochrome data
and processed monochrome data. Any idea ? :-)

> Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
> ---
>  src/libcamera/formats.cpp | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/src/libcamera/formats.cpp b/src/libcamera/formats.cpp
> index 283ecb3d..7b98fef2 100644
> --- a/src/libcamera/formats.cpp
> +++ b/src/libcamera/formats.cpp
> @@ -531,7 +531,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
>  			.multi = V4L2PixelFormat(),
>  		},
>  		.bitsPerPixel = 8,
> -		.colourEncoding = PixelFormatInfo::ColourEncodingYUV,
> +		.colourEncoding = PixelFormatInfo::ColourEncodingRAW,
>  		.packed = false,
>  		.pixelsPerGroup = 1,
>  		.planes = {{ { 1, 1 }, { 0, 0 }, { 0, 0 } }},
> @@ -544,7 +544,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
>  			.multi = V4L2PixelFormat(),
>  		},
>  		.bitsPerPixel = 10,
> -		.colourEncoding = PixelFormatInfo::ColourEncodingYUV,
> +		.colourEncoding = PixelFormatInfo::ColourEncodingRAW,
>  		.packed = false,
>  		.pixelsPerGroup = 1,
>  		.planes = {{ { 2, 1 }, { 0, 0 }, { 0, 0 } }},
> @@ -557,7 +557,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
>  			.multi = V4L2PixelFormat(),
>  		},
>  		.bitsPerPixel = 12,
> -		.colourEncoding = PixelFormatInfo::ColourEncodingYUV,
> +		.colourEncoding = PixelFormatInfo::ColourEncodingRAW,
>  		.packed = false,
>  		.pixelsPerGroup = 1,
>  		.planes = {{ { 2, 1 }, { 0, 0 }, { 0, 0 } }},
> @@ -570,7 +570,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
>  			.multi = V4L2PixelFormat(),
>  		},
>  		.bitsPerPixel = 10,
> -		.colourEncoding = PixelFormatInfo::ColourEncodingYUV,
> +		.colourEncoding = PixelFormatInfo::ColourEncodingRAW,
>  		.packed = true,
>  		.pixelsPerGroup = 4,
>  		.planes = {{ { 5, 1 }, { 0, 0 }, { 0, 0 } }},
David Plowman Aug. 8, 2022, 11:18 a.m. UTC | #2
Hi Laurent

Thanks for the feedback.

On Thu, 4 Aug 2022 at 19:25, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi David,
>
> Thank you for the patch.
>
> On Thu, Aug 04, 2022 at 11:45:49AM +0100, David Plowman via libcamera-devel wrote:
> > These are being used for raw monochrome sensors and so the colour
> > encoding should be "raw".
>
> Ouch. This is something I didn't foresee, but we have a problem here.
> These can refer to both raw data from monochrome sensors, but also
> greyscale data processed by an ISP. We currently don't consider as RAW
> formats, which means that
>
> - The CameraConfiguration::validateColorSpaces() will accept other color
>   spaces that ColorSpace::Raw.
> - The IPU3 and Raspberry Pi pipeline handlers, and the Android HAL, will
>   consider them as processed streams, not raw streams.
>
> This patch will change that, quite likely introducing breakages.
>
> Now, the interesting question is how to support both raw monochrome data
> and processed monochrome data. Any idea ? :-)

Hmm. That's all a bit awkward. Am I right in thinking that this comes
fundamentally from the V4L2 drivers which tell us a format (e.g.
"10-bit greyscale") but not whether it's raw or processed? Which
presumably makes it a bit awkward to fix "properly".

Alternatively, I suppose we could add (slightly ugly) workarounds in
the Pi pipeline handler, along the lines of "oh, it says R10, I know
that's really a raw format", because that's true for us. I don't
really think that I'm seeing anything better than this at the
moment...

Thanks!

David

>
> > Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
> > ---
> >  src/libcamera/formats.cpp | 8 ++++----
> >  1 file changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/src/libcamera/formats.cpp b/src/libcamera/formats.cpp
> > index 283ecb3d..7b98fef2 100644
> > --- a/src/libcamera/formats.cpp
> > +++ b/src/libcamera/formats.cpp
> > @@ -531,7 +531,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> >                       .multi = V4L2PixelFormat(),
> >               },
> >               .bitsPerPixel = 8,
> > -             .colourEncoding = PixelFormatInfo::ColourEncodingYUV,
> > +             .colourEncoding = PixelFormatInfo::ColourEncodingRAW,
> >               .packed = false,
> >               .pixelsPerGroup = 1,
> >               .planes = {{ { 1, 1 }, { 0, 0 }, { 0, 0 } }},
> > @@ -544,7 +544,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> >                       .multi = V4L2PixelFormat(),
> >               },
> >               .bitsPerPixel = 10,
> > -             .colourEncoding = PixelFormatInfo::ColourEncodingYUV,
> > +             .colourEncoding = PixelFormatInfo::ColourEncodingRAW,
> >               .packed = false,
> >               .pixelsPerGroup = 1,
> >               .planes = {{ { 2, 1 }, { 0, 0 }, { 0, 0 } }},
> > @@ -557,7 +557,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> >                       .multi = V4L2PixelFormat(),
> >               },
> >               .bitsPerPixel = 12,
> > -             .colourEncoding = PixelFormatInfo::ColourEncodingYUV,
> > +             .colourEncoding = PixelFormatInfo::ColourEncodingRAW,
> >               .packed = false,
> >               .pixelsPerGroup = 1,
> >               .planes = {{ { 2, 1 }, { 0, 0 }, { 0, 0 } }},
> > @@ -570,7 +570,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> >                       .multi = V4L2PixelFormat(),
> >               },
> >               .bitsPerPixel = 10,
> > -             .colourEncoding = PixelFormatInfo::ColourEncodingYUV,
> > +             .colourEncoding = PixelFormatInfo::ColourEncodingRAW,
> >               .packed = true,
> >               .pixelsPerGroup = 4,
> >               .planes = {{ { 5, 1 }, { 0, 0 }, { 0, 0 } }},
>
> --
> Regards,
>
> Laurent Pinchart
David Plowman Aug. 10, 2022, 11:14 a.m. UTC | #3
Hi again

On Mon, 8 Aug 2022 at 12:18, David Plowman
<david.plowman@raspberrypi.com> wrote:
>
> Hi Laurent
>
> Thanks for the feedback.
>
> On Thu, 4 Aug 2022 at 19:25, Laurent Pinchart
> <laurent.pinchart@ideasonboard.com> wrote:
> >
> > Hi David,
> >
> > Thank you for the patch.
> >
> > On Thu, Aug 04, 2022 at 11:45:49AM +0100, David Plowman via libcamera-devel wrote:
> > > These are being used for raw monochrome sensors and so the colour
> > > encoding should be "raw".
> >
> > Ouch. This is something I didn't foresee, but we have a problem here.
> > These can refer to both raw data from monochrome sensors, but also
> > greyscale data processed by an ISP. We currently don't consider as RAW
> > formats, which means that
> >
> > - The CameraConfiguration::validateColorSpaces() will accept other color
> >   spaces that ColorSpace::Raw.
> > - The IPU3 and Raspberry Pi pipeline handlers, and the Android HAL, will
> >   consider them as processed streams, not raw streams.
> >
> > This patch will change that, quite likely introducing breakages.
> >
> > Now, the interesting question is how to support both raw monochrome data
> > and processed monochrome data. Any idea ? :-)
>
> Hmm. That's all a bit awkward. Am I right in thinking that this comes
> fundamentally from the V4L2 drivers which tell us a format (e.g.
> "10-bit greyscale") but not whether it's raw or processed? Which
> presumably makes it a bit awkward to fix "properly".
>
> Alternatively, I suppose we could add (slightly ugly) workarounds in
> the Pi pipeline handler, along the lines of "oh, it says R10, I know
> that's really a raw format", because that's true for us. I don't
> really think that I'm seeing anything better than this at the
> moment...

Here's another thought...

The problem is that we have more than one possible colour encoding for
the "R" formats. If we aren't going to change V4L2 to have raw and
non-raw "R" formats anytime soon then maybe we should let the
PixelFormatInfo record all the possible colour encodings.

Now, I'm guessing that turning the PixelFormatInfo.colourEncoding
field into a std::vector<enum ColourEncoding> is probably going to be
a bit annoying to existing code. We could decide to do it, though
another option might be to add an "alternate" colour encoding field
instead.

This too could just be a std::vector<enum ColourEncoding> that
contains all the other possibilities. The benefit is that it doesn't
upset existing code which wouldn't check the new field and nothing
would change. The table in formats.h could omit the field altogether
(leaving an empty vector) except where it is needed.

Thoughts?

David

>
> Thanks!
>
> David
>
> >
> > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
> > > ---
> > >  src/libcamera/formats.cpp | 8 ++++----
> > >  1 file changed, 4 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/src/libcamera/formats.cpp b/src/libcamera/formats.cpp
> > > index 283ecb3d..7b98fef2 100644
> > > --- a/src/libcamera/formats.cpp
> > > +++ b/src/libcamera/formats.cpp
> > > @@ -531,7 +531,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> > >                       .multi = V4L2PixelFormat(),
> > >               },
> > >               .bitsPerPixel = 8,
> > > -             .colourEncoding = PixelFormatInfo::ColourEncodingYUV,
> > > +             .colourEncoding = PixelFormatInfo::ColourEncodingRAW,
> > >               .packed = false,
> > >               .pixelsPerGroup = 1,
> > >               .planes = {{ { 1, 1 }, { 0, 0 }, { 0, 0 } }},
> > > @@ -544,7 +544,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> > >                       .multi = V4L2PixelFormat(),
> > >               },
> > >               .bitsPerPixel = 10,
> > > -             .colourEncoding = PixelFormatInfo::ColourEncodingYUV,
> > > +             .colourEncoding = PixelFormatInfo::ColourEncodingRAW,
> > >               .packed = false,
> > >               .pixelsPerGroup = 1,
> > >               .planes = {{ { 2, 1 }, { 0, 0 }, { 0, 0 } }},
> > > @@ -557,7 +557,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> > >                       .multi = V4L2PixelFormat(),
> > >               },
> > >               .bitsPerPixel = 12,
> > > -             .colourEncoding = PixelFormatInfo::ColourEncodingYUV,
> > > +             .colourEncoding = PixelFormatInfo::ColourEncodingRAW,
> > >               .packed = false,
> > >               .pixelsPerGroup = 1,
> > >               .planes = {{ { 2, 1 }, { 0, 0 }, { 0, 0 } }},
> > > @@ -570,7 +570,7 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> > >                       .multi = V4L2PixelFormat(),
> > >               },
> > >               .bitsPerPixel = 10,
> > > -             .colourEncoding = PixelFormatInfo::ColourEncodingYUV,
> > > +             .colourEncoding = PixelFormatInfo::ColourEncodingRAW,
> > >               .packed = true,
> > >               .pixelsPerGroup = 4,
> > >               .planes = {{ { 5, 1 }, { 0, 0 }, { 0, 0 } }},
> >
> > --
> > Regards,
> >
> > Laurent Pinchart

Patch
diff mbox series

diff --git a/src/libcamera/formats.cpp b/src/libcamera/formats.cpp
index 283ecb3d..7b98fef2 100644
--- a/src/libcamera/formats.cpp
+++ b/src/libcamera/formats.cpp
@@ -531,7 +531,7 @@  const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
 			.multi = V4L2PixelFormat(),
 		},
 		.bitsPerPixel = 8,
-		.colourEncoding = PixelFormatInfo::ColourEncodingYUV,
+		.colourEncoding = PixelFormatInfo::ColourEncodingRAW,
 		.packed = false,
 		.pixelsPerGroup = 1,
 		.planes = {{ { 1, 1 }, { 0, 0 }, { 0, 0 } }},
@@ -544,7 +544,7 @@  const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
 			.multi = V4L2PixelFormat(),
 		},
 		.bitsPerPixel = 10,
-		.colourEncoding = PixelFormatInfo::ColourEncodingYUV,
+		.colourEncoding = PixelFormatInfo::ColourEncodingRAW,
 		.packed = false,
 		.pixelsPerGroup = 1,
 		.planes = {{ { 2, 1 }, { 0, 0 }, { 0, 0 } }},
@@ -557,7 +557,7 @@  const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
 			.multi = V4L2PixelFormat(),
 		},
 		.bitsPerPixel = 12,
-		.colourEncoding = PixelFormatInfo::ColourEncodingYUV,
+		.colourEncoding = PixelFormatInfo::ColourEncodingRAW,
 		.packed = false,
 		.pixelsPerGroup = 1,
 		.planes = {{ { 2, 1 }, { 0, 0 }, { 0, 0 } }},
@@ -570,7 +570,7 @@  const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
 			.multi = V4L2PixelFormat(),
 		},
 		.bitsPerPixel = 10,
-		.colourEncoding = PixelFormatInfo::ColourEncodingYUV,
+		.colourEncoding = PixelFormatInfo::ColourEncodingRAW,
 		.packed = true,
 		.pixelsPerGroup = 4,
 		.planes = {{ { 5, 1 }, { 0, 0 }, { 0, 0 } }},