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

[Laurent Pinchart]

Hi David,

On Wed, Aug 10, 2022 at 12:14:04PM +0100, David Plowman wrote:
> On Mon, 8 Aug 2022 at 12:18, David Plowman wrote:
> > On Thu, 4 Aug 2022 at 19:25, Laurent Pinchart wrote:
> > > 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".

In some cases that can be problematic, such as when an ISP receives
greyscale raw data and has the ability to output greyscale processed
data, or bypass all processing to capture the raw data. We wouldn't be
able to configure this through the format on the ISP source pad only. In
that case it's probably best to use a different means to configure the
ISP bypass (V4L2 control, MC link, ISP parameter buffer, ...). In all
other cases, the pipeline handler should be able to figure things out.

> > 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.

The libcamera PixelFormat class shares with V4L2 the inability to
discriminate between raw and processed formats, but I don't think V4L2
limits us as such. It is hidden within the pipeline handler and not
exposed to applications, so we could change the PixelFormat class
without requiring a similar change in V4L2 pixel formats.

> 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?

I've merged your patch that works around this issue for the Raspberry Pi
pipeline handler, so from that point of view the problem is solved. I'd
still like to share my thoughts though.

I'm not sure the lack of discrimination ability in the pixel format is
actually an issue. The pixel format is conceptually the same in both
case, it's 8-bit or 10-bit or 12-bit greyscale data in memory. I like
the idea that the pixel format describes the memory format, and not what
processing has been applied. I would like to keep this.

What we're missing, in my opinion, is a way to determine where in the
pipeline to capture a frame based on other information than just the
pixel format. I think this is something we could get from streams, by
making them take a more important role in the camera configuration.
Applications should be able to decide which streams to capture in a more
explicit way. I'll resume at some poit work I started a while ago to
rework the configuration API, and I will likely go in this direction.

This of course leaves open the question of what the colourEncoding
member of the PixelFormatInfo class should store. Your idea of using a
vector isn't a bad one, but I'd like to revisit that on top of the
configuration API rework, as usage of the colourEncoding field will
possibly be quite different then.

> > > > 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