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

[Naushir Patuck]

Hi David,

On Mon, 25 Oct 2021 at 10:37, David Plowman <david.plowman at raspberrypi.com>
wrote:

> Hi Naush
>
> Thanks for the feedback!
>
> On Mon, 25 Oct 2021 at 08:45, Naushir Patuck <naush at raspberrypi.com>
> wrote:
> >
> > Hi David,
> >
> > Thank you for your patch.
> >
> > On Wed, 20 Oct 2021 at 12:08, David Plowman <
> david.plowman at raspberrypi.com> wrote:
> >>
> >> 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.
> >>
> >> 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;
> >> +
> >
> >
> > I had a brief look-ahead in this series, but it was not immediately
> > obvious to me why we would need requested and actual? Could
> > we make do with just one and update the user requested value
> > with what the HW will actually give us?
>
> Yes, this is an important point so we need to understand if this is
> necessary!
>
> The thing that was bothering me here was if the application asks for
> some colour space but the driver gives us something else that the
> ColorSpace class doesn't recognise, so we get "undefined" back, at
> least in some of the fields.
>
> Now, if the application sees that validate() returns "adjusted", but
> thinks "whatever, I'll just go with it", then having "undefined" in
> the colour space that we now request is actually an error. (Or at
> least, I've defined it to be such.)
>
> Alternatively, if you look at the returned colour space, see that it's
> "fully defined", you can now set "requested = actual" and try
> validate() again, and it won't get "adjusted" (on account of the
> colour space, anyway).
>
> Does this make any sense, or am I being way too devious?
>

Yes, that reasoning makes sense.  I suppose we can achieve the same result
with
a single member variable, but add a tad more logic to the applications. I
have no strong
inclination either way.

Naush


>
> Thanks!
>
> Davis
>
> >
> >>
> >>         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
> >>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20211025/f4ee89eb/attachment.htm>