[libcamera-devel] [PATCH 15/30] libcamera: buffer: Move capture information to BufferInfo
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Mon Dec 9 22:19:33 CET 2019
Hi Niklas,
Thank you for the patch.
On Wed, Nov 27, 2019 at 04:06:54PM +0100, Jacopo Mondi wrote:
> On Wed, Nov 27, 2019 at 12:36:05AM +0100, Niklas Söderlund wrote:
> > Move the metadata information retrieved when dequeuing a V4L2 buffer
"metadata" or "information", not "metadata information". I would pick
one or the other based on whether you want to rename BufferInfo to
FrameInfo or FrameMetadata (with a preference for the latter on my
side).
> > into a BufferInfo. This is done as a step to migrate to the FrameBuffer
> > interface as the functions added to Buffer around BufferInfo matches the
s/matches/match/
> > one in FrameBuffer.
s/one/ones/
> >
> > Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> > ---
> > include/libcamera/buffer.h | 2 ++
> > src/android/camera_device.cpp | 4 +--
> > src/cam/buffer_writer.cpp | 2 +-
> > src/cam/capture.cpp | 8 ++++--
> > src/libcamera/buffer.cpp | 34 +++++++++++++++++-------
> > src/libcamera/pipeline/ipu3/ipu3.cpp | 4 +--
> > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 8 +++---
> > src/libcamera/request.cpp | 2 +-
> > src/libcamera/v4l2_videodevice.cpp | 23 +++++++++-------
> > src/qcam/main_window.cpp | 13 ++++-----
> > test/camera/buffer_import.cpp | 2 +-
> > test/camera/capture.cpp | 2 +-
> > test/v4l2_videodevice/buffer_sharing.cpp | 12 ++++++---
> > 13 files changed, 72 insertions(+), 44 deletions(-)
> >
> > diff --git a/include/libcamera/buffer.h b/include/libcamera/buffer.h
> > index d6db6138ca11d5fe..2e5376fb8b53a4c5 100644
> > --- a/include/libcamera/buffer.h
> > +++ b/include/libcamera/buffer.h
> > @@ -122,6 +122,7 @@ public:
> > unsigned int bytesused() const { return bytesused_; }
> > uint64_t timestamp() const { return timestamp_; }
> > unsigned int sequence() const { return sequence_; }
> > + const BufferInfo &info() const { return info_; };
> >
> > Status status() const { return status_; }
> > Request *request() const { return request_; }
> > @@ -142,6 +143,7 @@ private:
> > unsigned int bytesused_;
> > uint64_t timestamp_;
> > unsigned int sequence_;
> > + BufferInfo info_;
> >
> > Status status_;
> > Request *request_;
> > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> > index 09588c16a6301649..55b29a9a41ab8943 100644
> > --- a/src/android/camera_device.cpp
> > +++ b/src/android/camera_device.cpp
> > @@ -803,11 +803,11 @@ void CameraDevice::requestComplete(Request *request)
> >
> > if (status == CAMERA3_BUFFER_STATUS_OK) {
> > notifyShutter(descriptor->frameNumber,
> > - libcameraBuffer->timestamp());
> > + libcameraBuffer->info().timestamp());
> >
> > captureResult.partial_result = 1;
> > resultMetadata = getResultMetadata(descriptor->frameNumber,
> > - libcameraBuffer->timestamp());
> > + libcameraBuffer->info().timestamp());
> > captureResult.result = resultMetadata->get();
> > }
> >
> > diff --git a/src/cam/buffer_writer.cpp b/src/cam/buffer_writer.cpp
> > index 5967efca07254666..b6b40baeee661df6 100644
> > --- a/src/cam/buffer_writer.cpp
> > +++ b/src/cam/buffer_writer.cpp
> > @@ -32,7 +32,7 @@ int BufferWriter::write(Buffer *buffer, const std::string &streamName)
> > if (pos != std::string::npos) {
> > std::stringstream ss;
> > ss << streamName << "-" << std::setw(6)
> > - << std::setfill('0') << buffer->sequence();
> > + << std::setfill('0') << buffer->info().sequence();
> > filename.replace(pos, 1, ss.str());
> > }
> >
> > diff --git a/src/cam/capture.cpp b/src/cam/capture.cpp
> > index 1a4dbe7ce4a15a2d..a4fa88a8d99669bc 100644
> > --- a/src/cam/capture.cpp
> > +++ b/src/cam/capture.cpp
> > @@ -155,8 +155,12 @@ void Capture::requestComplete(Request *request)
> > const std::string &name = streamName_[stream];
> >
> > info << " " << name
> > - << " seq: " << std::setw(6) << std::setfill('0') << buffer->sequence()
> > - << " bytesused: " << buffer->bytesused();
> > + << " seq: " << std::setw(6) << std::setfill('0') << buffer->info().sequence();
> > +
> > + unsigned int nplane = 0;
> > + for (const BufferInfo::Plane &plane : buffer->info().planes())
> > + info << " bytesused(" << nplane++ << "): "
> > + << plane.bytesused;
Nitpicking, this makes it quite long. How about
const BufferInfo &info = buffer->info();
info << " " << name
<< " seq: " << std::setw(6) << std::setfill('0') << info..sequence()
<< " bytesused: ";
unsigned int nplane = 0;
for (const BufferInfo::Plane &plane : info.planes()) {
info << plane.bytesused;
if (++nplane < info.planes().size())
info << "/";
}
> >
> > if (writer_)
> > writer_->write(buffer, name);
> > diff --git a/src/libcamera/buffer.cpp b/src/libcamera/buffer.cpp
> > index 7043345c3f3207cd..d5a4815a0bb8c528 100644
> > --- a/src/libcamera/buffer.cpp
> > +++ b/src/libcamera/buffer.cpp
> > @@ -375,15 +375,22 @@ Buffer::Buffer(unsigned int index, const Buffer *metadata)
> > status_(Buffer::BufferSuccess), request_(nullptr),
> > stream_(nullptr)
> > {
> > + unsigned int sequence;
> > + uint64_t timestamp;
> > + unsigned int bytesused;
> > +
> > if (metadata) {
> > - bytesused_ = metadata->bytesused_;
> > - sequence_ = metadata->sequence_;
> > - timestamp_ = metadata->timestamp_;
> > + bytesused = metadata->info().planes()[0].bytesused;
> > + sequence = metadata->info().sequence();
> > + timestamp = metadata->info().timestamp();
> > } else {
> > - bytesused_ = 0;
> > - sequence_ = 0;
> > - timestamp_ = 0;
> > + bytesused = 0;
> > + sequence = 0;
> > + timestamp = 0;
> > }
> > +
> > + info_.update(BufferInfo::BufferSuccess, sequence, timestamp,
> > + { { bytesused } });
This would be simplified is BufferInfo was turned into a structure with
public members.
>
> I am missing a bit of context here, but is it fair to assume we have
> a single plane ?
> > }
> >
> > /**
> > @@ -439,6 +446,16 @@ Buffer::Buffer(unsigned int index, const Buffer *metadata)
> > * \return Sequence number of the buffer
> > */
> >
> > +/**
> > + * \fn Buffer::info()
> > + * \brief Retrieve the buffer metadata information
> > + *
>
> metadata + information is a bit of repetition, but that's fine
As commented above, I think we should use one or the other. If we go for
metadata, I would name the method metadata(). It probably doesn't matter
much here though, as the Buffer class is removed later in the series.
> > + * The buffer metadata information is update every time the buffer contained
>
> as in the previous patch, did you mean "buffer content" ?
>
> > + * are changed, for example when it is dequeued from hardware.
>
> buffer -> is
>
> > + *
> > + * \return Metadata of the buffer
> > + */
> > +
> > /**
> > * \fn Buffer::status()
> > * \brief Retrieve the buffer status
> > @@ -482,10 +499,7 @@ Buffer::Buffer(unsigned int index, const Buffer *metadata)
> > */
> > void Buffer::cancel()
> > {
> > - bytesused_ = 0;
> > - timestamp_ = 0;
> > - sequence_ = 0;
> > - status_ = BufferCancelled;
> > + info_.update(BufferInfo::BufferCancelled, 0, 0, { {} });
> > }
> >
> > /**
> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > index ad223d9101bdc6ed..8ba08351c950f5e2 100644
> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > @@ -928,7 +928,7 @@ int PipelineHandlerIPU3::registerCameras()
> > void IPU3CameraData::imguInputBufferReady(Buffer *buffer)
> > {
> > /* \todo Handle buffer failures when state is set to BufferError. */
> > - if (buffer->status() == Buffer::BufferCancelled)
> > + if (buffer->info().status() == BufferInfo::BufferCancelled)
> > return;
> >
> > cio2_.output_->queueBuffer(buffer);
> > @@ -962,7 +962,7 @@ void IPU3CameraData::imguOutputBufferReady(Buffer *buffer)
> > void IPU3CameraData::cio2BufferReady(Buffer *buffer)
> > {
> > /* \todo Handle buffer failures when state is set to BufferError. */
> > - if (buffer->status() == Buffer::BufferCancelled)
> > + if (buffer->info().status() == BufferInfo::BufferCancelled)
> > return;
> >
> > imgu_->input_->queueBuffer(buffer);
> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > index e8b6a278e97b0ba0..6ad9b57d8353896c 100644
> > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > @@ -100,10 +100,10 @@ public:
> > ASSERT(frameOffset(SOE) == 0);
> >
> > utils::time_point soe = std::chrono::time_point<utils::clock>()
> > - + std::chrono::nanoseconds(buffer->timestamp())
> > + + std::chrono::nanoseconds(buffer->info().timestamp())
> > + timeOffset(SOE);
> >
> > - notifyStartOfExposure(buffer->sequence(), soe);
> > + notifyStartOfExposure(buffer->info().sequence(), soe);
> > }
> >
> > void setDelay(unsigned int type, int frame, int msdelay)
> > @@ -1006,8 +1006,8 @@ void PipelineHandlerRkISP1::bufferReady(Buffer *buffer)
> >
> > data->timeline_.bufferReady(buffer);
> >
> > - if (data->frame_ <= buffer->sequence())
> > - data->frame_ = buffer->sequence() + 1;
> > + if (data->frame_ <= buffer->info().sequence())
> > + data->frame_ = buffer->info().sequence() + 1;
> >
> > completeBuffer(activeCamera_, request, buffer);
> > tryCompleteRequest(request);
> > diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp
> > index 3b49e52510918eee..7593bf9dfa546401 100644
> > --- a/src/libcamera/request.cpp
> > +++ b/src/libcamera/request.cpp
> > @@ -260,7 +260,7 @@ bool Request::completeBuffer(Buffer *buffer)
> >
> > buffer->request_ = nullptr;
> >
> > - if (buffer->status() == Buffer::BufferCancelled)
> > + if (buffer->info().status() == BufferInfo::BufferCancelled)
> > cancelled_ = true;
> >
> > return !hasPendingBuffers();
> > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp
> > index cc0a1c9382a2b1ed..8f962c7e9d0c7d01 100644
> > --- a/src/libcamera/v4l2_videodevice.cpp
> > +++ b/src/libcamera/v4l2_videodevice.cpp
> > @@ -1012,10 +1012,12 @@ int V4L2VideoDevice::queueBuffer(Buffer *buffer)
> > }
> >
> > if (V4L2_TYPE_IS_OUTPUT(buf.type)) {
> > - buf.bytesused = buffer->bytesused_;
> > - buf.sequence = buffer->sequence_;
> > - buf.timestamp.tv_sec = buffer->timestamp_ / 1000000000;
> > - buf.timestamp.tv_usec = (buffer->timestamp_ / 1000) % 1000000;
> > + const BufferInfo &info = buffer->info();
> > +
> > + buf.bytesused = info.planes()[0].bytesused;
> > + buf.sequence = info.sequence();
> > + buf.timestamp.tv_sec = info.timestamp() / 1000000000;
> > + buf.timestamp.tv_usec = (info.timestamp() / 1000) % 1000000;
> > }
> >
> > LOG(V4L2, Debug) << "Queueing buffer " << buf.index;
> > @@ -1121,12 +1123,13 @@ Buffer *V4L2VideoDevice::dequeueBuffer()
> > fdEvent_->setEnabled(false);
> >
> > buffer->index_ = buf.index;
> > - buffer->bytesused_ = buf.bytesused;
> > - buffer->timestamp_ = buf.timestamp.tv_sec * 1000000000ULL
> > - + buf.timestamp.tv_usec * 1000ULL;
> > - buffer->sequence_ = buf.sequence;
> > - buffer->status_ = buf.flags & V4L2_BUF_FLAG_ERROR
> > - ? Buffer::BufferError : Buffer::BufferSuccess;
> > +
> > + BufferInfo::Status status = buf.flags & V4L2_BUF_FLAG_ERROR
> > + ? BufferInfo::BufferError : BufferInfo::BufferSuccess;
> > + uint64_t timestamp = buf.timestamp.tv_sec * 1000000000ULL
> > + + buf.timestamp.tv_usec * 1000ULL;
Could you align the ? and + under the = ?
> > +
> > + buffer->info_.update(status, buf.sequence, timestamp, { { buf.bytesused } });
>
> Here it's where I wondered if we should not provide an update()
> version which takes a struct v4l2_buffer, but it would maybe be a bit
> of a layering violation.
As it is needed in a single place I'd prefer keeping it in
v4l2_videodevice.cpp.
> >
> > return buffer;
> > }
> > diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp
> > index 4cecf7e351214f3d..771020112f09b1ef 100644
> > --- a/src/qcam/main_window.cpp
> > +++ b/src/qcam/main_window.cpp
> > @@ -258,14 +258,15 @@ void MainWindow::requestComplete(Request *request)
> > framesCaptured_++;
> >
> > Buffer *buffer = buffers.begin()->second;
> > + const BufferInfo &info = buffer->info();
> >
> > - double fps = buffer->timestamp() - lastBufferTime_;
> > + double fps = info.timestamp() - lastBufferTime_;
> > fps = lastBufferTime_ && fps ? 1000000000.0 / fps : 0.0;
> > - lastBufferTime_ = buffer->timestamp();
> > + lastBufferTime_ = info.timestamp();
> >
> > - std::cout << "seq: " << std::setw(6) << std::setfill('0') << buffer->sequence()
> > - << " bytesused: " << buffer->bytesused()
> > - << " timestamp: " << buffer->timestamp()
> > + std::cout << "seq: " << std::setw(6) << std::setfill('0') << info.sequence()
> > + << " bytesused: " << info.planes()[0].bytesused
>
> Assuming a single plane is fine ?
It doesn't introduce any regression, but will need to be fixed. We need
to address multiplanar support.
> > + << " timestamp: " << info.timestamp()
> > << " fps: " << std::fixed << std::setprecision(2) << fps
> > << std::endl;
> >
> > @@ -302,7 +303,7 @@ int MainWindow::display(Buffer *buffer)
> >
> > Dmabuf &dmabuf = mem->planes().front();
> > unsigned char *raw = static_cast<unsigned char *>(dmabuf.mem());
> > - viewfinder_->display(raw, buffer->bytesused());
> > + viewfinder_->display(raw, buffer->info().planes()[0].bytesused);
> >
> > return 0;
> > }
> > diff --git a/test/camera/buffer_import.cpp b/test/camera/buffer_import.cpp
> > index dae907f9f41841c9..5dbaed9255d3d60c 100644
> > --- a/test/camera/buffer_import.cpp
> > +++ b/test/camera/buffer_import.cpp
> > @@ -275,7 +275,7 @@ public:
> > protected:
> > void bufferComplete(Request *request, Buffer *buffer)
> > {
> > - if (buffer->status() != Buffer::BufferSuccess)
> > + if (buffer->info().status() != BufferInfo::BufferSuccess)
> > return;
> >
> > unsigned int index = buffer->index();
> > diff --git a/test/camera/capture.cpp b/test/camera/capture.cpp
> > index ca1ebe419946dd4d..8307ea2629801679 100644
> > --- a/test/camera/capture.cpp
> > +++ b/test/camera/capture.cpp
> > @@ -28,7 +28,7 @@ protected:
> >
> > void bufferComplete(Request *request, Buffer *buffer)
> > {
> > - if (buffer->status() != Buffer::BufferSuccess)
> > + if (buffer->info().status() != BufferInfo::BufferSuccess)
> > return;
> >
> > completeBuffersCount_++;
> > diff --git a/test/v4l2_videodevice/buffer_sharing.cpp b/test/v4l2_videodevice/buffer_sharing.cpp
> > index d02c391b95933922..6b71caef111693d6 100644
> > --- a/test/v4l2_videodevice/buffer_sharing.cpp
> > +++ b/test/v4l2_videodevice/buffer_sharing.cpp
> > @@ -92,10 +92,12 @@ protected:
> >
> > void captureBufferReady(Buffer *buffer)
> > {
> > + const BufferInfo &info = buffer->info();
> > +
> > std::cout << "Received capture buffer sequence "
> > - << buffer->sequence() << std::endl;
> > + << info.sequence() << std::endl;
> >
> > - if (buffer->status() != Buffer::BufferSuccess)
> > + if (info.status() != BufferInfo::BufferSuccess)
> > return;
> >
> > output_->queueBuffer(buffer);
> > @@ -104,10 +106,12 @@ protected:
> >
> > void outputBufferReady(Buffer *buffer)
> > {
> > + const BufferInfo &info = buffer->info();
> > +
> > std::cout << "Received output buffer sequence "
> > - << buffer->sequence() << std::endl;
> > + << info.sequence() << std::endl;
> >
> > - if (buffer->status() != Buffer::BufferSuccess)
> > + if (info.status() != BufferInfo::BufferSuccess)
> > return;
> >
> > capture_->queueBuffer(buffer);
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list