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

[Kieran Bingham]

Quoting Kieran Bingham (2021-12-01 15:23:06)
> From: Benjamin Schaaf <ben.schaaf at gmail.com>
> 
> Use the list of supported ranges from the video format to configure the
> stream and subdev instead of the capture size, allowing streams to be
> configured below the maximum resolution.
> 
> Bug: https://bugs.libcamera.org/show_bug.cgi?id=96
> Signed-off-by: Benjamin Schaaf <ben.schaaf at gmail.com>
> ---
> Sent on behalf of Benjamin as posted at https://bugs.libcamera.org/attachment.cgi?id=33
> 
>  src/libcamera/pipeline/simple/simple.cpp | 34 ++++++++++++++++--------
>  1 file changed, 23 insertions(+), 11 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
> index 701fb4be0b71..a597e27f6139 100644
> --- a/src/libcamera/pipeline/simple/simple.cpp
> +++ b/src/libcamera/pipeline/simple/simple.cpp
> @@ -213,7 +213,7 @@ public:
>                 PixelFormat captureFormat;
>                 Size captureSize;
>                 std::vector<PixelFormat> outputFormats;
> -               SizeRange outputSizes;
> +               std::vector<SizeRange> outputSizes;
>         };
>  
>         std::vector<Stream> streams_;
> @@ -492,10 +492,10 @@ int SimpleCameraData::init()
>  
>                         if (!converter_) {
>                                 config.outputFormats = { pixelFormat };
> -                               config.outputSizes = config.captureSize;
> +                               config.outputSizes = videoFormat.second;
>                         } else {
>                                 config.outputFormats = converter_->formats(pixelFormat);
> -                               config.outputSizes = converter_->sizes(format.size);
> +                               config.outputSizes = { converter_->sizes(format.size) };
>                         }
>  
>                         configs_.push_back(config);
> @@ -804,17 +804,23 @@ CameraConfiguration::Status SimpleCameraConfiguration::validate()
>                         status = Adjusted;
>                 }
>  
> -               if (!pipeConfig_->outputSizes.contains(cfg.size)) {
> +               auto sizeIt = std::find_if(pipeConfig_->outputSizes.begin(),
> +                                          pipeConfig_->outputSizes.end(),
> +                                          [&](SizeRange r) { return r.contains(cfg.size); });
> +               if (sizeIt == pipeConfig_->outputSizes.end()) {
>                         LOG(SimplePipeline, Debug)
>                                 << "Adjusting size from " << cfg.size.toString()
>                                 << " to " << pipeConfig_->captureSize.toString();
> -                       cfg.size = pipeConfig_->captureSize;
> +
> +                       cfg.size = pipeConfig_->outputSizes[0].max;
>                         status = Adjusted;
> +
> +                       /* \todo Create a libcamera core class to group format and size */
> +                       if (cfg.size != pipeConfig_->captureSize)
> +                               needConversion_ = true;
>                 }
>  
> -               /* \todo Create a libcamera core class to group format and size */
> -               if (cfg.pixelFormat != pipeConfig_->captureFormat ||
> -                   cfg.size != pipeConfig_->captureSize)
> +               if (cfg.pixelFormat != pipeConfig_->captureFormat)
>                         needConversion_ = true;
>  
>                 /* Set the stride, frameSize and bufferCount. */
> @@ -907,8 +913,14 @@ int SimplePipelineHandler::configure(Camera *camera, CameraConfiguration *c)
>         if (ret < 0)
>                 return ret;
>  

It took me a moment to realize what this code is doing. A block comment
would help

	/*
	 * Iterate the requested streams and identify the largest
	 * frame size. Use that to configure the capture device.
	 */

> +       Size size;

I think it would help to call this something more specific, perhaps sensorSize;

> +       for (unsigned int i = 0; i < config->size(); ++i) {
> +               StreamConfiguration &cfg = config->at(i);
> +               size.expandTo(cfg.size);
> +       }

What would happen if the user requests a stream larger than the
Sensor/capture device can produce (and expects a convertor to upscale
it?)

Maybe this is as simple as restricting it a max of
data->sensor_->resolution().

But I'm not sure this is quite right ;-(

It worries me that this might be 'changing' the configuration after the
validation process, which we must make sure we don't do.


> +
>         const SimpleCameraData::Configuration *pipeConfig = config->pipeConfig();
> -       V4L2SubdeviceFormat format{ pipeConfig->code, data->sensor_->resolution() };
> +       V4L2SubdeviceFormat format{ pipeConfig->code, size };
>  
>         ret = data->setupFormats(&format, V4L2Subdevice::ActiveFormat);
>         if (ret < 0)
> @@ -919,7 +931,7 @@ int SimplePipelineHandler::configure(Camera *camera, CameraConfiguration *c)
>  
>         V4L2DeviceFormat captureFormat;
>         captureFormat.fourcc = videoFormat;
> -       captureFormat.size = pipeConfig->captureSize;
> +       captureFormat.size = size;

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


I believe the use case you are extending is 1), so that it can expose
multiple stream sizes from the device.

I am worried that it might be breaking use case 2) though, as we must
make sure the video_ is configured to a format that's compatible there,
while the Stream0 produces the (possibly format converted, and rescaled)
output from the convertor_.

During validate we need to:

Check the number of streams:
  If 2: we're using a convertor.
   video_ and convertor_ should be configured as set in those streams
   explicitly, or fail, (or return adjusted).


 if 1 stream:
   *1 iterate supported frame sizes from the video_
   *2 choose closest match

   set the convertor (if available) to try to process any remaining change.


>From what I understand, *1 currently without this patch is simply taking
the biggest size supported, and there is no *2, so those are the parts
that this patch is adding.

Is that close?

The important part might be how we convey between validate() and
configure() which mode (1,2,3) we're in and ensure that we do not make
any changes to that during configure.

--
Kieran



>         ret = video->setFormat(&captureFormat);
>         if (ret)
> @@ -932,7 +944,7 @@ int SimplePipelineHandler::configure(Camera *camera, CameraConfiguration *c)
>         }
>  
>         if (captureFormat.fourcc != videoFormat ||
> -           captureFormat.size != pipeConfig->captureSize) {
> +           captureFormat.size != size) {
>                 LOG(SimplePipeline, Error)
>                         << "Unable to configure capture in "
>                         << pipeConfig->captureSize.toString() << "-"
> -- 
> 2.30.2
>