[libcamera-devel,v4,15/21] libcamera: raspberrypi: Fill stride and frameSize at config validation

8684 diff mbox series

Fill the stride and frameSize fields of the StreamConfiguration at
configuration validation time instead of at camera configuration time.
This allows applications to get the stride when trying a configuration
without modifying the active configuration of the camera.

Also set a default configuration size, in case the size is zero.

Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>

---
Changes in v4:
- mention motivation in commit message
- also fill stride and frameSize in the default format

New in v3
---
 .../pipeline/raspberrypi/raspberrypi.cpp      | 45 +++++++++++++++----
 1 file changed, 37 insertions(+), 8 deletions(-)

Hi Paul,

Thank you for the patch.

On Wed, Jul 08, 2020 at 10:44:11PM +0900, Paul Elder wrote:
> Fill the stride and frameSize fields of the StreamConfiguration at
> configuration validation time instead of at camera configuration time.
> This allows applications to get the stride when trying a configuration
> without modifying the active configuration of the camera.
> 
> Also set a default configuration size, in case the size is zero.

This would normally be split in a separate commit.

> 
> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> 
> ---
> Changes in v4:
> - mention motivation in commit message
> - also fill stride and frameSize in the default format
> 
> New in v3
> ---
>  .../pipeline/raspberrypi/raspberrypi.cpp      | 45 +++++++++++++++----
>  1 file changed, 37 insertions(+), 8 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> index 1822ac9..d0ce446 100644
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> @@ -427,6 +427,10 @@ CameraConfiguration::Status RPiCameraConfiguration::validate()
>  			 */
>  			V4L2PixFmtMap fmts = data_->unicam_[Unicam::Image].dev()->formats();
>  			V4L2DeviceFormat sensorFormat = findBestMode(fmts, cfg.size);
> +			int ret = data_->unicam_[Unicam::Image].dev()->tryFormat(&sensorFormat);
> +			if (ret)
> +				return Invalid;
> +
>  			PixelFormat sensorPixFormat = sensorFormat.fourcc.toPixelFormat();
>  			if (cfg.size != sensorFormat.size ||
>  			    cfg.pixelFormat != sensorPixFormat) {
> @@ -434,6 +438,10 @@ CameraConfiguration::Status RPiCameraConfiguration::validate()
>  				cfg.pixelFormat = sensorPixFormat;
>  				status = Adjusted;
>  			}
> +
> +			cfg.stride = sensorFormat.planes[0].bpl;
> +			cfg.frameSize = sensorFormat.planes[0].size;
> +
>  			rawCount++;
>  		} else {
>  			outSize[outCount] = std::make_pair(count, cfg.size);
> @@ -478,19 +486,43 @@ CameraConfiguration::Status RPiCameraConfiguration::validate()
>  		 * have that fixed up in the code above.
>  		 *
>  		 */
> -		PixelFormat &cfgPixFmt = config_.at(outSize[i].first).pixelFormat;
> +		StreamConfiguration &cfg = config_.at(outSize[i].first);
> +		PixelFormat &cfgPixFmt = cfg.pixelFormat;
> +		V4L2VideoDevice *dev;
>  		V4L2PixFmtMap fmts;
>  
> -		if (i == maxIndex)
> -			fmts = data_->isp_[Isp::Output0].dev()->formats();
> -		else
> -			fmts = data_->isp_[Isp::Output1].dev()->formats();
> +		if (i == maxIndex) {
> +			dev = data_->isp_[Isp::Output0].dev();
> +			fmts = dev->formats();
> +		} else {
> +			dev = data_->isp_[Isp::Output1].dev();
> +			fmts = dev->formats();
> +		}
>  

If would remove fmts above and write here

  		V4L2PixFmtMap fmts = dev->formats();

>  		if (fmts.find(V4L2PixelFormat::fromPixelFormat(cfgPixFmt, false)) == fmts.end()) {
>  			/* If we cannot find a native format, use a default one. */
>  			cfgPixFmt = formats::NV12;
> +
> +			const PixelFormatInfo &info = PixelFormatInfo::info(cfgPixFmt);
> +			cfg.size.width = 1920;
> +			cfg.size.height = 1080;

Why is this needed, isn't the code above supposed to pick a suitable
size ?

> +			cfg.stride = info.stride(cfg.size.width, 0);
> +			cfg.frameSize = info.frameSize(cfg.size);

I think you can drop those two lines, as you set the stride and
frameSize below.

> +
>  			status = Adjusted;
>  		}
> +
> +		V4L2DeviceFormat format = {};
> +		format.fourcc = dev->toV4L2PixelFormat(cfg.pixelFormat);
> +		format.size = cfg.size;
> +
> +		int ret = dev->tryFormat(&format);
> +		if (ret)
> +			return Invalid;
> +
> +		cfg.stride = format.planes[0].bpl;
> +		cfg.frameSize = format.planes[0].size;
> +
>  	}
>  
>  	return status;
> @@ -655,7 +687,6 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)
>  
>  		if (isRaw(cfg.pixelFormat)) {
>  			cfg.setStream(&data->isp_[Isp::Input]);
> -			cfg.stride = sensorFormat.planes[0].bpl;
>  			data->isp_[Isp::Input].setExternal(true);
>  			continue;
>  		}
> @@ -679,7 +710,6 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)
>  			}
>  
>  			cfg.setStream(&data->isp_[Isp::Output0]);
> -			cfg.stride = format.planes[0].bpl;
>  			data->isp_[Isp::Output0].setExternal(true);
>  		}
>  
> @@ -705,7 +735,6 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)
>  		 */
>  		if (!cfg.stream()) {
>  			cfg.setStream(&data->isp_[Isp::Output1]);
> -			cfg.stride = format.planes[0].bpl;
>  			data->isp_[Isp::Output1].setExternal(true);
>  		}
>  	}

Hi Paul,

On Wed, Jul 08, 2020 at 10:44:11PM +0900, Paul Elder wrote:
> Fill the stride and frameSize fields of the StreamConfiguration at
> configuration validation time instead of at camera configuration time.
> This allows applications to get the stride when trying a configuration
> without modifying the active configuration of the camera.
>
> Also set a default configuration size, in case the size is zero.
>
> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
>
> ---
> Changes in v4:
> - mention motivation in commit message
> - also fill stride and frameSize in the default format
>
> New in v3
> ---
>  .../pipeline/raspberrypi/raspberrypi.cpp      | 45 +++++++++++++++----
>  1 file changed, 37 insertions(+), 8 deletions(-)
>
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> index 1822ac9..d0ce446 100644
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> @@ -427,6 +427,10 @@ CameraConfiguration::Status RPiCameraConfiguration::validate()
>  			 */
>  			V4L2PixFmtMap fmts = data_->unicam_[Unicam::Image].dev()->formats();
>  			V4L2DeviceFormat sensorFormat = findBestMode(fmts, cfg.size);
> +			int ret = data_->unicam_[Unicam::Image].dev()->tryFormat(&sensorFormat);
> +			if (ret)
> +				return Invalid;
> +

Is this related ?

>  			PixelFormat sensorPixFormat = sensorFormat.fourcc.toPixelFormat();
>  			if (cfg.size != sensorFormat.size ||
>  			    cfg.pixelFormat != sensorPixFormat) {
> @@ -434,6 +438,10 @@ CameraConfiguration::Status RPiCameraConfiguration::validate()
>  				cfg.pixelFormat = sensorPixFormat;
>  				status = Adjusted;
>  			}
> +
> +			cfg.stride = sensorFormat.planes[0].bpl;
> +			cfg.frameSize = sensorFormat.planes[0].size;
> +
>  			rawCount++;
>  		} else {
>  			outSize[outCount] = std::make_pair(count, cfg.size);
> @@ -478,19 +486,43 @@ CameraConfiguration::Status RPiCameraConfiguration::validate()
>  		 * have that fixed up in the code above.
>  		 *
>  		 */
> -		PixelFormat &cfgPixFmt = config_.at(outSize[i].first).pixelFormat;
> +		StreamConfiguration &cfg = config_.at(outSize[i].first);
> +		PixelFormat &cfgPixFmt = cfg.pixelFormat;
> +		V4L2VideoDevice *dev;
>  		V4L2PixFmtMap fmts;
>
> -		if (i == maxIndex)
> -			fmts = data_->isp_[Isp::Output0].dev()->formats();
> -		else
> -			fmts = data_->isp_[Isp::Output1].dev()->formats();
> +		if (i == maxIndex) {
> +			dev = data_->isp_[Isp::Output0].dev();
> +			fmts = dev->formats();
> +		} else {
> +			dev = data_->isp_[Isp::Output1].dev();
> +			fmts = dev->formats();
> +		}
>
>  		if (fmts.find(V4L2PixelFormat::fromPixelFormat(cfgPixFmt, false)) == fmts.end()) {
>  			/* If we cannot find a native format, use a default one. */
>  			cfgPixFmt = formats::NV12;
> +
> +			const PixelFormatInfo &info = PixelFormatInfo::info(cfgPixFmt);
> +			cfg.size.width = 1920;
> +			cfg.size.height = 1080;
> +			cfg.stride = info.stride(cfg.size.width, 0);
> +			cfg.frameSize = info.frameSize(cfg.size);
> +
>  			status = Adjusted;
>  		}
> +
> +		V4L2DeviceFormat format = {};
> +		format.fourcc = dev->toV4L2PixelFormat(cfg.pixelFormat);
> +		format.size = cfg.size;
> +
> +		int ret = dev->tryFormat(&format);
> +		if (ret)
> +			return Invalid;
> +
> +		cfg.stride = format.planes[0].bpl;
> +		cfg.frameSize = format.planes[0].size;
> +
>  	}
>
>  	return status;
> @@ -655,7 +687,6 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)
>
>  		if (isRaw(cfg.pixelFormat)) {
>  			cfg.setStream(&data->isp_[Isp::Input]);
> -			cfg.stride = sensorFormat.planes[0].bpl;
>  			data->isp_[Isp::Input].setExternal(true);
>  			continue;
>  		}
> @@ -679,7 +710,6 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)
>  			}
>
>  			cfg.setStream(&data->isp_[Isp::Output0]);
> -			cfg.stride = format.planes[0].bpl;
>  			data->isp_[Isp::Output0].setExternal(true);
>  		}
>
> @@ -705,7 +735,6 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)
>  		 */
>  		if (!cfg.stream()) {
>  			cfg.setStream(&data->isp_[Isp::Output1]);
> -			cfg.stride = format.planes[0].bpl;
>  			data->isp_[Isp::Output1].setExternal(true);
>  		}
>  	}
> --
> 2.27.0
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

On Wed, Jul 08, 2020 at 11:30:53PM +0200, Jacopo Mondi wrote:
> Hi Paul,
>
> On Wed, Jul 08, 2020 at 10:44:11PM +0900, Paul Elder wrote:
> > Fill the stride and frameSize fields of the StreamConfiguration at
> > configuration validation time instead of at camera configuration time.
> > This allows applications to get the stride when trying a configuration
> > without modifying the active configuration of the camera.
> >
> > Also set a default configuration size, in case the size is zero.
> >
> > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> >
> > ---
> > Changes in v4:
> > - mention motivation in commit message
> > - also fill stride and frameSize in the default format
> >
> > New in v3
> > ---
> >  .../pipeline/raspberrypi/raspberrypi.cpp      | 45 +++++++++++++++----
> >  1 file changed, 37 insertions(+), 8 deletions(-)
> >
> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > index 1822ac9..d0ce446 100644
> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > @@ -427,6 +427,10 @@ CameraConfiguration::Status RPiCameraConfiguration::validate()
> >  			 */
> >  			V4L2PixFmtMap fmts = data_->unicam_[Unicam::Image].dev()->formats();
> >  			V4L2DeviceFormat sensorFormat = findBestMode(fmts, cfg.size);
> > +			int ret = data_->unicam_[Unicam::Image].dev()->tryFormat(&sensorFormat);
> > +			if (ret)
> > +				return Invalid;
> > +
>
> Is this related ?
>

Of course it is, sorry, I mis-read that :)

> >  			PixelFormat sensorPixFormat = sensorFormat.fourcc.toPixelFormat();
> >  			if (cfg.size != sensorFormat.size ||
> >  			    cfg.pixelFormat != sensorPixFormat) {
> > @@ -434,6 +438,10 @@ CameraConfiguration::Status RPiCameraConfiguration::validate()
> >  				cfg.pixelFormat = sensorPixFormat;
> >  				status = Adjusted;
> >  			}
> > +
> > +			cfg.stride = sensorFormat.planes[0].bpl;
> > +			cfg.frameSize = sensorFormat.planes[0].size;
> > +
> >  			rawCount++;
> >  		} else {
> >  			outSize[outCount] = std::make_pair(count, cfg.size);
> > @@ -478,19 +486,43 @@ CameraConfiguration::Status RPiCameraConfiguration::validate()
> >  		 * have that fixed up in the code above.
> >  		 *
> >  		 */
> > -		PixelFormat &cfgPixFmt = config_.at(outSize[i].first).pixelFormat;
> > +		StreamConfiguration &cfg = config_.at(outSize[i].first);
> > +		PixelFormat &cfgPixFmt = cfg.pixelFormat;
> > +		V4L2VideoDevice *dev;
> >  		V4L2PixFmtMap fmts;
> >
> > -		if (i == maxIndex)
> > -			fmts = data_->isp_[Isp::Output0].dev()->formats();
> > -		else
> > -			fmts = data_->isp_[Isp::Output1].dev()->formats();
> > +		if (i == maxIndex) {
> > +			dev = data_->isp_[Isp::Output0].dev();
> > +			fmts = dev->formats();
> > +		} else {
> > +			dev = data_->isp_[Isp::Output1].dev();
> > +			fmts = dev->formats();
> > +		}
> >
> >  		if (fmts.find(V4L2PixelFormat::fromPixelFormat(cfgPixFmt, false)) == fmts.end()) {
> >  			/* If we cannot find a native format, use a default one. */
> >  			cfgPixFmt = formats::NV12;
> > +
> > +			const PixelFormatInfo &info = PixelFormatInfo::info(cfgPixFmt);
> > +			cfg.size.width = 1920;
> > +			cfg.size.height = 1080;
> > +			cfg.stride = info.stride(cfg.size.width, 0);
> > +			cfg.frameSize = info.frameSize(cfg.size);
> > +
> >  			status = Adjusted;
> >  		}
> > +
> > +		V4L2DeviceFormat format = {};
> > +		format.fourcc = dev->toV4L2PixelFormat(cfg.pixelFormat);
> > +		format.size = cfg.size;
> > +
> > +		int ret = dev->tryFormat(&format);
> > +		if (ret)
> > +			return Invalid;
> > +
> > +		cfg.stride = format.planes[0].bpl;
> > +		cfg.frameSize = format.planes[0].size;
> > +
> >  	}
> >
> >  	return status;
> > @@ -655,7 +687,6 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)
> >
> >  		if (isRaw(cfg.pixelFormat)) {
> >  			cfg.setStream(&data->isp_[Isp::Input]);
> > -			cfg.stride = sensorFormat.planes[0].bpl;
> >  			data->isp_[Isp::Input].setExternal(true);
> >  			continue;
> >  		}
> > @@ -679,7 +710,6 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)
> >  			}
> >
> >  			cfg.setStream(&data->isp_[Isp::Output0]);
> > -			cfg.stride = format.planes[0].bpl;
> >  			data->isp_[Isp::Output0].setExternal(true);
> >  		}
> >
> > @@ -705,7 +735,6 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)
> >  		 */
> >  		if (!cfg.stream()) {
> >  			cfg.setStream(&data->isp_[Isp::Output1]);
> > -			cfg.stride = format.planes[0].bpl;
> >  			data->isp_[Isp::Output1].setExternal(true);
> >  		}
> >  	}
> > --
> > 2.27.0
> >
> > _______________________________________________
> > libcamera-devel mailing list
> > libcamera-devel@lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel