<div dir="ltr"><div dir="ltr">Hi Kieran and Jacopo,</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, 7 Mar 2023 at 10:19, 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 Jacopo Mondi via libcamera-devel (2023-03-02 09:39:39)<br>
> Hi Naush<br>
> <br>
> I've seen (and read) the huge backlog on this series, as I don't want<br>
> to start the discussion over I will only review the most trivial<br>
> things if any but I guess the design has been debated already in great<br>
> detail<br>
> <br>
> On Fri, Feb 03, 2023 at 09:44:17AM +0000, Naushir Patuck via libcamera-devel wrote:<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, MandatoryStream, which the application can<br>
> > set to inform the pipeline handler that a buffer will always be provided on<br>
> > every Request for a given stream.<br>
> ><br>
> <br>
> Just for sake of bikeshedding, the name says "MandatoryStream" but it<br>
> informs the ph that "a buffer will always be provided with every<br>
> request". I would name it "MandatoryBuffer"<br>
<br>
Because it's on the StreamConfiguration, My vote would be to stick with<br>
MandatoryStream.<br>
<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 | 25 +++++++++++++++++++++++++<br>
> > 2 files changed, 33 insertions(+)<br>
> ><br>
> > diff --git a/include/libcamera/stream.h b/include/libcamera/stream.h<br>
> > index 29235ddf0d8a..56e5a74b2aab 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>
> > + MandatoryStream = (1 << 0),<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..9b0e8dd548cf 100644<br>
> > --- a/src/libcamera/stream.cpp<br>
> > +++ b/src/libcamera/stream.cpp<br>
> > @@ -349,6 +349,31 @@ 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::MandatoryStream<br>
> > + * Informs the pipeline handler that the application will always provide a<br>
> > + * buffer for the configured stream in every Request. This may ensure the<br>
> > + * pipeline handler does not need any additional stream buffer allocations for<br>
> > + * internal use.<br>
> <br>
> I'm not sure if the last part is actually platform specific or not.<br>
> Other pipeline migh use the hint for other reasons ? It can be changed<br>
> later if/when other PH will eventually use it<br>
<br>
I would expect the hint to be consistent on other platforms if it's used.<br>
We would have the same restrictions/requirements.<br></blockquote><div><br></div><div>See below :)</div><div> </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>
<br>
> <br>
> > + */<br>
> > +<br>
> > +/**<br>
> > + * \var StreamConfiguration::hints<br>
> > + * \brief Application provided StreamConfiguration::Hint flags for specific<br>
> > + * stream behavior<br>
> <br>
> I would just drop "for specific stram behavior"<br>
> <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 not allocating additional<br>
> > + * buffers for internal use if an application guarantees buffers will be<br>
> > + * provided in every Request for a stream.<br>
> <br>
> This repeats the StreamConfiguration::Hint::MandatoryStream<br>
> documentation.<br>
> <br>
> I would rather describe what happens if a hint is not supported by a<br>
> platform. Will the field be zeroed or adjusted ?<br>
<br>
That's an interesting idea. I had presumed hints were 'optional'<br>
directions ... but zeroing out unsupported flags when it gets validated<br>
would make sense as a way for the application to know if something would<br>
actually happen from the flag. But it probably wouldn't make much<br>
difference? I guess it depends on how it's usage grows.<br></blockquote><div><br></div><div>My intention for the "hints" is that they are all entirely optional to support<br>in any pipeline handler. Applications cannot and should not expect specific<br>behavioral change from the pipeline handlers based on the hints. They are<br>explicitly for the pipeline handler to understand how the application might be<br>driving the API, and optimise some things (resource allocations in this case) if<br>possible. As such, I didn't see a need for advertising hints that are handled<br>out of the pipeline handler.<br></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>
<br>
> <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>