[libcamera-devel] [PATCH 14/20] libcamera: ipu3: Adjust and assign streams in validate()
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Wed Jul 15 00:06:52 CEST 2020
Hi Jacopo,
Thank you for the patch.
On Tue, Jul 14, 2020 at 12:42:06PM +0200, Jacopo Mondi wrote:
> Remove the adjustStream() and assignStream() methods, and perform stream
> adjustment and assignment while iterating the StreamConfiguration
> items.
>
> The adjustStream() implementation had some arbitrary assumption, like
> the main output having to be as large as the sensor resolution, and did
> not take into account the different alignment requirements between the
> main output and the viewfinder output.
>
> The assignStream() implementation also assumes only full-size streams
> can be produced by the main output, and having it as a separate function
> prevents adjusting streams according to which output they are assigned.
>
> Blend the two implementation in a single loop and perform the required
> stream adjustment and assignment in one go.
>
> As streams are now assigned at validate() time, remove the same
> operation from the configure() function.
>
> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> ---
> src/libcamera/pipeline/ipu3/ipu3.cpp | 247 ++++++++++++---------------
> 1 file changed, 108 insertions(+), 139 deletions(-)
>
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index 19e07fd57e39..1161987a4322 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -70,9 +70,6 @@ public:
> const std::vector<const Stream *> &streams() { return streams_; }
>
> private:
> - void assignStreams();
> - void adjustStream(StreamConfiguration &cfg, bool scale);
> -
> /*
> * The IPU3CameraData instance is guaranteed to be valid as long as the
> * corresponding Camera instance is valid. In order to borrow a
> @@ -137,96 +134,6 @@ IPU3CameraConfiguration::IPU3CameraConfiguration(Camera *camera,
> data_ = data;
> }
>
> -void IPU3CameraConfiguration::assignStreams()
> -{
> - /*
> - * Verify and update all configuration entries, and assign a stream to
> - * each of them. The viewfinder stream can scale, while the output
> - * stream can crop only, so select the output stream when the requested
> - * resolution is equal to the sensor resolution, and the viewfinder
> - * stream otherwise.
> - */
> - std::set<const Stream *> availableStreams = {
> - &data_->outStream_,
> - &data_->vfStream_,
> - &data_->rawStream_,
> - };
> -
> - /*
> - * The caller is responsible to limit the number of requested streams
> - * to a number supported by the pipeline before calling this function.
> - */
> - ASSERT(availableStreams.size() >= config_.size());
> -
> - streams_.clear();
> - streams_.reserve(config_.size());
> -
> - for (const StreamConfiguration &cfg : config_) {
> - const PixelFormatInfo &info =
> - PixelFormatInfo::info(cfg.pixelFormat);
> - const Stream *stream;
> -
> - if (info.colourEncoding == PixelFormatInfo::ColourEncodingRAW)
> - stream = &data_->rawStream_;
> - else if (cfg.size == cio2Configuration_.size)
> - stream = &data_->outStream_;
> - else
> - stream = &data_->vfStream_;
> -
> - if (availableStreams.find(stream) == availableStreams.end())
> - stream = *availableStreams.begin();
> -
> - streams_.push_back(stream);
> - availableStreams.erase(stream);
> - }
> -}
> -
> -void IPU3CameraConfiguration::adjustStream(StreamConfiguration &cfg, bool scale)
> -{
> - /* The only pixel format the driver supports is NV12. */
> - cfg.pixelFormat = formats::NV12;
> -
> - if (scale) {
> - /*
> - * Provide a suitable default that matches the sensor aspect
> - * ratio.
> - */
> - if (cfg.size.isNull()) {
> - cfg.size.width = 1280;
> - cfg.size.height = 1280 * cio2Configuration_.size.height
> - / cio2Configuration_.size.width;
> - }
> -
> - /*
> - * \todo: Clamp the size to the hardware bounds when we will
> - * figure them out.
> - *
> - * \todo: Handle the scaler (BDS) restrictions. The BDS can
> - * only scale with the same factor in both directions, and the
> - * scaling factor is limited to a multiple of 1/32. At the
> - * moment the ImgU driver hides these constraints by applying
> - * additional cropping, this should be fixed on the driver
> - * side, and cropping should be exposed to us.
> - */
> - } else {
> - /*
> - * \todo: Properly support cropping when the ImgU driver
> - * interface will be cleaned up.
> - */
> - cfg.size = cio2Configuration_.size;
> - }
> -
> - /*
> - * Clamp the size to match the ImgU alignment constraints. The width
> - * shall be a multiple of 8 pixels and the height a multiple of 4
> - * pixels.
> - */
> - if (cfg.size.width % 8 || cfg.size.height % 4) {
> - cfg.size.width &= ~7;
> - cfg.size.height &= ~3;
> - }
> -}
> -
> CameraConfiguration::Status IPU3CameraConfiguration::validate()
> {
> Status status = Valid;
> @@ -242,71 +149,141 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()
>
> /*
> * Validate the requested stream configuration and select the sensor
> - * format by collecting the maximum width and height and picking the
> - * closest larger match, as the IPU3 can downscale only. If no
> - * resolution is requested for any stream, or if no sensor resolution is
> - * large enough, pick the largest one.
> + * format by collecting the maximum RAW stream width and height and
> + * picking the closest larger match, as the IPU3 can downscale only. If
> + * no resolution is requested for the RAW stream, use the one from the
> + * largest YUV stream, plus margins pixels for the IF and BDS to scale.
> + * If no resolution is requested for any stream, pick the largest one.
> */
> unsigned int rawCount = 0;
> unsigned int yuvCount = 0;
> - Size size;
> + Size maxYuvSize;
> + Size maxRawSize;
>
> for (const StreamConfiguration &cfg : config_) {
> const PixelFormatInfo &info = PixelFormatInfo::info(cfg.pixelFormat);
>
> - if (info.colourEncoding == PixelFormatInfo::ColourEncodingRAW)
> + if (info.colourEncoding == PixelFormatInfo::ColourEncodingRAW) {
> rawCount++;
> - else
> + maxRawSize = maxRawSize.expandedTo(cfg.size);
Not something that is needed as a prerequisite for this series, but do
you think it would be useful to also have in-place versions of the Size
(and Rectangle) helpers ? This line would become
maxRawSize.expandTo(cfg.size);
> + } else {
> yuvCount++;
> -
> - if (cfg.size.width > size.width)
> - size.width = cfg.size.width;
> - if (cfg.size.height > size.height)
> - size.height = cfg.size.height;
> + maxYuvSize = maxYuvSize.expandedTo(cfg.size);
> + }
> }
> if (rawCount > 1 || yuvCount > 2) {
> LOG(IPU3, Debug)
> << "Camera configuration not supported";
> return Invalid;
> }
Maybe a blank line here (and above this block too) ?
> + if (maxRawSize.isNull()) {
> + maxRawSize.width = utils::alignUp(maxYuvSize.width,
> + IMGU_OUTPUT_WIDTH_MARGIN);
> + maxRawSize.height = utils::alignUp(maxYuvSize.height,
> + IMGU_OUTPUT_HEIGHT_MARGIN);
Maybe
maxRawSize = maxYuvSize.alignedUpTo(IMGU_OUTPUT_WIDTH_MARGIN,
IMGU_OUTPUT_HEIGHT_MARGIN);
?
> + maxRawSize = maxRawSize.boundedTo(data_->cio2_.sensor()->resolution());
Or even
maxRawSize = maxYuvSize.alignedUpTo(IMGU_OUTPUT_WIDTH_MARGIN,
IMGU_OUTPUT_HEIGHT_MARGIN)
.boundedTo(data_->cio2_.sensor()->resolution());
> + }
>
> - /* Generate raw configuration from CIO2. */
> - cio2Configuration_ = data_->cio2_.generateConfiguration(size);
> + /*
> + * Generate raw configuration from CIO2.
> + *
> + * The output YUV streams will be limited in size to the maximum
> + * frame size requested for the RAW stream.
> + */
> + cio2Configuration_ = data_->cio2_.generateConfiguration(maxRawSize);
> if (!cio2Configuration_.pixelFormat.isValid())
> return Invalid;
>
> - /* Assign streams to each configuration entry. */
> - assignStreams();
> + LOG(IPU3, Debug) << "CIO2 configuration: " << cio2Configuration_.toString();
>
> - /* Verify and adjust configuration if needed. */
> + /*
> + * Adjust the configurations if needed and assign streams while
> + * iterating them.
> + */
> + bool mainOutputAvailable = true;
> for (unsigned int i = 0; i < config_.size(); ++i) {
> - StreamConfiguration &cfg = config_[i];
> - const StreamConfiguration oldCfg = cfg;
> - const Stream *stream = streams_[i];
> -
> - if (stream == &data_->rawStream_) {
> - cfg.size = cio2Configuration_.size;
> - cfg.pixelFormat = cio2Configuration_.pixelFormat;
> - cfg.bufferCount = cio2Configuration_.bufferCount;
> + const PixelFormatInfo &info = PixelFormatInfo::info(config_[i].pixelFormat);
> + const StreamConfiguration originalCfg = config_[i];
> + StreamConfiguration *cfg = &config_[i];
I'm not asking for this to be changed, but out of curiotiry, why are you
turning the reference into a pointer ?
> +
> + LOG(IPU3, Debug) << "Validating configuration: " << config_[i].toString();
Would it make sense to s/configuration/stream/ ?
> +
> +
Extra blank line ?
> + if (info.colourEncoding == PixelFormatInfo::ColourEncodingRAW) {
> + /* Initialize the RAW stream with the CIO2 configuration. */
> + cfg->size = cio2Configuration_.size;
> + cfg->pixelFormat = cio2Configuration_.pixelFormat;
> + cfg->bufferCount = cio2Configuration_.bufferCount;
> + cfg->stride = info.stride(cfg->size.width, 0, 64);
> + cfg->frameSize = info.frameSize(cfg->size, 64);
> + cfg->setStream(const_cast<Stream *>(&data_->rawStream_));
> +
> + LOG(IPU3, Debug) << "Assigned " << cfg->toString()
> + << " to the raw stream";
> } else {
> - bool scale = stream == &data_->vfStream_;
> - adjustStream(config_[i], scale);
> - cfg.bufferCount = IPU3_BUFFER_COUNT;
> + /* Assign and configure the main and viewfinder outputs. */
> +
> + /*
> + * Clamp the size to match the ImgU size limits and the
> + * margins from the CIO2 output frame size.
> + *
> + * The ImgU outputs needs to be rounded down to 64
> + * pixels in width and 32 pixels in height from the
> + * input frame size.
> + *
> + * \todo Verify this assumption and find out if it
> + * depends on the BDS scaling factor of 1/32, as the
> + * main output has no YUV scaler as the viewfinder
> + * output has.
> + */
> + unsigned int limit;
> + limit = utils::alignDown(cio2Configuration_.size.width - 1,
> + IMGU_OUTPUT_WIDTH_MARGIN);
Where does the - 1 come from ?
> + cfg->size.width = utils::clamp(cfg->size.width,
> + IMGU_OUTPUT_MIN_SIZE.width,
> + limit);
> +
> + limit = utils::alignDown(cio2Configuration_.size.height - 1,
> + IMGU_OUTPUT_HEIGHT_MARGIN);
> + cfg->size.height = utils::clamp(cfg->size.height,
> + IMGU_OUTPUT_MIN_SIZE.height,
> + limit);
> +
> + cfg->size = cfg->size.alignedDownTo(IMGU_OUTPUT_WIDTH_ALIGN,
> + IMGU_OUTPUT_HEIGHT_ALIGN);
> +
> + cfg->pixelFormat = formats::NV12;
> + cfg->bufferCount = IPU3_BUFFER_COUNT;
> + cfg->stride = info.stride(cfg->size.width, 0, 1);
> + cfg->frameSize = info.frameSize(cfg->size, 1);
> +
> + /*
> + * Use the main output stream in case only one stream is
> + * requested or if the current configuration is the one with
> + * the maximum YUV output size.
> + */
> + if (mainOutputAvailable &&
> + (originalCfg.size == maxYuvSize || yuvCount == 1)) {
> + cfg->setStream(const_cast<Stream *>(&data_->outStream_));
> + mainOutputAvailable = false;
> +
> + LOG(IPU3, Debug) << "Assigned " << cfg->toString()
> + << " to the main output";
> + } else {
> + cfg->setStream(const_cast<Stream *>(&data_->vfStream_));
> +
> + LOG(IPU3, Debug) << "Assigned " << cfg->toString()
> + << " to the viewfinder output";
> + }
> }
>
> - if (cfg.pixelFormat != oldCfg.pixelFormat ||
> - cfg.size != oldCfg.size) {
> + if (cfg->pixelFormat != originalCfg.pixelFormat ||
> + cfg->size != originalCfg.size) {
> LOG(IPU3, Debug)
> << "Stream " << i << " configuration adjusted to "
> - << cfg.toString();
> + << cfg->toString();
> status = Adjusted;
> }
> -
> - const PixelFormatInfo &info = PixelFormatInfo::info(cfg.pixelFormat);
> - bool packedRaw = info.colourEncoding == PixelFormatInfo::ColourEncodingRAW;
> -
> - cfg.stride = info.stride(cfg.size.width, 0, packedRaw ? 64 : 1);
> - cfg.frameSize = info.frameSize(cfg.size, packedRaw ? 64 : 1);
> }
>
> return status;
> @@ -475,16 +452,8 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c)
> bool vfActive = false;
>
> for (unsigned int i = 0; i < config->size(); ++i) {
> - /*
> - * Use a const_cast<> here instead of storing a mutable stream
> - * pointer in the configuration to let the compiler catch
> - * unwanted modifications of camera data in the configuration
> - * validate() implementation.
> - */
> - Stream *stream = const_cast<Stream *>(config->streams()[i]);
> StreamConfiguration &cfg = (*config)[i];
> -
> - cfg.setStream(stream);
> + Stream *stream = cfg.stream();
>
> if (stream == outStream) {
> ret = imgu->configureOutput(cfg, &outputFormat);
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list