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

[David Plowman]

Hi Laurent

Thanks for merging our "workaround" patch. That helps us to keep going
in the meantime! On the wider subject...

On Tue, 4 Oct 2022 at 21:27, Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> 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.

I agree with this. I guess the question is whether the PixelFormat
class is describing just the memory format, or something more. In the
former case I think things like "colour encoding" simply shouldn't be
there. In the latter case... well, there's a long history of folks
mixing memory formats, colour encodings and colour spaces together in
slightly implicit ways, and I'm not sure it's been doing us much good,
but it's a hard thing to fix!!

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

Would be interested to hear more about this! I'm not quite sure what
"which streams to capture in a more explicit way" might mean?

>From my point of view the configuration API seems mostly pretty good,
apart from some niggles with the format / colour encoding (as
discussed above) or recent colour space changes which are unhelpful to
us. libcamera of course already has a lot of active users (a good
thing, I hasten to add) so I'm a bit nervous about rocking the boat
too much!!

Thanks!
David

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