| Message ID | 20211020110825.12902-3-david.plowman@raspberrypi.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi David, Thank you for your patch. On Wed, 20 Oct 2021 at 12:08, David Plowman <david.plowman@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@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? > 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 > >
Hi Naush Thanks for the feedback! On Mon, 25 Oct 2021 at 08:45, Naushir Patuck <naush@raspberrypi.com> wrote: > > Hi David, > > Thank you for your patch. > > On Wed, 20 Oct 2021 at 12:08, David Plowman <david.plowman@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@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? 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 >>
Hi David, On Mon, 25 Oct 2021 at 10:37, David Plowman <david.plowman@raspberrypi.com> wrote: > Hi Naush > > Thanks for the feedback! > > On Mon, 25 Oct 2021 at 08:45, Naushir Patuck <naush@raspberrypi.com> > wrote: > > > > Hi David, > > > > Thank you for your patch. > > > > On Wed, 20 Oct 2021 at 12:08, David Plowman < > david.plowman@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@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 > >> >
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; + 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
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@raspberrypi.com> --- include/libcamera/stream.h | 4 ++++ src/libcamera/stream.cpp | 25 +++++++++++++++++++++++++ 2 files changed, 29 insertions(+)