[libcamera-devel] [PATCH 02/15] libcamera: ipu3: Remove streams from generateConfiguration

[Kieran Bingham]

Hi Jacopo,

On 01/07/2020 13:30, Jacopo Mondi wrote:
> Remove stream assignement from the IPU3 pipeline handler
> generateConfiguration() implementation.
> 
> The function aims to provide a suitable default for the requested use
> cases. Defer stream assignement to validation and only assign sizes
> and formats to stream configurations.

s/assignement/assignment/ in two locations above


Excellent, this sounds like exactly what will be needed as part of the
use of empty configurations for the Android HAL multi-stream work.

Is all the code being removed already in validate()?
I see an 'assignStreams()' there so I guess that covers it.

Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>

Trivial comment below but no blocker, and I saw Niklas had a couple of
minor comments too, but those aside, I'll see how the rest of this
series affects the HAL multi-stream work.

--
Kieran


> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> ---
>  src/libcamera/pipeline/ipu3/ipu3.cpp | 93 ++++++++--------------------
>  1 file changed, 27 insertions(+), 66 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index 1bdad209de6e..97fc8b60c3cb 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -31,6 +31,9 @@ namespace libcamera {
>  
>  LOG_DEFINE_CATEGORY(IPU3)
>  
> +static constexpr unsigned int IPU3_BUFFER_COUNT = 4;
> +static constexpr unsigned int IPU3_MAX_STREAMS = 3;
> +
>  class IPU3CameraData : public CameraData
>  {
>  public:
> @@ -61,9 +64,6 @@ public:
>  	const std::vector<const Stream *> &streams() { return streams_; }
>  
>  private:
> -	static constexpr unsigned int IPU3_BUFFER_COUNT = 4;
> -	static constexpr unsigned int IPU3_MAX_STREAMS = 3;
> -
>  	void assignStreams();
>  	void adjustStream(StreamConfiguration &cfg, bool scale);
>  
> @@ -293,94 +293,55 @@ CameraConfiguration *PipelineHandlerIPU3::generateConfiguration(Camera *camera,
>  {
>  	IPU3CameraData *data = cameraData(camera);
>  	IPU3CameraConfiguration *config = new IPU3CameraConfiguration(camera, data);
> -	std::set<Stream *> streams = {
> -		&data->outStream_,
> -		&data->vfStream_,
> -		&data->rawStream_,
> -	};
>  
>  	if (roles.empty())
>  		return config;
>  
> +	Size sensorResolution = data->cio2_.sensor()->resolution();
> +	unsigned int rawCount = 0;
> +	unsigned int outCount = 0;
>  	for (const StreamRole role : roles) {
>  		StreamConfiguration cfg = {};
> -		Stream *stream = nullptr;
> -
> -		cfg.pixelFormat = formats::NV12;
>  
>  		switch (role) {
>  		case StreamRole::StillCapture:
>  			/*
> -			 * Pick the output stream by default as the Viewfinder
> -			 * and VideoRecording roles are not allowed on
> -			 * the output stream.
> +			 * Use the sensor resolution adjusted to respect the
> +			 * ImgU output alignement contraints.
>  			 */
> -			if (streams.find(&data->outStream_) != streams.end()) {
> -				stream = &data->outStream_;
> -			} else if (streams.find(&data->vfStream_) != streams.end()) {
> -				stream = &data->vfStream_;
> -			} else {
> -				LOG(IPU3, Error)
> -					<< "No stream available for requested role "
> -					<< role;
> -				break;
> -			}
> +			cfg.pixelFormat = formats::NV12;
> +			cfg.size = sensorResolution;
> +			cfg.size.width &= ~7;
> +			cfg.size.height &= ~3;

Hrm ... do we need some align macros to make this more readable?
Not necessarily required for this patch though.

> +			cfg.bufferCount = IPU3_BUFFER_COUNT;
>  
> -			/*
> -			 * FIXME: Soraka: the maximum resolution reported by
> -			 * both sensors (2592x1944 for ov5670 and 4224x3136 for
> -			 * ov13858) are returned as default configurations but
> -			 * they're not correctly processed by the ImgU.
> -			 * Resolutions up tp 2560x1920 have been validated.
> -			 *
> -			 * \todo Clarify ImgU alignment requirements.
> -			 */
> -			cfg.size = { 2560, 1920 };
> +			outCount++;
>  
>  			break;
>  
>  		case StreamRole::StillCaptureRaw: {
> -			if (streams.find(&data->rawStream_) == streams.end()) {
> -				LOG(IPU3, Error)
> -					<< "Multiple raw streams are not supported";
> -				break;
> -			}
> +			cfg = data->cio2_.generateConfiguration(sensorResolution);
> +			cfg.bufferCount = 1;
>  
> -			stream = &data->rawStream_;
> +			rawCount++;
>  
> -			cfg.size = data->cio2_.sensor()->resolution();
> -
> -			cfg = data->cio2_.generateConfiguration(cfg.size);
>  			break;
>  		}
>  
>  		case StreamRole::Viewfinder:
>  		case StreamRole::VideoRecording: {
> -			/*
> -			 * We can't use the 'output' stream for viewfinder or
> -			 * video capture roles.
> -			 *
> -			 * \todo This is an artificial limitation until we
> -			 * figure out the exact capabilities of the hardware.
> -			 */
> -			if (streams.find(&data->vfStream_) == streams.end()) {
> -				LOG(IPU3, Error)
> -					<< "No stream available for requested role "
> -					<< role;
> -				break;
> -			}
> -
> -			stream = &data->vfStream_;
> -
>  			/*
>  			 * Align the default viewfinder size to the maximum
>  			 * available sensor resolution and to the IPU3
>  			 * alignment constraints.
>  			 */
> -			const Size &res = data->cio2_.sensor()->resolution();
> -			unsigned int width = std::min(1280U, res.width);
> -			unsigned int height = std::min(720U, res.height);
> +			unsigned int width = std::min(1280U, sensorResolution.width);
> +			unsigned int height = std::min(720U, sensorResolution.height);
>  			cfg.size = { width & ~7, height & ~3 };
> +			cfg.pixelFormat = formats::NV12;
> +			cfg.bufferCount = IPU3_BUFFER_COUNT;
> +
> +			outCount++;
>  
>  			break;
>  		}
> @@ -388,16 +349,16 @@ CameraConfiguration *PipelineHandlerIPU3::generateConfiguration(Camera *camera,
>  		default:
>  			LOG(IPU3, Error)
>  				<< "Requested stream role not supported: " << role;
> -			break;
> +			delete config;
> +			return nullptr;
>  		}
>  
> -		if (!stream) {
> +		if (rawCount > 1 || outCount > 2) {
> +			LOG(IPU3, Error) << "Invalid stream roles requested";
>  			delete config;
>  			return nullptr;
>  		}
>  
> -		streams.erase(stream);
> -
>  		config->addConfiguration(cfg);
>  	}
>  
> 

-- 
Regards
--
Kieran