| Message ID | 20191126233620.1695316-3-niklas.soderlund@ragnatech.se |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Niklas, 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... > 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@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"; > > 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 > + << 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; > > 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 > 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@jmondi.org> Thanks j > captureFrames_++; > > -- > 2.24.0 > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
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@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@jmondi.org> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > captureFrames_++; > >
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"; 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 " + << 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; 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; 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; captureFrames_++;
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 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@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(-)