[libcamera-devel] [PATCH 03/11] libcamera: camera: Log requested configuration in configureStreams()

[Niklas Söderlund]

Hi,

On 2019-04-17 12:02:45 +0300, Laurent Pinchart wrote:
> Hi Jacopo,
> 
> On Wed, Apr 17, 2019 at 09:32:56AM +0200, Jacopo Mondi wrote:
> > On Tue, Apr 16, 2019 at 07:56:21PM +0300, Laurent Pinchart wrote:
> > > On Tue, Apr 16, 2019 at 04:57:03PM +0200, Jacopo Mondi wrote:
> > >> On Mon, Apr 15, 2019 at 07:56:52PM +0300, Laurent Pinchart wrote:
> > >>> The IPU3 pipeline handler logs the requested configuration in its
> > >>> configureStreams() handler. This is useful for other pipeline handlers
> > >>> as well, move it to the Camera class.
> > >>>
> > >>> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > >>> ---
> > >>>  src/libcamera/camera.cpp             | 14 ++++++++++++++
> > >>>  src/libcamera/pipeline/ipu3/ipu3.cpp |  6 ------
> > >>>  2 files changed, 14 insertions(+), 6 deletions(-)
> > >>>
> > >>> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> > >>> index bdf14b31d8ee..55f724a8db17 100644
> > >>> --- a/src/libcamera/camera.cpp
> > >>> +++ b/src/libcamera/camera.cpp
> > >>> @@ -5,6 +5,8 @@
> > >>>   * camera.cpp - Camera device
> > >>>   */
> > >>>
> > >>> +#include <iomanip>
> > >>> +
> > >>>  #include <libcamera/camera.h>
> > >>>  #include <libcamera/request.h>
> > >>>  #include <libcamera/stream.h>
> > >>> @@ -595,11 +597,23 @@ int Camera::configureStreams(const CameraConfiguration &config)
> > >>>  		return -EINVAL;
> > >>>  	}
> > >>>
> > >>> +	std::ostringstream msg("configuring streams:");
> > >>> +	unsigned int index = 0;
> > >>> +
> > >>>  	for (Stream *stream : config) {
> > >>>  		if (streams_.find(stream) == streams_.end())
> > >>>  			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;
> > >>
> > >> std::endl at the end?
> > >
> > > I don't think that's needed, as the message is then output by a single
> > > LOG() call, which adds the std::endl. My goal was to print this on a
> > > single line. Do you think it should be split to multiple lines ?
> > 
> > With a single stream is fine, with multiple streams I think it should
> > be on two different lines. Or have you considered the multiple streams
> > case and judge they fit on a single line?
> 
> I wrote this specifically for multiple streams to be logged on a single
> line. It could however be reconsidered.

I like bike shedding!

I think it should be a single line, this is for the log. Better to group 
things on the same line, even if it turns out to be a long line.

> 
> > >> Apart from that:
> > >> Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
> > >>
> > >>> +
> > >>> +		index++;
> > >>>  	}
> > >>>
> > >>> +	LOG(Camera, Info) << msg.str();
> > >>> +
> > >>>  	ret = pipe_->configureStreams(this, config);
> > >>>  	if (ret)
> > >>>  		return ret;
> > >>> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > >>> index ca09da753b90..ff72be14d696 100644
> > >>> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> > >>> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > >>> @@ -242,12 +242,6 @@ int PipelineHandlerIPU3::configureStreams(Camera *camera,
> > >>>  	ImgUDevice *imgu = data->imgu_;
> > >>>  	int ret;
> > >>>
> > >>> -	LOG(IPU3, Info)
> > >>> -		<< "Requested image format " << cfg.width << "x"
> > >>> -		<< cfg.height << "-0x" << std::hex << std::setfill('0')
> > >>> -		<< std::setw(8) << cfg.pixelFormat << " on camera '"
> > >>> -		<< camera->name() << "'";
> > >>> -
> > >>>  	/*
> > >>>  	 * Verify that the requested size respects the IPU3 alignement
> > >>>  	 * requirements (the image width shall be a multiple of 8 pixels and
> 
> -- 
> Regards,
> 
> Laurent Pinchart
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

-- 
Regards,
Niklas Söderlund