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

[Hirokazu Honda]

On Mon, Nov 2, 2020 at 3:14 PM Hirokazu Honda <hiroh at chromium.org> wrote:
>
> 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
>

After rethinking the "best-effort" method, I think we would not rather
have a requirement that earlier configuration must be configurable.
We would rather pick up as many configurations as possible, keeping
the earlier capable configurations. See the sample implementation in
below.
This would increase the number of accepted configurations.
For instance, a user app requests {NV12-hd, Jpeg-vga}, and then a
libcamera adaptation layer adds NV12-vga for the case that jpeg cannot
be configured by the camera.
So {NV12-max, Jpeg-vga, NV12-vga} is given in validate().
With the previous best-effort algorithm, if a native camera doesn't
accept jpeg config, {NV12-max} is configured.
On the other hand, with the current algorithm, {NV12-max, NV12-vga} is
configured because NV12-vga is tried after Jpeg-vga config fails.

[Sample implementation]
Status validate() {
  decltype(config_) configs;
  bool requireAdjusted = false;
  for (size_t i = 0; i < config_.size() && configs.size() < MAX_STREAMS; ++i) {
    configs.push_back(config_[i]);
    bool adjusted = requireAdjusted | doAdjust(&config_[i]); //
Returns true if c is adjusted.
    if (!tryConfig(configs)) // Returns true pipeline accepts |configs|.
      configs.pop_back();
    else
      requireAdjusted = adjusted;
  }

  if (configs.empty())
    return Invalid;
  if (configs.size() != config_.size())
    return Adjusted;
  return requireAdjusted ? Adjusted : Valid;
}

How do you think?
-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