[libcamera-devel] [PATCH 2/3] libcamera: stream: Add and use toString() method to StreamConfiguration

[Jacopo Mondi]

Hi Laurent,

On Thu, Apr 18, 2019 at 06:44:52PM +0300, Laurent Pinchart wrote:
> Add a toString() method to the StreamConfiguration class, and replace
> all manually coded implementations through the source code.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> ---
>  include/libcamera/stream.h               |  4 ++++
>  src/libcamera/camera.cpp                 |  4 +---
>  src/libcamera/pipeline/ipu3/ipu3.cpp     |  5 +----
>  src/libcamera/pipeline/rkisp1/rkisp1.cpp | 14 ++++----------
>  src/libcamera/stream.cpp                 | 19 +++++++++++++++++++
>  5 files changed, 29 insertions(+), 17 deletions(-)
>
> diff --git a/include/libcamera/stream.h b/include/libcamera/stream.h
> index 8a47930f8614..3caaefc9f8e9 100644
> --- a/include/libcamera/stream.h
> +++ b/include/libcamera/stream.h
> @@ -7,6 +7,8 @@
>  #ifndef __LIBCAMERA_STREAM_H__
>  #define __LIBCAMERA_STREAM_H__
>
> +#include <string>
> +
>  #include <libcamera/buffer.h>
>  #include <libcamera/geometry.h>
>
> @@ -20,6 +22,8 @@ struct StreamConfiguration {
>  	unsigned int pixelFormat;
>
>  	unsigned int bufferCount;
> +
> +	std::string toString() const;
>  };
>
>  class StreamUsage
> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> index 69406b515700..a52769626446 100644
> --- a/src/libcamera/camera.cpp
> +++ b/src/libcamera/camera.cpp
> @@ -596,9 +596,7 @@ int Camera::configureStreams(const CameraConfiguration &config)
>  			return -EINVAL;
>
>  		const StreamConfiguration &cfg = config[stream];
> -		msg << " (" << index << ") " << cfg.width << "x"
> -		    << cfg.height << "-0x" << std::hex << std::setfill('0')
> -		    << std::setw(8) << cfg.pixelFormat;
> +		msg << " (" << index << ") " << cfg.toString();
>
>  		index++;
>  	}
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index 405d6548fd01..7443224d4f45 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -224,10 +224,7 @@ PipelineHandlerIPU3::streamConfiguration(Camera *camera,
>  	config->pixelFormat = V4L2_PIX_FMT_NV12;
>  	config->bufferCount = IPU3_BUFFER_COUNT;
>
> -	LOG(IPU3, Debug)
> -		<< "Stream format set to " << config->width << "x"
> -		<< config->height << "-0x" << std::hex << std::setfill('0')
> -		<< std::setw(8) << config->pixelFormat;
> +	LOG(IPU3, Debug) << "Stream format set to " << config->toString();
>
>  	return configs;
>  }
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> index 8ed0ba84780a..d21c6266c6ba 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> @@ -124,10 +124,7 @@ CameraConfiguration PipelineHandlerRkISP1::streamConfiguration(Camera *camera,
>
>  	configs[&data->stream_] = config;
>
> -	LOG(RkISP1, Debug)
> -		<< "Stream format set to " << config.width << "x"
> -		<< config.height << "-0x" << std::hex << std::setfill('0')
> -		<< std::setw(8) << config.pixelFormat;
> +	LOG(RkISP1, Debug) << "Stream format set to " << config.toString();
>
>  	return configs;
>  }
> @@ -223,7 +220,7 @@ int PipelineHandlerRkISP1::configureStreams(Camera *camera,
>  	V4L2DeviceFormat outputFormat = {};
>  	outputFormat.width = cfg.width;
>  	outputFormat.height = cfg.height;
> -	outputFormat.fourcc = V4L2_PIX_FMT_NV12;
> +	outputFormat.fourcc = cfg.pixelFormat;
>  	outputFormat.planesCount = 2;
>
>  	ret = video_->setFormat(&outputFormat);
> @@ -232,12 +229,9 @@ int PipelineHandlerRkISP1::configureStreams(Camera *camera,
>
>  	if (outputFormat.width != cfg.width ||
>  	    outputFormat.height != cfg.height ||
> -	    outputFormat.fourcc != V4L2_PIX_FMT_NV12) {
> +	    outputFormat.fourcc != cfg.pixelFormat) {

This and the above hunk seems to be unrelated to this patch to me...

>  		LOG(RkISP1, Error)
> -			<< "Unable to configure capture in " << cfg.width
> -			<< "x" << cfg.height << "-0x"
> -			<< std::hex << std::setfill('0') << std::setw(8)
> -			<< V4L2_PIX_FMT_NV12;
> +			<< "Unable to configure capture in " << cfg.toString();
>  		return -EINVAL;
>  	}
>
> diff --git a/src/libcamera/stream.cpp b/src/libcamera/stream.cpp
> index d4ef506cea95..06db9797ff7e 100644
> --- a/src/libcamera/stream.cpp
> +++ b/src/libcamera/stream.cpp
> @@ -5,6 +5,9 @@
>   * stream.cpp - Video stream for a Camera
>   */
>
> +#include <iomanip>
> +#include <sstream>
> +
>  #include <libcamera/stream.h>
>
>  /**
> @@ -55,6 +58,22 @@ namespace libcamera {
>   * format described in V4L2 using the V4L2_PIX_FMT_* definitions.
>   */
>
> +/**
> + * \brief Assemble and return a string describing the configuration
> + *
> + * \return A string describing the StreamConfiguration
> + */
> +std::string StreamConfiguration::toString() const
> +{
> +	std::stringstream ss;
> +
> +	ss.fill(0);
> +	ss << width << "x" << height << "-0x" << std::hex
> +	   << std::setw(8) << pixelFormat;
> +
> +	return ss.str();
> +}
> +

In the struct declaration the toString() method comes after the
bufferCount member, if we want to follow the declaration order.

Apart from that:
Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>

>  /**
>   * \var StreamConfiguration::bufferCount
>   * \brief Requested number of buffers to allocate for the stream
> --
> Regards,
>
> Laurent Pinchart
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
-------------- 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/d1fae733/attachment.sig>