[libcamera-devel] [PATCH v4 11/12] libcamera: ipu3: Use roles in stream configuration

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Sun, Apr 14, 2019 at 10:45:18PM +0200, Niklas Söderlund wrote:
> On 2019-04-09 21:25:47 +0200, Jacopo Mondi wrote:
> > Use and inspect the stream roles provided by the application to
> > streamConfiguration() to assign streams to their intended roles and
> > return a default configuration associated with them.
> > 
> > Support a limited number of usages, as the viewfinder stream can
> > optionally be used for capturing still images, but the main output
> > stream cannot be used as viewfinder or for video recording purposes.

Is this a limitation of the device, or of the pipeline handler ?

> > 
> > Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> > ---
> >  src/libcamera/pipeline/ipu3/ipu3.cpp | 105 ++++++++++++++++++++-------
> >  1 file changed, 79 insertions(+), 26 deletions(-)
> > 
> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > index 75ffdc56d157..70a92783076f 100644
> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > @@ -227,36 +227,89 @@ CameraConfiguration
> >  PipelineHandlerIPU3::streamConfiguration(Camera *camera,
> >  					 const std::vector<StreamUsage> &usages)
> >  {
> > -	CameraConfiguration configs;
> >  	IPU3CameraData *data = cameraData(camera);
> > -	StreamConfiguration config = {};
> > +	CameraConfiguration configs;

I wouldn't use the plural for a single object. How about
CameraConfiguration config and StreamConfiguration cfg ? Or
CameraConfiguration ccfg and StreamConfiguration scfg ? There's also the
option of cameraConfig and streamConfig but that may be a bit long.

> > +	std::vector<IPU3Stream *> availableStreams = {
> 
> Using a std::set instead of std::vector here would be useful. As you 
> can't have the same stream in the set twice there is no limitation 
> moving from a vector to a set.
> 
> > +		&data->outStream_,
> > +		&data->vfStream_,
> > +	};
> >  
> > -	/*
> > -	 * 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 alignement requirements.
> > -	 */
> > -	config.width = 2560;
> > -	config.height = 1920;
> > -	config.pixelFormat = V4L2_PIX_FMT_NV12;
> > -	config.bufferCount = IPU3_BUFFER_COUNT;
> > -
> > -	configs[&data->outStream_] = config;
> > -	LOG(IPU3, Debug)
> > -		<< "Stream 'output' format set to " << config.width << "x"
> > -		<< config.height << "-0x" << std::hex << std::setfill('0')
> > -		<< std::setw(8) << config.pixelFormat;
> > -
> > -	configs[&data->vfStream_] = config;
> > -	LOG(IPU3, Debug)
> > -		<< "Stream 'viewfinder' format set to " << config.width << "x"
> > -		<< config.height << "-0x" << std::hex << std::setfill('0')
> > -		<< std::setw(8) << config.pixelFormat;
> > +	for (const StreamUsage &usage : usages) {
> > +		std::vector<IPU3Stream *>::iterator found;
> 
> Can be refactored away if availableStreams is a std::set, see example 
> bellow.
> 
> > +		enum StreamUsage::Role r = usage.role();
> 
> s/enum//

And s/r/role/ ? r is a bit confusing, it also commonly refers to a
rectangle.

> > +		StreamConfiguration config = {};
> > +		IPU3Stream *stream = nullptr;
> > +
> > +		std::vector<IPU3Stream *>::iterator s = availableStreams.begin();
> > +		if (r == StreamUsage::Role::StillCapture) {
> > +			for (; s < availableStreams.end(); ++s) {
> > +				/*
> > +				 * We can use the viewfinder stream in case
> > +				 * the 'StillCapture' usage is required
> > +				 * multiple times.
> > +				 */
> > +				if (*s == &data->outStream_) {
> > +					stream = &data->outStream_;
> > +					found = s;
> > +					break;
> > +				} else {
> > +					stream = &data->vfStream_;
> > +					found = s;
> > +				}
> > +			}
> 
> The for() loop can be replaced with this if availableStreams is std::set
> 
> 
>     if (availableStreams.find(&data->outStream_) != availableStreams.end())
>         stream = &data->outStream_;
>     else
>         stream = &data->vfStream_;
> 
> > +
> > +			/*
> > +			 * 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.
> > +			 */
> > +			config.width = 2560;
> > +			config.height = 1920;
> > +		} else if (r == StreamUsage::Role::Viewfinder ||
> > +			   r == StreamUsage::Role::VideoRecording) {
> > +			for (; s < availableStreams.end(); ++s) {
> > +				/*
> > +				 * We can't use the 'output' stream for
> > +				 * viewfinder or video capture usages.
> > +				 */
> > +				if (*s != &data->vfStream_)
> > +					continue;
> > +
> > +				stream = &data->vfStream_;
> > +				found = s;
> > +				break;
> > +			}

And here

			if (availableStreams.find(&data->vfStream_) !=
			    availableStreams.end())
				stream = &data->vfStream_;

You may want to rename availableStreams to streams if it helps
shortening lines, up to you.

> > +
> > +			config.width = 640;
> > +			config.height = 480;

Should you use the resolution requested by the Viewfinder usage ?

> > +		}
> > +
> > +		if (stream == nullptr)
> > +			goto error;
> > +
> > +		availableStreams.erase(found);
> 
> With std::set
> 
>     availableStreams.erase(stream);
> 
> > +
> > +		config.pixelFormat = V4L2_PIX_FMT_NV12;
> > +		config.bufferCount = IPU3_BUFFER_COUNT;
> > +
> > +		configs[stream] = config;
> > +
> > +		LOG(IPU3, Debug)
> > +			<< "Stream " << stream->name_ << " format set to "
> > +			<< config.width << "x" << config.height
> > +			<< "-0x" << std::hex << std::setfill('0')
> > +			<< std::setw(8) << config.pixelFormat;
> > +	}
> >  
> >  	return configs;
> > +
> > +error:
> > +	LOG(IPU3, Error) << "Requested stream roles not supported";
> > +	return CameraConfiguration{};
> >  }
> >  
> >  int PipelineHandlerIPU3::configureStreams(Camera *camera,

-- 
Regards,

Laurent Pinchart