[libcamera-devel] [PATCH v5 2/7] libcamera: Add ColorSpace fields to StreamConfiguration

[David Plowman]

Hi again Kieran

Sorry, forgot to "reply all" on my previous reply, hopefully folks can
find the missing email trail below!

On Fri, 5 Nov 2021 at 13:29, Kieran Bingham
<kieran.bingham at ideasonboard.com> wrote:
>
> Quoting David Plowman (2021-11-05 13:16:44)
> > Hi Kieran
> >
> > Thanks for looking at this patch set!
> >
> > On Fri, 5 Nov 2021 at 12:02, Kieran Bingham
> > <kieran.bingham at ideasonboard.com> wrote:
> > >
> > > Hi David,
> > >
> > > Quoting David Plowman (2021-11-04 13:58:00)
> > > > This is so that applications can choose appropriate color spaces which
> > > > will then be passed down to the V4L2 devices.
> > > >
> > > > There are two fields: the "requestedColorSpace" which is the one an
> > > > application wants, and the "actualColorSpace" which is what the
> > > > underlying hardware can deliver, and which is filled in by the
> > > > validate() method.
> > >
> > > Is it necessary to store both? We don't do that with the other fields...
> > > the StreamConfiguration structure is used in different states to handle
> > > that I think.
> > >
> > > When a CameraConfiguration is 'generated' Potential StreamConfigurations
> > > are 'offered' to the application.
> > >
> > > Applications can either fill in those StreamConfigurations, (or start
> > > from an empty one if it will set all fields anyway...) and 'validate'
> > > the overall CameraConfiguration.
> > >
> > > The StreamConfiguration at that point contains the 'requested' values
> > > ... and when returned back from validate() - it has the values that can
> > > be used by hardware...
> >
> > Yes, I think this is one of the critical points that we need to agree
> > on. So my reasoning was as follows:
> >
> > You specify the colour space that you want, but what if the driver
> > gives you something else. Specifically, what if the driver gives us
> > some wacky colour space that the ColorSpace class can't represent at
> > all?
> >
> > It feels a bit onerous to insist that we have a representation for
> > every colour space that we might ever encounter, particularly if there
> > might be non-V4L2 things in future, so I've gone with the notion of an
> > "undefined" colour space, which can be returned to us from the driver
> > layer.
> >
> > Now this is a dangerous thing so I've explicitly forbidden this as a
> > value that you can request (what would it mean? sounds like confusion
> > all round). So the worry is that we make a reasonable request, but the
> > driver changes it to "undefined", and now we can't call validate with
> > the same configuration.
>
> I suspect that at that point it's up to the pipeline handler to provide
> a reasonable color space?

So I think this is the crux of the matter. Do we want to insist that
we have a representation for every colour space that we might ever
encounter? If we do, then yes - we can promise to pass a meaningful
ColorSpace back up the stack and we never have to worry about
anything.

But colour spaces are in fact infinitely variable (unlike pixel
formats), though in practice V4L2 doesn't represent them so we'll
almost never encounter any. For the moment, anyway. Do we want to
pretend we can add every colour space we will ever encounter? In
practice it's probably true, but are we sure it will always be like
this?

>
> Can drivers really return an undefined colorspace? What does that mean?
> (In the same regards as what does 'asking' for an undefined color space
> mean?).

Note that a colour space is never actually "undefined", the driver
will have chosen one, it just means "we don't have a ColorSpace to
describe what it chose". So after validate() we could be told, "it's
going to use a colour space that I can't describe to you", but at the
same time it's nonsensical to request a colour space "that I can't
describe", which is why that is banned.

I don't know if any of this helps. Perhaps we need a face-to-face.

David

>
> > So my solution was to separate this into a "requested" and "actual"
> > colour space. I'm imagining that the user either leaves the
> > requestedColorSpace field alone, and they'll keep getting back
> > "adjusted" if they call validate again. Or if they don't like that
> > they could do something like
> >
> > if (actualColorSpace.isFullyDefined())
> >     requestedColorSpace = actualColorSpace;
> >
> > which will at least get rid of the "adjusted" return value in cases
> > where that's possible. Am I making any sense, or are there better
> > solutions?
>
> I think if userspace continues to ask for a color space which is not
> supported (and somehow returns an undefined color space, rather than an
> acceptable one), then we are probably into 'fail to configure'
> territory.
>
> Is it any different from asking the hardware to give me YUYV, but it
> always returns NV12? I either accept that the hardware can only do NV12,
> and acknowledge that it's not going to be YUYV, or ... ?
>
>
> > Thanks!
> >
> > David
> >
> > >
> > >
> > > > Signed-off-by: David Plowman <david.plowman at raspberrypi.com>
> > > > ---
> > > >  include/libcamera/stream.h |  4 ++++
> > > >  src/libcamera/stream.cpp   | 25 +++++++++++++++++++++++++
> > > >  2 files changed, 29 insertions(+)
> > > >
> > > > diff --git a/include/libcamera/stream.h b/include/libcamera/stream.h
> > > > index 0c55e716..fe491ff5 100644
> > > > --- a/include/libcamera/stream.h
> > > > +++ b/include/libcamera/stream.h
> > > > @@ -12,6 +12,7 @@
> > > >  #include <string>
> > > >  #include <vector>
> > > >
> > > > +#include <libcamera/color_space.h>
> > > >  #include <libcamera/framebuffer.h>
> > > >  #include <libcamera/geometry.h>
> > > >  #include <libcamera/pixel_format.h>
> > > > @@ -47,6 +48,9 @@ struct StreamConfiguration {
> > > >
> > > >         unsigned int bufferCount;
> > > >
> > > > +       ColorSpace requestedColorSpace;
> > > > +       ColorSpace actualColorSpace;
> > >
> > > Given the above, I think this should be just colorSpace;
> > >
> > > > +
> > > >         Stream *stream() const { return stream_; }
> > > >         void setStream(Stream *stream) { stream_ = stream; }
> > > >         const StreamFormats &formats() const { return formats_; }
> > > > diff --git a/src/libcamera/stream.cpp b/src/libcamera/stream.cpp
> > > > index b421e17e..1ddbbb8c 100644
> > > > --- a/src/libcamera/stream.cpp
> > > > +++ b/src/libcamera/stream.cpp
> > > > @@ -329,6 +329,31 @@ StreamConfiguration::StreamConfiguration(const StreamFormats &formats)
> > > >   * \brief Requested number of buffers to allocate for the stream
> > > >   */
> > > >
> > > > +/**
> > > > + * \var StreamConfiguration::requestedColorSpace
> > > > + * \brief The ColorSpace this stream should have
> > > > + *
> > > > + * The generateConfiguration method will generate reasonable default
> > > > + * values (ColorSpace::Jpeg for stills, ColorSpace::Rec709 for video and
> > > > + * ColorSpace::Raw for raw streams) but applications are free to overwrite
> > > > + * this value.
> > > > + */
> > > > +
> > > > +/**
> > > > + * \var StreamConfiguration::actualColorSpace
> > > > + * \brief The ColorSpace the will be used for this stream
> > > > + *
> > > > + * This field is filled in by CameraConfiguration::validate().
> > > > + * Normally this should match the requestedColorSpace, but it may differ
> > > > + * if the hardware does not support it.
> > > > + *
> > > > + * In general cameras may have different constraints here, for example,
> > > > + * they may require all output streams to share the same color space.
> > > > + * Sometimes the fields within this color space may report "Undefined"
> > > > + * values if the hardware drivers are going to use a color space that
> > > > + * is not recognised by the ColorSpace class.
> > > > + */
> > > > +
> > > >  /**
> > > >   * \fn StreamConfiguration::stream()
> > > >   * \brief Retrieve the stream associated with the configuration
> > > > --
> > > > 2.20.1
> > > >