[libcamera-devel] [PATCH v1 1/8] libcamera: stream: Add stream hints to StreamConfiguration

[Jacopo Mondi]

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 at 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
>