[libcamera-devel] Filter out unsupported configurations from |config_| in CameraConfiguration::validate()
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Tue Dec 15 03:48:00 CET 2020
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).
> > 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
More information about the libcamera-devel
mailing list