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

[Kieran Bingham]

Quoting Naushir Patuck (2023-01-17 09:02:53)
> 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.

That's true, of course the test can be done in both orientations ;-)

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

I'll highlight this to the others to see if anyone can object or
'prefer' one way or the other sooner rather than later.

--
Kieran


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