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

[Jacopo Mondi]

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.

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

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