[libcamera-devel] [PATCH v8 4/8] libcamera: ipu3: Use roles in stream configuration

[Jacopo Mondi]

Hi Laurent,

On Fri, Apr 19, 2019 at 04:35:27PM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Fri, Apr 19, 2019 at 03:25:27PM +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, with the viewfinder stream able to
> > capture both continuous video streams and still images, and the main
> > output stream supporting still images only. This is an artificial
> > limitation until we figure out the exact capabilities of the hardware.
> >
> > Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> > ---
> >  src/libcamera/pipeline/ipu3/ipu3.cpp | 124 +++++++++++++++++++++------
> >  1 file changed, 98 insertions(+), 26 deletions(-)
> >
> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > index 46384d88dddd..0130a83973ca 100644
> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > @@ -5,6 +5,7 @@
> >   * ipu3.cpp - Pipeline handler for Intel IPU3
> >   */
> >
> > +#include <algorithm>
> >  #include <iomanip>
> >  #include <memory>
> >  #include <vector>
> > @@ -221,34 +222,105 @@ CameraConfiguration
> >  PipelineHandlerIPU3::streamConfiguration(Camera *camera,
> >  					 const std::vector<StreamUsage> &usages)
> >  {
> > -	CameraConfiguration configs;
> >  	IPU3CameraData *data = cameraData(camera);
> > -	StreamConfiguration config = {};
> > +	CameraConfiguration cameraConfig = {};
> > +	std::set<IPU3Stream *> streams = {
> > +		&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 '" << data->outStream_.name_ << "' format set to "
> > -		<< config.toString();
> > -
> > -	configs[&data->vfStream_] = config;
> > -	LOG(IPU3, Debug)
> > -		<< "Stream '" << data->vfStream_.name_ << "' format set to "
> > -		<< config.toString();
> > -
> > -	return configs;
> > +	for (const StreamUsage &usage : usages) {
> > +		StreamConfiguration streamConfig = {};
> > +		StreamUsage::Role role = usage.role();
> > +		IPU3Stream *stream = nullptr;
> > +
> > +		switch (role) {
> > +		case StreamUsage::Role::StillCapture:
> > +			/*
> > +			 * Don't allow viewfinder or video capture on the
> > +			 * 'output' stream. This is an artificial limitation
> > +			 * until we figure out the capabilities of the
> > +			 * hardware.
>
> This comment is a bit confusing. How about "Pick the output stream by
> default as the Viewfinder and VideoRecording roles are not allowed on
> the viewfinder stream." ?
>

Taken in with
s/viewfinder stream/output stream/

> > +			 */
> > +			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;
> > +			}
> > +
> > +			/*
> > +			 * 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.
> > +			 */
> > +			streamConfig.width = 2560;
> > +			streamConfig.height = 1920;
> > +
> > +			break;
>
> Please add a blank line here.
>
> > +		case StreamUsage::Role::Viewfinder:
> > +		case StreamUsage::Role::VideoRecording: {
> > +			/*
> > +			 * We can't use the 'output' stream for viewfinder or
> > +			 * video capture usages.
> > +			 *
> > +			 * \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 requested 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(usage.size().width,
> > +						      res.width);
> > +			unsigned int height = std::min(usage.size().height,
> > +						       res.height);
> > +			streamConfig.width = width & ~7;
> > +			streamConfig.height = height & ~3;
> > +
> > +			break;
> > +		}
>
> And a blank line here too.
>
> > +		default:
> > +			LOG(IPU3, Error)
> > +				<< "Requested stream role not supported: " << role;
> > +			break;
> > +		}
> > +
> > +		if (!stream)
> > +			return cameraConfig;
>
> This will return a valid configuration if the first role was fulfilled
> but the first one wasn't. You need to return CameraConfiguration{} here.

Indeed.

>
> With this fixed,
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

Thanks
   j

>
> > +
> > +		streams.erase(stream);
> > +
> > +		streamConfig.pixelFormat = V4L2_PIX_FMT_NV12;
> > +		streamConfig.bufferCount = IPU3_BUFFER_COUNT;
> > +
> > +		cameraConfig[stream] = streamConfig;
> > +
> > +		LOG(IPU3, Debug)
> > +			<< "Stream '" << stream->name_ << "' format set to "
> > +			<< streamConfig.toString();
> > +	}
> > +
> > +	return cameraConfig;
> >  }
> >
> >  int PipelineHandlerIPU3::configureStreams(Camera *camera,
>
> --
> Regards,
>
> Laurent Pinchart
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20190419/99916281/attachment.sig>