| Message ID | 20211118151933.15627-3-david.plowman@raspberrypi.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi David, Thank you for the patch On 11/18/21 8:49 PM, David Plowman wrote: > This is so that applications can choose appropriate color spaces which > will then be passed down to the V4L2 devices. Was expecting a plumbing to cam here, but it might follow up in later in the series or on top. Anyways not a blocker so Reviewed-by: Umang Jain <umang.jain@ideasonboard.com> > 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@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. > + * Alternatively the color space may be left unset, in which case it will > + * be up to the driver to choose a color space. > + * > + * 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. > + */ > + > /** > * \fn StreamConfiguration::stream() > * \brief Retrieve the stream associated with the configuration
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@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 ? > + * 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. > + * > + * 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. Thanks j > + */ > + > /** > * \fn StreamConfiguration::stream() > * \brief Retrieve the stream associated with the configuration > -- > 2.20.1 >
Hi Jacopo Thanks for the review! On Wed, 24 Nov 2021 at 23:38, Jacopo Mondi <jacopo@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@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 > >
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. + * Alternatively the color space may be left unset, in which case it will + * be up to the driver to choose a color space. + * + * 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. + */ + /** * \fn StreamConfiguration::stream() * \brief Retrieve the stream associated with the configuration
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@raspberrypi.com> --- include/libcamera/stream.h | 3 +++ src/libcamera/stream.cpp | 14 ++++++++++++++ 2 files changed, 17 insertions(+)