[libcamera-devel] A proposal about implementation rules and return values for CameraConfiguration::validate()

[Hirokazu Honda]

Hi Jacopo and Laurent,

On Fri, Oct 30, 2020 at 12:11 PM Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> Hello Hiro-san and Jacopo,
>
> On Thu, Oct 29, 2020 at 06:47:57PM +0100, Jacopo Mondi wrote:
> > On Thu, Oct 29, 2020 at 06:37:44PM +0900, Hirokazu Honda wrote:
> > > I look at validate() implementations of CameraConfigurations (e.g.
> > > IPU3CameraConfiguration) thinking of mapping the accepted
> > > configurations to ones requested by Android HAL client.
> > > CameraConfiguration::validate() returns Valid, Adjusted and Invalid.
> > > From the implementations, each of them is returned
> > > [Valid]
> > > when all configurations are accepted by a native camera. In other
> > > words, the native camera will produce the same streams as requested.
> > > [Adjusted]
> > > when some configurations can be accepted by native camera, while some
> > > of them are modified like a dimension is aligned and a format is
> > > changed.
> > > [Invalid]
> > > when there is a configuration that cannot be accepted by a native camera.
> > >
> > > Although these may not match the definitions, the implementations look
> > > to follow definitions like this.
> >
> > I concur we need to establish stricter rules and enrich documentation
> > to make different CameraConfiguration implementations consistent.
> >
> > Have you find discrepancies between different pipeline handler
> > implementations that you want to report ?
> >

I think there is no discrepancy among them as far as I looked the
pipeline handlers.

> > > However, the difference between Adjusted and Invalid is very ambiguous.
> > > I find two problems.
> > >
> > > First, which value is returned Adjusted or Invalid, is dependent on
> > > the order of configurations and CameraConfiguration implementation,
> > > because present CameraConfiguration classes
> > > 1.) drop given configurations and regard as Adjusted e.g. [1], and
> > > 2.) returns Invalid if there is a configuration that cannot be
> > > accepted by a native camera. e.g. [2, 3]
> > > For example, if the configuration is {acceptable, non-acceptable,
> > > non-acceptable}, a CameraConfiguration returns Adjusted if it drops
> > > the last two ones thanks to 1.).
> > > On the other hand, if the configuration is {non-acceptable,
> > > acceptable, no-acceptable}, a CameraConfiguration returns Invalid
> > > because the first one is configured to a native camera thanks to 2.).
> >
> > Not to be picky, but I read [1]. [2] and [3] as follow
> >
> > [1] The number of requested streams is not supported ( > 3 for IPU3).
> >     Adjusting it is trivial as the last one can be dropped and
> >     proceed to validate the actual request content
> >
> > [2] The stream combination is not supported (ie {raw raw nv12})
> >     Deciding which stream to drop is not trivial. Even if we can
> >     establish a priority ordering and decide, in example, the first
> >     has higher priority like in 1.) in this example it would not work
> >     as {raw, raw} won't be supported anyway.
>
> I think we could decide to drop streams starting from the least priority
> one. In your {raw, raw, nv12} example, the result would be {raw}. We
> could argue that we should do better as {raw, nv12} is supported and
> should thus be what the pipeline handler returns, but I'd then argue
> it's the application's fault for asking for {raw, raw, nv12} in the
> first place. We need to keep a decent balance between the effort we put
> in adjusting the configuration to provide a result as close as possible
> to the requested configuration, and the difficulty to implement the
> heuristics.
>
> > [3] One stream configuration is not supported and cannot be trivially
> >     adjusted by, in example, rounding or changing its format. In this
> >     case the IPU3 pipe config calculation fails, and it's not easy to
> >     correct it as it would require:
> >     a) finding out which StreamConfiguration makes the calculation
> >     fail and why
> >     b) adjust it to a size that satisfies the pipeline configuration
> >     which is slightly obscure and not completely documented.
> >     As you can see, it's very platform specific.
>
> I know you don't like this, but that's a bug in the IPU3 pipeline
> handler, and should thus be fixed. We may not have enough information
> today to correctly understand how the ImgU operates, and thus how to
> address this issue, but that's not an excuse for not trying to get
> support.
>
> > All in all I think the criteria is "if it can be trivially adjusted,
> > adjust it, otherwise consider the configuration invalid".
>
> This could be replaced by "if a stream is adjustable, adjust it,
> otherwise drop it". The validate() call would then return Invalid only
> when no stream can be produced at all. What is a reasonable adjustement
> needs to be defined, trying too hard could be counterproductive. If we
> adjust {raw} to {nv12} because the pipeline handler doesn't support raw,
> I don't see what value that would bring given that an application
> requesting raw explicitly will likely just look at the adjusted
> configuration, raise its eyebrows, and say "seriously?" :-) On the other
> hand, adjusting {nv12} to {nv16} or {raw16} to {raw12} would make more
> sense. Let's not however forget that we also expose a list of supported
> formats, so we may not need to implement a complex negotiation procedure
> if the application can look at the supported formats first to avoid
> asking for something impossible.
>

Yes, what Laurent described sounds good to me.

> > Unfortunately, what can be "trivially adjusted" depends on the
> > platform constraints, and we should do better in documenting the most
> > trivial cases and ensure they are treated uniformly in all the
> > different pipelines.
> >
> > > Let's consider the second problem with another example, Android HAL
> > > requests NV12 360p and MJPEG 360p, and a native camera can output only
> > > a single stream, not both of them, at the same time.
> > > Libcamera Android HAL adaptation layer executes
> > > Configuration::validate() with them.
> > > The validate() returns Invalid because both of them cannot be produced
> > > simultaneously.
> > > However, then Android HAL adaptation layer may want to reproduce MJPEG
> > > 360p from NV12 360p.
> > > We would like to know "best-effort" accepted configurations and manage
> > > to produce all the requested streams from the accepted configurations.
> >
> > I think this 'optimizations' (use the less possible streams from the
> > camera and rely on software conversion as much as possible, as in this
> > example) should be kept within the android HAL. It is not easy to
> > guarantee a mapping that works on one platform works on all others,
> > and in the example you have provided it's not trivial for the camera
> > to decide which stream to drop (assuming dropping one stream is enough
> > to make the configuration Valid).
>
> I read Hiro-san's proposal as agreeing with this. What the HAL need to
> make this work is to receive {nv12 360p} when it asks for {nv12 360p,
> mjpeg 360p}, instead of validate() failing completely. How to then
> produce the additional stream using post-processors is indeed the job of
> the HAL (although I wouldn't rule out one day moving that code to
> helpers in libcamera so that any application could benefit from this
> feature).
>
> > I know I will be shot down quickly, but I'm afraid mappings between
> > Android requests and what has to be requested to the libcamera::Camera
> > are better recorded in a platform-specific configuration file. At
> > least for 'optimized' devices that have high quality requirements.
>
> You know my position on this, and I don't think it's very productive to
> try and reopen the discussion every time this topic comes up. I want the
> HAL to use a generic implementation that will produce good results. Best
> effort it is, but it must by no means be considered to allow a lousy
> implementation. I'm not opposed to a configuration file that would
> override some choices, or, better, influence the heuristics to force the
> selection of a particular option when multiple options are possible, but
> that will not be considered as a default. We can do better than that.
>

I'm afraid a considerable disadvantage of the static configuration
mapping is less scalability.
In my humble opinion, this should be a final backup solution for the
cases that a heuristics algorithm cannot handle.

> > If no configuration file is provided, mapping works 'best effort' and
> > we will try to do our best, without guarantees of consistent behaviour
> > among different platforms.
> >
> > > My proposal is to
> > > 1.) Change the Invalid definition to "no configuration is accepted or
> > > other critical fatals like config is empty".
> >
> > I would be very happy to see a more detailed documentation of the
> > return codes as you proposed doing for 'Invalid'
>
> I think that's aligned with what I wrote above, so there seems to be no
> disagreement :-)
>
> > > 2.) [Filter-rule] When CameraConfiguration drops given configurations,
> > > it drops ones given later. In other words, earlier the configuration
> > > is put in cong lists, higher priority the config has.
> > > 3.) [Best-effort config] When CameraConfiguration find a native camera
> > > cannot accept all the configurations, it tries to try the given
> > > configurations from begin to end increasing one by one.
> >
> > I think this could be made standard behaviour, but they're less
> > trivial than what it looks like.
> >
> > In example, in 2.) how does a pipeline handler know that it has to
> > drop one stream configuration to make the camera configuration valid ?
> > The trivial case is when the requested number of streams is larger
> > than the maximum number of streams, in this case yes, dropping the
> > last one is enough to make sure the validate() process can continue to
> > actually test what's inside each StreamConfiguration.
> >
> > But in other cases it might be less trivial to find out that just
> > dropping one stream makes the rest of the combination valid. Using the
> > fictional example of a platform with a single scaler that cannot be
> > shared among different streams, if you're asked for 2 scaled stream
> > what is the right behavior ? Drop one of the two or adjust one of the
> > two to the non-scaled size ?
>
> We currently consider camera configurations as sorted by priority order,

Thanks for telling me.
This is not documented and actually I can detect this by looking the
Android HAL adaptation layer implementation. ;)

> and allocate streams one by one, starting at the highest priority
> stream. We could thus drop any stream that can't be supported, moving to
> the next one. That's a simple heuristics.
>
> > 3.) is interesting. If I got it right, the camera configuration looks
> > like {stream1, stream2, ..., streamN}. The pipeline handler first try
> > {stream1} then {stream1, stream2} and so on until the configuration is
> > not valid. Again, the distinction between what is Invalid or
> > Adjustable might be tricky.
> >
> > > 4.) (Optional) Add a new return value between Adjusted and Invalid
> > > like Dropped/Filtered, so that we can know not all the streams are
> > > accepted. Though we can detect this in a caller by seeing the returned
> > > config.
> >
> > Dropped/Filtered is actually an 'Adjusted' special case, they can also
> > be provided as an 'hint' to validate() to suggest which adjustment
> > strategy to adopt.
> >
> > However, I see your problem and I think mapping:
> >
> >         [Android] x [software converter] x [camera capabilities]
> >
> > Is a problem in a too complex space to be solved to the level
> > requested by the typical Android certified devices. A configuration
> > file for the HAL that tells which post-processor to use, how to map
> > android requests to [processors * libcamera::Camera] and other product
> > specific details (I'm thinking about in example properties of the
> > installed lenses) might be a more effective solution.
>
> Once we'll have a good enough general heuristics in place, I'll be happy
> to discuss a configuration file to influence the results of the
> heuristics :-)
>

Thanks for detailed comments, Jacopo and Laurent.
You'all are roughly fine to go with my proposal.
Let me summarize my proposal as pseudo code.
[a typical validate() implementation]
Status validate() {
  size_t numStreams = 0;
  bool requireAdjusted = false;
  for (size_t i = 0; i < std::min(config_.size(), MAX_STREAMS); i++) {
    decltype(config_) config(config_, config_.size() + i + 1);
    bool adjusted = false;
    for (auto& c : config)
      adjusted |= doAdjust(&c); // Returns true if c needs to be adjusted.
    if (!tryConfig(config)) { // Returns true pipeline accepts |config|.
      break;
    }
    numStreams = config.size();
    requireAdjusted = adjusted;
  }

  if (numStreams == 0)
    return Invalid;
  if (numStreams != config_.size()) {
    return Adjusted;
  }
  return requireAdjusted ? Adjusted : Valid;
}

How do you think of this?

Best Regards,
-Hiro



> > > Any comments are appreciated.
> > > Thanks in advance!
> > >
> > > Best Regards,
> > > -Hiro
> > > ---------
> > > [1] https://git.linuxtv.org/libcamera.git/tree/src/libcamera/pipeline/ipu3/ipu3.cpp#n154
> > > [2] https://git.linuxtv.org/libcamera.git/tree/src/libcamera/pipeline/ipu3/ipu3.cpp#n175
> > > [3] https://git.linuxtv.org/libcamera.git/tree/src/libcamera/pipeline/ipu3/ipu3.cpp#n306
>
> --
> Regards,
>
> Laurent Pinchart