| Message ID | 20211201152306.3401143-1-kieran.bingham@ideasonboard.com |
|---|---|
| State | Superseded |
| Delegated to: | Kieran Bingham |
| Headers | show |
| Series |
|
| Related | show |
Quoting Kieran Bingham (2021-12-01 15:23:06) > From: Benjamin Schaaf <ben.schaaf@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@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 >
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@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@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() << "-"
On Fri, Dec 10, 2021 at 11:53 AM Laurent Pinchart <laurent.pinchart@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@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@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
On Thu, Dec 9, 2021 at 1:02 AM Kieran Bingham <kieran.bingham@ideasonboard.com> wrote: > > Quoting Kieran Bingham (2021-12-01 15:23:06) > > From: Benjamin Schaaf <ben.schaaf@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@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? 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? > 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 > >
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? 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. 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 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. ``` 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? 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 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. Thanks, Dorota
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
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.
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; + Size size; + for (unsigned int i = 0; i < config->size(); ++i) { + StreamConfiguration &cfg = config->at(i); + size.expandTo(cfg.size); + } + 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; 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() << "-"