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

[Hirokazu Honda]

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.

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

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