[libcamera-devel] [PATCH v4 01/12] libcamera: stream: Add stream hints to StreamConfiguration

[Naushir Patuck]

Hi Kieran,

Thanks for your feedback!

On Mon, 16 Jan 2023 at 17:43, Kieran Bingham <
kieran.bingham at ideasonboard.com> wrote:

> Quoting Kieran Bingham (2023-01-16 17:41:38)
> > Quoting Kieran Bingham (2023-01-16 16:15:56)
> > > Quoting Naushir Patuck via libcamera-devel (2022-12-09 09:00:39)
> > > > 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, MandatoryRequestBuffer, which the
> > > > application sets to guarantee that a buffer will be provided in the
> Request for
> > > > each configured stream.
> > > >
> > > > Signed-off-by: Naushir Patuck <naush at 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..1c5273004297 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,
> > > > +               MandatoryRequestBuffer = (1 << 0),
> > >
> > > Might be bikeshedding, but reading later in the series I understand
> this
> > > to mean that this is a 'Mandatory Stream'.
> > >
> > > That makes me think "MandatoryStream" is what we're hinting at ? But
> > > even that might not convey it fully, so I'm not opposed to
> > > MandatoryRequestBuffer.
> > >
> > > I get the idea here, but I don't currently know how we might expose to
> > > applications if the hints are 'possible'/'usable'/'valid'?
> > >
> > > Or maybe it doesn't matter for hints. Applications could just set them
> > > to say "I'm going to do this..." ? And it doesn't matter if the
> pipeline
> > > supports it or not...
> >
> > And to throw in a spanner / go for discussions:
> >
> > Trying to think about this, I would expect that the 'default' case of
> > most applications handling a stream would be such that on that single
> > stream - it's expected that the stream is a mandatory stream.
> >
> > That means I'm tempted to suggest this is inverted...
> >
> > Make the stream hint 'optional' ... and only if the stream is marked as
> > optional can the buffers not be added to a request?
> >
> > That way - applications for single streams get the correct behaviour 'by
> > default' ... while more complex systems with multiple streams define
> > which stream is likely to be only provided optionally...
>

So your suggestion would be to have the hint be inverted so perhaps called
optionalStream, and buffers must always be passed in through the requests
for
streams where optionalStream == false?  That seems reasonable to me.


>
> This way around, we can also add a check when the request is queued. If
> any stream is not marked as optional - and doesn't have a buffer, -
> fail the request ?
>

We do exactly this a few commits in - but for the current sense of the hint.
It should be trivial to flip the sense of this test.

I'll wait a bit for any more feedback on the rest of the patches and post a
v6
with this change soon.

Regards,
Naush


>
> --
> Kieran
>
>
> >
> > Thoughts anyone?
> >
> > --
> > Kieran
> >
> >
> > >
> > >
> > > > +       };
> > > > +       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..504c6d86cd04 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::MandatoryRequestBuffer
> > > > + * Informs the pipeline handler that the application guarantee to
> provide a
> > >
> > > s/guarantee/guarantees/
> > > or ..
> > > will guarantee
> > >
> > > > + * buffer for the configured stream in the Request. This may allow
> the pipeline
> > >
> > > /in the Request/in every Request/
> > >
> > > > + * handler to allocate fewer (or no) 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
> fewer buffers
> > > > + * for internal use if an application guarantees to always provide
> a buffer in
> > > > + * the Request for a stream.
> > > > + */
> > > > +
> > > >  /**
> > > >   * \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/20230117/4aaacd18/attachment.htm>