[libcamera-devel] Colour space adjustment

[David Plowman]

Hi Laurent

Thanks for the reply!

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

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

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.

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!

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

Thanks!
David

>
> --
> Regards,
>
> Laurent Pinchart