| Message ID | 20230203094424.25243-2-naush@raspberrypi.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
Hi Naush I've seen (and read) the huge backlog on this series, as I don't want to start the discussion over I will only review the most trivial things if any but I guess the design has been debated already in great detail On Fri, Feb 03, 2023 at 09:44:17AM +0000, Naushir Patuck via libcamera-devel wrote: > 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, MandatoryStream, which the application can > set to inform the pipeline handler that a buffer will always be provided on > every Request for a given stream. > Just for sake of bikeshedding, the name says "MandatoryStream" but it informs the ph that "a buffer will always be provided with every request". I would name it "MandatoryBuffer" > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > --- > include/libcamera/stream.h | 8 ++++++++ > src/libcamera/stream.cpp | 25 +++++++++++++++++++++++++ > 2 files changed, 33 insertions(+) > > diff --git a/include/libcamera/stream.h b/include/libcamera/stream.h > index 29235ddf0d8a..56e5a74b2aab 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, > + MandatoryStream = (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..9b0e8dd548cf 100644 > --- a/src/libcamera/stream.cpp > +++ b/src/libcamera/stream.cpp > @@ -349,6 +349,31 @@ 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::MandatoryStream > + * Informs the pipeline handler that the application will always provide a > + * buffer for the configured stream in every Request. This may ensure the > + * pipeline handler does not need any additional stream buffer allocations for > + * internal use. I'm not sure if the last part is actually platform specific or not. Other pipeline migh use the hint for other reasons ? It can be changed later if/when other PH will eventually use it > + */ > + > +/** > + * \var StreamConfiguration::hints > + * \brief Application provided StreamConfiguration::Hint flags for specific > + * stream behavior I would just drop "for specific stram 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 not allocating additional > + * buffers for internal use if an application guarantees buffers will be > + * provided in every Request for a stream. This repeats the StreamConfiguration::Hint::MandatoryStream documentation. I would rather describe what happens if a hint is not supported by a platform. Will the field be zeroed or adjusted ? > + */ > + > /** > * \fn StreamConfiguration::stream() > * \brief Retrieve the stream associated with the configuration > -- > 2.25.1 >
Quoting Jacopo Mondi via libcamera-devel (2023-03-02 09:39:39) > Hi Naush > > I've seen (and read) the huge backlog on this series, as I don't want > to start the discussion over I will only review the most trivial > things if any but I guess the design has been debated already in great > detail > > On Fri, Feb 03, 2023 at 09:44:17AM +0000, Naushir Patuck via libcamera-devel wrote: > > 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, MandatoryStream, which the application can > > set to inform the pipeline handler that a buffer will always be provided on > > every Request for a given stream. > > > > Just for sake of bikeshedding, the name says "MandatoryStream" but it > informs the ph that "a buffer will always be provided with every > request". I would name it "MandatoryBuffer" Because it's on the StreamConfiguration, My vote would be to stick with MandatoryStream. > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > --- > > include/libcamera/stream.h | 8 ++++++++ > > src/libcamera/stream.cpp | 25 +++++++++++++++++++++++++ > > 2 files changed, 33 insertions(+) > > > > diff --git a/include/libcamera/stream.h b/include/libcamera/stream.h > > index 29235ddf0d8a..56e5a74b2aab 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, > > + MandatoryStream = (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..9b0e8dd548cf 100644 > > --- a/src/libcamera/stream.cpp > > +++ b/src/libcamera/stream.cpp > > @@ -349,6 +349,31 @@ 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::MandatoryStream > > + * Informs the pipeline handler that the application will always provide a > > + * buffer for the configured stream in every Request. This may ensure the > > + * pipeline handler does not need any additional stream buffer allocations for > > + * internal use. > > I'm not sure if the last part is actually platform specific or not. > Other pipeline migh use the hint for other reasons ? It can be changed > later if/when other PH will eventually use it I would expect the hint to be consistent on other platforms if it's used. We would have the same restrictions/requirements. > > > + */ > > + > > +/** > > + * \var StreamConfiguration::hints > > + * \brief Application provided StreamConfiguration::Hint flags for specific > > + * stream behavior > > I would just drop "for specific stram 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 not allocating additional > > + * buffers for internal use if an application guarantees buffers will be > > + * provided in every Request for a stream. > > This repeats the StreamConfiguration::Hint::MandatoryStream > documentation. > > I would rather describe what happens if a hint is not supported by a > platform. Will the field be zeroed or adjusted ? That's an interesting idea. I had presumed hints were 'optional' directions ... but zeroing out unsupported flags when it gets validated would make sense as a way for the application to know if something would actually happen from the flag. But it probably wouldn't make much difference? I guess it depends on how it's usage grows. > > > + */ > > + > > /** > > * \fn StreamConfiguration::stream() > > * \brief Retrieve the stream associated with the configuration > > -- > > 2.25.1 > >
Hi Kieran and Jacopo, On Tue, 7 Mar 2023 at 10:19, Kieran Bingham <kieran.bingham@ideasonboard.com> wrote: > Quoting Jacopo Mondi via libcamera-devel (2023-03-02 09:39:39) > > Hi Naush > > > > I've seen (and read) the huge backlog on this series, as I don't want > > to start the discussion over I will only review the most trivial > > things if any but I guess the design has been debated already in great > > detail > > > > On Fri, Feb 03, 2023 at 09:44:17AM +0000, Naushir Patuck via > libcamera-devel wrote: > > > 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, MandatoryStream, which the > application can > > > set to inform the pipeline handler that a buffer will always be > provided on > > > every Request for a given stream. > > > > > > > Just for sake of bikeshedding, the name says "MandatoryStream" but it > > informs the ph that "a buffer will always be provided with every > > request". I would name it "MandatoryBuffer" > > Because it's on the StreamConfiguration, My vote would be to stick with > MandatoryStream. > > > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > > --- > > > include/libcamera/stream.h | 8 ++++++++ > > > src/libcamera/stream.cpp | 25 +++++++++++++++++++++++++ > > > 2 files changed, 33 insertions(+) > > > > > > diff --git a/include/libcamera/stream.h b/include/libcamera/stream.h > > > index 29235ddf0d8a..56e5a74b2aab 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, > > > + MandatoryStream = (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..9b0e8dd548cf 100644 > > > --- a/src/libcamera/stream.cpp > > > +++ b/src/libcamera/stream.cpp > > > @@ -349,6 +349,31 @@ 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::MandatoryStream > > > + * Informs the pipeline handler that the application will always > provide a > > > + * buffer for the configured stream in every Request. This may ensure > the > > > + * pipeline handler does not need any additional stream buffer > allocations for > > > + * internal use. > > > > I'm not sure if the last part is actually platform specific or not. > > Other pipeline migh use the hint for other reasons ? It can be changed > > later if/when other PH will eventually use it > > I would expect the hint to be consistent on other platforms if it's used. > We would have the same restrictions/requirements. > See below :) > > > > > > > > + */ > > > + > > > +/** > > > + * \var StreamConfiguration::hints > > > + * \brief Application provided StreamConfiguration::Hint flags for > specific > > > + * stream behavior > > > > I would just drop "for specific stram 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 not allocating > additional > > > + * buffers for internal use if an application guarantees buffers will > be > > > + * provided in every Request for a stream. > > > > This repeats the StreamConfiguration::Hint::MandatoryStream > > documentation. > > > > I would rather describe what happens if a hint is not supported by a > > platform. Will the field be zeroed or adjusted ? > > That's an interesting idea. I had presumed hints were 'optional' > directions ... but zeroing out unsupported flags when it gets validated > would make sense as a way for the application to know if something would > actually happen from the flag. But it probably wouldn't make much > difference? I guess it depends on how it's usage grows. > My intention for the "hints" is that they are all entirely optional to support in any pipeline handler. Applications cannot and should not expect specific behavioral change from the pipeline handlers based on the hints. They are explicitly for the pipeline handler to understand how the application might be driving the API, and optimise some things (resource allocations in this case) if possible. As such, I didn't see a need for advertising hints that are handled out of the pipeline handler. Regards, Naush > > > > > > > + */ > > > + > > > /** > > > * \fn StreamConfiguration::stream() > > > * \brief Retrieve the stream associated with the configuration > > > -- > > > 2.25.1 > > > >
diff --git a/include/libcamera/stream.h b/include/libcamera/stream.h index 29235ddf0d8a..56e5a74b2aab 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, + MandatoryStream = (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..9b0e8dd548cf 100644 --- a/src/libcamera/stream.cpp +++ b/src/libcamera/stream.cpp @@ -349,6 +349,31 @@ 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::MandatoryStream + * Informs the pipeline handler that the application will always provide a + * buffer for the configured stream in every Request. This may ensure the + * pipeline handler does not need any additional stream buffer allocations 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 not allocating additional + * buffers for internal use if an application guarantees 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, MandatoryStream, which the application can set to inform the pipeline handler that a buffer will always 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 | 25 +++++++++++++++++++++++++ 2 files changed, 33 insertions(+)