[libcamera-devel] Colour space adjustment

[David Plowman]

Hi Laurent

I thought I might try and take a step back first and discuss the ideas
before launching into the implementation details...

Firstly, when I implemented the ColorSpace class I decided to include
all those other bits (transfer function, YCbCr matrix, range) in the
class because it seemed better to group things together. Of course a
colour space is strictly just the primaries, but given that certain
values are expected for those other things, it seemed good to include
them. Uncontroversial so far, I hope!!

Next, looking at V4L2, because of its history it's in the position of
having several different ways to represent the same colour space. I
found this very painful and thought that "one representation for one
colour space" is the approach you would choose if you could. Maybe
that's more controversial, but I really liked the "one representation"
idea and grouping all the fields together, so that's where I landed -
for us, a ColorSpace includes all four things, and any standard
ColorSpace is encoded by just one combination of values.

I'm also of the view - others may disagree - that a colour space is a
"platonic thing". It exists "out there" and doesn't  change. So this
led to those constant ColorSpace values, Jpeg (now Sycc), Rec709 etc.
and the intention that most (almost all) applications don't care about
the individual fields, they'll just choose one of the "standard"
values, and which then doesn't/shouldn't change (unless your choice is
unsupported).

The consequence of all this is that it's easy to test if you have a
particular colour space (for example Rec709). You use "== Rec709". If
a pipeline handler or an application wants to check if they still have
Rec709, it's just "== Rec709". One colour space, one representation.
But it seems to have become more complicated now.

On a slightly different topic, but still on a philosophical note, I
think I disagree that sYCC doesn't make sense for RGB streams. It
would be like saying you shouldn't ask for an RGB stream for Rec2020
because Rec2020 defines a YCbCr matrix. The user would need to know
exactly which fields of the ColorSpace to overwrite (to force it to
"make sense"), or we'd have to define some kind of RGB2020 which is
like sRGB but for Rec2020 and say "use that one instead". I don't feel
like we're helping our users there!

In fact all my applications and Picamera2 use Sycc a lot. I really
liked the decoupling of the ColorSpace and the output encodings, and I
made constant use of it! In the new scheme, the ColorSpace is no
longer my "platonic thing", it's a collection of values that represent
some processing that may change depending on what the output encoding
is, and the net result may be the same as what you had before but you
have to check it for yourself.

So anyway, it seems to me like we've lost something, but I realise
it's way too late to be raising all these objections now. I also take
the point that there's a lot of thinking behind what I originally did
that was never consolidated into any form of documentation, so I can
only apologise for that too.

Now, to try and answer some of the other points:

On Mon, 5 Sept 2022 at 17:49, Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> 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 ?

No I don't think it does, but we are left having to go and check
exactly what happens down in the firmware before we can move on.
(Colour space stuff down in the firmware is one of my least favourite
things on the planet.)

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

Yes probably, but I guess the gist of this email is that it's not
really the implementation details that I'm struggling with.

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

Possibly, though I am missing the former world where I could just do
"== Sycc" or "== Rec709" to see if the colour space has changed. Now
they change even when it makes no difference, and I need more code to
work it out.

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

I don't think I agree here - I talked about that above. It's also an
example of pushing annoying stuff onto applications which, having
written a few, I don't appreciate! :)

>
> > 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 proit's a bit like defining our own validateColorSpaces. We would simply be handling colour spaces differently. blem 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.

I believe the current validateColorSpaces (if I have read it right) is
actually broken for us, which is why I bring it up. It will overwrite
the YCbCr matrix of any RGB stream to none, and then it may "share"
this "fixed" colour space with any YUV streams that you have. So you
end up with no YCbCr matrix anywhere.

We could either replace it in our pipeline handler, or fix it in some
way. Not sure what would be better there. Of course, I'd rather it
didn't splat my YCbCr endoings at all!

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

Yes, it would be nice. As you can probably tell, implementation
details, though annoying, bother me less. It seems to be the loss of
my ideal platonic colour spaces that I am mourning. I'm sorry I only
woke up to all this so late.

Best regards
David

>
> --
> Regards,
>
> Laurent Pinchart