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

[Laurent Pinchart]

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.

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

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