[libcamera-devel] [PATCH 02/30] libcamera: Remove buffer index from logging

[Laurent Pinchart]

Hello,

On Wed, Nov 27, 2019 at 10:47:38AM +0100, Jacopo Mondi wrote:
> On Wed, Nov 27, 2019 at 12:35:52AM +0100, Niklas Söderlund wrote:
> > The buffer index is a V4L2 concept that will be hidden from users when
> > the switch to FrameBuffer is complete, in preparation remove it from
> 
> What's a FrameBuffer ? I guess I'll find it out, but it's mentioned in
> a few commit messages before it's actually introdduced...

One way to fix this is to write the commit message as

The buffer index is a V4L2 concept that will be hidden from users with
the introduction of a new FrameBuffer class. In preparation for this,
remove the index from log messages.

> > logging.
> >
> > Keep and move one debug log message where the index is available as the
> > V4L2 buffer is being dequeued for the video device and it's useful when
> > debugging.
> >
> > Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> > ---
> >  src/cam/capture.cpp                      | 3 +--
> >  src/libcamera/v4l2_videodevice.cpp       | 3 +--
> >  src/qcam/main_window.cpp                 | 3 +--
> >  test/v4l2_videodevice/buffer_sharing.cpp | 8 ++++----
> >  test/v4l2_videodevice/capture_async.cpp  | 2 +-
> >  test/v4l2_videodevice/v4l2_m2mdevice.cpp | 4 ++--
> >  6 files changed, 10 insertions(+), 13 deletions(-)
> >
> > diff --git a/src/cam/capture.cpp b/src/cam/capture.cpp
> > index e665d819fb777a90..4b65b1d0a2dbed35 100644
> > --- a/src/cam/capture.cpp
> > +++ b/src/cam/capture.cpp
> > @@ -155,7 +155,6 @@ void Capture::requestComplete(Request *request)
> >  		const std::string &name = streamName_[stream];
> >
> >  		info << " " << name
> > -		     << " (" << buffer->index() << ")"
> >  		     << " seq: " << std::setw(6) << std::setfill('0') << buffer->sequence()
> >  		     << " bytesused: " << buffer->bytesused();
> >
> > @@ -182,7 +181,7 @@ void Capture::requestComplete(Request *request)
> >
> >  		std::unique_ptr<Buffer> newBuffer = stream->createBuffer(index);
> >  		if (!newBuffer) {
> > -			std::cerr << "Can't create buffer " << index << std::endl;
> > +			std::cerr << "Can't create buffer" << std::endl;
> >  			return;
> >  		}
> >
> > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp
> > index 992130751286994c..dbb5c3982334e243 100644
> > --- a/src/libcamera/v4l2_videodevice.cpp
> > +++ b/src/libcamera/v4l2_videodevice.cpp
> > @@ -1103,6 +1103,7 @@ Buffer *V4L2VideoDevice::dequeueBuffer()
> >  	}
> >
> >  	ASSERT(buf.index < bufferPool_->count());
> > +	LOG(V4L2, Debug) << "Buffer " << buf.index << " is available";

I would write this

	LOG(V4L2, Debug) << "Dequeuing buffer " << buf.index;

and move this above the ASSERT (the index could be useful when debugging
assertion failures).

> >
> >  	auto it = queuedBuffers_.find(buf.index);
> >  	Buffer *buffer = it->second;
> > @@ -1138,8 +1139,6 @@ void V4L2VideoDevice::bufferAvailable(EventNotifier *notifier)
> >  	if (!buffer)
> >  		return;
> >
> > -	LOG(V4L2, Debug) << "Buffer " << buffer->index() << " is available";
> > -
> >  	/* Notify anyone listening to the device. */
> >  	bufferReady.emit(buffer);
> >  }
> > diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp
> > index cca7365ae75687f9..0c7ca61ac12ec41c 100644
> > --- a/src/qcam/main_window.cpp
> > +++ b/src/qcam/main_window.cpp
> > @@ -264,7 +264,6 @@ void MainWindow::requestComplete(Request *request)
> >  	lastBufferTime_ = buffer->timestamp();
> >
> >  	std::cout << "seq: " << std::setw(6) << std::setfill('0') << buffer->sequence()
> > -		  << " buf: " << buffer->index()
> >  		  << " bytesused: " << buffer->bytesused()
> >  		  << " timestamp: " << buffer->timestamp()
> >  		  << " fps: " << std::fixed << std::setprecision(2) << fps
> > @@ -285,7 +284,7 @@ void MainWindow::requestComplete(Request *request)
> >
> >  		std::unique_ptr<Buffer> newBuffer = stream->createBuffer(index);
> >  		if (!newBuffer) {
> > -			std::cerr << "Can't create buffer " << index << std::endl;
> > +			std::cerr << "Can't create buffer" << std::endl;
> >  			return;
> >  		}
> >
> > diff --git a/test/v4l2_videodevice/buffer_sharing.cpp b/test/v4l2_videodevice/buffer_sharing.cpp
> > index 1629f34cfa6c79fe..d02c391b95933922 100644
> > --- a/test/v4l2_videodevice/buffer_sharing.cpp
> > +++ b/test/v4l2_videodevice/buffer_sharing.cpp
> > @@ -92,8 +92,8 @@ protected:
> >
> >  	void captureBufferReady(Buffer *buffer)
> >  	{
> > -		std::cout << "Received capture buffer: " << buffer->index()
> > -			  << " sequence " << buffer->sequence() << std::endl;
> > +		std::cout << "Received capture buffer  sequence "
>                                                       ^- Double space
> I would drop 'sequence' completely

Ack.

> > +			  << buffer->sequence() << std::endl;
> >
> >  		if (buffer->status() != Buffer::BufferSuccess)
> >  			return;
> > @@ -104,8 +104,8 @@ protected:
> >
> >  	void outputBufferReady(Buffer *buffer)
> >  	{
> > -		std::cout << "Received output buffer: " << buffer->index()
> > -			  << " sequence " << buffer->sequence() << std::endl;
> > +		std::cout << "Received output buffer sequence "
> > +			  << buffer->sequence() << std::endl;

Same here.

> >  		if (buffer->status() != Buffer::BufferSuccess)
> >  			return;
> > diff --git a/test/v4l2_videodevice/capture_async.cpp b/test/v4l2_videodevice/capture_async.cpp
> > index 442a4fe56eace57e..0bc0067c50909c9d 100644
> > --- a/test/v4l2_videodevice/capture_async.cpp
> > +++ b/test/v4l2_videodevice/capture_async.cpp
> > @@ -22,7 +22,7 @@ public:
> >
> >  	void receiveBuffer(Buffer *buffer)
> >  	{
> > -		std::cout << "Received buffer " << buffer->index() << std::endl;
> > +		std::cout << "Received buffer" << std::endl;
> 
> Without the index this should be "Buffer received"
> Here and below

I think both work, but I'm fine with switching to "Buffer received".

> >  		frames++;
> >
> >  		/* Requeue the buffer for further use. */
> > diff --git a/test/v4l2_videodevice/v4l2_m2mdevice.cpp b/test/v4l2_videodevice/v4l2_m2mdevice.cpp
> > index 4d3644c2d28792f1..442bcac5dc49cc59 100644
> > --- a/test/v4l2_videodevice/v4l2_m2mdevice.cpp
> > +++ b/test/v4l2_videodevice/v4l2_m2mdevice.cpp
> > @@ -31,7 +31,7 @@ public:
> >
> >  	void outputBufferComplete(Buffer *buffer)
> >  	{
> > -		cout << "Received output buffer " << buffer->index() << endl;
> > +		cout << "Received output buffer" << endl;
> >
> >  		outputFrames_++;
> >
> > @@ -41,7 +41,7 @@ public:
> >
> >  	void receiveCaptureBuffer(Buffer *buffer)
> >  	{
> > -		cout << "Received capture buffer " << buffer->index() << endl;
> > +		cout << "Received capture buffer" << endl;
> >
> 
> All minors, please add
> Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>

Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

> >  		captureFrames_++;
> >

-- 
Regards,

Laurent Pinchart