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

[Hirokazu Honda]

Hi,

gentle ping.
I am going to implement soon as I proposed if nobody is against.

Best Regards,
-Hiro

On Fri, Nov 6, 2020 at 6:44 PM Hirokazu Honda <hiroh at chromium.org> wrote:
>
> 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