[libcamera-devel] [PATCH 1/7] libcamera: formats: Move isRaw() helper to formats.cpp

[Kieran Bingham]

Hi Kaaira,

Aha, this sounds useful. A few fixups though

On 22/07/2020 14:30, Kaaira Gupta wrote:
> isRaw() helper is used in subsequent patches by VIMC as well. Hence move
> it from raspberrypi pippeline handler to a common place to make it
> reusable.

s/pippeline/pippeline/


> Signed-off-by: Kaaira Gupta <kgupta at es.iitr.ac.in>
> ---
>  include/libcamera/internal/formats.h          |  1 +
>  src/libcamera/formats.cpp                     | 13 +++++++++
>  .../pipeline/raspberrypi/raspberrypi.cpp      | 27 ++++++++-----------
>  3 files changed, 25 insertions(+), 16 deletions(-)
> 
> diff --git a/include/libcamera/internal/formats.h b/include/libcamera/internal/formats.h
> index cad41ad..8032fab 100644
> --- a/include/libcamera/internal/formats.h
> +++ b/include/libcamera/internal/formats.h
> @@ -49,6 +49,7 @@ public:
>  	};
>  
>  	bool isValid() const { return format.isValid(); }
> +	bool isRaw(PixelFormat &pixFmt) const;

We need to parse the PixelFormatInfo, and you are adding the method to
that class (which is good), but that makes passing the PixelFormat
redundant.



>  
>  	static const PixelFormatInfo &info(const PixelFormat &format);
>  	static const PixelFormatInfo &info(const V4L2PixelFormat &format);
> diff --git a/src/libcamera/formats.cpp b/src/libcamera/formats.cpp
> index af3996c..efb7de9 100644
> --- a/src/libcamera/formats.cpp
> +++ b/src/libcamera/formats.cpp
> @@ -853,4 +853,17 @@ PixelFormatInfo::frameSize(const Size &size,
>  	return sum;
>  }
>  
> +/**
> + * \brief Check if given pixel format is RAW or not
> + * \return True if the format is RAW, false otherwise
> + */
> +bool PixelFormatInfo::isRaw(PixelFormat &pixFmt) const
> +{
> +	const PixelFormatInfo &info = PixelFormatInfo::info(pixFmt);
> +	if (!info.isValid())
> +		return false;

You don't need to 'find' the PixelFormatInfo, because you're already
executing code 'on' a PixelFormatInfo (this member function is a
function on a PixelFormatInfo), so you can directly access it's member
variables.

> +
> +	return info.colourEncoding == PixelFormatInfo::ColourEncodingRAW;


So I believe this could then become:

	return colourEncoding == ColourEncodingRAW;



> +}
> +
>  } /* namespace libcamera */
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> index bf1c771..8f35434 100644
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> @@ -43,19 +43,6 @@ using V4L2PixFmtMap = std::map<V4L2PixelFormat, std::vector<SizeRange>>;
>  
>  namespace {
>  
> -bool isRaw(PixelFormat &pixFmt)
> -{
> -	/*
> -	 * The isRaw test might be redundant right now the pipeline handler only
> -	 * supports RAW sensors. Leave it in for now, just as a sanity check.
> -	 */
> -	const PixelFormatInfo &info = PixelFormatInfo::info(pixFmt);
> -	if (!info.isValid())
> -		return false;
> -
> -	return info.colourEncoding == PixelFormatInfo::ColourEncodingRAW;
> -}
> -
>  double scoreFormat(double desired, double actual)
>  {
>  	double score = desired - actual;
> @@ -405,7 +392,13 @@ CameraConfiguration::Status RPiCameraConfiguration::validate()
>  	std::pair<int, Size> outSize[2];
>  	Size maxSize;
>  	for (StreamConfiguration &cfg : config_) {
> -		if (isRaw(cfg.pixelFormat)) {
> +		/*
> +		 * The isRaw test might be redundant right now the pipeline handler only
> +		 * supports RAW sensors. Leave it in for now, just as a sanity check.
> +		 */
> +		const PixelFormatInfo &info = PixelFormatInfo::info(cfg.pixelFormat);
> +
> +		if (info.isRaw(cfg.pixelFormat)) {
>  			/*
>  			 * Calculate the best sensor mode we can use based on
>  			 * the user request.
> @@ -616,8 +609,9 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)
>  	 */
>  	for (unsigned i = 0; i < config->size(); i++) {
>  		StreamConfiguration &cfg = config->at(i);
> +		const PixelFormatInfo &info = PixelFormatInfo::info(cfg.pixelFormat);
>  
> -		if (isRaw(cfg.pixelFormat)) {
> +		if (info.isRaw(cfg.pixelFormat)) {
>  			/*
>  			 * If we have been given a RAW stream, use that size
>  			 * for setting up the sensor.
> @@ -661,7 +655,8 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)
>  	for (unsigned i = 0; i < config->size(); i++) {
>  		StreamConfiguration &cfg = config->at(i);
>  
> -		if (isRaw(cfg.pixelFormat)) {
> +		const PixelFormatInfo &info = PixelFormatInfo::info(cfg..pixelFormat);
> +		if (info.isRaw(cfg.pixelFormat)) {
>  			cfg.setStream(&data->isp_[Isp::Input]);
>  			data->isp_[Isp::Input].setExternal(true);
>  			continue;
> 

-- 
Regards
--
Kieran