[libcamera-devel] [PATCH] libcamera: pipeline: simple: Support output size ranges
Benjamin Schaaf
ben.schaaf at gmail.com
Fri Dec 10 12:35:49 CET 2021
On Fri, Dec 10, 2021 at 11:53 AM Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> 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 ?
The sensor supports arbitrary resolutions with a built-in scaler.
> > > } 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