[libcamera-devel] Filter out unsupported configurations from |config_| in CameraConfiguration::validate()
Hirokazu Honda
hiroh at chromium.org
Tue Jan 19 02:35:34 CET 2021
Hi all,
Sorry for the late reply. I was busy for other tasks.
I think tryValidate() may be a good direction to not break the app
compatibility.
I implement tryValidate() and see how android HAL layer is with it.
I upload the patch in ChromiumOS Gerrit as it is WIP and can be seen by anyone.
https://chromium-review.googlesource.com/c/chromiumos/third_party/libcamera/+/2583432
How do you think the way?
Thanks so much in advance.
Best Regards,
-Hiro
On Thu, Dec 17, 2020 at 1:06 PM Tomasz Figa <tfiga at chromium.org> wrote:
>
> 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
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
More information about the libcamera-devel
mailing list