| Message ID | 20230118085953.7027-2-naush@raspberrypi.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Naush, Quoting Naushir Patuck via libcamera-devel (2023-01-18 08:59:42) > Add a new hints flags field in the StreamConfiguration structure to allow the > application to specify certain intended behavior when driving libcamera. > Pipeline handlers are expected to look at these hint flags and may optimise > internal operations based on them. > > Currently, only one flag is listed, OptionalStream, which the application can > set to inform the pipeline handler that a buffer may not be provided on every > Request for a given stream. Sorry - Laurent had comments on this yesterday when I was discussing with him, so I don't know which way it will go yet... I still think this is the better way around, but we'll have to consider that this is an 'ABI' breakage, as now apps that were able to use two streams won't unless they explicitly provide all buffers or inform libcamera that they won't provide all buffers.... > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > --- > include/libcamera/stream.h | 8 ++++++++ > src/libcamera/stream.cpp | 24 ++++++++++++++++++++++++ > 2 files changed, 32 insertions(+) > > diff --git a/include/libcamera/stream.h b/include/libcamera/stream.h > index 29235ddf0d8a..233ff0450451 100644 > --- a/include/libcamera/stream.h > +++ b/include/libcamera/stream.h > @@ -13,6 +13,8 @@ > #include <string> > #include <vector> > > +#include <libcamera/base/flags.h> > + > #include <libcamera/color_space.h> > #include <libcamera/framebuffer.h> > #include <libcamera/geometry.h> > @@ -51,6 +53,12 @@ struct StreamConfiguration { > > std::optional<ColorSpace> colorSpace; > > + enum class Hint { > + None = 0, > + OptionalStream = (1 << 0), > + }; > + Flags<Hint> hints; > + > 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 67f308157fbf..b7de85f8b9c7 100644 > --- a/src/libcamera/stream.cpp > +++ b/src/libcamera/stream.cpp > @@ -349,6 +349,30 @@ StreamConfiguration::StreamConfiguration(const StreamFormats &formats) > * color spaces can be supported and in what combinations. > */ > > +/** > + * \enum StreamConfiguration::Hint > + * \brief List of available hint flags provided by the application for a stream > + * > + * \var StreamConfiguration::Hint::None > + * No hints for this stream. > + * \var StreamConfiguration::Hint::OptionalStream > + * Informs the pipeline handler that the application may not always provide a > + * buffer for the configured stream in every Request. This may ensure the > + * pipeline handler allocates additional stream buffers for internal use. > + */ > + > +/** > + * \var StreamConfiguration::hints > + * \brief Application provided StreamConfiguration::Hint flags for specific > + * stream behavior > + * > + * Provides hints from the application to the pipeline handlers on how it > + * intends on handling a given configured stream. These hints may alter the > + * behavior of the pipeline handlers, for example, by allocating additional > + * buffers for internal use if an application does not guarantee buffers will be > + * provided in every Request for a stream. > + */ > + > /** > * \fn StreamConfiguration::stream() > * \brief Retrieve the stream associated with the configuration > -- > 2.25.1 >
On Wed, 18 Jan 2023 at 10:09, Kieran Bingham < kieran.bingham@ideasonboard.com> wrote: > Hi Naush, > > Quoting Naushir Patuck via libcamera-devel (2023-01-18 08:59:42) > > Add a new hints flags field in the StreamConfiguration structure to > allow the > > application to specify certain intended behavior when driving libcamera. > > Pipeline handlers are expected to look at these hint flags and may > optimise > > internal operations based on them. > > > > Currently, only one flag is listed, OptionalStream, which the > application can > > set to inform the pipeline handler that a buffer may not be provided on > every > > Request for a given stream. > > Sorry - Laurent had comments on this yesterday when I was discussing > with him, so I don't know which way it will go yet... > > I still think this is the better way around, but we'll have to consider > that this is an 'ABI' breakage, as now apps that were able to use two > streams won't unless they explicitly provide all buffers or inform > libcamera that they won't provide all buffers.... > In the earlier versions of this series, we didn't have hints but used config params to indicate this. Perhaps I should go back to this mechanism for the time being to avoid any ABI breakages until there is an agreed path forward? Naush > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > --- > > include/libcamera/stream.h | 8 ++++++++ > > src/libcamera/stream.cpp | 24 ++++++++++++++++++++++++ > > 2 files changed, 32 insertions(+) > > > > diff --git a/include/libcamera/stream.h b/include/libcamera/stream.h > > index 29235ddf0d8a..233ff0450451 100644 > > --- a/include/libcamera/stream.h > > +++ b/include/libcamera/stream.h > > @@ -13,6 +13,8 @@ > > #include <string> > > #include <vector> > > > > +#include <libcamera/base/flags.h> > > + > > #include <libcamera/color_space.h> > > #include <libcamera/framebuffer.h> > > #include <libcamera/geometry.h> > > @@ -51,6 +53,12 @@ struct StreamConfiguration { > > > > std::optional<ColorSpace> colorSpace; > > > > + enum class Hint { > > + None = 0, > > + OptionalStream = (1 << 0), > > + }; > > + Flags<Hint> hints; > > + > > 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 67f308157fbf..b7de85f8b9c7 100644 > > --- a/src/libcamera/stream.cpp > > +++ b/src/libcamera/stream.cpp > > @@ -349,6 +349,30 @@ StreamConfiguration::StreamConfiguration(const > StreamFormats &formats) > > * color spaces can be supported and in what combinations. > > */ > > > > +/** > > + * \enum StreamConfiguration::Hint > > + * \brief List of available hint flags provided by the application for > a stream > > + * > > + * \var StreamConfiguration::Hint::None > > + * No hints for this stream. > > + * \var StreamConfiguration::Hint::OptionalStream > > + * Informs the pipeline handler that the application may not always > provide a > > + * buffer for the configured stream in every Request. This may ensure > the > > + * pipeline handler allocates additional stream buffers for internal > use. > > + */ > > + > > +/** > > + * \var StreamConfiguration::hints > > + * \brief Application provided StreamConfiguration::Hint flags for > specific > > + * stream behavior > > + * > > + * Provides hints from the application to the pipeline handlers on how > it > > + * intends on handling a given configured stream. These hints may alter > the > > + * behavior of the pipeline handlers, for example, by allocating > additional > > + * buffers for internal use if an application does not guarantee > buffers will be > > + * provided in every Request for a stream. > > + */ > > + > > /** > > * \fn StreamConfiguration::stream() > > * \brief Retrieve the stream associated with the configuration > > -- > > 2.25.1 > > >
Hi all, On Wed, 18 Jan 2023 at 10:09, Kieran Bingham < kieran.bingham@ideasonboard.com> wrote: > Hi Naush, > > Quoting Naushir Patuck via libcamera-devel (2023-01-18 08:59:42) > > Add a new hints flags field in the StreamConfiguration structure to > allow the > > application to specify certain intended behavior when driving libcamera. > > Pipeline handlers are expected to look at these hint flags and may > optimise > > internal operations based on them. > > > > Currently, only one flag is listed, OptionalStream, which the > application can > > set to inform the pipeline handler that a buffer may not be provided on > every > > Request for a given stream. > > Sorry - Laurent had comments on this yesterday when I was discussing > with him, so I don't know which way it will go yet... > > I still think this is the better way around, but we'll have to consider > that this is an 'ABI' breakage, as now apps that were able to use two > streams won't unless they explicitly provide all buffers or inform > libcamera that they won't provide all buffers.... > Has there been any consensus reached on this? I'm keen to get out an updated patch by the end of the day if any other changes are needed. Regards, Naush > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > --- > > include/libcamera/stream.h | 8 ++++++++ > > src/libcamera/stream.cpp | 24 ++++++++++++++++++++++++ > > 2 files changed, 32 insertions(+) > > > > diff --git a/include/libcamera/stream.h b/include/libcamera/stream.h > > index 29235ddf0d8a..233ff0450451 100644 > > --- a/include/libcamera/stream.h > > +++ b/include/libcamera/stream.h > > @@ -13,6 +13,8 @@ > > #include <string> > > #include <vector> > > > > +#include <libcamera/base/flags.h> > > + > > #include <libcamera/color_space.h> > > #include <libcamera/framebuffer.h> > > #include <libcamera/geometry.h> > > @@ -51,6 +53,12 @@ struct StreamConfiguration { > > > > std::optional<ColorSpace> colorSpace; > > > > + enum class Hint { > > + None = 0, > > + OptionalStream = (1 << 0), > > + }; > > + Flags<Hint> hints; > > + > > 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 67f308157fbf..b7de85f8b9c7 100644 > > --- a/src/libcamera/stream.cpp > > +++ b/src/libcamera/stream.cpp > > @@ -349,6 +349,30 @@ StreamConfiguration::StreamConfiguration(const > StreamFormats &formats) > > * color spaces can be supported and in what combinations. > > */ > > > > +/** > > + * \enum StreamConfiguration::Hint > > + * \brief List of available hint flags provided by the application for > a stream > > + * > > + * \var StreamConfiguration::Hint::None > > + * No hints for this stream. > > + * \var StreamConfiguration::Hint::OptionalStream > > + * Informs the pipeline handler that the application may not always > provide a > > + * buffer for the configured stream in every Request. This may ensure > the > > + * pipeline handler allocates additional stream buffers for internal > use. > > + */ > > + > > +/** > > + * \var StreamConfiguration::hints > > + * \brief Application provided StreamConfiguration::Hint flags for > specific > > + * stream behavior > > + * > > + * Provides hints from the application to the pipeline handlers on how > it > > + * intends on handling a given configured stream. These hints may alter > the > > + * behavior of the pipeline handlers, for example, by allocating > additional > > + * buffers for internal use if an application does not guarantee > buffers will be > > + * provided in every Request for a stream. > > + */ > > + > > /** > > * \fn StreamConfiguration::stream() > > * \brief Retrieve the stream associated with the configuration > > -- > > 2.25.1 > > >
Quoting Naushir Patuck (2023-01-20 08:04:41) > Hi all, > > > On Wed, 18 Jan 2023 at 10:09, Kieran Bingham < > kieran.bingham@ideasonboard.com> wrote: > > > Hi Naush, > > > > Quoting Naushir Patuck via libcamera-devel (2023-01-18 08:59:42) > > > Add a new hints flags field in the StreamConfiguration structure to > > allow the > > > application to specify certain intended behavior when driving libcamera. > > > Pipeline handlers are expected to look at these hint flags and may > > optimise > > > internal operations based on them. > > > > > > Currently, only one flag is listed, OptionalStream, which the > > application can > > > set to inform the pipeline handler that a buffer may not be provided on > > every > > > Request for a given stream. > > > > Sorry - Laurent had comments on this yesterday when I was discussing > > with him, so I don't know which way it will go yet... > > > > I still think this is the better way around, but we'll have to consider > > that this is an 'ABI' breakage, as now apps that were able to use two > > streams won't unless they explicitly provide all buffers or inform > > libcamera that they won't provide all buffers.... > > > > Has there been any consensus reached on this? > I'm keen to get out an updated patch by the end of the day if any other > changes are needed. I'm still trying to get to the bottom of all the consequences either way. I'm afraid now I have a worry about 'my' suggestion to require applications to specify optional streams. At the moment, I think the only way to directly configure the sensor mode is to configure a raw stream, and then simply not request buffers on that stream. If we make it so applictions must declare that the stream is an OptionalStream - setting the raw sensor mode would likely have to directly inform that the raw buffers are not going to be provided. That might not be a bad thing, but I wanted to highlight it. Are there any other / better options to configure the sensor mode? -- Kieran > > Regards, > Naush > > > > > > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > > --- > > > include/libcamera/stream.h | 8 ++++++++ > > > src/libcamera/stream.cpp | 24 ++++++++++++++++++++++++ > > > 2 files changed, 32 insertions(+) > > > > > > diff --git a/include/libcamera/stream.h b/include/libcamera/stream.h > > > index 29235ddf0d8a..233ff0450451 100644 > > > --- a/include/libcamera/stream.h > > > +++ b/include/libcamera/stream.h > > > @@ -13,6 +13,8 @@ > > > #include <string> > > > #include <vector> > > > > > > +#include <libcamera/base/flags.h> > > > + > > > #include <libcamera/color_space.h> > > > #include <libcamera/framebuffer.h> > > > #include <libcamera/geometry.h> > > > @@ -51,6 +53,12 @@ struct StreamConfiguration { > > > > > > std::optional<ColorSpace> colorSpace; > > > > > > + enum class Hint { > > > + None = 0, > > > + OptionalStream = (1 << 0), > > > + }; > > > + Flags<Hint> hints; > > > + > > > 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 67f308157fbf..b7de85f8b9c7 100644 > > > --- a/src/libcamera/stream.cpp > > > +++ b/src/libcamera/stream.cpp > > > @@ -349,6 +349,30 @@ StreamConfiguration::StreamConfiguration(const > > StreamFormats &formats) > > > * color spaces can be supported and in what combinations. > > > */ > > > > > > +/** > > > + * \enum StreamConfiguration::Hint > > > + * \brief List of available hint flags provided by the application for > > a stream > > > + * > > > + * \var StreamConfiguration::Hint::None > > > + * No hints for this stream. > > > + * \var StreamConfiguration::Hint::OptionalStream > > > + * Informs the pipeline handler that the application may not always > > provide a > > > + * buffer for the configured stream in every Request. This may ensure > > the > > > + * pipeline handler allocates additional stream buffers for internal > > use. > > > + */ > > > + > > > +/** > > > + * \var StreamConfiguration::hints > > > + * \brief Application provided StreamConfiguration::Hint flags for > > specific > > > + * stream behavior > > > + * > > > + * Provides hints from the application to the pipeline handlers on how > > it > > > + * intends on handling a given configured stream. These hints may alter > > the > > > + * behavior of the pipeline handlers, for example, by allocating > > additional > > > + * buffers for internal use if an application does not guarantee > > buffers will be > > > + * provided in every Request for a stream. > > > + */ > > > + > > > /** > > > * \fn StreamConfiguration::stream() > > > * \brief Retrieve the stream associated with the configuration > > > -- > > > 2.25.1 > > > > >
Quoting Naushir Patuck (2023-01-18 11:16:40) > On Wed, 18 Jan 2023 at 10:09, Kieran Bingham < > kieran.bingham@ideasonboard.com> wrote: > > > Hi Naush, > > > > Quoting Naushir Patuck via libcamera-devel (2023-01-18 08:59:42) > > > Add a new hints flags field in the StreamConfiguration structure to > > allow the > > > application to specify certain intended behavior when driving libcamera. > > > Pipeline handlers are expected to look at these hint flags and may > > optimise > > > internal operations based on them. > > > > > > Currently, only one flag is listed, OptionalStream, which the > > application can > > > set to inform the pipeline handler that a buffer may not be provided on > > every > > > Request for a given stream. > > > > Sorry - Laurent had comments on this yesterday when I was discussing > > with him, so I don't know which way it will go yet... > > > > I still think this is the better way around, but we'll have to consider > > that this is an 'ABI' breakage, as now apps that were able to use two > > streams won't unless they explicitly provide all buffers or inform > > libcamera that they won't provide all buffers.... > > > > In the earlier versions of this series, we didn't have hints but used config > params to indicate this. Perhaps I should go back to this mechanism for the > time being to avoid any ABI breakages until there is an agreed path forward? That's just swapping one ABI breakage for another - because then the hint would change (or we'd have both a 'MandatoryStream' hint, and an 'OptionalStream' hint ?) > > Naush > > > > > > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > > --- > > > include/libcamera/stream.h | 8 ++++++++ > > > src/libcamera/stream.cpp | 24 ++++++++++++++++++++++++ > > > 2 files changed, 32 insertions(+) > > > > > > diff --git a/include/libcamera/stream.h b/include/libcamera/stream.h > > > index 29235ddf0d8a..233ff0450451 100644 > > > --- a/include/libcamera/stream.h > > > +++ b/include/libcamera/stream.h > > > @@ -13,6 +13,8 @@ > > > #include <string> > > > #include <vector> > > > > > > +#include <libcamera/base/flags.h> > > > + > > > #include <libcamera/color_space.h> > > > #include <libcamera/framebuffer.h> > > > #include <libcamera/geometry.h> > > > @@ -51,6 +53,12 @@ struct StreamConfiguration { > > > > > > std::optional<ColorSpace> colorSpace; > > > > > > + enum class Hint { > > > + None = 0, > > > + OptionalStream = (1 << 0), > > > + }; > > > + Flags<Hint> hints; > > > + > > > 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 67f308157fbf..b7de85f8b9c7 100644 > > > --- a/src/libcamera/stream.cpp > > > +++ b/src/libcamera/stream.cpp > > > @@ -349,6 +349,30 @@ StreamConfiguration::StreamConfiguration(const > > StreamFormats &formats) > > > * color spaces can be supported and in what combinations. > > > */ > > > > > > +/** > > > + * \enum StreamConfiguration::Hint > > > + * \brief List of available hint flags provided by the application for > > a stream > > > + * > > > + * \var StreamConfiguration::Hint::None > > > + * No hints for this stream. > > > + * \var StreamConfiguration::Hint::OptionalStream > > > + * Informs the pipeline handler that the application may not always > > provide a > > > + * buffer for the configured stream in every Request. This may ensure > > the > > > + * pipeline handler allocates additional stream buffers for internal > > use. > > > + */ > > > + > > > +/** > > > + * \var StreamConfiguration::hints > > > + * \brief Application provided StreamConfiguration::Hint flags for > > specific > > > + * stream behavior > > > + * > > > + * Provides hints from the application to the pipeline handlers on how > > it > > > + * intends on handling a given configured stream. These hints may alter > > the > > > + * behavior of the pipeline handlers, for example, by allocating > > additional > > > + * buffers for internal use if an application does not guarantee > > buffers will be > > > + * provided in every Request for a stream. > > > + */ > > > + > > > /** > > > * \fn StreamConfiguration::stream() > > > * \brief Retrieve the stream associated with the configuration > > > -- > > > 2.25.1 > > > > >
On Fri, 20 Jan 2023 at 10:42, Kieran Bingham < kieran.bingham@ideasonboard.com> wrote: > Quoting Naushir Patuck (2023-01-18 11:16:40) > > On Wed, 18 Jan 2023 at 10:09, Kieran Bingham < > > kieran.bingham@ideasonboard.com> wrote: > > > > > Hi Naush, > > > > > > Quoting Naushir Patuck via libcamera-devel (2023-01-18 08:59:42) > > > > Add a new hints flags field in the StreamConfiguration structure to > > > allow the > > > > application to specify certain intended behavior when driving > libcamera. > > > > Pipeline handlers are expected to look at these hint flags and may > > > optimise > > > > internal operations based on them. > > > > > > > > Currently, only one flag is listed, OptionalStream, which the > > > application can > > > > set to inform the pipeline handler that a buffer may not be provided > on > > > every > > > > Request for a given stream. > > > > > > Sorry - Laurent had comments on this yesterday when I was discussing > > > with him, so I don't know which way it will go yet... > > > > > > I still think this is the better way around, but we'll have to consider > > > that this is an 'ABI' breakage, as now apps that were able to use two > > > streams won't unless they explicitly provide all buffers or inform > > > libcamera that they won't provide all buffers.... > > > > > > > In the earlier versions of this series, we didn't have hints but used > config > > params to indicate this. Perhaps I should go back to this mechanism for > the > > time being to avoid any ABI breakages until there is an agreed path > forward? > > That's just swapping one ABI breakage for another - because then the > hint would change (or we'd have both a 'MandatoryStream' hint, and an > 'OptionalStream' hint ?) > If the "hint" were to move back to the config file, I would make it such that the value is true for applications that provide buffers for every configured stream on every request. This would be defaulted to false, and libcamera-apps/picamera2 would set a config file with the flag to true. This would preserve behavior for any other existing applications that don't pass in a configuration file, though it may be suboptimal with allocations. No ABI breakages either... > > > > > > > Naush > > > > > > > > > > > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > > > --- > > > > include/libcamera/stream.h | 8 ++++++++ > > > > src/libcamera/stream.cpp | 24 ++++++++++++++++++++++++ > > > > 2 files changed, 32 insertions(+) > > > > > > > > diff --git a/include/libcamera/stream.h b/include/libcamera/stream.h > > > > index 29235ddf0d8a..233ff0450451 100644 > > > > --- a/include/libcamera/stream.h > > > > +++ b/include/libcamera/stream.h > > > > @@ -13,6 +13,8 @@ > > > > #include <string> > > > > #include <vector> > > > > > > > > +#include <libcamera/base/flags.h> > > > > + > > > > #include <libcamera/color_space.h> > > > > #include <libcamera/framebuffer.h> > > > > #include <libcamera/geometry.h> > > > > @@ -51,6 +53,12 @@ struct StreamConfiguration { > > > > > > > > std::optional<ColorSpace> colorSpace; > > > > > > > > + enum class Hint { > > > > + None = 0, > > > > + OptionalStream = (1 << 0), > > > > + }; > > > > + Flags<Hint> hints; > > > > + > > > > 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 67f308157fbf..b7de85f8b9c7 100644 > > > > --- a/src/libcamera/stream.cpp > > > > +++ b/src/libcamera/stream.cpp > > > > @@ -349,6 +349,30 @@ StreamConfiguration::StreamConfiguration(const > > > StreamFormats &formats) > > > > * color spaces can be supported and in what combinations. > > > > */ > > > > > > > > +/** > > > > + * \enum StreamConfiguration::Hint > > > > + * \brief List of available hint flags provided by the application > for > > > a stream > > > > + * > > > > + * \var StreamConfiguration::Hint::None > > > > + * No hints for this stream. > > > > + * \var StreamConfiguration::Hint::OptionalStream > > > > + * Informs the pipeline handler that the application may not always > > > provide a > > > > + * buffer for the configured stream in every Request. This may > ensure > > > the > > > > + * pipeline handler allocates additional stream buffers for internal > > > use. > > > > + */ > > > > + > > > > +/** > > > > + * \var StreamConfiguration::hints > > > > + * \brief Application provided StreamConfiguration::Hint flags for > > > specific > > > > + * stream behavior > > > > + * > > > > + * Provides hints from the application to the pipeline handlers on > how > > > it > > > > + * intends on handling a given configured stream. These hints may > alter > > > the > > > > + * behavior of the pipeline handlers, for example, by allocating > > > additional > > > > + * buffers for internal use if an application does not guarantee > > > buffers will be > > > > + * provided in every Request for a stream. > > > > + */ > > > > + > > > > /** > > > > * \fn StreamConfiguration::stream() > > > > * \brief Retrieve the stream associated with the configuration > > > > -- > > > > 2.25.1 > > > > > > > >
Hi Kieran, On Fri, 20 Jan 2023 at 10:40, Kieran Bingham < kieran.bingham@ideasonboard.com> wrote: > Quoting Naushir Patuck (2023-01-20 08:04:41) > > Hi all, > > > > > > On Wed, 18 Jan 2023 at 10:09, Kieran Bingham < > > kieran.bingham@ideasonboard.com> wrote: > > > > > Hi Naush, > > > > > > Quoting Naushir Patuck via libcamera-devel (2023-01-18 08:59:42) > > > > Add a new hints flags field in the StreamConfiguration structure to > > > allow the > > > > application to specify certain intended behavior when driving > libcamera. > > > > Pipeline handlers are expected to look at these hint flags and may > > > optimise > > > > internal operations based on them. > > > > > > > > Currently, only one flag is listed, OptionalStream, which the > > > application can > > > > set to inform the pipeline handler that a buffer may not be provided > on > > > every > > > > Request for a given stream. > > > > > > Sorry - Laurent had comments on this yesterday when I was discussing > > > with him, so I don't know which way it will go yet... > > > > > > I still think this is the better way around, but we'll have to consider > > > that this is an 'ABI' breakage, as now apps that were able to use two > > > streams won't unless they explicitly provide all buffers or inform > > > libcamera that they won't provide all buffers.... > > > > > > > Has there been any consensus reached on this? > > I'm keen to get out an updated patch by the end of the day if any other > > changes are needed. > > I'm still trying to get to the bottom of all the consequences either > way. > > I'm afraid now I have a worry about 'my' suggestion to require > applications to specify optional streams. > > At the moment, I think the only way to directly configure the sensor > mode is to configure a raw stream, and then simply not request buffers > on that stream. If we make it so applictions must declare that the > stream is an OptionalStream - setting the raw sensor mode would likely > have to directly inform that the raw buffers are not going to be > provided. > > That might not be a bad thing, but I wanted to highlight it. > > Are there any other / better options to configure the sensor mode? > This is the only way to configure a particular sensor mode with the existing API. Regards, Naush > > -- > Kieran > > > > > > > Regards, > > Naush > > > > > > > > > > > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > > > --- > > > > include/libcamera/stream.h | 8 ++++++++ > > > > src/libcamera/stream.cpp | 24 ++++++++++++++++++++++++ > > > > 2 files changed, 32 insertions(+) > > > > > > > > diff --git a/include/libcamera/stream.h b/include/libcamera/stream.h > > > > index 29235ddf0d8a..233ff0450451 100644 > > > > --- a/include/libcamera/stream.h > > > > +++ b/include/libcamera/stream.h > > > > @@ -13,6 +13,8 @@ > > > > #include <string> > > > > #include <vector> > > > > > > > > +#include <libcamera/base/flags.h> > > > > + > > > > #include <libcamera/color_space.h> > > > > #include <libcamera/framebuffer.h> > > > > #include <libcamera/geometry.h> > > > > @@ -51,6 +53,12 @@ struct StreamConfiguration { > > > > > > > > std::optional<ColorSpace> colorSpace; > > > > > > > > + enum class Hint { > > > > + None = 0, > > > > + OptionalStream = (1 << 0), > > > > + }; > > > > + Flags<Hint> hints; > > > > + > > > > 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 67f308157fbf..b7de85f8b9c7 100644 > > > > --- a/src/libcamera/stream.cpp > > > > +++ b/src/libcamera/stream.cpp > > > > @@ -349,6 +349,30 @@ StreamConfiguration::StreamConfiguration(const > > > StreamFormats &formats) > > > > * color spaces can be supported and in what combinations. > > > > */ > > > > > > > > +/** > > > > + * \enum StreamConfiguration::Hint > > > > + * \brief List of available hint flags provided by the application > for > > > a stream > > > > + * > > > > + * \var StreamConfiguration::Hint::None > > > > + * No hints for this stream. > > > > + * \var StreamConfiguration::Hint::OptionalStream > > > > + * Informs the pipeline handler that the application may not always > > > provide a > > > > + * buffer for the configured stream in every Request. This may > ensure > > > the > > > > + * pipeline handler allocates additional stream buffers for internal > > > use. > > > > + */ > > > > + > > > > +/** > > > > + * \var StreamConfiguration::hints > > > > + * \brief Application provided StreamConfiguration::Hint flags for > > > specific > > > > + * stream behavior > > > > + * > > > > + * Provides hints from the application to the pipeline handlers on > how > > > it > > > > + * intends on handling a given configured stream. These hints may > alter > > > the > > > > + * behavior of the pipeline handlers, for example, by allocating > > > additional > > > > + * buffers for internal use if an application does not guarantee > > > buffers will be > > > > + * provided in every Request for a stream. > > > > + */ > > > > + > > > > /** > > > > * \fn StreamConfiguration::stream() > > > > * \brief Retrieve the stream associated with the configuration > > > > -- > > > > 2.25.1 > > > > > > > >
Quoting Naushir Patuck (2023-01-20 10:53:38) > On Fri, 20 Jan 2023 at 10:42, Kieran Bingham < > kieran.bingham@ideasonboard.com> wrote: > > > Quoting Naushir Patuck (2023-01-18 11:16:40) > > > On Wed, 18 Jan 2023 at 10:09, Kieran Bingham < > > > kieran.bingham@ideasonboard.com> wrote: > > > > > > > Hi Naush, > > > > > > > > Quoting Naushir Patuck via libcamera-devel (2023-01-18 08:59:42) > > > > > Add a new hints flags field in the StreamConfiguration structure to > > > > allow the > > > > > application to specify certain intended behavior when driving > > libcamera. > > > > > Pipeline handlers are expected to look at these hint flags and may > > > > optimise > > > > > internal operations based on them. > > > > > > > > > > Currently, only one flag is listed, OptionalStream, which the > > > > application can > > > > > set to inform the pipeline handler that a buffer may not be provided > > on > > > > every > > > > > Request for a given stream. > > > > > > > > Sorry - Laurent had comments on this yesterday when I was discussing > > > > with him, so I don't know which way it will go yet... > > > > > > > > I still think this is the better way around, but we'll have to consider > > > > that this is an 'ABI' breakage, as now apps that were able to use two > > > > streams won't unless they explicitly provide all buffers or inform > > > > libcamera that they won't provide all buffers.... > > > > > > > > > > In the earlier versions of this series, we didn't have hints but used > > config > > > params to indicate this. Perhaps I should go back to this mechanism for > > the > > > time being to avoid any ABI breakages until there is an agreed path > > forward? > > > > That's just swapping one ABI breakage for another - because then the > > hint would change (or we'd have both a 'MandatoryStream' hint, and an > > 'OptionalStream' hint ?) > > > > If the "hint" were to move back to the config file, I would make it such Move back to ? Was there previous discussion that moved this out of a config file that I've missed? > that the value is true for applications that provide buffers for every > configured stream on every request. > This would be defaulted to false, and libcamera-apps/picamera2 would set a > config file with the flag to true. This would preserve behavior for any > other existing applications that don't pass in a configuration file, though > it may be suboptimal with allocations. No ABI breakages either... -- Kieran
On Fri, 20 Jan 2023 at 11:27, Kieran Bingham < kieran.bingham@ideasonboard.com> wrote: > Quoting Naushir Patuck (2023-01-20 10:53:38) > > On Fri, 20 Jan 2023 at 10:42, Kieran Bingham < > > kieran.bingham@ideasonboard.com> wrote: > > > > > Quoting Naushir Patuck (2023-01-18 11:16:40) > > > > On Wed, 18 Jan 2023 at 10:09, Kieran Bingham < > > > > kieran.bingham@ideasonboard.com> wrote: > > > > > > > > > Hi Naush, > > > > > > > > > > Quoting Naushir Patuck via libcamera-devel (2023-01-18 08:59:42) > > > > > > Add a new hints flags field in the StreamConfiguration structure > to > > > > > allow the > > > > > > application to specify certain intended behavior when driving > > > libcamera. > > > > > > Pipeline handlers are expected to look at these hint flags and > may > > > > > optimise > > > > > > internal operations based on them. > > > > > > > > > > > > Currently, only one flag is listed, OptionalStream, which the > > > > > application can > > > > > > set to inform the pipeline handler that a buffer may not be > provided > > > on > > > > > every > > > > > > Request for a given stream. > > > > > > > > > > Sorry - Laurent had comments on this yesterday when I was > discussing > > > > > with him, so I don't know which way it will go yet... > > > > > > > > > > I still think this is the better way around, but we'll have to > consider > > > > > that this is an 'ABI' breakage, as now apps that were able to use > two > > > > > streams won't unless they explicitly provide all buffers or inform > > > > > libcamera that they won't provide all buffers.... > > > > > > > > > > > > > In the earlier versions of this series, we didn't have hints but used > > > config > > > > params to indicate this. Perhaps I should go back to this mechanism > for > > > the > > > > time being to avoid any ABI breakages until there is an agreed path > > > forward? > > > > > > That's just swapping one ABI breakage for another - because then the > > > hint would change (or we'd have both a 'MandatoryStream' hint, and an > > > 'OptionalStream' hint ?) > > > > > > > If the "hint" were to move back to the config file, I would make it such > > Move back to ? Was there previous discussion that moved this out of a > config file that I've missed? > In earlier versions of this series, these hints were effectively flags in the config files. It was suggested to move these into the StreamConfig as hints since they may be useful to other pipeline handlers, and enforce explicit application behavior through the API. > > > that the value is true for applications that provide buffers for every > > configured stream on every request. > > This would be defaulted to false, and libcamera-apps/picamera2 would set > a > > config file with the flag to true. This would preserve behavior for any > > other existing applications that don't pass in a configuration file, > though > > it may be suboptimal with allocations. No ABI breakages either... > > -- > Kieran >
Quoting Naushir Patuck (2023-01-20 11:34:30) > On Fri, 20 Jan 2023 at 11:27, Kieran Bingham < > kieran.bingham@ideasonboard.com> wrote: > > > Quoting Naushir Patuck (2023-01-20 10:53:38) > > > On Fri, 20 Jan 2023 at 10:42, Kieran Bingham < > > > kieran.bingham@ideasonboard.com> wrote: > > > > > > > Quoting Naushir Patuck (2023-01-18 11:16:40) > > > > > On Wed, 18 Jan 2023 at 10:09, Kieran Bingham < > > > > > kieran.bingham@ideasonboard.com> wrote: > > > > > > > > > > > Hi Naush, > > > > > > > > > > > > Quoting Naushir Patuck via libcamera-devel (2023-01-18 08:59:42) > > > > > > > Add a new hints flags field in the StreamConfiguration structure > > to > > > > > > allow the > > > > > > > application to specify certain intended behavior when driving > > > > libcamera. > > > > > > > Pipeline handlers are expected to look at these hint flags and > > may > > > > > > optimise > > > > > > > internal operations based on them. > > > > > > > > > > > > > > Currently, only one flag is listed, OptionalStream, which the > > > > > > application can > > > > > > > set to inform the pipeline handler that a buffer may not be > > provided > > > > on > > > > > > every > > > > > > > Request for a given stream. > > > > > > > > > > > > Sorry - Laurent had comments on this yesterday when I was > > discussing > > > > > > with him, so I don't know which way it will go yet... > > > > > > > > > > > > I still think this is the better way around, but we'll have to > > consider > > > > > > that this is an 'ABI' breakage, as now apps that were able to use > > two > > > > > > streams won't unless they explicitly provide all buffers or inform > > > > > > libcamera that they won't provide all buffers.... > > > > > > > > > > > > > > > > In the earlier versions of this series, we didn't have hints but used > > > > config > > > > > params to indicate this. Perhaps I should go back to this mechanism > > for > > > > the > > > > > time being to avoid any ABI breakages until there is an agreed path > > > > forward? > > > > > > > > That's just swapping one ABI breakage for another - because then the > > > > hint would change (or we'd have both a 'MandatoryStream' hint, and an > > > > 'OptionalStream' hint ?) > > > > > > > > > > If the "hint" were to move back to the config file, I would make it such > > > > Move back to ? Was there previous discussion that moved this out of a > > config file that I've missed? > > > > In earlier versions of this series, these hints were effectively flags in > the config files. > It was suggested to move these into the StreamConfig as hints since they > may be useful to other pipeline handlers, and enforce explicit application > behavior through the API. > Ok - in that case, that confirms that the checks/assertions in [9/12] should be in the core, not just against raspberry pi pipeline handler. This may also require updating any tests that could be affected, and lc-compliance. It may be helpful to split out this core API 'hint's part to it's own series as I don't think it should necessarily block the configuration file work here. -- KB > > > that the value is true for applications that provide buffers for every > > > configured stream on every request. > > > This would be defaulted to false, and libcamera-apps/picamera2 would set > > a > > > config file with the flag to true. This would preserve behavior for any > > > other existing applications that don't pass in a configuration file, > > though > > > it may be suboptimal with allocations. No ABI breakages either... > > > > -- > > Kieran > >
Hi Kieran, On Fri, 20 Jan 2023 at 11:42, Kieran Bingham < kieran.bingham@ideasonboard.com> wrote: > Quoting Naushir Patuck (2023-01-20 11:34:30) > > On Fri, 20 Jan 2023 at 11:27, Kieran Bingham < > > kieran.bingham@ideasonboard.com> wrote: > > > > > Quoting Naushir Patuck (2023-01-20 10:53:38) > > > > On Fri, 20 Jan 2023 at 10:42, Kieran Bingham < > > > > kieran.bingham@ideasonboard.com> wrote: > > > > > > > > > Quoting Naushir Patuck (2023-01-18 11:16:40) > > > > > > On Wed, 18 Jan 2023 at 10:09, Kieran Bingham < > > > > > > kieran.bingham@ideasonboard.com> wrote: > > > > > > > > > > > > > Hi Naush, > > > > > > > > > > > > > > Quoting Naushir Patuck via libcamera-devel (2023-01-18 > 08:59:42) > > > > > > > > Add a new hints flags field in the StreamConfiguration > structure > > > to > > > > > > > allow the > > > > > > > > application to specify certain intended behavior when driving > > > > > libcamera. > > > > > > > > Pipeline handlers are expected to look at these hint flags > and > > > may > > > > > > > optimise > > > > > > > > internal operations based on them. > > > > > > > > > > > > > > > > Currently, only one flag is listed, OptionalStream, which the > > > > > > > application can > > > > > > > > set to inform the pipeline handler that a buffer may not be > > > provided > > > > > on > > > > > > > every > > > > > > > > Request for a given stream. > > > > > > > > > > > > > > Sorry - Laurent had comments on this yesterday when I was > > > discussing > > > > > > > with him, so I don't know which way it will go yet... > > > > > > > > > > > > > > I still think this is the better way around, but we'll have to > > > consider > > > > > > > that this is an 'ABI' breakage, as now apps that were able to > use > > > two > > > > > > > streams won't unless they explicitly provide all buffers or > inform > > > > > > > libcamera that they won't provide all buffers.... > > > > > > > > > > > > > > > > > > > In the earlier versions of this series, we didn't have hints but > used > > > > > config > > > > > > params to indicate this. Perhaps I should go back to this > mechanism > > > for > > > > > the > > > > > > time being to avoid any ABI breakages until there is an agreed > path > > > > > forward? > > > > > > > > > > That's just swapping one ABI breakage for another - because then > the > > > > > hint would change (or we'd have both a 'MandatoryStream' hint, and > an > > > > > 'OptionalStream' hint ?) > > > > > > > > > > > > > If the "hint" were to move back to the config file, I would make it > such > > > > > > Move back to ? Was there previous discussion that moved this out of a > > > config file that I've missed? > > > > > > > In earlier versions of this series, these hints were effectively flags in > > the config files. > > It was suggested to move these into the StreamConfig as hints since they > > may be useful to other pipeline handlers, and enforce explicit > application > > behavior through the API. > > > > Ok - in that case, that confirms that the checks/assertions in [9/12] > should be in the core, not just against raspberry pi pipeline handler. > I did consider this at the start, but didn't exactly know where to put the test. I tried to put it in PipelineHandler::doQueueRequest(), but we don't have access to the advertised StreamConfig structures there, do we? Any other place you could suggest where we have access to the objects? Naush > This may also require updating any tests that could be affected, and > lc-compliance. > > It may be helpful to split out this core API 'hint's part to it's own > series as I don't think it should necessarily block the configuration > file work here. > > -- > KB > > > > > > that the value is true for applications that provide buffers for > every > > > > configured stream on every request. > > > > This would be defaulted to false, and libcamera-apps/picamera2 would > set > > > a > > > > config file with the flag to true. This would preserve behavior for > any > > > > other existing applications that don't pass in a configuration file, > > > though > > > > it may be suboptimal with allocations. No ABI breakages either... > > > > > > -- > > > Kieran > > > >
On Fri, 20 Jan 2023 at 12:29, Naushir Patuck <naush@raspberrypi.com> wrote: > Hi Kieran, > > On Fri, 20 Jan 2023 at 11:42, Kieran Bingham < > kieran.bingham@ideasonboard.com> wrote: > >> Quoting Naushir Patuck (2023-01-20 11:34:30) >> > On Fri, 20 Jan 2023 at 11:27, Kieran Bingham < >> > kieran.bingham@ideasonboard.com> wrote: >> > >> > > Quoting Naushir Patuck (2023-01-20 10:53:38) >> > > > On Fri, 20 Jan 2023 at 10:42, Kieran Bingham < >> > > > kieran.bingham@ideasonboard.com> wrote: >> > > > >> > > > > Quoting Naushir Patuck (2023-01-18 11:16:40) >> > > > > > On Wed, 18 Jan 2023 at 10:09, Kieran Bingham < >> > > > > > kieran.bingham@ideasonboard.com> wrote: >> > > > > > >> > > > > > > Hi Naush, >> > > > > > > >> > > > > > > Quoting Naushir Patuck via libcamera-devel (2023-01-18 >> 08:59:42) >> > > > > > > > Add a new hints flags field in the StreamConfiguration >> structure >> > > to >> > > > > > > allow the >> > > > > > > > application to specify certain intended behavior when >> driving >> > > > > libcamera. >> > > > > > > > Pipeline handlers are expected to look at these hint flags >> and >> > > may >> > > > > > > optimise >> > > > > > > > internal operations based on them. >> > > > > > > > >> > > > > > > > Currently, only one flag is listed, OptionalStream, which >> the >> > > > > > > application can >> > > > > > > > set to inform the pipeline handler that a buffer may not be >> > > provided >> > > > > on >> > > > > > > every >> > > > > > > > Request for a given stream. >> > > > > > > >> > > > > > > Sorry - Laurent had comments on this yesterday when I was >> > > discussing >> > > > > > > with him, so I don't know which way it will go yet... >> > > > > > > >> > > > > > > I still think this is the better way around, but we'll have to >> > > consider >> > > > > > > that this is an 'ABI' breakage, as now apps that were able to >> use >> > > two >> > > > > > > streams won't unless they explicitly provide all buffers or >> inform >> > > > > > > libcamera that they won't provide all buffers.... >> > > > > > > >> > > > > > >> > > > > > In the earlier versions of this series, we didn't have hints >> but used >> > > > > config >> > > > > > params to indicate this. Perhaps I should go back to this >> mechanism >> > > for >> > > > > the >> > > > > > time being to avoid any ABI breakages until there is an agreed >> path >> > > > > forward? >> > > > > >> > > > > That's just swapping one ABI breakage for another - because then >> the >> > > > > hint would change (or we'd have both a 'MandatoryStream' hint, >> and an >> > > > > 'OptionalStream' hint ?) >> > > > > >> > > > >> > > > If the "hint" were to move back to the config file, I would make it >> such >> > > >> > > Move back to ? Was there previous discussion that moved this out of a >> > > config file that I've missed? >> > > >> > >> > In earlier versions of this series, these hints were effectively flags >> in >> > the config files. >> > It was suggested to move these into the StreamConfig as hints since they >> > may be useful to other pipeline handlers, and enforce explicit >> application >> > behavior through the API. >> > >> >> Ok - in that case, that confirms that the checks/assertions in [9/12] >> should be in the core, not just against raspberry pi pipeline handler. >> > > I did consider this at the start, but didn't exactly know where to put the > test. > I tried to put it in PipelineHandler::doQueueRequest(), but we don't have > access to the advertised StreamConfig structures there, do we? Any other > place you could suggest where we have access to the objects? > Actually, I did not look hard enough. Request::camera->stream() will give me access to the StreamConfiguration objects I think. I can then do the validation in PipelineHandler::doQueueRequest() as I originally intended. I'll make that change. The question of what sense to use for the hints is still outstanding though.... Regards, Naush > Naush > > >> This may also require updating any tests that could be affected, and >> lc-compliance. >> >> It may be helpful to split out this core API 'hint's part to it's own >> series as I don't think it should necessarily block the configuration >> file work here. >> >> -- >> KB >> >> >> > > > that the value is true for applications that provide buffers for >> every >> > > > configured stream on every request. >> > > > This would be defaulted to false, and libcamera-apps/picamera2 >> would set >> > > a >> > > > config file with the flag to true. This would preserve behavior >> for any >> > > > other existing applications that don't pass in a configuration file, >> > > though >> > > > it may be suboptimal with allocations. No ABI breakages either... >> > > >> > > -- >> > > Kieran >> > > >> >
On Fri, Jan 20, 2023 at 03:10:23PM +0000, Naushir Patuck via libcamera-devel wrote: > On Fri, 20 Jan 2023 at 12:29, Naushir Patuck wrote: > > On Fri, 20 Jan 2023 at 11:42, Kieran Bingham wrote: > >> Quoting Naushir Patuck (2023-01-20 11:34:30) > >> > On Fri, 20 Jan 2023 at 11:27, Kieran Bingham wrote: > >> > > Quoting Naushir Patuck (2023-01-20 10:53:38) > >> > > > On Fri, 20 Jan 2023 at 10:42, Kieran Bingham wrote: > >> > > > > Quoting Naushir Patuck (2023-01-18 11:16:40) > >> > > > > > On Wed, 18 Jan 2023 at 10:09, Kieran Bingham wrote: > >> > > > > > > Quoting Naushir Patuck via libcamera-devel (2023-01-18 08:59:42) > >> > > > > > > > Add a new hints flags field in the StreamConfiguration structure to allow the > >> > > > > > > > application to specify certain intended behavior when driving libcamera. > >> > > > > > > > Pipeline handlers are expected to look at these hint flags and may optimise > >> > > > > > > > internal operations based on them. > >> > > > > > > > > >> > > > > > > > Currently, only one flag is listed, OptionalStream, which the application can > >> > > > > > > > set to inform the pipeline handler that a buffer may not be provided on every > >> > > > > > > > Request for a given stream. > >> > > > > > > > >> > > > > > > Sorry - Laurent had comments on this yesterday when I was discussing > >> > > > > > > with him, so I don't know which way it will go yet... > >> > > > > > > > >> > > > > > > I still think this is the better way around, but we'll have to consider > >> > > > > > > that this is an 'ABI' breakage, as now apps that were able to use two > >> > > > > > > streams won't unless they explicitly provide all buffers or inform > >> > > > > > > libcamera that they won't provide all buffers.... > >> > > > > > > >> > > > > > In the earlier versions of this series, we didn't have hints but used config > >> > > > > > params to indicate this. Perhaps I should go back to this mechanism for the > >> > > > > > time being to avoid any ABI breakages until there is an agreed path forward? No no please, let's keep hints. The only way to agree on a path forward is to try things out, send out patches, and discuss them (I wish it could happen by magic instead, but I haven't found the right spell so far). Your series does it right, even if the road can sometimes be bumpy. > >> > > > > That's just swapping one ABI breakage for another - because then the > >> > > > > hint would change (or we'd have both a 'MandatoryStream' hint, and an > >> > > > > 'OptionalStream' hint ?) > >> > > > > >> > > > If the "hint" were to move back to the config file, I would make it such > >> > > > >> > > Move back to ? Was there previous discussion that moved this out of a > >> > > config file that I've missed? > >> > > >> > In earlier versions of this series, these hints were effectively flags in > >> > the config files. > >> > It was suggested to move these into the StreamConfig as hints since they > >> > may be useful to other pipeline handlers, and enforce explicit application > >> > behavior through the API. > >> > >> Ok - in that case, that confirms that the checks/assertions in [9/12] > >> should be in the core, not just against raspberry pi pipeline handler. Agreed. > > I did consider this at the start, but didn't exactly know where to put the test. > > I tried to put it in PipelineHandler::doQueueRequest(), but we don't have > > access to the advertised StreamConfig structures there, do we? Any other > > place you could suggest where we have access to the objects? > > Actually, I did not look hard enough. Request::camera->stream() will give me access > to the StreamConfiguration objects I think. I can then do the validation in > PipelineHandler::doQueueRequest() as I originally intended. I'll make that change. It would be best if we could validate requests synchronously though, but we can probably live with asynchronous validation to start with. Please test this with an application that submits an invalid request. Even better, a test in lc-compliance would be good. > The question of what sense to use for the hints is still outstanding > though.... The libcamera API makes buffers optional by default, to support, for instance, use cases with a viewfinder stream and a still image capture stream. Requiring a buffer for all streams in all requests by default would be a big change with lots of implications that we need to consider. Maybe we'll do so, but I don't think now is the right time. That's why I prefer for now a hint to indicate that the application will provide a buffer for the stream in every request. > >> This may also require updating any tests that could be affected, and > >> lc-compliance. > >> > >> It may be helpful to split out this core API 'hint's part to it's own > >> series as I don't think it should necessarily block the configuration > >> file work here. > >> > >> > > > that the value is true for applications that provide buffers for every > >> > > > configured stream on every request. > >> > > > This would be defaulted to false, and libcamera-apps/picamera2 would set a > >> > > > config file with the flag to true. This would preserve behavior for any > >> > > > other existing applications that don't pass in a configuration file, though > >> > > > it may be suboptimal with allocations. No ABI breakages either...
Hi Laurent, Thank you for the feedback. On Sun, 22 Jan 2023 at 21:31, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > On Fri, Jan 20, 2023 at 03:10:23PM +0000, Naushir Patuck via libcamera-devel wrote: > > On Fri, 20 Jan 2023 at 12:29, Naushir Patuck wrote: > > > On Fri, 20 Jan 2023 at 11:42, Kieran Bingham wrote: > > >> Quoting Naushir Patuck (2023-01-20 11:34:30) > > >> > On Fri, 20 Jan 2023 at 11:27, Kieran Bingham wrote: > > >> > > Quoting Naushir Patuck (2023-01-20 10:53:38) > > >> > > > On Fri, 20 Jan 2023 at 10:42, Kieran Bingham wrote: > > >> > > > > Quoting Naushir Patuck (2023-01-18 11:16:40) > > >> > > > > > On Wed, 18 Jan 2023 at 10:09, Kieran Bingham wrote: > > >> > > > > > > Quoting Naushir Patuck via libcamera-devel (2023-01-18 08:59:42) > > >> > > > > > > > Add a new hints flags field in the StreamConfiguration structure to allow the > > >> > > > > > > > application to specify certain intended behavior when driving libcamera. > > >> > > > > > > > Pipeline handlers are expected to look at these hint flags and may optimise > > >> > > > > > > > internal operations based on them. > > >> > > > > > > > > > >> > > > > > > > Currently, only one flag is listed, OptionalStream, which the application can > > >> > > > > > > > set to inform the pipeline handler that a buffer may not be provided on every > > >> > > > > > > > Request for a given stream. > > >> > > > > > > > > >> > > > > > > Sorry - Laurent had comments on this yesterday when I was discussing > > >> > > > > > > with him, so I don't know which way it will go yet... > > >> > > > > > > > > >> > > > > > > I still think this is the better way around, but we'll have to consider > > >> > > > > > > that this is an 'ABI' breakage, as now apps that were able to use two > > >> > > > > > > streams won't unless they explicitly provide all buffers or inform > > >> > > > > > > libcamera that they won't provide all buffers.... > > >> > > > > > > > >> > > > > > In the earlier versions of this series, we didn't have hints but used config > > >> > > > > > params to indicate this. Perhaps I should go back to this mechanism for the > > >> > > > > > time being to avoid any ABI breakages until there is an agreed path forward? > > No no please, let's keep hints. The only way to agree on a path forward > is to try things out, send out patches, and discuss them (I wish it > could happen by magic instead, but I haven't found the right spell so > far). Your series does it right, even if the road can sometimes be > bumpy. > > > >> > > > > That's just swapping one ABI breakage for another - because then the > > >> > > > > hint would change (or we'd have both a 'MandatoryStream' hint, and an > > >> > > > > 'OptionalStream' hint ?) > > >> > > > > > >> > > > If the "hint" were to move back to the config file, I would make it such > > >> > > > > >> > > Move back to ? Was there previous discussion that moved this out of a > > >> > > config file that I've missed? > > >> > > > >> > In earlier versions of this series, these hints were effectively flags in > > >> > the config files. > > >> > It was suggested to move these into the StreamConfig as hints since they > > >> > may be useful to other pipeline handlers, and enforce explicit application > > >> > behavior through the API. > > >> > > >> Ok - in that case, that confirms that the checks/assertions in [9/12] > > >> should be in the core, not just against raspberry pi pipeline handler. > > Agreed. > > > > I did consider this at the start, but didn't exactly know where to put the test. > > > I tried to put it in PipelineHandler::doQueueRequest(), but we don't have > > > access to the advertised StreamConfig structures there, do we? Any other > > > place you could suggest where we have access to the objects? > > > > Actually, I did not look hard enough. Request::camera->stream() will give me access > > to the StreamConfiguration objects I think. I can then do the validation in > > PipelineHandler::doQueueRequest() as I originally intended. I'll make that change. > > It would be best if we could validate requests synchronously though, but > we can probably live with asynchronous validation to start with. Please > test this with an application that submits an invalid request. Even > better, a test in lc-compliance would be good. I'll add a test in lc-compliance to simulate this. Having had a brief look at the code, lc-compliance does not seem geared up for multiple streams, so the test may take me some time to implement. > > > The question of what sense to use for the hints is still outstanding > > though.... > > The libcamera API makes buffers optional by default, to support, for > instance, use cases with a viewfinder stream and a still image capture > stream. Requiring a buffer for all streams in all requests by default > would be a big change with lots of implications that we need to > consider. Maybe we'll do so, but I don't think now is the right time. > That's why I prefer for now a hint to indicate that the application will > provide a buffer for the stream in every request. Got it, I'll update this patch and replace OptionalStream with MandatoryStream as the hint. Regards, Naush > > > >> This may also require updating any tests that could be affected, and > > >> lc-compliance. > > >> > > >> It may be helpful to split out this core API 'hint's part to it's own > > >> series as I don't think it should necessarily block the configuration > > >> file work here. > > >> > > >> > > > that the value is true for applications that provide buffers for every > > >> > > > configured stream on every request. > > >> > > > This would be defaulted to false, and libcamera-apps/picamera2 would set a > > >> > > > config file with the flag to true. This would preserve behavior for any > > >> > > > other existing applications that don't pass in a configuration file, though > > >> > > > it may be suboptimal with allocations. No ABI breakages either... > > -- > Regards, > > Laurent Pinchart
diff --git a/include/libcamera/stream.h b/include/libcamera/stream.h index 29235ddf0d8a..233ff0450451 100644 --- a/include/libcamera/stream.h +++ b/include/libcamera/stream.h @@ -13,6 +13,8 @@ #include <string> #include <vector> +#include <libcamera/base/flags.h> + #include <libcamera/color_space.h> #include <libcamera/framebuffer.h> #include <libcamera/geometry.h> @@ -51,6 +53,12 @@ struct StreamConfiguration { std::optional<ColorSpace> colorSpace; + enum class Hint { + None = 0, + OptionalStream = (1 << 0), + }; + Flags<Hint> hints; + 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 67f308157fbf..b7de85f8b9c7 100644 --- a/src/libcamera/stream.cpp +++ b/src/libcamera/stream.cpp @@ -349,6 +349,30 @@ StreamConfiguration::StreamConfiguration(const StreamFormats &formats) * color spaces can be supported and in what combinations. */ +/** + * \enum StreamConfiguration::Hint + * \brief List of available hint flags provided by the application for a stream + * + * \var StreamConfiguration::Hint::None + * No hints for this stream. + * \var StreamConfiguration::Hint::OptionalStream + * Informs the pipeline handler that the application may not always provide a + * buffer for the configured stream in every Request. This may ensure the + * pipeline handler allocates additional stream buffers for internal use. + */ + +/** + * \var StreamConfiguration::hints + * \brief Application provided StreamConfiguration::Hint flags for specific + * stream behavior + * + * Provides hints from the application to the pipeline handlers on how it + * intends on handling a given configured stream. These hints may alter the + * behavior of the pipeline handlers, for example, by allocating additional + * buffers for internal use if an application does not guarantee buffers will be + * provided in every Request for a stream. + */ + /** * \fn StreamConfiguration::stream() * \brief Retrieve the stream associated with the configuration
Add a new hints flags field in the StreamConfiguration structure to allow the application to specify certain intended behavior when driving libcamera. Pipeline handlers are expected to look at these hint flags and may optimise internal operations based on them. Currently, only one flag is listed, OptionalStream, which the application can set to inform the pipeline handler that a buffer may not be provided on every Request for a given stream. Signed-off-by: Naushir Patuck <naush@raspberrypi.com> --- include/libcamera/stream.h | 8 ++++++++ src/libcamera/stream.cpp | 24 ++++++++++++++++++++++++ 2 files changed, 32 insertions(+)