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

[Kieran Bingham]

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

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