[libcamera-devel] [RFC 10/12] libcamera: buffer: Store buffer information in separate container
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Mon Nov 18 21:11:29 CET 2019
Hi Andrey,
On Wed, Nov 06, 2019 at 02:15:44PM +0300, Andrey Konovalov wrote:
> On 06.11.2019 13:18, Jacopo Mondi wrote:
> > On Fri, Nov 01, 2019 at 01:15:58PM +0000, Kieran Bingham wrote:
> >> On 28/10/2019 02:25, Niklas Söderlund wrote:
> >>> Use the new BufferInfo container to store information from when a buffer
> >>> is dequeued. This will aid in the ongoing buffer rework and simplify the
> >>> code.
> >>>
> >>> This commits breaks the buffer sharing test case as it only deals with
> >>> buffer information going out of the video device. The next patch will
> >>> restore the test as it will address the incoming information. All other
> >>> tests as well as cam and qcam works as expected on pipelines not require
> >>> buffer importing.
> >>
> >> I certainly like moving all the BufferInfo data to it's own class to
> >> keep it distinct (and tidy), but I wonder - why not then just put the
> >> BufferInfo into the Buffer?
> >>
> >> Then the association between the Buffer and the BufferInfo is
> >> maintained, rather than having to provide separate lists and determine
> >> which BufferInfo belongs to which Buffer?
> >>
> >> (Or at the very least, doing so in this patch might prevent the
> >> temporary test breakage you report?)
> >>
> >> Do we have cases where we have a Buffer - but we do not need or care
> >> about the associated BufferInfo data?
> >
> > I was about to ask the same actually.
>
> Not sure if such a use case for libcamera could ever be possible,
>
> but when people deal with secure playback, the CPU is often has no physical access to the (unencrypted)
> buffer contents, but needs to process "the metadata" embedded into the stream. I don't know any details,
> but I've heard they have to create a separate copy of those "metadata" just to make them accessible to the CPU.
> So if there could be cases when the Buffer and the BufferInfo might have different access permissions,
> having the BufferInfo separated from the Buffer, or movable would be a plus.
Thank you for the feedback.
As far as I understand (and this will be clarified by proper
documentation), the Buffer class is meant to represent a piece of
memory that stores a frame, but that doesn't mean the contents of the
buffer have to be accessible by the CPU. There will still be a Buffer
instance, as there will be a buffer, but it will only be used as a
handle to refer to the buffer memory, not to access the contents itself.
I'll let Niklas correct me if this isn't what he had intended.
> > The BufferInfo is created by the V4L2VideoDevice at queueBuffer() and
> > streaOff() time, and associated to a Buffer for its whole lifetime.
> > I would store a BufferInfo * in a Buffer, but then the question is:
> > could the pointer be retreived and accessed before it gets initialized
> > by V4L2VideoDevice::queueBuffer() ? In that case I agree it might make
> > sense to make the BufferInfo accessible only after a Request or a
> > Buffer has completed, to guarantee the association is in place.
> >
> > But then in that case, we should really avoid copies, so I would make
> > BufferInfo movable, as in Request::completeBuffer() the BufferInfo is
> > actually copied into the Request::info_ map.
> >
> > Or we could use the BufferInfo as temporary instances. At
> > Request::completeBuffer() time, we use the fields of the BufferInfo
> > received as parameter to create a new BufferInfo and store it in the
> > map, but at that point, a move constructor might be better. As
> > BufferInfo has fields which are not trivially movable we need to
> > define our own move constructor. (right? BufferInfo has a vector, so
> > the implicitly movable constructor is not generated if I'm not
> > mistaken)
> >
> >>
> >>> Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> >>> ---
> >>> include/libcamera/request.h | 6 ++-
> >>> src/cam/buffer_writer.cpp | 5 ++-
> >>> src/cam/buffer_writer.h | 4 +-
> >>> src/cam/capture.cpp | 21 ++++-----
> >>> src/libcamera/include/pipeline_handler.h | 3 +-
> >>> src/libcamera/include/v4l2_videodevice.h | 4 +-
> >>> src/libcamera/pipeline/ipu3/ipu3.cpp | 20 ++++-----
> >>> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 40 ++++++++---------
> >>> src/libcamera/pipeline/uvcvideo.cpp | 6 +--
> >>> src/libcamera/pipeline/vimc.cpp | 6 +--
> >>> src/libcamera/pipeline_handler.cpp | 5 ++-
> >>> src/libcamera/request.cpp | 6 ++-
> >>> src/libcamera/v4l2_videodevice.cpp | 55 ++++++++----------------
> >>> src/qcam/main_window.cpp | 18 ++++----
> >>> src/qcam/main_window.h | 2 +-
> >>> test/camera/capture.cpp | 4 +-
> >>> test/v4l2_videodevice/buffer_sharing.cpp | 17 ++++----
> >>> test/v4l2_videodevice/capture_async.cpp | 4 +-
> >>> test/v4l2_videodevice/v4l2_m2mdevice.cpp | 8 ++--
> >>> 19 files changed, 114 insertions(+), 120 deletions(-)
> >>>
> >>> diff --git a/include/libcamera/request.h b/include/libcamera/request.h
> >>> index 2d5a5964e99eb75f..88ef7bf03fcfb77b 100644
> >>> --- a/include/libcamera/request.h
> >>> +++ b/include/libcamera/request.h
> >>> @@ -12,12 +12,12 @@
> >>> #include <stdint.h>
> >>> #include <unordered_set>
> >>>
> >>> +#include <libcamera/buffer.h>
> >>> #include <libcamera/controls.h>
> >>> #include <libcamera/signal.h>
> >>>
> >>> namespace libcamera {
> >>>
> >>> -class Buffer;
> >>> class Camera;
> >>> class CameraControlValidator;
> >>> class Stream;
> >>> @@ -39,6 +39,7 @@ public:
> >>> ControlList &controls() { return *controls_; }
> >>> ControlList &metadata() { return *metadata_; }
> >>> const std::map<Stream *, Buffer *> &buffers() const { return bufferMap_; }
> >>> + const BufferInfo &info(Buffer *frame) const { return info_.find(frame)->second; };
> >>> int addBuffer(std::unique_ptr<Buffer> buffer);
> >>> Buffer *findBuffer(Stream *stream) const;
> >>>
> >>> @@ -54,13 +55,14 @@ private:
> >>> int prepare();
> >>> void complete();
> >>>
> >>> - bool completeBuffer(Buffer *buffer);
> >>> + bool completeBuffer(Buffer *buffer, const BufferInfo &info);
> >>>
> >>> Camera *camera_;
> >>> CameraControlValidator *validator_;
> >>> ControlList *controls_;
> >>> ControlList *metadata_;
> >>> std::map<Stream *, Buffer *> bufferMap_;
> >>> + std::map<Buffer *, BufferInfo> info_;
> >>> std::unordered_set<Buffer *> pending_;
> >>>
> >>> const uint64_t cookie_;
> >>> diff --git a/src/cam/buffer_writer.cpp b/src/cam/buffer_writer.cpp
> >>> index c33e99c5f8173db8..3ee9e82ba216abb6 100644
> >>> --- a/src/cam/buffer_writer.cpp
> >>> +++ b/src/cam/buffer_writer.cpp
> >>> @@ -21,7 +21,8 @@ BufferWriter::BufferWriter(const std::string &pattern)
> >>> {
> >>> }
> >>>
> >>> -int BufferWriter::write(Buffer *buffer, const std::string &streamName)
> >>> +int BufferWriter::write(Buffer *buffer, const BufferInfo &info,
> >>> + const std::string &streamName)
> >>> {
> >>> std::string filename;
> >>> size_t pos;
> >>> @@ -32,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') << info.sequence();
> >>> filename.replace(pos, 1, ss.str());
> >>> }
> >>>
> >>> diff --git a/src/cam/buffer_writer.h b/src/cam/buffer_writer.h
> >>> index 7bf785d1e83235ff..38f7045c7d4e68a4 100644
> >>> --- a/src/cam/buffer_writer.h
> >>> +++ b/src/cam/buffer_writer.h
> >>> @@ -16,7 +16,9 @@ class BufferWriter
> >>> public:
> >>> BufferWriter(const std::string &pattern = "frame-#.bin");
> >>>
> >>> - int write(libcamera::Buffer *buffer, const std::string &streamName);
> >>> + int write(libcamera::Buffer *buffer,
> >>> + const libcamera::BufferInfo &info,
> >>> + const std::string &streamName);
> >>>
> >>> private:
> >>> std::string pattern_;
> >>> diff --git a/src/cam/capture.cpp b/src/cam/capture.cpp
> >>> index e665d819fb777a90..251e9f86c86b508d 100644
> >>> --- a/src/cam/capture.cpp
> >>> +++ b/src/cam/capture.cpp
> >>> @@ -138,32 +138,33 @@ void Capture::requestComplete(Request *request)
> >>> if (request->status() == Request::RequestCancelled)
> >>> return;
> >>>
> >>> - const std::map<Stream *, Buffer *> &buffers = request->buffers();
> >>> -
> >>> std::chrono::steady_clock::time_point now = std::chrono::steady_clock::now();
> >>> double fps = std::chrono::duration_cast<std::chrono::milliseconds>(now - last_).count();
> >>> fps = last_ != std::chrono::steady_clock::time_point() && fps
> >>> ? 1000.0 / fps : 0.0;
> >>> last_ = now;
> >>>
> >>> - std::stringstream info;
> >>> - info << "fps: " << std::fixed << std::setprecision(2) << fps;
> >>> + std::stringstream infostr;
> >>> + infostr << "fps: " << std::fixed << std::setprecision(2) << fps;
> >>>
> >>> + const std::map<Stream *, Buffer *> &buffers = request->buffers();
> >>> for (auto it = buffers.begin(); it != buffers.end(); ++it) {
> >>> Stream *stream = it->first;
> >>> Buffer *buffer = it->second;
> >>> + const BufferInfo &info = request->info(buffer);
> >>> const std::string &name = streamName_[stream];
> >>>
> >>> - info << " " << name
> >>> - << " (" << buffer->index() << ")"
> >>> - << " seq: " << std::setw(6) << std::setfill('0') << buffer->sequence()
> >>> - << " bytesused: " << buffer->bytesused();
> >>> + infostr << " " << name
> >>> + << " seq: " << std::setw(6) << std::setfill('0') << info.sequence();
> >>> +
> >>> + for (unsigned int i = 0; i < info.planes(); i++)
> >>> + infostr << " bytesused(" << i << "): " << info.plane(i).bytesused;
> >>>
> >>> if (writer_)
> >>> - writer_->write(buffer, name);
> >>> + writer_->write(buffer, info, name);
> >>> }
> >>>
> >>> - std::cout << info.str() << std::endl;
> >>> + std::cout << infostr.str() << std::endl;
> >>>
> >>> /*
> >>> * Create a new request and populate it with one buffer for each
> >>> diff --git a/src/libcamera/include/pipeline_handler.h b/src/libcamera/include/pipeline_handler.h
> >>> index 6024357e266c2e2b..e6dbd7687021e4ff 100644
> >>> --- a/src/libcamera/include/pipeline_handler.h
> >>> +++ b/src/libcamera/include/pipeline_handler.h
> >>> @@ -81,7 +81,8 @@ public:
> >>>
> >>> virtual int queueRequest(Camera *camera, Request *request);
> >>>
> >>> - bool completeBuffer(Camera *camera, Request *request, Buffer *buffer);
> >>> + bool completeBuffer(Camera *camera, Request *request, Buffer *buffer,
> >>> + const BufferInfo &info);
> >>> void completeRequest(Camera *camera, Request *request);
> >>>
> >>> const char *name() const { return name_; }
> >>> diff --git a/src/libcamera/include/v4l2_videodevice.h b/src/libcamera/include/v4l2_videodevice.h
> >>> index 5b178339d0ce7e2c..01b90ab4654bfba2 100644
> >>> --- a/src/libcamera/include/v4l2_videodevice.h
> >>> +++ b/src/libcamera/include/v4l2_videodevice.h
> >>> @@ -12,6 +12,7 @@
> >>>
> >>> #include <linux/videodev2.h>
> >>>
> >>> +#include <libcamera/buffer.h>
> >>> #include <libcamera/geometry.h>
> >>> #include <libcamera/signal.h>
> >>>
> >>> @@ -166,7 +167,7 @@ public:
> >>>
> >>> int queueBuffer(Buffer *buffer);
> >>> std::vector<std::unique_ptr<Buffer>> queueAllBuffers();
> >>> - Signal<Buffer *> bufferReady;
> >>> + Signal<Buffer *, const BufferInfo &> bufferReady;
> >>>
> >>> int streamOn();
> >>> int streamOff();
> >>> @@ -194,7 +195,6 @@ private:
> >>> int createPlane(BufferMemory *buffer, unsigned int index,
> >>> unsigned int plane, unsigned int length);
> >>>
> >>> - Buffer *dequeueBuffer();
> >>> void bufferAvailable(EventNotifier *notifier);
> >>>
> >>> V4L2Capability caps_;
> >>> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> >>> index 8aa5f34febf16585..01064ac09859155d 100644
> >>> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> >>> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> >>> @@ -161,9 +161,9 @@ public:
> >>> delete vfStream_;
> >>> }
> >>>
> >>> - void imguOutputBufferReady(Buffer *buffer);
> >>> - void imguInputBufferReady(Buffer *buffer);
> >>> - void cio2BufferReady(Buffer *buffer);
> >>> + void imguOutputBufferReady(Buffer *buffer, const BufferInfo &info);
> >>> + void imguInputBufferReady(Buffer *buffer, const BufferInfo &info);
> >>> + void cio2BufferReady(Buffer *buffer, const BufferInfo &info);
> >>>
> >>> CIO2Device cio2_;
> >>> ImgUDevice *imgu_;
> >>> @@ -931,10 +931,10 @@ int PipelineHandlerIPU3::registerCameras()
> >>> * Buffers completed from the ImgU input are immediately queued back to the
> >>> * CIO2 unit to continue frame capture.
> >>> */
> >>> -void IPU3CameraData::imguInputBufferReady(Buffer *buffer)
> >>> +void IPU3CameraData::imguInputBufferReady(Buffer *buffer, const BufferInfo &info)
> >>> {
> >>> /* \todo Handle buffer failures when state is set to BufferError. */
> >>> - if (buffer->status() == Buffer::BufferCancelled)
> >>> + if (info.status() == BufferInfo::BufferCancelled)
> >>> return;
> >>>
> >>> cio2_.output_->queueBuffer(buffer);
> >>> @@ -946,15 +946,13 @@ void IPU3CameraData::imguInputBufferReady(Buffer *buffer)
> >>> *
> >>> * Buffers completed from the ImgU output are directed to the application.
> >>> */
> >>> -void IPU3CameraData::imguOutputBufferReady(Buffer *buffer)
> >>> +void IPU3CameraData::imguOutputBufferReady(Buffer *buffer, const BufferInfo &info)
> >>> {
> >>> Request *request = requestFromBuffer(buffer);
> >>>
> >>> - if (!pipe_->completeBuffer(camera_, request, buffer))
> >>> - /* Request not completed yet, return here. */
> >>> + if (!pipe_->completeBuffer(camera_, request, buffer, info))
> >>> return;
> >>>
> >>> - /* Mark the request as complete. */
> >>> pipe_->completeRequest(camera_, request);
> >>> }
> >>>
> >>> @@ -965,10 +963,10 @@ void IPU3CameraData::imguOutputBufferReady(Buffer *buffer)
> >>> * Buffers completed from the CIO2 are immediately queued to the ImgU unit
> >>> * for further processing.
> >>> */
> >>> -void IPU3CameraData::cio2BufferReady(Buffer *buffer)
> >>> +void IPU3CameraData::cio2BufferReady(Buffer *buffer, const BufferInfo &info)
> >>> {
> >>> /* \todo Handle buffer failures when state is set to BufferError. */
> >>> - if (buffer->status() == Buffer::BufferCancelled)
> >>> + if (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 0803572754364beb..33a058de18b8cf2e 100644
> >>> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> >>> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> >>> @@ -88,7 +88,7 @@ public:
> >>> setDelay(QueueBuffers, -1, 10);
> >>> }
> >>>
> >>> - void bufferReady(Buffer *buffer)
> >>> + void bufferReady(Buffer *buffer, const BufferInfo &info)
> >>> {
> >>> /*
> >>> * Calculate SOE by taking the end of DMA set by the kernel and applying
> >>> @@ -99,10 +99,10 @@ public:
> >>> ASSERT(frameOffset(SOE) == 0);
> >>>
> >>> utils::time_point soe = std::chrono::time_point<utils::clock>()
> >>> - + std::chrono::nanoseconds(buffer->timestamp())
> >>> + + std::chrono::nanoseconds(info.timestamp())
> >>> + timeOffset(SOE);
> >>>
> >>> - notifyStartOfExposure(buffer->sequence(), soe);
> >>> + notifyStartOfExposure(info.sequence(), soe);
> >>> }
> >>>
> >>> void setDelay(unsigned int type, int frame, int msdelay)
> >>> @@ -202,9 +202,9 @@ private:
> >>> int initLinks();
> >>> int createCamera(MediaEntity *sensor);
> >>> void tryCompleteRequest(Request *request);
> >>> - void bufferReady(Buffer *buffer);
> >>> - void paramReady(Buffer *buffer);
> >>> - void statReady(Buffer *buffer);
> >>> + void bufferReady(Buffer *buffer, const BufferInfo &info);
> >>> + void paramReady(Buffer *buffer, const BufferInfo &info);
> >>> + void statReady(Buffer *buffer, const BufferInfo &info);
> >>>
> >>> MediaDevice *media_;
> >>> V4L2Subdevice *dphy_;
> >>> @@ -987,43 +987,43 @@ void PipelineHandlerRkISP1::tryCompleteRequest(Request *request)
> >>> data->frameInfo_.destroy(info->frame);
> >>> }
> >>>
> >>> -void PipelineHandlerRkISP1::bufferReady(Buffer *buffer)
> >>> +void PipelineHandlerRkISP1::bufferReady(Buffer *buffer, const BufferInfo &info)
> >>> {
> >>> ASSERT(activeCamera_);
> >>> RkISP1CameraData *data = cameraData(activeCamera_);
> >>> Request *request = data->requestFromBuffer(buffer);
> >>>
> >>> - data->timeline_.bufferReady(buffer);
> >>> + data->timeline_.bufferReady(buffer, info);
> >>>
> >>> - if (data->frame_ <= buffer->sequence())
> >>> - data->frame_ = buffer->sequence() + 1;
> >>> + if (data->frame_ <= info.sequence())
> >>> + data->frame_ = info.sequence() + 1;
> >>>
> >>> - completeBuffer(activeCamera_, request, buffer);
> >>> + completeBuffer(activeCamera_, request, buffer, info);
> >>> tryCompleteRequest(request);
> >>> }
> >>>
> >>> -void PipelineHandlerRkISP1::paramReady(Buffer *buffer)
> >>> +void PipelineHandlerRkISP1::paramReady(Buffer *buffer, const BufferInfo &info)
> >>> {
> >>> ASSERT(activeCamera_);
> >>> RkISP1CameraData *data = cameraData(activeCamera_);
> >>>
> >>> - RkISP1FrameInfo *info = data->frameInfo_.find(buffer);
> >>> + RkISP1FrameInfo *rkinfo = data->frameInfo_.find(buffer);
> >>>
> >>> - info->paramDequeued = true;
> >>> - tryCompleteRequest(info->request);
> >>> + rkinfo->paramDequeued = true;
> >>> + tryCompleteRequest(rkinfo->request);
> >>> }
> >>>
> >>> -void PipelineHandlerRkISP1::statReady(Buffer *buffer)
> >>> +void PipelineHandlerRkISP1::statReady(Buffer *buffer, const BufferInfo &info)
> >>> {
> >>> ASSERT(activeCamera_);
> >>> RkISP1CameraData *data = cameraData(activeCamera_);
> >>>
> >>> - RkISP1FrameInfo *info = data->frameInfo_.find(buffer);
> >>> - if (!info)
> >>> + RkISP1FrameInfo *rkinfo = data->frameInfo_.find(buffer);
> >>> + if (!rkinfo)
> >>> return;
> >>>
> >>> - unsigned int frame = info->frame;
> >>> - unsigned int statid = RKISP1_STAT_BASE | info->statBuffer->index();
> >>> + unsigned int frame = rkinfo->frame;
> >>> + unsigned int statid = RKISP1_STAT_BASE | rkinfo->statBuffer->index();
> >>>
> >>> IPAOperationData op;
> >>> op.operation = RKISP1_IPA_EVENT_SIGNAL_STAT_BUFFER;
> >>> diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp
> >>> index 679d82d38227b991..ef2e8c9734f844ce 100644
> >>> --- a/src/libcamera/pipeline/uvcvideo.cpp
> >>> +++ b/src/libcamera/pipeline/uvcvideo.cpp
> >>> @@ -42,7 +42,7 @@ public:
> >>> }
> >>>
> >>> int init(MediaEntity *entity);
> >>> - void bufferReady(Buffer *buffer);
> >>> + void bufferReady(Buffer *buffer, const BufferInfo &info);
> >>>
> >>> V4L2VideoDevice *video_;
> >>> Stream *stream_;
> >>> @@ -373,11 +373,11 @@ int UVCCameraData::init(MediaEntity *entity)
> >>> return 0;
> >>> }
> >>>
> >>> -void UVCCameraData::bufferReady(Buffer *buffer)
> >>> +void UVCCameraData::bufferReady(Buffer *buffer, const BufferInfo &info)
> >>> {
> >>> Request *request = requestFromBuffer(buffer);
> >>>
> >>> - pipe_->completeBuffer(camera_, request, buffer);
> >>> + pipe_->completeBuffer(camera_, request, buffer, info);
> >>> pipe_->completeRequest(camera_, request);
> >>> }
> >>>
> >>> diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp
> >>> index 56898716a8cde074..e3eefc49135179f2 100644
> >>> --- a/src/libcamera/pipeline/vimc.cpp
> >>> +++ b/src/libcamera/pipeline/vimc.cpp
> >>> @@ -56,7 +56,7 @@ public:
> >>> }
> >>>
> >>> int init(MediaDevice *media);
> >>> - void bufferReady(Buffer *buffer);
> >>> + void bufferReady(Buffer *buffer, const BufferInfo &info);
> >>>
> >>> CameraSensor *sensor_;
> >>> V4L2Subdevice *debayer_;
> >>> @@ -458,11 +458,11 @@ int VimcCameraData::init(MediaDevice *media)
> >>> return 0;
> >>> }
> >>>
> >>> -void VimcCameraData::bufferReady(Buffer *buffer)
> >>> +void VimcCameraData::bufferReady(Buffer *buffer, const BufferInfo &info)
> >>> {
> >>> Request *request = requestFromBuffer(buffer);
> >>>
> >>> - pipe_->completeBuffer(camera_, request, buffer);
> >>> + pipe_->completeBuffer(camera_, request, buffer, info);
> >>> pipe_->completeRequest(camera_, request);
> >>> }
> >>>
> >>> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
> >>> index d70e286661aded8e..9ce9432449e0e133 100644
> >>> --- a/src/libcamera/pipeline_handler.cpp
> >>> +++ b/src/libcamera/pipeline_handler.cpp
> >>> @@ -393,10 +393,11 @@ int PipelineHandler::queueRequest(Camera *camera, Request *request)
> >>> * otherwise
> >>> */
> >>> bool PipelineHandler::completeBuffer(Camera *camera, Request *request,
> >>> - Buffer *buffer)
> >>> + Buffer *buffer, const BufferInfo &info)
> >>> {
> >>> + bool ret = request->completeBuffer(buffer, info);
> >>> camera->bufferCompleted.emit(request, buffer);
> >>> - return request->completeBuffer(buffer);
> >>> + return ret;
> >>> }
> >>>
> >>> /**
> >>> diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp
> >>> index a9468ed4b0512a7f..9a0e439a3d05d780 100644
> >>> --- a/src/libcamera/request.cpp
> >>> +++ b/src/libcamera/request.cpp
> >>> @@ -252,12 +252,14 @@ void Request::complete()
> >>> * \return True if all buffers contained in the request have completed, false
> >>> * otherwise
> >>> */
> >>> -bool Request::completeBuffer(Buffer *buffer)
> >>> +bool Request::completeBuffer(Buffer *buffer, const BufferInfo &info)
> >>> {
> >>> int ret = pending_.erase(buffer);
> >>> ASSERT(ret == 1);
> >>>
> >>> - if (buffer->status() == Buffer::BufferCancelled)
> >>> + info_.emplace(buffer, info);
> >>> +
> >>> + if (info.status() == BufferInfo::BufferCancelled)
> >>> cancelled_ = true;
> >>>
> >>> return !hasPendingBuffers();
> >>> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp
> >>> index 8bc2e439e4faeb68..97c6722b8c4c98cf 100644
> >>> --- a/src/libcamera/v4l2_videodevice.cpp
> >>> +++ b/src/libcamera/v4l2_videodevice.cpp
> >>> @@ -1119,14 +1119,16 @@ std::vector<std::unique_ptr<Buffer>> V4L2VideoDevice::queueAllBuffers()
> >>> }
> >>>
> >>> /**
> >>> - * \brief Dequeue the next available buffer from the video device
> >>> + * \brief Slot to handle completed buffer events from the V4L2 video device
> >>> + * \param[in] notifier The event notifier
> >>> *
> >>> - * This method dequeues the next available buffer from the device. If no buffer
> >>> - * is available to be dequeued it will return nullptr immediately.
> >>> + * When this slot is called, a Buffer has become available from the device, and
> >>> + * will be emitted through the bufferReady Signal.
> >>> *
> >>> - * \return A pointer to the dequeued buffer on success, or nullptr otherwise
> >>> + * For Capture video devices the Buffer will contain valid data.
> >>> + * For Output video devices the Buffer can be considered empty.
> >>> */
> >>> -Buffer *V4L2VideoDevice::dequeueBuffer()
> >>> +void V4L2VideoDevice::bufferAvailable(EventNotifier *notifier)
> >>> {
> >>> struct v4l2_buffer buf = {};
> >>> struct v4l2_plane planes[VIDEO_MAX_PLANES] = {};
> >>> @@ -1144,10 +1146,10 @@ Buffer *V4L2VideoDevice::dequeueBuffer()
> >>> if (ret < 0) {
> >>> LOG(V4L2, Error)
> >>> << "Failed to dequeue buffer: " << strerror(-ret);
> >>> - return nullptr;
> >>> + return;
> >>> }
> >>>
> >>> - ASSERT(buf.index < bufferPool_->count());
> >>> + LOG(V4L2, Debug) << "Buffer " << buf.index << " is available";
> >>>
> >>> auto it = queuedBuffers_.find(buf.index);
> >>> Buffer *buffer = it->second;
> >>> @@ -1156,37 +1158,16 @@ Buffer *V4L2VideoDevice::dequeueBuffer()
> >>> if (queuedBuffers_.empty())
> >>> 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;
> >>> -
> >>> - return buffer;
> >>> -}
> >>> + 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;
> >>>
> >>> -/**
> >>> - * \brief Slot to handle completed buffer events from the V4L2 video device
> >>> - * \param[in] notifier The event notifier
> >>> - *
> >>> - * When this slot is called, a Buffer has become available from the device, and
> >>> - * will be emitted through the bufferReady Signal.
> >>> - *
> >>> - * For Capture video devices the Buffer will contain valid data.
> >>> - * For Output video devices the Buffer can be considered empty.
> >>> - */
> >>> -void V4L2VideoDevice::bufferAvailable(EventNotifier *notifier)
> >>> -{
> >>> - Buffer *buffer = dequeueBuffer();
> >>> - if (!buffer)
> >>> - return;
> >>> -
> >>> - LOG(V4L2, Debug) << "Buffer " << buffer->index() << " is available";
> >>> + BufferInfo info(status, buf.sequence, timestamp, { { buf.bytesused } });
> >>> + buffer->index_ = buf.index;
> >>>
> >>> /* Notify anyone listening to the device. */
> >>> - bufferReady.emit(buffer);
> >>> + bufferReady.emit(buffer, info);
> >>> }
> >>>
> >>> /**
> >>> @@ -1238,9 +1219,11 @@ int V4L2VideoDevice::streamOff()
> >>> unsigned int index = it.first;
> >>> Buffer *buffer = it.second;
> >>>
> >>> + BufferInfo info(BufferInfo::BufferCancelled, 0, 0, { {} });
> >>> +
> >>> buffer->index_ = index;
> >>> buffer->cancel();
> >>> - bufferReady.emit(buffer);
> >>> + bufferReady.emit(buffer, info);
> >>> }
> >>>
> >>> queuedBuffers_.clear();
> >>> diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp
> >>> index cca7365ae75687f9..2d7f4ba84fbb6838 100644
> >>> --- a/src/qcam/main_window.cpp
> >>> +++ b/src/qcam/main_window.cpp
> >>> @@ -258,19 +258,19 @@ void MainWindow::requestComplete(Request *request)
> >>> framesCaptured_++;
> >>>
> >>> Buffer *buffer = buffers.begin()->second;
> >>> + const BufferInfo &info = request->info(buffer);
> >>>
> >>> - 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()
> >>> - << " buf: " << buffer->index()
> >>> - << " bytesused: " << buffer->bytesused()
> >>> - << " timestamp: " << buffer->timestamp()
> >>> + std::cout << "seq: " << std::setw(6) << std::setfill('0') << info.sequence()
> >>> + << " bytesused: " << info.plane(0).bytesused
> >>> + << " timestamp: " << info.timestamp()
> >>> << " fps: " << std::fixed << std::setprecision(2) << fps
> >>> << std::endl;
> >>>
> >>> - display(buffer);
> >>> + display(buffer, info);
> >>>
> >>> request = camera_->createRequest();
> >>> if (!request) {
> >>> @@ -295,7 +295,7 @@ void MainWindow::requestComplete(Request *request)
> >>> camera_->queueRequest(request);
> >>> }
> >>>
> >>> -int MainWindow::display(Buffer *buffer)
> >>> +int MainWindow::display(Buffer *buffer, const BufferInfo &info)
> >>> {
> >>> BufferMemory *mem = buffer->mem();
> >>> if (mem->planes().size() != 1)
> >>> @@ -303,7 +303,7 @@ int MainWindow::display(Buffer *buffer)
> >>>
> >>> Plane &plane = mem->planes().front();
> >>> unsigned char *raw = static_cast<unsigned char *>(plane.mem());
> >>> - viewfinder_->display(raw, buffer->bytesused());
> >>> + viewfinder_->display(raw, info.plane(0).bytesused);
> >>>
> >>> return 0;
> >>> }
> >>> diff --git a/src/qcam/main_window.h b/src/qcam/main_window.h
> >>> index 78511581a8d5c025..8cbd72eb0d63cbea 100644
> >>> --- a/src/qcam/main_window.h
> >>> +++ b/src/qcam/main_window.h
> >>> @@ -50,7 +50,7 @@ private:
> >>> void stopCapture();
> >>>
> >>> void requestComplete(Request *request);
> >>> - int display(Buffer *buffer);
> >>> + int display(Buffer *frame, const BufferInfo &info);
> >>>
> >>> QString title_;
> >>> QTimer titleTimer_;
> >>> diff --git a/test/camera/capture.cpp b/test/camera/capture.cpp
> >>> index 83f974749affd3cd..3cf5b798448d01db 100644
> >>> --- a/test/camera/capture.cpp
> >>> +++ b/test/camera/capture.cpp
> >>> @@ -21,7 +21,9 @@ protected:
> >>>
> >>> void bufferComplete(Request *request, Buffer *buffer)
> >>> {
> >>> - if (buffer->status() != Buffer::BufferSuccess)
> >>> + const BufferInfo &info = request->info(buffer);
> >>> +
> >>> + if (info.status() != BufferInfo::BufferSuccess)
> >>> return;
> >>>
> >>> completeBuffersCount_++;
> >>> diff --git a/test/v4l2_videodevice/buffer_sharing.cpp b/test/v4l2_videodevice/buffer_sharing.cpp
> >>> index 1629f34cfa6c79fe..3609c331cf10e056 100644
> >>> --- a/test/v4l2_videodevice/buffer_sharing.cpp
> >>> +++ b/test/v4l2_videodevice/buffer_sharing.cpp
> >>> @@ -90,24 +90,25 @@ protected:
> >>> return 0;
> >>> }
> >>>
> >>> - void captureBufferReady(Buffer *buffer)
> >>> + void captureBufferReady(Buffer *buffer, const BufferInfo &info)
> >>> {
> >>> - std::cout << "Received capture buffer: " << buffer->index()
> >>> - << " sequence " << buffer->sequence() << std::endl;
> >>> + std::cout << "Received capture buffer sequence "
> >>> + << info.sequence() << std::endl;
> >>>
> >>> - if (buffer->status() != Buffer::BufferSuccess)
> >>> + if (info.status() != BufferInfo::BufferSuccess)
> >>> return;
> >>>
> >>> + BufferInfo infocopy = info;
> >>> output_->queueBuffer(buffer);
> >>> framesCaptured_++;
> >>> }
> >>>
> >>> - void outputBufferReady(Buffer *buffer)
> >>> + void outputBufferReady(Buffer *buffer, const BufferInfo &info)
> >>> {
> >>> - std::cout << "Received output buffer: " << buffer->index()
> >>> - << " sequence " << buffer->sequence() << std::endl;
> >>> + std::cout << "Received output buffer sequence "
> >>> + << info.sequence() << std::endl;
> >>>
> >>> - if (buffer->status() != Buffer::BufferSuccess)
> >>> + if (info.status() != BufferInfo::BufferSuccess)
> >>> return;
> >>>
> >>> capture_->queueBuffer(buffer);
> >>> diff --git a/test/v4l2_videodevice/capture_async.cpp b/test/v4l2_videodevice/capture_async.cpp
> >>> index 442a4fe56eace57e..26ea1d17fd901ef8 100644
> >>> --- a/test/v4l2_videodevice/capture_async.cpp
> >>> +++ b/test/v4l2_videodevice/capture_async.cpp
> >>> @@ -20,9 +20,9 @@ public:
> >>> CaptureAsyncTest()
> >>> : V4L2VideoDeviceTest("vimc", "Raw Capture 0"), frames(0) {}
> >>>
> >>> - void receiveBuffer(Buffer *buffer)
> >>> + void receiveBuffer(Buffer *buffer, const BufferInfo &info)
> >>> {
> >>> - 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..e6ca90a4604dfcda 100644
> >>> --- a/test/v4l2_videodevice/v4l2_m2mdevice.cpp
> >>> +++ b/test/v4l2_videodevice/v4l2_m2mdevice.cpp
> >>> @@ -29,9 +29,9 @@ public:
> >>> {
> >>> }
> >>>
> >>> - void outputBufferComplete(Buffer *buffer)
> >>> + void outputBufferComplete(Buffer *buffer, const BufferInfo &info)
> >>> {
> >>> - cout << "Received output buffer " << buffer->index() << endl;
> >>> + cout << "Received output buffer" << endl;
> >>>
> >>> outputFrames_++;
> >>>
> >>> @@ -39,9 +39,9 @@ public:
> >>> vim2m_->output()->queueBuffer(buffer);
> >>> }
> >>>
> >>> - void receiveCaptureBuffer(Buffer *buffer)
> >>> + void receiveCaptureBuffer(Buffer *buffer, const BufferInfo &info)
> >>> {
> >>> - cout << "Received capture buffer " << buffer->index() << endl;
> >>> + cout << "Received capture buffer" << endl;
> >>
> >> I actually liked seeing the indexes in my output - so I can see which
> >> buffers are in use... I guess we no longer have anything that maps to
> >> the index though...
> >>
> >> Perhaps at least making that info->sequence() would be better to keep
> >> some sort of a view of active data processing.
> >>
> >>> captureFrames_++;
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list