[libcamera-devel] Colour spaces (again!)

[Kieran Bingham]

Quoting David Plowman (2021-11-08 07:28:47)
> Hi Kieran, Jacopo and everyone
> 
> Thanks for the recent reviews of the colour space patches. I wanted to
> leave aside the "coding details" for the moment and discuss the more
> fundamental ideas that were raised.
> 
> 
> One ColorSpace field or two?
> 
> Kieran highlighted this, and it's a good question. I went with two
> ColorSpace fields because I was uncomfortable with the idea that
> driver layers (we only have V4L2 at the moment) might be able to use
> colour spaces that we haven't seen before.

If we come across a colorspace that we haven't seen before, what do we
want to do about it?

Propagate it along the pipeline, and ensure the application sees the
same color space representation?

Or something else?

> Again to pick up Kieran's question, colour spaces are not like pixel
> formats because there could be an infinite number of variations,
> though in practice it's doubtful we'll ever see more than a handful. I
> think we have a couple of options.
> 
> 1. Allow validate() to return an "undefined" ColorSpace. This simply
> means the driver has chosen a colour space for which the ColorSpace
> class no entry. At the same time I wanted to disallow applications
> from requesting this "undefined" colour space because the idea is
> meaningless, so it is banned.

Ah, so with a validate call, that will cause any un-seen colorspace to
fail?

Are there any 'rules' that we can validate rather than 'is this an entry
in the table'?

(I didn't recall seeing a table, only some predefined types - I'll have
to have another look back at the series).

> This led me to an implementation where the StreamConfiguration has two
> fields, one the "colour space you want" (where "undefined" is not
> allowed), and one being "the colour space you'll get", where getting
> back something you don't understand is possible.

Getting back something you don't understand, but can be fully
represented by the structures available in the ColorSpace class? Or ...
not representable by the ColorSpace class?

If it's representable, then I think it's fine - it just gets returned.

If it's something else ... then that's where I guess it needs a specific
"We don't know how to represent this" and that might be 'Undefined' or
'Unrepresentable' ... I guess you're saying there is a subtle difference
between, something invalid, and something that is valid that we can't
represent.


> 2. Or I think we could do away with the "undefined" return value and
> insist on a representation for every colour space we might ever
> encounter. For the moment, covering the V4L2 ones would do. If we ever
> got to a stage where alternative non-V4L2 driver layers use other
> colour spaces, then would have to add them to the ColorSpace class.

Certainly if we support beyond V4L2 - then the ColorSpace class would
need to be extended to support that other thing...

I'm not discounting the requirement for an 'Undefined' yet though ...
But I haven't seen/understood how it needs to be split into 'what I
want, and what I get' that can't already be handled through a single
entry in the StreamConfiguration...

> Should colour spaces be optional?
> 
> Jacopo raised this, prompted at least in part by an earlier version of
> my patches where I allowed this, using std::optional.
> 
> I've actually moved fairly strongly away from this. The more time I've
> spent with this issue the more convinced I am that letting
> users/applications "not care" about colour spaces is a bad idea. People
> tend to start thinking that "colour spaces are optional", but this is false -
> you get some colour space or other whether you choose one or not.
> 
> Actually I think it contributes to the general confusion about colour
> spaces, and letting folks accept "whatever the driver chooses" is a
> recipe for getting the wrong colour space much of the time. This is
> especially harmful because it's a problem you simply don't notice. So
> I think that:
> 
> 1. You must say what colour space you want. Letting
> generateConfiguration() choose JPEG for stills and Rec.709 for video
> is a reasonable start, and trivial to do.

What about situations where the user doesn't generate a configuration to
start with, and they create their own?

I presume the answer there is that they must specify a ColorSpace in
that case...?

But for example, the Android layer doesn't generate a configuration - it
just starts blank and validates as far as I understand it.

(There's no requirement for an applciation/layer to use
generateConfiguration with stream roles).


> 2. Colour space problems need to be made obvious, so that you know
> there is "something you need to investigate". Failing completely seems
> harsh, but allowing people simply to ignore problems and store them up
> for later feels counterproductive.

Agreed, but it might have to be 'someone who is able to' investigate.
I'm not sure I'd expect an end user of some camera app to want to wonder
why their camera won't capture a JPEG because of something they don't
understand or care about. But that's probably no different to
pixelformat issues anyway. It should be handled by the applications (or
whatever layer does the configuration).

> Finally, I want to stress that colour spaces have to be correct, even
> though we get lulled into a false sense of security by the fact that
> you can generally ignore them and things "seem OK" - but they are not.

Yes, I have quite visible color issues between my two monitors. So I can
see how things can easily 'go wrong silently' and have an impact.

It's subtle, and if there was only one monitor, it wouldn't be noticable
because it only shows up with a side-by-side comparison. But one of them
is clearly 'wrong' (Granted, I don't know which one is wrong, and which
one is right - but that's the point :D)


> So thanks to everyone for all the debate. I'd very much like to get
> these points settled - as you can tell, I think support for colour
> spaces is long overdue now!

Agreed, I want to help get this going as far as possible.

If you think it would help, would you like to schedule and announce a
video call/chat this week to get this unblocked?

> Thanks and best regards
> 
> David