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

[Jacopo Mondi]

Hi Hiro,

On Fri, Dec 11, 2020 at 03:34:13PM +0900, Hirokazu Honda wrote:
> Hi Jacopo,
>
> On Fri, Dec 11, 2020 at 2:04 AM Jacopo Mondi <jacopo at jmondi.org> wrote:
> >
> > Hello Hiro,
> >
> > 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.

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

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

>
> What do you think?
>
> Regards,
> -Hiro
>
> > Thanks
> >   j
> >
> > >
> > > 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
> > > _______________________________________________
> > > libcamera-devel mailing list
> > > libcamera-devel at lists.libcamera.org
> > > https://lists.libcamera.org/listinfo/libcamera-devel