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

Naushir Patuck naush at raspberrypi.com
Tue Mar 7 11:38:11 CET 2023


Hi Kieran and Jacopo,

On Tue, 7 Mar 2023 at 10:19, Kieran Bingham <kieran.bingham at 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 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.
>

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
> > >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20230307/20869e59/attachment.htm>


More information about the libcamera-devel mailing list