[libcamera-devel] [PATCH] libcamera: pipeline: simple: Support output size ranges

[Kieran Bingham]

Quoting Dorota Czaplejewicz (2021-12-17 18:43:26)
> Hi,
> 
> I think I know how to solve this, and I've been working on it today.
> 
> There's an open question that Benjamin posed: what's the policy for choosing size adjustments?

The tricky part is being able to expose what the hardware can provide,
and validate that the configuration requested by an application can be
supported by the underlying hardware.

If the configuration can not be supported, then the configuration should
be modified - "Adjusted" to fit the capabilities by any means the
pipeline handler sees fit to do so.

The configure() operation will call validate() and it is only allowed to
configure if validate() completes without making any adjustments.


Trying to go back to what I think is the 'open question':

    With the converter there seems to be a disconnect between two
    different functions of `SimpleCameraConfiguration::validate`: Some
    applications will want to use the closest native resolution of the
    camera, whereas others will want the converter to match whatever
    resolution is requested. There's no way to specify which with the
    current API. What's the solution here?


I believe we've handled this on the Raspberry Pi by exposing the native
resoltions of the sensor in the 'Raw' stream. (Which is the stream
directly out of the video_ node in this case.


> I think at this stage, that question is best answered by choosing an
> arbitrary policy and dedicating the effort to get this to work at all.

I'm not quite sure what this implies. What arbitrary policy do you have
in mind?

> Then to thinking about exposing new APIs.
> 
> Now, i have immediate questions about the convertor setup, before I
> submit my take on the same patch.
> 
> > I'm having a hard time to grasp if the core requirements are being met
> > though.
> > 
> > We have two devices that could be configured in 3 different ways:
> > 
> > 1)
> >   ┌────────────┐
> >   │   video_   ├──► Stream0
> >   └────────────┘
> > 
> > 2)
> >   ┌────────────┐   ┌────────────┐
> >   │   video_   ├──►│ convertor_ ├──►  Stream0
> >   └────────────┘   └────────────┘
> > 
> > 3)
> >   ┌────────────┐   ┌────────────┐
> >   │   video_   ├──►│ convertor_ ├──►  Stream1
> >   └───────┬────┘   └────────────┘
> >           │
> >           └► stream0
> > 
> 
> Is this last picture really correct? I was under the impression that
> the sensor is expected to create only one stream, and all extra
> streams are handled by the converter.  A comment in camera.cpp says:
> 
> > * It is possible to produce up to one stream without conversion

Correct, that is use case 1.

Perhaps it would be better to draw 3) as:


                ┌───────────────────►  stream0
                │
 ┌────────────┐ │   ┌────────────┐
 │   video_   ├─┴──►│ convertor_ ├──►  stream1
 └────────────┘     └────────────┘


> There's also the question about how independent the different streams
> can be. It's undocumented, but SimpleCameraData::Configuration seems
> to define the range of configurations which can be simultaneously
> deployed on multiple streams.

The only way two streams could be handled here, is by the buffer being
passed from the video_ to convertor_ ... and when the convertor is
finished reading, it can be passed to the application in the request.

So the application would see two streams.

   video_ output (exactly as is passed into the convertor_)
   convertor_ output.


Note when I say 'can be passed' - that doesn't mean it is currently
implemented. It means it could be implemented, if we need to expose
access to the 'raw' (pre-converted) frames.


Note also that the documentation for the simple pipeline handler states:

  * Format Conversion and Scaling
  * -----------------------------
  *
  * The capture pipeline isn't expected to include a scaler, and if a scaler is
  * available, it is ignored when configuring the pipeline.


So this current activity is about changing that as I understand it. 

 
> ```
> struct Configuration {
>         ...
>         std::vector<PixelFormat> outputFormats;
>         SizeRange outputSizes;
> }
> ```
> 
> Reading through the validate() code, any combination of those will be accepted, no matter how many streams.
> 
> Is that the intended function of this structure?

It is the job of the pipeline handler to validate the configuration is
acceptable and functional on the hardware.

So in this case, it's up to the simple pipeline handler to ensure that a
CameraConfiguration (with it's stream configurations) passed in can be
configured.

If the existing code base is not doing that, then it should be updated.


But that struct Configuration is internal to the SimplePipeline handler,
so it is not exposed to applications anyway, and doesn't need
'validation'.

It is the CameraConfiguration that must be validated.


And the number of streams there is certainly limited by the code:

    SimpleCameraConfiguration::validate()
    {
        ...

	/* Cap the number of entries to the available streams. */
	if (config_.size() > data_->streams_.size()) {
		config_.resize(data_->streams_.size());
		status = Adjusted;
	}

	...
    }

I would assume/expect that data_->streams_ is always restricted to a
single stream at the moment, but I have not confirmed this.


> Originally I was under an impression that it merely enumerated
> supported modes, and that replacing the outputFormats collection with
> multiple Configurations, each with a single outputFormat would not
> change any functionality. But now I think this is actually overloaded
> with converter configuration.

I'm not sure I fully understand here, There is a defined configuration
process... somewhat off the top of my head as a high-level view:

 # Optionally specify roles to prefill
 c = Camera->generateConfiguration({Raw, Viewfinder})

 # Set any parameters desired on the stream Configuration (stream0)
 c.at(0).size = { 1920, 1080 };

 # Set our viewFinder size
 c.at(1).size = { 640, 480 };

 # Validate it. If it can't be handled, the pipeline handler will update
 # to what can be.
 ret = c->validate();

 if (ret == Adjusted) {
	# The pipeline handler made changes, we might want to check, or
	# we can just accept it.
	# If we don't like what was returned, we can change it again,
	# but we should re-validate it before calling configure - or the
	# configure could simply fail.
 }

 # The now adjusted configuration should be guaranteed to provide a
 # useable configuration.
 ret = Camera.configure(c);

> I don't want to mess up the structuring of this code, so I'm going to
> hold on until I get an answer to this question.

That's understandable, I suspect there are a few more design questions
to solve along the way too.

--
Kieran


> Thanks,
> Dorota