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

[Naushir Patuck]

Hi Laurent,

Thank you for the feedback.

On Sun, 22 Jan 2023 at 21:31, Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> On Fri, Jan 20, 2023 at 03:10:23PM +0000, Naushir Patuck via libcamera-devel wrote:
> > On Fri, 20 Jan 2023 at 12:29, Naushir Patuck wrote:
> > > On Fri, 20 Jan 2023 at 11:42, Kieran Bingham wrote:
> > >> Quoting Naushir Patuck (2023-01-20 11:34:30)
> > >> > On Fri, 20 Jan 2023 at 11:27, Kieran Bingham wrote:
> > >> > > Quoting Naushir Patuck (2023-01-20 10:53:38)
> > >> > > > On Fri, 20 Jan 2023 at 10:42, Kieran Bingham wrote:
> > >> > > > > Quoting Naushir Patuck (2023-01-18 11:16:40)
> > >> > > > > > On Wed, 18 Jan 2023 at 10:09, Kieran Bingham  wrote:
> > >> > > > > > > Quoting Naushir Patuck via libcamera-devel (2023-01-18 08:59:42)
> > >> > > > > > > > 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, OptionalStream, which the application can
> > >> > > > > > > > set to inform the pipeline handler that a buffer may not be provided on every
> > >> > > > > > > > Request for a given stream.
> > >> > > > > > >
> > >> > > > > > > Sorry - Laurent had comments on this yesterday when I was discussing
> > >> > > > > > > with him, so I don't know which way it will go yet...
> > >> > > > > > >
> > >> > > > > > > I still think this is the better way around, but we'll have to consider
> > >> > > > > > > that this is an 'ABI' breakage, as now apps that were able to use two
> > >> > > > > > > streams won't unless they explicitly provide all buffers or inform
> > >> > > > > > > libcamera that they won't provide all buffers....
> > >> > > > > >
> > >> > > > > > In the earlier versions of this series, we didn't have hints but used config
> > >> > > > > > params to indicate this.  Perhaps I should go back to this mechanism for the
> > >> > > > > > time being to avoid any ABI breakages until there is an agreed path forward?
>
> No no please, let's keep hints. The only way to agree on a path forward
> is to try things out, send out patches, and discuss them (I wish it
> could happen by magic instead, but I haven't found the right spell so
> far). Your series does it right, even if the road can sometimes be
> bumpy.
>
> > >> > > > > That's just swapping one ABI breakage for another - because then the
> > >> > > > > hint would change (or we'd have both a 'MandatoryStream' hint, and an
> > >> > > > > 'OptionalStream' hint ?)
> > >> > > >
> > >> > > > If the "hint" were to move back to the config file, I would make it such
> > >> > >
> > >> > > Move back to ? Was there previous discussion that moved this out of a
> > >> > > config file that I've missed?
> > >> >
> > >> > In earlier versions of this series, these hints were effectively flags in
> > >> > the config files.
> > >> > It was suggested to move these into the StreamConfig as hints since they
> > >> > may be useful to other pipeline handlers, and enforce explicit application
> > >> > behavior through the API.
> > >>
> > >> Ok - in that case, that confirms that the checks/assertions in [9/12]
> > >> should be in the core, not just against raspberry pi pipeline handler.
>
> Agreed.
>
> > > I did consider this at the start, but didn't exactly know where to put the test.
> > > I tried to put it in  PipelineHandler::doQueueRequest(), but we don't have
> > > access to the advertised StreamConfig structures there, do we?  Any other
> > > place you could suggest where we have access to the objects?
> >
> > Actually, I did not look hard enough.  Request::camera->stream() will give me access
> > to the StreamConfiguration objects I think.  I can then do the validation in
> > PipelineHandler::doQueueRequest() as I originally intended.  I'll make that change.
>
> It would be best if we could validate requests synchronously though, but
> we can probably live with asynchronous validation to start with. Please
> test this with an application that submits an invalid request. Even
> better, a test in lc-compliance would be good.

I'll add a test in lc-compliance to simulate this.  Having had a brief look at
the code, lc-compliance does not seem geared up for multiple streams, so the
test may take me some time to implement.

>
> > The question of what sense to use for the hints is still outstanding
> > though....
>
> The libcamera API makes buffers optional by default, to support, for
> instance, use cases with a viewfinder stream and a still image capture
> stream. Requiring a buffer for all streams in all requests by default
> would be a big change with lots of implications that we need to
> consider. Maybe we'll do so, but I don't think now is the right time.
> That's why I prefer for now a hint to indicate that the application will
> provide a buffer for the stream in every request.

Got it, I'll update this patch and replace OptionalStream with MandatoryStream
as the hint.

Regards,
Naush

>
> > >> This may also require updating any tests that could be affected, and
> > >> lc-compliance.
> > >>
> > >> It may be helpful to split out this core API 'hint's part to it's own
> > >> series as I don't think it should necessarily block the configuration
> > >> file work here.
> > >>
> > >> > > > that the value is true for applications that provide buffers for every
> > >> > > > configured stream on every request.
> > >> > > > This would be defaulted to false, and libcamera-apps/picamera2 would set a
> > >> > > > config file with the flag to true.  This would preserve behavior for any
> > >> > > > other existing applications that don't pass in a configuration file, though
> > >> > > > it may be suboptimal with allocations.  No ABI breakages either...
>
> --
> Regards,
>
> Laurent Pinchart