[libcamera-devel] [PATCH 14/20] libcamera: ipu3: Adjust and assign streams in validate()

[Laurent Pinchart]

Hi Jacopo,

On Wed, Jul 15, 2020 at 09:35:51AM +0200, Jacopo Mondi wrote:
> On Wed, Jul 15, 2020 at 01:06:52AM +0300, Laurent Pinchart wrote:
> > 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);
> >
> 
> Having chased for like 15 minutes why my Size didn't get update when I
> used this, I would really say yes.

Sorry about causing you to lose time :-/ I wonder why the compiler
doesn't warn when calling a const method and ignoring the result
completely.

I've pushed the in-place helpers, feel free to use them in a new version
of this series if you want.

> > > +		} 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());
> >
> 
> Yes, sorry, I got here through several attempts where I mangled sizes
> individually and didn't notice I could have used the new Size methods

No worries :-)

> > > +	}
> > >
> > > -	/* 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 ?
> 
> As we actually modify it's content. This is not a function parameter,
> where we enforce that rule, but I still find more natural to use
> pointers for things that have to be modified, and references for
> things that stay constant.

It could be a good practice indeed.

> > > +
> > > +		LOG(IPU3, Debug) << "Validating configuration: " << config_[i].toString();
> >
> > Would it make sense to s/configuration/stream/ ?
> 
> You probably suggested this already and I missed it.
> 
> > > +
> > > +
> >
> > Extra blank line ?
> 
> Have I missed a checkstyle warning ?

I wish checkstyle was perfect :-)

> > > +		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 ?
> 
> The ouput sizes have to be strictly smaller than the input frame size.
> Here I'm looking for the largest multiple of IMGU_OUTPUT_WIDTH_MARGIN
> which is strictly smaller than cio2Configuration_.size.width.
> 
> That's for the -1. The reason why I'm searching for a number aligned
> to that margin value comes from inspecting the pipe configuration tool
> results and the xml files. This seems to be the combination that
> satisfies the tool in most cases. I know it's weak as a reasoning, but
> I was not able to find out what are the hardware/firmware requirements
> that lead to this, if not the way parameters are computed by the
> script (which probably reflect how the hw/fw work though).

I'm sure we'll rework this in the future when we'll get a better
understanding of how the hardware works. Can I ask you to make a note of
the points you don't fully understand ? We can try to get answers to our
questions.

> > > +			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