| Message ID | 20191126233620.1695316-16-niklas.soderlund@ragnatech.se |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
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@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@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
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@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);
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 } }); } /** @@ -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 + * + * The buffer metadata information is update every time the buffer contained + * are changed, for example when it is dequeued from hardware. + * + * \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 } }); 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 + << " 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);
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@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(-)