[libcamera-devel] [PATCH 02/30] libcamera: Remove buffer index from logging
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Mon Dec 9 14:22:04 CET 2019
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
More information about the libcamera-devel
mailing list