[libcamera-devel] Filter out unsupported configurations from |config_| in CameraConfiguration::validate()
Tomasz Figa
tfiga at chromium.org
Tue Dec 15 08:56:14 CET 2020
On Tue, Dec 15, 2020 at 11:48 AM Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> Hello,
>
> 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.
Maybe the answer here is some kind of internal API for adaptation layers?
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