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

[Laurent Pinchart]

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

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

> 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,
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 :-)

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