[libcamera-devel] [PATCH 15/30] libcamera: buffer: Move capture information to BufferInfo
Jacopo Mondi
jacopo at jmondi.org
Wed Nov 27 16:06:54 CET 2019
Hi Niklas,
On Wed, Nov 27, 2019 at 12:36:05AM +0100, Niklas Söderlund wrote:
> Move the metadata information retrieved when dequeuing a V4L2 buffer
> 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
> one in FrameBuffer.
>
> 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;
>
> 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 } });
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
> + * 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;
> +
> + 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.
>
> 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 ?
> + << " 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);
> --
> 2.24.0
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20191127/19a31281/attachment.sig>
More information about the libcamera-devel
mailing list