<div dir="ltr"><div dir="ltr">Hi Kieran,<div><br></div><div>Thanks for your feedback!</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, 16 Jan 2023 at 17:43, Kieran Bingham <<a href="mailto:kieran.bingham@ideasonboard.com">kieran.bingham@ideasonboard.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Quoting Kieran Bingham (2023-01-16 17:41:38)<br>
> Quoting Kieran Bingham (2023-01-16 16:15:56)<br>
> > Quoting Naushir Patuck via libcamera-devel (2022-12-09 09:00:39)<br>
> > > Add a new hints flags field in the StreamConfiguration structure to allow the<br>
> > > application to specify certain intended behavior when driving libcamera.<br>
> > > Pipeline handlers are expected to look at these hint flags and may optimise<br>
> > > internal operations based on them.<br>
> > > <br>
> > > Currently, only one flag is listed, MandatoryRequestBuffer, which the<br>
> > > application sets to guarantee that a buffer will be provided in the Request for<br>
> > > each configured stream.<br>
> > > <br>
> > > Signed-off-by: Naushir Patuck <<a href="mailto:naush@raspberrypi.com" target="_blank">naush@raspberrypi.com</a>><br>
> > > ---<br>
> > > include/libcamera/stream.h | 8 ++++++++<br>
> > > src/libcamera/stream.cpp | 24 ++++++++++++++++++++++++<br>
> > > 2 files changed, 32 insertions(+)<br>
> > > <br>
> > > diff --git a/include/libcamera/stream.h b/include/libcamera/stream.h<br>
> > > index 29235ddf0d8a..1c5273004297 100644<br>
> > > --- a/include/libcamera/stream.h<br>
> > > +++ b/include/libcamera/stream.h<br>
> > > @@ -13,6 +13,8 @@<br>
> > > #include <string><br>
> > > #include <vector><br>
> > > <br>
> > > +#include <libcamera/base/flags.h><br>
> > > +<br>
> > > #include <libcamera/color_space.h><br>
> > > #include <libcamera/framebuffer.h><br>
> > > #include <libcamera/geometry.h><br>
> > > @@ -51,6 +53,12 @@ struct StreamConfiguration {<br>
> > > <br>
> > > std::optional<ColorSpace> colorSpace;<br>
> > > <br>
> > > + enum class Hint {<br>
> > > + None = 0,<br>
> > > + MandatoryRequestBuffer = (1 << 0),<br>
> > <br>
> > Might be bikeshedding, but reading later in the series I understand this<br>
> > to mean that this is a 'Mandatory Stream'.<br>
> > <br>
> > That makes me think "MandatoryStream" is what we're hinting at ? But<br>
> > even that might not convey it fully, so I'm not opposed to<br>
> > MandatoryRequestBuffer.<br>
> > <br>
> > I get the idea here, but I don't currently know how we might expose to<br>
> > applications if the hints are 'possible'/'usable'/'valid'?<br>
> > <br>
> > Or maybe it doesn't matter for hints. Applications could just set them<br>
> > to say "I'm going to do this..." ? And it doesn't matter if the pipeline<br>
> > supports it or not...<br>
> <br>
> And to throw in a spanner / go for discussions:<br>
> <br>
> Trying to think about this, I would expect that the 'default' case of<br>
> most applications handling a stream would be such that on that single<br>
> stream - it's expected that the stream is a mandatory stream.<br>
> <br>
> That means I'm tempted to suggest this is inverted...<br>
> <br>
> Make the stream hint 'optional' ... and only if the stream is marked as<br>
> optional can the buffers not be added to a request?<br>
> <br>
> That way - applications for single streams get the correct behaviour 'by<br>
> default' ... while more complex systems with multiple streams define<br>
> which stream is likely to be only provided optionally...<br></blockquote><div><br></div><div>So your suggestion would be to have the hint be inverted so perhaps called<br>optionalStream, and buffers must always be passed in through the requests for<br>streams where optionalStream == false? That seems reasonable to me.<br></div><div> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
This way around, we can also add a check when the request is queued. If<br>
any stream is not marked as optional - and doesn't have a buffer, -<br>
fail the request ?<br></blockquote><div><br></div><div>We do exactly this a few commits in - but for the current sense of the hint.</div><div>It should be trivial to flip the sense of this test.</div><div><br></div><div>I'll wait a bit for any more feedback on the rest of the patches and post a v6</div><div>with this change soon.</div><div><br></div><div>Regards,</div><div>Naush</div><div> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
--<br>
Kieran<br>
<br>
<br>
> <br>
> Thoughts anyone?<br>
> <br>
> --<br>
> Kieran<br>
> <br>
> <br>
> > <br>
> > <br>
> > > + };<br>
> > > + Flags<Hint> hints;<br>
> > > +<br>
> > > Stream *stream() const { return stream_; }<br>
> > > void setStream(Stream *stream) { stream_ = stream; }<br>
> > > const StreamFormats &formats() const { return formats_; }<br>
> > > diff --git a/src/libcamera/stream.cpp b/src/libcamera/stream.cpp<br>
> > > index 67f308157fbf..504c6d86cd04 100644<br>
> > > --- a/src/libcamera/stream.cpp<br>
> > > +++ b/src/libcamera/stream.cpp<br>
> > > @@ -349,6 +349,30 @@ StreamConfiguration::StreamConfiguration(const StreamFormats &formats)<br>
> > > * color spaces can be supported and in what combinations.<br>
> > > */<br>
> > > <br>
> > > +/**<br>
> > > + * \enum StreamConfiguration::Hint<br>
> > > + * \brief List of available hint flags provided by the application for a stream<br>
> > > + *<br>
> > > + * \var StreamConfiguration::Hint::None<br>
> > > + * No hints for this stream.<br>
> > > + * \var StreamConfiguration::Hint::MandatoryRequestBuffer<br>
> > > + * Informs the pipeline handler that the application guarantee to provide a<br>
> > <br>
> > s/guarantee/guarantees/<br>
> > or ..<br>
> > will guarantee<br>
> > <br>
> > > + * buffer for the configured stream in the Request. This may allow the pipeline<br>
> > <br>
> > /in the Request/in every Request/<br>
> > <br>
> > > + * handler to allocate fewer (or no) buffers for internal use.<br>
> > > + */<br>
> > > +<br>
> > > +/**<br>
> > > + * \var StreamConfiguration::hints<br>
> > > + * \brief Application provided StreamConfiguration::Hint flags for specific<br>
> > > + * stream behavior<br>
> > > + *<br>
> > > + * Provides hints from the application to the pipeline handlers on how it<br>
> > > + * intends on handling a given configured stream. These hints may alter the<br>
> > > + * behavior of the pipeline handlers, for example, by allocating fewer buffers<br>
> > > + * for internal use if an application guarantees to always provide a buffer in<br>
> > > + * the Request for a stream.<br>
> > > + */<br>
> > > +<br>
> > > /**<br>
> > > * \fn StreamConfiguration::stream()<br>
> > > * \brief Retrieve the stream associated with the configuration<br>
> > > -- <br>
> > > 2.25.1<br>
> > ><br>
</blockquote></div></div>