[libcamera-devel] [PATCH v2 10/25] libcamera: buffer: Move captured metadata to FrameMetadata
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Tue Jan 7 23:19:30 CET 2020
Hi Niklas,
Thank you for the patch.
On Mon, Dec 30, 2019 at 01:04:55PM +0100, Niklas Söderlund wrote:
> Move the metadata retrieved when dequeuing a V4L2 buffer into a
> FrameMetadata object. This is done as a step to migrate to the
> FrameBuffer interface as the functions added to Buffer around
> FrameMetadata match the ones in FrameBuffer.
>
> Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> ---
> * Changes since v1
> - Rework to match FrameMetadata being a struct instead of a class
> - Align statements broken over multiple lines
> - Spiffy up bytesused printing in cam
> - Merged with patch who removes the old fields in Buffer.
> ---
> include/libcamera/buffer.h | 16 +----
> src/android/camera_device.cpp | 4 +-
> src/cam/buffer_writer.cpp | 2 +-
> src/cam/capture.cpp | 13 +++-
> src/libcamera/buffer.cpp | 75 ++++++------------------
> src/libcamera/pipeline/ipu3/ipu3.cpp | 4 +-
> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 8 +--
> src/libcamera/request.cpp | 2 +-
> src/libcamera/v4l2_videodevice.cpp | 24 ++++----
> src/qcam/main_window.cpp | 13 ++--
> test/camera/buffer_import.cpp | 2 +-
> test/camera/capture.cpp | 2 +-
> test/v4l2_videodevice/buffer_sharing.cpp | 8 ++-
> 13 files changed, 70 insertions(+), 103 deletions(-)
>
> diff --git a/include/libcamera/buffer.h b/include/libcamera/buffer.h
> index 8dd9f91272291648..8bd61f786748af5f 100644
> --- a/include/libcamera/buffer.h
> +++ b/include/libcamera/buffer.h
> @@ -93,12 +93,6 @@ private:
> class Buffer final
> {
> public:
> - enum Status {
> - BufferSuccess,
> - BufferError,
> - BufferCancelled,
> - };
> -
> Buffer(unsigned int index = -1, const Buffer *metadata = nullptr);
> Buffer(const Buffer &) = delete;
> Buffer &operator=(const Buffer &) = delete;
> @@ -107,11 +101,8 @@ public:
> const std::array<int, 3> &dmabufs() const { return dmabuf_; }
> BufferMemory *mem() { return mem_; }
>
> - unsigned int bytesused() const { return bytesused_; }
> - uint64_t timestamp() const { return timestamp_; }
> - unsigned int sequence() const { return sequence_; }
> + const FrameMetadata &metadata() const { return metadata_; };
>
> - Status status() const { return status_; }
> Request *request() const { return request_; }
> Stream *stream() const { return stream_; }
>
> @@ -127,11 +118,8 @@ private:
> std::array<int, 3> dmabuf_;
> BufferMemory *mem_;
>
> - unsigned int bytesused_;
> - uint64_t timestamp_;
> - unsigned int sequence_;
> + FrameMetadata metadata_;
>
> - Status status_;
> Request *request_;
> Stream *stream_;
> };
> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> index 09588c16a6301649..ebe91ea8af4f6436 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->metadata().timestamp);
>
> captureResult.partial_result = 1;
> resultMetadata = getResultMetadata(descriptor->frameNumber,
> - libcameraBuffer->timestamp());
> + libcameraBuffer->metadata().timestamp);
> captureResult.result = resultMetadata->get();
> }
>
> diff --git a/src/cam/buffer_writer.cpp b/src/cam/buffer_writer.cpp
> index 3e84068e66bb4dd7..7c58a1f50829f290 100644
> --- a/src/cam/buffer_writer.cpp
> +++ b/src/cam/buffer_writer.cpp
> @@ -33,7 +33,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->metadata().sequence;
> filename.replace(pos, 1, ss.str());
> }
>
> diff --git a/src/cam/capture.cpp b/src/cam/capture.cpp
> index 1a4dbe7ce4a15a2d..da942f56118983bd 100644
> --- a/src/cam/capture.cpp
> +++ b/src/cam/capture.cpp
> @@ -154,9 +154,18 @@ void Capture::requestComplete(Request *request)
> Buffer *buffer = it->second;
> const std::string &name = streamName_[stream];
>
> + const FrameMetadata &metadata = buffer->metadata();
> +
> info << " " << name
> - << " seq: " << std::setw(6) << std::setfill('0') << buffer->sequence()
> - << " bytesused: " << buffer->bytesused();
> + << " seq: " << std::setw(6) << std::setfill('0') << metadata.sequence
> + << " bytesused: ";
> +
> + unsigned int nplane = 0;
> + for (const FrameMetadata::Plane &plane : metadata.planes) {
> + info << plane.bytesused;
> + if (++nplane < metadata.planes.size())
> + info << "/";
> + }
>
> if (writer_)
> writer_->write(buffer, name);
> diff --git a/src/libcamera/buffer.cpp b/src/libcamera/buffer.cpp
> index 4a77f20751408b7c..686d0412bf54b2c1 100644
> --- a/src/libcamera/buffer.cpp
> +++ b/src/libcamera/buffer.cpp
> @@ -166,20 +166,6 @@ void BufferPool::destroyBuffers()
> * deleted automatically after the request complete handler returns.
> */
>
> -/**
> - * \enum Buffer::Status
> - * Buffer completion status
> - * \var Buffer::BufferSuccess
> - * The buffer has completed with success and contains valid data. All its other
> - * metadata (such as bytesused(), timestamp() or sequence() number) are valid.
> - * \var Buffer::BufferError
> - * The buffer has completed with an error and doesn't contain valid data. Its
> - * other metadata are valid.
> - * \var Buffer::BufferCancelled
> - * The buffer has been cancelled due to capture stop. Its other metadata are
> - * invalid and shall not be used.
> - */
> -
> /**
> * \brief Construct a buffer not associated with any stream
> *
> @@ -188,18 +174,19 @@ void BufferPool::destroyBuffers()
> * for a stream with Stream::createBuffer().
> */
> Buffer::Buffer(unsigned int index, const Buffer *metadata)
> - : index_(index), dmabuf_({ -1, -1, -1 }),
> - status_(Buffer::BufferSuccess), request_(nullptr),
> + : index_(index), dmabuf_({ -1, -1, -1 }), request_(nullptr),
> stream_(nullptr)
> {
> + metadata_.status = FrameMetadata::FrameSuccess;
> +
> if (metadata) {
> - bytesused_ = metadata->bytesused_;
> - sequence_ = metadata->sequence_;
> - timestamp_ = metadata->timestamp_;
> + metadata_.sequence = metadata->metadata().sequence;
> + metadata_.timestamp = metadata->metadata().timestamp;
> + metadata_.planes = metadata->metadata().planes;
> } else {
> - bytesused_ = 0;
> - sequence_ = 0;
> - timestamp_ = 0;
> + metadata_.sequence = 0;
> + metadata_.timestamp = 0;
> + metadata_.planes = { { 0 } };
> }
I believe this code will go away, but otherwise it could have been
written as
if (metadata)
metadata_ = metadata->metadata();
else
metadata_ = {};
metadata_.status = FrameMetadata::FrameSuccess;
> }
>
> @@ -231,39 +218,13 @@ Buffer::Buffer(unsigned int index, const Buffer *metadata)
> */
>
> /**
> - * \fn Buffer::bytesused()
> - * \brief Retrieve the number of bytes occupied by the data in the buffer
> - * \return Number of bytes occupied in the buffer
> - */
> -
> -/**
> - * \fn Buffer::timestamp()
> - * \brief Retrieve the time when the buffer was processed
> - *
> - * The timestamp is expressed as a number of nanoseconds since the epoch.
> - *
> - * \return Timestamp when the buffer was processed
> - */
> -
> -/**
> - * \fn Buffer::sequence()
> - * \brief Retrieve the buffer sequence number
> - *
> - * The sequence number is a monotonically increasing number assigned to the
> - * buffer processed by the stream. Gaps in the sequence numbers indicate
> - * dropped frames.
> - *
> - * \return Sequence number of the buffer
> - */
> -
> -/**
> - * \fn Buffer::status()
> - * \brief Retrieve the buffer status
> + * \fn Buffer::metadata()
> + * \brief Retrieve the buffer metadata
> *
> - * The buffer status reports whether the buffer has completed successfully
> - * (BufferSuccess) or if an error occurred (BufferError).
> + * The buffer metadata is update every time the buffer contained are changed,
s/update/updated/
* The buffer metadata is updated when the buffer contents are modified, for
* example when a frame has been captured to the buffer by the hardware.
(not that it matters too much as this will go away)
Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> + * for example when it is dequeued from hardware.
> *
> - * \return The buffer status
> + * \return Metadata for the buffer
> */
>
> /**
> @@ -299,10 +260,10 @@ Buffer::Buffer(unsigned int index, const Buffer *metadata)
> */
> void Buffer::cancel()
> {
> - bytesused_ = 0;
> - timestamp_ = 0;
> - sequence_ = 0;
> - status_ = BufferCancelled;
> + metadata_.status = FrameMetadata::FrameCancelled;
> + metadata_.sequence = 0;
> + metadata_.timestamp = 0;
> + metadata_.planes = {};
> }
>
> /**
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index 6d8c3fada127310e..34fc792977d151be 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->metadata().status == FrameMetadata::FrameCancelled)
> 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->metadata().status == FrameMetadata::FrameCancelled)
> return;
>
> imgu_->input_->queueBuffer(buffer);
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> index bb652d0da9c6df52..46df871a51105ee4 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->metadata().timestamp)
> + timeOffset(SOE);
>
> - notifyStartOfExposure(buffer->sequence(), soe);
> + notifyStartOfExposure(buffer->metadata().sequence, soe);
> }
>
> void setDelay(unsigned int type, int frame, int msdelay)
> @@ -1002,8 +1002,8 @@ void PipelineHandlerRkISP1::bufferReady(Buffer *buffer)
>
> data->timeline_.bufferReady(buffer);
>
> - if (data->frame_ <= buffer->sequence())
> - data->frame_ = buffer->sequence() + 1;
> + if (data->frame_ <= buffer->metadata().sequence)
> + data->frame_ = buffer->metadata().sequence + 1;
>
> completeBuffer(activeCamera_, request, buffer);
> tryCompleteRequest(request);
> diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp
> index 3b49e52510918eee..b17a6c2278361f00 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->metadata().status == FrameMetadata::FrameCancelled)
> cancelled_ = true;
>
> return !hasPendingBuffers();
> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp
> index 7d585ce03ed9a45a..51112d197068cf0a 100644
> --- a/src/libcamera/v4l2_videodevice.cpp
> +++ b/src/libcamera/v4l2_videodevice.cpp
> @@ -1015,10 +1015,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 FrameMetadata &metadata = buffer->metadata();
> +
> + buf.bytesused = metadata.planes[0].bytesused;
> + buf.sequence = metadata.sequence;
> + buf.timestamp.tv_sec = metadata.timestamp / 1000000000;
> + buf.timestamp.tv_usec = (metadata.timestamp / 1000) % 1000000;
> }
>
> LOG(V4L2, Debug) << "Queueing buffer " << buf.index;
> @@ -1125,12 +1127,14 @@ 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;
> +
> + buffer->metadata_.status = buf.flags & V4L2_BUF_FLAG_ERROR
> + ? FrameMetadata::FrameError
> + : FrameMetadata::FrameSuccess;
> + buffer->metadata_.sequence = buf.sequence;
> + buffer->metadata_.timestamp = buf.timestamp.tv_sec * 1000000000ULL
> + + buf.timestamp.tv_usec * 1000ULL;
> + buffer->metadata_.planes = { { buf.bytesused } };
>
> return buffer;
> }
> diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp
> index 5dec9252898100db..47793702b9aa0ee9 100644
> --- a/src/qcam/main_window.cpp
> +++ b/src/qcam/main_window.cpp
> @@ -259,14 +259,15 @@ void MainWindow::requestComplete(Request *request)
> framesCaptured_++;
>
> Buffer *buffer = buffers.begin()->second;
> + const FrameMetadata &metadata = buffer->metadata();
>
> - double fps = buffer->timestamp() - lastBufferTime_;
> + double fps = metadata.timestamp - lastBufferTime_;
> fps = lastBufferTime_ && fps ? 1000000000.0 / fps : 0.0;
> - lastBufferTime_ = buffer->timestamp();
> + lastBufferTime_ = metadata.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') << metadata.sequence
> + << " bytesused: " << metadata.planes[0].bytesused
> + << " timestamp: " << metadata.timestamp
> << " fps: " << std::fixed << std::setprecision(2) << fps
> << std::endl;
>
> @@ -306,7 +307,7 @@ int MainWindow::display(Buffer *buffer)
> plane.fd.fd(), 0);
>
> unsigned char *raw = static_cast<unsigned char *>(memory);
> - viewfinder_->display(raw, buffer->bytesused());
> + viewfinder_->display(raw, buffer->metadata().planes[0].bytesused);
>
> munmap(memory, plane.length);
>
> diff --git a/test/camera/buffer_import.cpp b/test/camera/buffer_import.cpp
> index e5c010d81b8d6e0e..3ba6ce9690f29329 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->metadata().status != FrameMetadata::FrameSuccess)
> return;
>
> unsigned int index = buffer->index();
> diff --git a/test/camera/capture.cpp b/test/camera/capture.cpp
> index ca1ebe419946dd4d..0d9ffc476650f414 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->metadata().status != FrameMetadata::FrameSuccess)
> return;
>
> completeBuffersCount_++;
> diff --git a/test/v4l2_videodevice/buffer_sharing.cpp b/test/v4l2_videodevice/buffer_sharing.cpp
> index 3a56862cb2b77d38..fe48b2e98fdada8d 100644
> --- a/test/v4l2_videodevice/buffer_sharing.cpp
> +++ b/test/v4l2_videodevice/buffer_sharing.cpp
> @@ -92,9 +92,11 @@ protected:
>
> void captureBufferReady(Buffer *buffer)
> {
> + const FrameMetadata &metadata = buffer->metadata();
> +
> std::cout << "Received capture buffer" << std::endl;
>
> - if (buffer->status() != Buffer::BufferSuccess)
> + if (metadata.status != FrameMetadata::FrameSuccess)
> return;
>
> output_->queueBuffer(buffer);
> @@ -103,9 +105,11 @@ protected:
>
> void outputBufferReady(Buffer *buffer)
> {
> + const FrameMetadata &metadata = buffer->metadata();
> +
> std::cout << "Received output buffer" << std::endl;
>
> - if (buffer->status() != Buffer::BufferSuccess)
> + if (metadata.status != FrameMetadata::FrameSuccess)
> return;
>
> capture_->queueBuffer(buffer);
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list