[libcamera-devel,v4,01/12] libcamera: stream: Add stream hints to StreamConfiguration
diff mbox series

Message ID 20221209090050.19441-2-naush@raspberrypi.com
State Superseded
Headers show
Series
  • Raspberry Pi: Platform configuration and buffer allocation improvements
Related show

Commit Message

Naushir Patuck Dec. 9, 2022, 9 a.m. UTC
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@raspberrypi.com>
---
 include/libcamera/stream.h |  8 ++++++++
 src/libcamera/stream.cpp   | 24 ++++++++++++++++++++++++
 2 files changed, 32 insertions(+)

Comments

Kieran Bingham Jan. 16, 2023, 4:15 p.m. UTC | #1
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@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...


> +       };
> +       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
>
Kieran Bingham Jan. 16, 2023, 5:41 p.m. UTC | #2
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@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...

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
> >
Kieran Bingham Jan. 16, 2023, 5:43 p.m. UTC | #3
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@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...

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 ?

--
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
> > >
Naushir Patuck Jan. 17, 2023, 9:02 a.m. UTC | #4
Hi Kieran,

Thanks for your feedback!

On Mon, 16 Jan 2023 at 17:43, Kieran Bingham <
kieran.bingham@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@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
> > > >
>
Kieran Bingham Jan. 17, 2023, 9:25 a.m. UTC | #5
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@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@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
> > > > >
> >

Patch
diff mbox series

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),
+	};
+	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
+ * buffer for the configured stream in the Request. This may allow the pipeline
+ * 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