[libcamera-devel] [PATCH] libcamera: pipeline: simple: Support output size ranges
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Fri Dec 10 01:52:34 CET 2021
Hi Benjamin,
Thank you for the patch.
On Wed, Dec 08, 2021 at 02:02:21PM +0000, Kieran Bingham wrote:
> 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;
I think this will be a problem. videoFormat.second comes from the
enumeration of supported sizes by the video node, but doesn't take into
account what is produced by the pipeline. It only exposes the intrinsic
capabilities of the video node. I'm pretty sure this will break other
supported platforms.
I assume your platform doesn't have a converter. How do you get support
for lower resolutions, are they generated directly by the sensor, or do
you have a scaler in the pipeline ?
> > } 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.
>
> > 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() << "-"
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list