[libcamera-devel,v6,2/6] libcamera: ipu3: Use roles in stream configuration
diff mbox series

Message ID 20190418164638.400-3-jacopo@jmondi.org
State Superseded
Headers show
Series
  • libcamera: ipu3: Multiple streams support
Related show

Commit Message

Jacopo Mondi April 18, 2019, 4:46 p.m. UTC 
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.

Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
---
 src/libcamera/pipeline/ipu3/ipu3.cpp | 99 +++++++++++++++++++---------
 1 file changed, 69 insertions(+), 30 deletions(-)

Comments

Laurent Pinchart April 18, 2019, 8:39 p.m. UTC | #1 
Hi Jacopo,

Thank you for the patch.

On Thu, Apr 18, 2019 at 06:46:34PM +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.

This is an artificial limitation, isn't it ? I'd like that to be
recorded in the commit message. Maybe

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 images. This is an artificial
limitation until we figure out the exact capabilities of the hardware.

> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
>  src/libcamera/pipeline/ipu3/ipu3.cpp | 99 +++++++++++++++++++---------
>  1 file changed, 69 insertions(+), 30 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index b9eb872fd5b2..8c2504499f33 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -221,38 +221,77 @@ 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_ << "' 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 '" << data->vfStream_.name_ << "' set to "
> -		<< config.width << "x" << config.height << "-0x"
> -		<< std::hex << std::setfill('0') << std::setw(8)
> -		<< config.pixelFormat;
> -
> -	return configs;
> +	for (const StreamUsage &usage : usages) {
> +		std::vector<IPU3Stream *>::iterator found;
> +		StreamUsage::Role role = usage.role();
> +		StreamConfiguration streamConfig = {};
> +		IPU3Stream *stream = nullptr;
> +
> +		if (role == StreamUsage::Role::StillCapture) {
> +			/*
> +			 * We can use the viewfinder stream in case the
> +			 * 'StillCapture' usage is required multiple times.
> +			 */
> +			if (streams.find(&data->outStream_) != streams.end())
> +				stream = &data->outStream_;
> +			else if (streams.find(&data->vfStream_) != streams.end())
> +				stream = &data->vfStream_;
> +			else
> +				goto error;
> +
> +			/*
> +			 * 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;
> +		} else if (role == StreamUsage::Role::Viewfinder ||
> +			   role == StreamUsage::Role::VideoRecording) {
> +			/*
> +			 * We can't use the 'output' stream for viewfinder or
> +			 * video capture usages.

And here, "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."

> +			 */
> +			if (streams.find(&data->vfStream_) == streams.end())
> +				goto error;
> +
> +			stream = &data->vfStream_;
> +
> +			streamConfig.width = usage.size().width;
> +			streamConfig.height = usage.size().height;

Should this be rounded to the alignment requirements of the ImgU, and
clamped to the minimum - maximum achievable sizes ?

> +		}

You will crash if none of those roles match. Let's use a switch() ...
case and return an error in the default case.

> +
> +		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.width << "x" << streamConfig.height
> +			<< "-0x" << std::hex << std::setfill('0')
> +			<< std::setw(8) << streamConfig.pixelFormat;

Here too you can use streamConfig.toString() if my patches get merged
first. Otherwise I'll rebase them.

> +	}
> +
> +	return cameraConfig;
> +
> +error:
> +	LOG(IPU3, Error) << "Requested stream roles not supported";
> +
> +	return CameraConfiguration{};
>  }
>  
>  int PipelineHandlerIPU3::configureStreams(Camera *camera,
Jacopo Mondi April 19, 2019, 6:47 a.m. UTC | #2 
HI Laurent,

On Thu, Apr 18, 2019 at 11:39:23PM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Thu, Apr 18, 2019 at 06:46:34PM +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.
>
> This is an artificial limitation, isn't it ? I'd like that to be
> recorded in the commit message. Maybe
>
> 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 images. This is an artificial
> limitation until we figure out the exact capabilities of the hardware.
>
> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > ---
> >  src/libcamera/pipeline/ipu3/ipu3.cpp | 99 +++++++++++++++++++---------
> >  1 file changed, 69 insertions(+), 30 deletions(-)
> >
> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > index b9eb872fd5b2..8c2504499f33 100644
> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > @@ -221,38 +221,77 @@ 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_ << "' 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 '" << data->vfStream_.name_ << "' set to "
> > -		<< config.width << "x" << config.height << "-0x"
> > -		<< std::hex << std::setfill('0') << std::setw(8)
> > -		<< config.pixelFormat;
> > -
> > -	return configs;
> > +	for (const StreamUsage &usage : usages) {
> > +		std::vector<IPU3Stream *>::iterator found;
> > +		StreamUsage::Role role = usage.role();
> > +		StreamConfiguration streamConfig = {};
> > +		IPU3Stream *stream = nullptr;
> > +
> > +		if (role == StreamUsage::Role::StillCapture) {
> > +			/*
> > +			 * We can use the viewfinder stream in case the
> > +			 * 'StillCapture' usage is required multiple times.
> > +			 */
> > +			if (streams.find(&data->outStream_) != streams.end())
> > +				stream = &data->outStream_;
> > +			else if (streams.find(&data->vfStream_) != streams.end())
> > +				stream = &data->vfStream_;
> > +			else
> > +				goto error;
> > +
> > +			/*
> > +			 * 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;
> > +		} else if (role == StreamUsage::Role::Viewfinder ||
> > +			   role == StreamUsage::Role::VideoRecording) {
> > +			/*
> > +			 * We can't use the 'output' stream for viewfinder or
> > +			 * video capture usages.
>
> And here, "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."
>
> > +			 */
> > +			if (streams.find(&data->vfStream_) == streams.end())
> > +				goto error;
> > +
> > +			stream = &data->vfStream_;
> > +
> > +			streamConfig.width = usage.size().width;
> > +			streamConfig.height = usage.size().height;
>
> Should this be rounded to the alignment requirements of the ImgU, and
> clamped to the minimum - maximum achievable sizes ?
>

I should get them from the camera sensor, at least the max size...

> > +		}
>
> You will crash if none of those roles match. Let's use a switch() ...
> case and return an error in the default case.
>

Actully there are no more roles to match, but this will hopefully
change soon. I'll goto error in that case.

> > +
> > +		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.width << "x" << streamConfig.height
> > +			<< "-0x" << std::hex << std::setfill('0')
> > +			<< std::setw(8) << streamConfig.pixelFormat;
>
> Here too you can use streamConfig.toString() if my patches get merged
> first. Otherwise I'll rebase them.
>

It just depends which one will get in first.

Thanks
  j

> > +	}
> > +
> > +	return cameraConfig;
> > +
> > +error:
> > +	LOG(IPU3, Error) << "Requested stream roles not supported";
> > +
> > +	return CameraConfiguration{};
> >  }
> >
> >  int PipelineHandlerIPU3::configureStreams(Camera *camera,
>
> --
> Regards,
>
> Laurent Pinchart

Patch
diff mbox series 

diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
index b9eb872fd5b2..8c2504499f33 100644
--- a/src/libcamera/pipeline/ipu3/ipu3.cpp
+++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
@@ -221,38 +221,77 @@  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_ << "' 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 '" << data->vfStream_.name_ << "' set to "
-		<< config.width << "x" << config.height << "-0x"
-		<< std::hex << std::setfill('0') << std::setw(8)
-		<< config.pixelFormat;
-
-	return configs;
+	for (const StreamUsage &usage : usages) {
+		std::vector<IPU3Stream *>::iterator found;
+		StreamUsage::Role role = usage.role();
+		StreamConfiguration streamConfig = {};
+		IPU3Stream *stream = nullptr;
+
+		if (role == StreamUsage::Role::StillCapture) {
+			/*
+			 * We can use the viewfinder stream in case the
+			 * 'StillCapture' usage is required multiple times.
+			 */
+			if (streams.find(&data->outStream_) != streams.end())
+				stream = &data->outStream_;
+			else if (streams.find(&data->vfStream_) != streams.end())
+				stream = &data->vfStream_;
+			else
+				goto error;
+
+			/*
+			 * 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;
+		} else if (role == StreamUsage::Role::Viewfinder ||
+			   role == StreamUsage::Role::VideoRecording) {
+			/*
+			 * We can't use the 'output' stream for viewfinder or
+			 * video capture usages.
+			 */
+			if (streams.find(&data->vfStream_) == streams.end())
+				goto error;
+
+			stream = &data->vfStream_;
+
+			streamConfig.width = usage.size().width;
+			streamConfig.height = usage.size().height;
+		}
+
+		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.width << "x" << streamConfig.height
+			<< "-0x" << std::hex << std::setfill('0')
+			<< std::setw(8) << streamConfig.pixelFormat;
+	}
+
+	return cameraConfig;
+
+error:
+	LOG(IPU3, Error) << "Requested stream roles not supported";
+
+	return CameraConfiguration{};
 }
 
 int PipelineHandlerIPU3::configureStreams(Camera *camera,