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

[David Plowman]

Hi Jacopo

Thanks for the review!

On Wed, 24 Nov 2021 at 23:38, Jacopo Mondi <jacopo at jmondi.org> wrote:
>
> Hi David,
>
> On Thu, Nov 18, 2021 at 03:19:28PM +0000, David Plowman wrote:
> > This is so that applications can choose appropriate color spaces which
> > will then be passed down to the V4L2 devices.
> >
> > The ColorSpace field is actually optional. If it is not set you will
> > get the driver's default color space.
> >
> > Signed-off-by: David Plowman <david.plowman at raspberrypi.com>
> > ---
> >  include/libcamera/stream.h |  3 +++
> >  src/libcamera/stream.cpp   | 14 ++++++++++++++
> >  2 files changed, 17 insertions(+)
> >
> > diff --git a/include/libcamera/stream.h b/include/libcamera/stream.h
> > index 0c55e716..bea88eb4 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,8 @@ struct StreamConfiguration {
> >
> >       unsigned int bufferCount;
> >
> > +     std::optional<ColorSpace> 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..300b3af7 100644
> > --- a/src/libcamera/stream.cpp
> > +++ b/src/libcamera/stream.cpp
> > @@ -329,6 +329,20 @@ StreamConfiguration::StreamConfiguration(const StreamFormats &formats)
> >   * \brief Requested number of buffers to allocate for the stream
> >   */
> >
> > +/**
> > + * \var StreamConfiguration::colorSpace
> > + * \brief The ColorSpace for this stream
> > + *
> > + * A suitable color space may be set here or chosen by an application.
>
> What is the difference between "set here" OR "chosen by an application" ?
> Aren't application in charge to select a ColorSpace in the
> configuration ?

Well, no great difference really. I suppose a pipeline handler may or
may not set a value. Then the application code may or may not
overwrite this.

>
>
> > + * Alternatively the color space may be left unset, in which case it will
> > + * be up to the driver to choose a color space.
>
> I wil model this from the application point of view.
>
> * This field allow to select a ColorSpace to use for the Stream.
> *
> * The field is optional and application can chose to leave it unset.
> * The library will adjust it to what the Camera can provide and the
> * CameraConfiguration this StreamConfiguration is part of will be
> * adjusted.

Of course, a pipeline handler may (or may not) provide a value here
too. But I can try and re-word it to be a bit more like this!

>
> > + *
> > + * If the system cannot deliver the requested color space, the validate()
> > + * method will overwrite the value here with what it can deliver. Note that
> > + * platforms may enforce extra contraints here, such as requiring all output
> > + * streams to share the same color space.
>
> What about something along the lines of:
>
> * If a specific ColorSpace is requested here but the Camera cannot
> * produce it, the StreamConfiguration will be Adjusted to what can be
> * delivered, as there might be platform specific contraints to respect
> * such as all output streams sharing the same color space.
>
> Feel free to adjust, as a native speaker you'll surely do better.

Yes, I'll try and adopt some of this in the next revision.

Thanks!
David

>
> Thanks
>    j
>
> > + */
> > +
> >  /**
> >   * \fn StreamConfiguration::stream()
> >   * \brief Retrieve the stream associated with the configuration
> > --
> > 2.20.1
> >