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

[Laurent Pinchart]

Reviving a very old thread (time flies...)

On Mon, Dec 20, 2021 at 04:35:57PM +0000, Kieran Bingham wrote:
> 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.

The simple pipeline handler doesn't deal with raw streams in this case
(it can capture raw data, but when a converter is involved, the sensor
output has to be RGB or YUV, and is thus not raw in the traditional
sense), but conceptually, we have one stream coming from the camera
receiver, and a set of additional streams generated by the converter.
The same concept is thus applicable, but would require the ability to
expose some more information about streams to differentiate "direct" and
"post-processed" streams. I think that's a direction we should take
anyway.

> > 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
>  └────────────┘     └────────────┘

Note that there is no direct hardware connection between video_ and
convertor_, there's a memory buffer in-between.

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

And to add more fun, we can have two scalers in the capture pipeline,
one in the sensor (commonly through binning/skipping, but sometimes with
a real scaler as well) and one in the SoC.

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

Yes and no. The number of streams comes from the pipeline
SimplePipelineInfo structure, and is a static property of a platform.
The only platform that supports a converter today in the master branch
is imx7 (which covers some i.MX8 SoCs as well), and it is limited to one
stream, but we could conceptually have more.

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

FYI, I've resumed working on this.

-- 
Regards,

Laurent Pinchart