[libcamera-devel] [PATCH 1/2] libcamera: formats: Fix colour encoding for "R" raw greyscale formats

[David Plowman]

Hi again

On Mon, 8 Aug 2022 at 12:18, David Plowman
<david.plowman at raspberrypi.com> wrote:
>
> Hi Laurent
>
> Thanks for the feedback.
>
> On Thu, 4 Aug 2022 at 19:25, Laurent Pinchart
> <laurent.pinchart at 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 at 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