[libcamera-devel] Colour space adjustment

[Laurent Pinchart]

Hi David,

On Mon, Sep 05, 2022 at 05:11:08PM +0100, David Plowman wrote:
> On Mon, 5 Sept 2022 at 14:59, Laurent Pinchart wrote:
> > On Mon, Sep 05, 2022 at 01:37:00PM +0100, Naushir Patuck wrote:
> > > Hi,
> > >
> > > On a related note, we are also getting this warning on the Raspberry Pi
> > > platform with commit e297673e7686:
> > >
> > > WARN V4L2 v4l2_subdevice.cpp:512 'imx477 10-001a': Unknown subdev format
> > > 0x7002, defaulting to RGB encoding
> > >
> > > This is because the embedded buffer stream format (i.e. a non-image data
> > > format) does not map to a libcamera PixelFormat* at present.  In the past,
> > > we've had similar issues and decided not to use WARN level to flag this.
> > > Can we do the same here please?
> >
> > Absolutely. I recall a discussion about this, and thought we had solved
> > the problem already, but it seems I'm wrong.
> >
> > > Not directly related to the logging message, but I suppose the same
> > > question should also be asked here - should we be overwriting the
> > > colorEncoding in these circumstances?
> >
> > It depends what you mean by "these cirscumstances". For metadata formats
> > colorspace doesn't make sense, so we should set the colorSpace field to
> > nullopt without any warning. For unknown media bus codes in general,
> > that's different, as the simple pipeline handler can propagate them
> > through the pipeline in a generic way without caring what they are
> > exactly.
> >
> > The imx477 driver seems to always report V4L2_COLORSPACE_DEFAULT for the
> > colorspace field. I think setting colorSpace to nullopt in that case
> > would be the right thing to do. I'll prepare a patch and will CC you.
> >
> > > On Mon, 5 Sept 2022 at 13:25, David Plowman via libcamera-devel wrote:
> > >
> > > > Hi everyone
> > > >
> > > > We've recently checked out the latest version of libcamera and are having
> > > > a few colour space issues. Obviously I'm sorry to be raising this at a
> > > > stage where the commits have already been applied, but maybe we can think
> > > > of some kind of acceptable workaround.
> >
> > Or even an acceptable good solution :-) Sorry for breaking it for you in
> > the first place.
> >
> > > > The problems come with commit 4aa71c ("libcamera: color_space: Move color
> > > > space adjustment to ColorSpace class"). The specific behaviour that causes
> > > > us grief is where it overwrites the YCbCr encoding matrix to "None" when
> > > > the pixel format is an RGB one.
> > > >
> > > > In the Pi's pipeline, pixels fundamentally end up as YUV and there's an
> > > > extra conversion that happens if you want RGB. So this means we do need to
> > > > know that YCbCr matrix.
> >
> > Does that mean that the ISP will internally encode RGB to YUV and then
> > decode YUV to RGB ? If so, does the YCbCr matrix matter, or os the only
> > requirement that both conversions should use the same matrix ?
> 
> The matrices certainly need to be consistent (i.e. one the inverse of
> the other). But in some circumstances it matters that we have the
> right matrix. On the Pi, if you ask for a "low resolution" stream then
> that's always a YUV stream, so it does need to have the correct
> matrix. I'd certainly prefer to use the "correct" matrix all the time,
> whether or not the low resolution stream is being output, and not make
> the code "more fiddly" in this respect.

I understand that as meaning that if a user requests a high resolution
RGB stream and a low-resolution YUV stream, the Y'CbCr encoding will be
dictated by the low-resolution stream, and the ISP has to be programmed
accordingly. Is it that the ISP driver expects the Y'CbCr encoding to be
programmed through V4L2 on the high-resolution stream in all cases ?

If that's the core of the issue, the pipeline handler should take the
libcamera color space from the YUV stream and apply it as a V4L2
colorspace on the video node for the RGB stream, as it's the
responsibility of the pipeline handler to translate between the
libcamera API and the backend API. The conversion is done in
V4L2Device::fromColorSpace(), which does not take the pixel format into
account, so you have full control of the color space settings there and
can apply a YUV color space with an RGB pixel format to a V4L2 video
node. Would that work in your case ?

If the issue is somewhere else, then I'm afraid my slow brain will need
another explanation :-)

> > > > More generally, I'm not sure how I feel about taking "standard colour
> > > > spaces" and  then overwriting some parts of them, so that afterwards
> > > > something that is equivalent to (for example) sYCC is then no longer ==
> > > > Sycc(). Possibly I'm being a bit paranoid here, I'm not sure.
> >
> > From an application point of view, an RGB format should always have its
> > YCbCrEncoding set to None, as the data isn't encoded in YCbCr. I don't
> > really see what other good alternatives we would have.
> 
> I think the alternative is not to touch fields that we think (rightly
> or wrongly) "don't matter". But I feel this is probably more of a
> symptom, not the root cause.
> 
> The heart of my difficulty may be more that I don't like the colour
> space changing because of the output format. They really should be
> independent. Now, whether you regard the "true" colour space as being
> "the whole ColorSpace object" or just the "the primaries field in the
> ColorSpace object" is perhaps the crux of this question.
> 
> I know that I really wanted to avoid having multiple representations
> of the same colour space, which I had chosen to mean the entire
> ColorSpace object (see comments near the top of colour_space.cpp). I
> understand the history why V4L2 has "default" values for its fields,
> but it leads to multiple representations of the same things - and that
> just seemed really awkward to me. I'm worrying that we've now created
> exactly this - whether two colour spaces are actually the same depends
> on quite a complicated test involving the output formats at hand. To
> try and illustrate this:
> 
> 1. An application asks for sYCC and an RGB output. The YCbCr encoding
> gets changed and flagged as "adjusted". Do I have to check that the
> "adjustment" didn't actually change the "true" colour space (by which
> I mean the "primaries" field, and maybe some others)? "adjusted"
> doesn't seem so helpful any more!

The adjusted flag covers the whole configuration, so you have to check
the whole configuration indeed (and not just the color space). That's a
limitation of the configuration API, maybe we'll want finer-grained
adjust flags in the future, I don't know yet.

> 2. Suppose I want to check if I have the sYCC colour space. Now
> checking "myColSpace == Sycc" isn't enough, even if Sycc is what I put
> in. I also need to check "|| myColSpace == <something-else>" but only
> if there are no YUV outputs. It just all seems more awkward than it
> was before.

Is that on the application side ? I don't think an application should
use Sycc at all for RGB data, it should use Srgb instead, and can
compare with == Srgb.

> On an unrelated (and more minor) note, I'm a bit uneasy about using
> the colour encoding in the PixelFormat (in ColorSpace::adjust),
> because we know that for Raspberry Pi, some of them are wrong (the "R"
> formats most obviously). I don't think this is causing any problems,
> but seeing them being used always makes me a bit anxious!

I need to look at your patches for the R8 format, sorry about the delay.
I'll keep the color space in mind.

> > > > Anyway, I'm not quite sure what to do about this. Is this behaviour now
> > > > important to other platforms? I guess we have the option to override the
> > > > validateColorSpaces method for the RPiCameraConfiguration. Or add another
> > > > flag "don't-touch-my-colour-spaces". But I'm not sensing great elegance
> > > > here...
> > > >
> > > > Any suggestions?
> >
> > I'm not sure to see what the problem is. Could you describe what issues
> > you see in the application-facing behaviour and in the V4L2-facing
> > behaviour ?
> 
> Currently we're seeing "unrecognised YCbCr encoding" warnings in the
> fromColorSpace method.

That was a bug, I've pushed the fix today, see commit a03ce60cf969
("libcamera: v4l2_device: Map YCbCrEncoding::None to V4L2"). I've also
sent "[PATCH] libcamera: v4l2_subdevice: Silence warning for unknown
metadata formats" to the list. Could you please test those ?

> Probably it does the right thing "by luck", but
> I think it could break if someone was recording a Rec.709 video and we
> asked for a simultaneous RGB stream. It could clobber the correct
> colour space information in the RGB stream, and then use that for both
> streams.
> 
> I'm wondering what the benefits would be in adapting to the new
> scheme? Or if we had our own version of validateColourSpaces (which
> kept the old behaviour) would we be loosing anything?

It would give different behaviours to applications depending on the
platform, I'm not thrilled by that. How the color spaces are used in the
public API should be clearly documentation and consistent.

Let's try to decouple the two questions (how to configure the ISP
through V4L2 to ensure correct color spaces in all cases, and the
behaviour of the public API).

-- 
Regards,

Laurent Pinchart