[libcamera-devel] Filter out unsupported configurations from |config_| in CameraConfiguration::validate()

[Tomasz Figa]

On Thu, Dec 17, 2020 at 12:51 PM Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> Hi Tomasz,
>
> On Tue, Dec 15, 2020 at 04:56:14PM +0900, Tomasz Figa wrote:
> > On Tue, Dec 15, 2020 at 11:48 AM Laurent Pinchart wrote:
> > > On Mon, Dec 14, 2020 at 09:18:10AM +0100, Jacopo Mondi wrote:
> > > > On Fri, Dec 11, 2020 at 03:34:13PM +0900, Hirokazu Honda wrote:
> > > > > On Fri, Dec 11, 2020 at 2:04 AM Jacopo Mondi wrote:
> > > > > > On Thu, Dec 10, 2020 at 07:03:05PM +0900, Hirokazu Honda wrote:
> > > > > > > Hi all,
> > > > > > >
> > > > > > > I have implemented the proposal [1] in ipu3 experimentally [2].
> > > > > > > In my proposal, |config_| is updated in validate() so that it only
> > > > > > > contains either Valid or Adjusted streams.
> > > > > > > In other words, |config_| after validate() doesn't contain that
> > > > > > > libcamera::Camera cannot produce.
> > > > > >
> > > > > > If I read it right, what your patch does is construct the largest
> > > > > > possible set of (ordered) stream configurations that are
> > > > > > valid/adjusted, filtering out the ones that make the configuration
> > > > > > Invalid.
> > > > > >
> > > > > > I think this direction has itself merits, but I wonder if we should
> > > > > > make this standard behavior for all libcamera pipeline handlers.
> > > > > > In that case this should not only be addressed in the
> > > > > > CameraConfiguratio base classes, but the base class' validate() should
> > > > > > enforce this behaviour by doing what your patch does for the IPU3
> > > > > > implementation.
> > > > >
> > > > > Yeah, I implemented in ipu3 pipeline handler for experiment.
> > > > > If we all agree with this behavior, I am going to change other camera
> > > > > configuration.
> > > >
> > > > Thanks.
> > > >
> > > > If that's the direction we want to take, I would make a base class
> > > > CameraConfiguration::validate() that iteratively tries a derived class
> > > > ::tryValidate(vector<StreamConfiguration>) and iteratively builds a
> > > > vector of valid/adjusted stream configurations.
> > > >
> > > > What I don't like is that CameraConfiguration has already a vector of
> > > > StreamConfiguration and a new method that operates on a different
> > > > vector might seem weird. Alternatively we can defer the
> > > > tryConfig(vector<StreamConfiguration) to the CameraData.
> > >
> > > I think the direction is good, the user-facing functionality makes
> > > sense. We need to sort out the user-facing API, to ensure the user case
> > > easily match the validated configuration with what it has requested, or,
> > > more generally speaking, with its needs. Dropping a stream in the middle
> > > could make it challenging for applications to figure out what has
> > > happened, and to react to the configuration change. This part is very
> > > important.
> > >
> > > When it comes to the implementation, could we do better than a complete
> > > iterative approach where we try different combinations of streams ?
> > > Looking at patch [2], there's quite a bit of complexity left in
> > > IPU3CameraConfiguration::tryValidate(). Brainstorming a little bit,
> > > maybe we could have a function similar to
> > > IPU3CameraConfiguration::validate() in the CameraConfiguration class
> > > that would construct the configuration iteratively, by adding one stream
> > > at a time. The pipeline handler would then receive a valid configuration
> > > object and a new stream to be added, and would either reject the new
> > > stream completely, or accept it (with possibly adjustements).
> > >
> > > It should be noted that we could also push the iterative build of
> > > configurations all the way to the applications if we consider that this
> > > would be a good application-facing API. I'm not sure it would be in this
> > > specific case, but in general API changes are possible.
> > >
> > > Other options are certainly possible. What we should aim for is a
> > > well-documented behaviour, easy to understand *and* use for application
> > > developers, and shared helper code to make pipeline handler development
> > > as easy as possible. I don't know if we can force all pipeline handlers
> > > to use the same helper though, depending on the API choice, maybe it
> > > will not be possible to create a single helper implementation that would
> > > work for all pipeline handlers. In that case we should expose at the
> > > PipelineHandler level an API that matches the user-facing API very
> > > closely, and provide helpers classes that pipeline handlers can use
> > > explicitly to simplify the implementation. Multiple helpers could then
> > > coexist for different types of pipeline handlers. This is the approach
> > > that the PipelineHandler API has taken so far: we should only make
> > > mid-layers mandatory if we are very certain that they will fit all use
> > > cases.
> > >
> > > > > > > However, this matters because a caller might assume the config is not updated.
> > > > > > > For example, camera_device.cpp stores the index of |config_| before
> > > > > > > validate() [3].
> > > > > > > I think filtering out unsupported streams in |config_| is reasonable.
> > > > > > > So I would like to change the caller implementations.
> > > > > >
> > > > > > If validate() has to remove stream configurations as they make the
> > > > > > configurtion invalid, it means we cannot satisfy what the framework
> > > > > > requested. Semantically this seems equivalent to returning Invalid
> > > > > > from validate().
> > > > > >
> > > > > > Sure, if some stream gets filtered out we can try to map it to some
> > > > > > other valid one.
> > > > > >
> > > > > > A simple example is
> > > > > >
> > > > > > config_ = { NV12-XGA, NV12-VGA, RAW }
> > > > > > validate() = { NV12-XGA, RAW }
> > > > > >
> > > > > > If the camera cannot produce the second NV12 stream, the HAL can try
> > > > > > to produce it by software processing the supported NV12-XGA. If none
> > > > > > of the software processors are able to map the refused stream on an
> > > > > > existing one, then we should really fail.
> > > > > >
> > > > > > Of course, and that my original fear, this can quickly escalate, and
> > > > > > the heuristics to get this right can quickly become very complex.
> > > > > >
> > > > > > But I think something like:
> > > > > >
> > > > > > {
> > > > > >         configs = populate Camera3StreamConfigs;
> > > > > >         configs = sortConfigs(configs);
> > > > > >         newconfig = validate(configs);
> > > > > >         for (each c in configs but not in newconfig) {
> > > > > >                 int streamConfigIndex = tryMap(c, newconfigs);
> > > > > >                 if (streamConfigIndex < 0)
> > > > > >                         fail;
> > > > > >
> > > > > >                 c->index = streamConfigIndex;
> > > > > >         }
> > > > > >
> > > > > >         create the streams_ vector;
> > > > > > }
> > > > > >
> > > > > > Might work ?
> > > > > >
> > > > > > What do you think ?
> > > > >
> > > > > Returning new valid/adjusted configurations sounds a good idea.
> > > >
> > > > Am I wrong or if the configuration is valid/adjusted there's no need
> > > > to return a new configuration vector ? The existing one is good
> > > > enough.
> > > >
> > > > What if we make this an "if the CameraConfiguration status is Invalid,
> > > > a valid configuration vector is returned as a possible alternative
> > > > valid configuration".
> > > >
> > > > This would make validate() return a:
> > > >
> > > >         pair<status, vector<StreamConfiguration>>
> > > >
> > > > With the caveat that:
> > > >         if status == adjusted || status == valid the vector is empty
> > > >         if status == invalid vector is a valid alternative to the
> > > >         existin configuration
> > > >
> > > > I'm not sure I like this too much to be honest.
> > >
> > > Neither do I. Let's consider this from an application point of view,
> > > returning an alternative configuration without defining very explicit
> > > rules on how it's built will create an unusable API. And if rules can be
> > > defined very clearly, the only difference between Adjusted and Invalid,
> > > if any, would be what type of adjustement is allowed on either case. The
> > > name "Invalid" would then not really describe the API behaviour.
> > >
> > > > > The implementation looks almost good to me.
> > > > > One question is how we can implement this.
> > > > > >  for (each c in configs but not in newconfig) {
> > > > >
> > > > > There are some ways.
> > > > > I would attach Status to the passed StreamConfigurations.
> > > > > void validate(std::vector<std::pair<Status, StreamConfiguration *>>
> > > > > &streamConfigs).
> > > >
> > > > CameraConfiguration already has the list of stream configuration to
> > > > validate as class members. Are you suggesting to pass them as
> > > > parameters and drop the internal vector ? I won't be thrilled, as we
> > > > would lose the concept of self-validating CameraConfiguration, and we
> > > > would not be able to easily validate the configuration in
> > > > Camera::configure() anymore (validate() could also be made a static
> > > > method if it operates on a list of configuration passed in as
> > > > parameters).
> > >
> > > In order to evaluate the proposal, I think we need to extend it with
> > > documentation (to ensure the behaviour can be described clearly), and
> > > with an implementation of handling adjusted configurations (in the
> > > camera HAL and/or in the cam application).
> >
> > One thing I see in general in this discussion is that it's hard to
> > come up with the specific behavior that would suit the applications,
> > because there are no applications. The existing Linux userspace would
> > use a single stream, usually with any pixel format advertised by the
> > camera. I suspect the same level of functionality would remain for
> > quite a while, just the underlying API would change from raw V4L2 to
> > libcamera.
>
> There's a chicken and egg issue there, it's not surprising that the vast
> majority of applications don't make use of multiple streams when that
> feature usually requires closed-source vendor stacks. I want libcamera
> to enable this for all Linux systems :-)

I'd say that it's not exactly like that. Multiple streams are not that
useful on some of the systems. Take a Linux laptop as an example. Your
typical video conferencing application wouldn't really have much
interest in anything else than just 1 720p (or maybe 1080p) stream and
that is perfectly good for preview as well. In fact, I think we might
even want to stick to that for video recording, because it means less
work to do for the ISP, so should translate to less power consumption.

Then again, I think we have a good foundation in place, which is
Android with the HAL3 API and the huge application base that should
tell us a lot about the various use cases and how to model that in the
API. If we really want to peek into the future, I suspect that
leveraging Android experience here would help a lot. Maybe rather than
trying to reinvent the wheel we should just stick to what the Android
HAL3 API does for this aspect?

>
> > Maybe the answer here is some kind of internal API for adaptation layers?
>
> The adaptation layers use internal libcamera classes to avoid
> reinventing the wheel, but from a Camera class API point of view, the
> goal is to expose all the features needed by adaptation layers in the
> public API. We may make expections, but they will remain, well,
> exceptional.

Again, as I said, I don't want us to try to guess the future. We need
to have some strong evidence that some functionality would actually be
useful in the public libcamera API and that should imply that we
collected enough information on related use cases to come up with the
API elements.

Best regards,
Tomasz

>
> > > > > What do you think?
> > > > >
> > > > > > > How do you'all think?
> > > > > > >
> > > > > > > [1] https://lists.libcamera.org/pipermail/libcamera-devel/2020-November/014723.html
> > > > > > > [2] https://chromium-review.googlesource.com/c/chromiumos/third_party/libcamera/+/2583432
> > > > > > > [3] https://source.chromium.org/chromiumos/chromiumos/codesearch/+/master:src/third_party/libcamera/src/android/camera_device.cpp;l=1287
>
> --
> Regards,
>
> Laurent Pinchart