[libcamera-devel] [RFC 10/12] libcamera: buffer: Store buffer information in separate container
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Mon Nov 18 21:22:54 CET 2019
Hi Niklas,
Thank you for the patch.
On Wed, Nov 06, 2019 at 11:18:30AM +0100, 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.
>
> 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.
BufferInfo holds data directly, a move constructor or assignment
operator won't help much. Data will still be copied, perhaps with the
exception of the PlaneInfo vector, but that's very small anyway.
> Or we could use the BufferInfo as temporary instances. At
We shouldn't make BufferInfo temporary, as we shouldn't allocate any
memory during the streaming cycle. I would thus either store the
BufferInfo inside Buffer, or store it in a separate container that is
allocated at stream start (which may be what this patch is doing).
> 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)
std::vector has a move constructor, so I don't think it would block
anything.
> > > 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; };
The additional complexity is a bit of a shame :-( I wonder if this
calls for storing BufferInfo in Buffer.
> > > 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);
It would also help keeping this simpler.
> > >
> > > 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();
This isn't nice :-( It would be better if pipeline handler didn't have
to dequeue buffers synchronously with the bufferReady signal, to make
the API more flexible for them.
> > > 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 we want to store BufferInfo in a separate container, it should be
allocated at stream start time (or, possibly, the first time a Buffer is
queued), not at every completion (or queue operation). This seems to
call for storing BufferInfo in Buffer.
> > > +
> > > + 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