[libcamera-devel] [PATCH v4 6/9] libcamera: pipeline: raspberrypi: Rework stream buffer logic for zero-copy
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Wed Jul 22 17:25:13 CEST 2020
Hi Naush,
Thank you for the patch.
On Wed, Jul 22, 2020 at 01:21:33PM +0100, Kieran Bingham wrote:
> On 22/07/2020 09:04, Naushir Patuck wrote:
> > On Tue, 21 Jul 2020 at 16:41, Kieran Bingham wrote:
> >> On 20/07/2020 10:13, Naushir Patuck wrote:
> >>> Stop using v4l2_videodevice::allocateBuffer() for internal buffers and
> >>> instead export/import all buffers. This allows the pipeline to return
> >>> any stream buffer requested by the application as zero-copy.
> >>>
> >>> Advertise the Unicam Image stream as the RAW capture stream now.
> >>>
> >>> The RPiStream object now maintains a new list of buffers that are
> >>> available to queue into a device. This is needed to distinguish between
> >>> FrameBuffers allocated for internal use vs externally provided buffers.
> >>> When a Request comes in, if a buffer is not provided for an exported
> >>> stream, we re-use a buffer from this list.
> >>>
> >>> Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> >>> Reviewed-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> >>
> >> A few small nit fixups below but they could possibly be fixed while
> >> applying anyway if you prefer.
> >>
> >> I've hit review fatigue though, so I'll look at this again tomorrow.
> >
> > I understand :)
> > I will make the necessary fixups based on your review comments, and
> > submit a new patch set. Will wait until you have finished reviewing
> > this one before I send the next revision.
>
> Looking good, I think the only actionable thing remaining here is
> potentially to use ASSERT() over assert() :-)
>
> >>> ---
> >>> .../pipeline/raspberrypi/raspberrypi.cpp | 228 ++++++++++--------
> >>> .../pipeline/raspberrypi/rpi_stream.cpp | 122 +++++++---
> >>> .../pipeline/raspberrypi/rpi_stream.h | 30 ++-
> >>> 3 files changed, 226 insertions(+), 154 deletions(-)
> >>>
> >>> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> >>> index 8f6a999b..dbc22521 100644
> >>> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> >>> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> >>> @@ -187,7 +187,8 @@ private:
> >>> void checkRequestCompleted();
> >>> void tryRunPipeline();
> >>> void tryFlushQueues();
> >>> - FrameBuffer *updateQueue(std::queue<FrameBuffer *> &q, uint64_t timestamp, V4L2VideoDevice *dev);
> >>> + FrameBuffer *updateQueue(std::queue<FrameBuffer *> &q, uint64_t timestamp,
> >>> + RPi::RPiStream *stream);
> >>>
> >>> unsigned int ispOutputCount_;
> >>> };
> >>> @@ -508,8 +509,15 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)
> >>> StreamConfiguration &cfg = config->at(i);
> >>>
> >>> if (isRaw(cfg.pixelFormat)) {
> >>> - cfg.setStream(&data->isp_[Isp::Input]);
> >>> - data->isp_[Isp::Input].setExternal(true);
> >>> + cfg.setStream(&data->unicam_[Unicam::Image]);
> >>> + /*
> >>> + * We must set both Unicam streams as external, even
> >>> + * though the application may only request RAW frames.
> >>> + * This is because we match timestamps on both streams
> >>> + * to synchronise buffers.
> >>> + */
> >>> + data->unicam_[Unicam::Image].setExternal(true);
> >>> + data->unicam_[Unicam::Embedded].setExternal(true);
> >>> continue;
> >>> }
> >>>
> >>> @@ -612,7 +620,7 @@ int PipelineHandlerRPi::exportFrameBuffers(Camera *camera, Stream *stream,
> >>> unsigned int count = stream->configuration().bufferCount;
> >>> int ret = s->dev()->exportBuffers(count, buffers);
> >>>
> >>> - s->setExternalBuffers(buffers);
> >>> + s->setExportedBuffers(buffers);
> >>>
> >>> return ret;
> >>> }
> >>> @@ -712,14 +720,23 @@ int PipelineHandlerRPi::queueRequestDevice(Camera *camera, Request *request)
> >>> if (data->state_ == RPiCameraData::State::Stopped)
> >>> return -EINVAL;
> >>>
> >>> - /* Ensure all external streams have associated buffers! */
> >>> - for (auto &stream : data->isp_) {
> >>> - if (!stream.isExternal())
> >>> - continue;
> >>> + LOG(RPI, Debug) << "queueRequestDevice: New request.";
> >>>
> >>> - if (!request->findBuffer(&stream)) {
> >>> - LOG(RPI, Error) << "Attempt to queue request with invalid stream.";
> >>> - return -ENOENT;
> >>> + /* Push all buffers supplied in the Request to the respective streams. */
> >>> + for (auto stream : data->streams_) {
> >>> + if (stream->isExternal()) {
> >>
> >> Are all streams now 'external' ?
> >
> > All streams but the ISP input (which only imports buffers) *can* be
> > external. They are marked external only if the app configures the
> > pipeline handler by saying it may request a buffer from the stream.
> > In reality, all streams could be marked external all the time, but the
> > buffer handling will take a slightly less efficient path, so I kept
> > the distinction.
> >
> >>> + FrameBuffer *buffer = request->findBuffer(stream);
> >>> + /*
> >>> + * A nullptr in buffer means that we should queue an internally
> >>> + * allocated buffer.
>
> Now I understand what's going on here, perhaps this could be expanded a
> bit to explain?
>
> "If no buffer is provided by the request for this stream, we queue a
> nullptr to the stream to signify that it must use an internally
> allocated buffer for this capture request which will not be given back
> to the application, but can be used to support the internal pipeline flow."
>
> Now I see how this leads towards zero-copy raw support ;-)
>
> >>> + *
> >>> + * The below queueBuffer() call will do nothing if there are not
> >>> + * enough internal buffers allocated, but this will be handled by
> >>> + * queuing the request for buffers in the RPiStream object.
> >>> + */
> >>> + int ret = stream->queueBuffer(buffer);
> >>> + if (ret)
> >>> + return ret;
> >>> }
> >>> }
> >>>
> >>> @@ -808,12 +825,10 @@ bool PipelineHandlerRPi::match(DeviceEnumerator *enumerator)
> >>> /* Initialize the camera properties. */
> >>> data->properties_ = data->sensor_->properties();
> >>>
> >>> - /*
> >>> - * List the available output streams.
> >>> - * Currently cannot do Unicam streams!
> >>> - */
> >>> + /*List the available streams an application may request. */
> >>
> >> Minor nit, there's a space missing between /* and List ...
> >>
> >>> std::set<Stream *> streams;
> >>> - streams.insert(&data->isp_[Isp::Input]);
> >>> + streams.insert(&data->unicam_[Unicam::Image]);
> >>> + streams.insert(&data->unicam_[Unicam::Embedded]);
Can an application request the embedded data ?
> >>> streams.insert(&data->isp_[Isp::Output0]);
> >>> streams.insert(&data->isp_[Isp::Output1]);
> >>> streams.insert(&data->isp_[Isp::Stats]);
> >>> @@ -831,9 +846,28 @@ int PipelineHandlerRPi::queueAllBuffers(Camera *camera)
> >>> int ret;
> >>>
> >>> for (auto const stream : data->streams_) {
> >>> - ret = stream->queueBuffers();
> >>> - if (ret < 0)
> >>> - return ret;
> >>> + if (!stream->isExternal()) {
> >>> + ret = stream->queueAllBuffers();
> >>> + if (ret < 0)
> >>> + return ret;
> >>> + } else {
> >>> + /*
> >>> + * For external streams, we must queue up a set of internal
> >>> + * buffers to handle the number of drop frames requested by
> >>> + * the IPA. This is done by passing nullptr in queueBuffer().
> >>> + *
> >>> + * The below queueBuffer() call will do nothing if there
> >>> + * are not enough internal buffers allocated, but this will
> >>> + * be handled by queuing the request for buffers in the
> >>> + * RPiStream object.
> >>> + */
> >>> + unsigned int i;
> >>> + for (i = 0; i < data->dropFrameCount_; i++) {
> >>> + int ret = stream->queueBuffer(nullptr);
> >>> + if (ret)
> >>> + return ret;
> >>> + }
> >>> + }
> >>> }
> >>>
> >>> return 0;
> >>> @@ -847,7 +881,8 @@ int PipelineHandlerRPi::prepareBuffers(Camera *camera)
> >>> /*
> >>> * Decide how many internal buffers to allocate. For now, simply look
> >>> * at how many external buffers will be provided. Will need to improve
> >>> - * this logic.
> >>> + * this logic. However, we really must have all stream allocate the same
> >>
> >> s/stream/streams/
> >>
> >>> + * number of buffers to simplify error handling in queueRequestDevice().
> >>> */
> >>
> >> Does this include Raw streams? I thought that allocates less buffers? Or
> >> perhaps it's the same number of internal buffers, and only 2 extra
> >> external buffers for that case...
> >
> > That's correct. For now all (including Raw) streams allocate the same
> > number of internal buffers. Hence the comment that we could improve
> > this logic, it is a bit wasteful on memory. This bit does need
> > refinement at some point.
> >
> >>> unsigned int maxBuffers = 0;
> >>> for (const Stream *s : camera->streams())
> >>> @@ -855,33 +890,9 @@ int PipelineHandlerRPi::prepareBuffers(Camera *camera)
> >>> maxBuffers = std::max(maxBuffers, s->configuration().bufferCount);
> >>>
> >>> for (auto const stream : data->streams_) {
> >>> - if (stream->isExternal() || stream->isImporter()) {
> >>> - /*
> >>> - * If a stream is marked as external reserve memory to
> >>> - * prepare to import as many buffers are requested in
> >>> - * the stream configuration.
> >>> - *
> >>> - * If a stream is an internal stream with importer
> >>> - * role, reserve as many buffers as possible.
> >>> - */
> >>> - unsigned int count = stream->isExternal()
> >>> - ? stream->configuration().bufferCount
> >>> - : maxBuffers;
> >>> - ret = stream->importBuffers(count);
> >>> - if (ret < 0)
> >>> - return ret;
> >>> - } else {
> >>> - /*
> >>> - * If the stream is an internal exporter allocate and
> >>> - * export as many buffers as possible to its internal
> >>> - * pool.
> >>> - */
> >>> - ret = stream->allocateBuffers(maxBuffers);
> >>> - if (ret < 0) {
> >>> - freeBuffers(camera);
> >>> - return ret;
> >>> - }
> >>> - }
> >>> + ret = stream->prepareBuffers(maxBuffers);
> >>> + if (ret < 0)
> >>> + return ret;
This doesn't free buffers anymore on failure, is that an oversight ?
> >>> }
> >>>
> >>> /*
> >>> @@ -889,7 +900,7 @@ int PipelineHandlerRPi::prepareBuffers(Camera *camera)
> >>> * the IPA and RPI_IPA_EVENT_SIGNAL_ISP_PREPARE event.
> >>> */
> >>> count = 0;
> >>> - for (auto const &b : *data->unicam_[Unicam::Image].getBuffers()) {
> >>> + for (auto const &b : data->unicam_[Unicam::Image].getBuffers()) {
> >>> b->setCookie(count++);
> >>> }
> >>>
> >>> @@ -898,14 +909,14 @@ int PipelineHandlerRPi::prepareBuffers(Camera *camera)
> >>> * the IPA.
> >>> */
> >>> count = 0;
> >>> - for (auto const &b : *data->isp_[Isp::Stats].getBuffers()) {
> >>> + for (auto const &b : data->isp_[Isp::Stats].getBuffers()) {
> >>> b->setCookie(count++);
> >>> data->ipaBuffers_.push_back({ .id = RPiIpaMask::STATS | b->cookie(),
> >>> .planes = b->planes() });
> >>> }
> >>>
> >>> count = 0;
> >>> - for (auto const &b : *data->unicam_[Unicam::Embedded].getBuffers()) {
> >>> + for (auto const &b : data->unicam_[Unicam::Embedded].getBuffers()) {
> >>> b->setCookie(count++);
> >>> data->ipaBuffers_.push_back({ .id = RPiIpaMask::EMBEDDED_DATA | b->cookie(),
> >>> .planes = b->planes() });
> >>> @@ -1066,7 +1077,7 @@ void RPiCameraData::queueFrameAction(unsigned int frame, const IPAOperationData
> >>> switch (action.operation) {
> >>> case RPI_IPA_ACTION_STATS_METADATA_COMPLETE: {
> >>> unsigned int bufferId = action.data[0];
> >>> - FrameBuffer *buffer = isp_[Isp::Stats].getBuffers()->at(bufferId).get();
> >>> + FrameBuffer *buffer = isp_[Isp::Stats].getBuffers().at(bufferId);
> >>>
> >>> handleStreamBuffer(buffer, &isp_[Isp::Stats]);
> >>> /* Fill the Request metadata buffer with what the IPA has provided */
> >>> @@ -1077,19 +1088,19 @@ void RPiCameraData::queueFrameAction(unsigned int frame, const IPAOperationData
> >>>
> >>> case RPI_IPA_ACTION_EMBEDDED_COMPLETE: {
> >>> unsigned int bufferId = action.data[0];
> >>> - FrameBuffer *buffer = unicam_[Unicam::Embedded].getBuffers()->at(bufferId).get();
> >>> + FrameBuffer *buffer = unicam_[Unicam::Embedded].getBuffers().at(bufferId);
> >>> handleStreamBuffer(buffer, &unicam_[Unicam::Embedded]);
> >>> break;
> >>> }
> >>>
> >>> case RPI_IPA_ACTION_RUN_ISP: {
> >>> unsigned int bufferId = action.data[0];
> >>> - FrameBuffer *buffer = unicam_[Unicam::Image].getBuffers()->at(bufferId).get();
> >>> + FrameBuffer *buffer = unicam_[Unicam::Image].getBuffers().at(bufferId);
> >>>
> >>> LOG(RPI, Debug) << "Input re-queue to ISP, buffer id " << buffer->cookie()
> >>> << ", timestamp: " << buffer->metadata().timestamp;
> >>>
> >>> - isp_[Isp::Input].dev()->queueBuffer(buffer);
> >>> + isp_[Isp::Input].queueBuffer(buffer);
> >>> ispOutputCount_ = 0;
> >>> break;
> >>> }
> >>> @@ -1190,22 +1201,27 @@ void RPiCameraData::ispOutputDequeue(FrameBuffer *buffer)
> >>> << ", buffer id " << buffer->cookie()
> >>> << ", timestamp: " << buffer->metadata().timestamp;
> >>>
> >>> - handleStreamBuffer(buffer, stream);
> >>> -
> >>> /*
> >>> - * Increment the number of ISP outputs generated.
> >>> - * This is needed to track dropped frames.
> >>> + * ISP statistics buffer must not be re-queued or sent back to the
> >>> + * application until after the IPA signals so.
> >>> */
> >>> - ispOutputCount_++;
> >>> -
> >>> - /* If this is a stats output, hand it to the IPA now. */
> >>> if (stream == &isp_[Isp::Stats]) {
> >>> IPAOperationData op;
> >>> op.operation = RPI_IPA_EVENT_SIGNAL_STAT_READY;
> >>> op.data = { RPiIpaMask::STATS | buffer->cookie() };
> >>> ipa_->processEvent(op);
> >>> + } else {
> >>> + /* Any other ISP output can be handed back to the application now. */
> >>> + handleStreamBuffer(buffer, stream);
> >>> }
> >>>
> >>> + /*
> >>> + * Increment the number of ISP outputs generated.
> >>> + * This is needed to track dropped frames.
> >>> + */
> >>> + ispOutputCount_++;
> >>> +
> >>> +
Extra blank line ?
> >>> handleState();
> >>> }
> >>>
> >>> @@ -1218,8 +1234,12 @@ void RPiCameraData::clearIncompleteRequests()
> >>> */
> >>> for (auto const request : requestQueue_) {
> >>> for (auto const stream : streams_) {
> >>> - if (stream->isExternal())
> >>> - stream->dev()->queueBuffer(request->findBuffer(stream));
> >>> + if (!stream->isExternal())
> >>> + continue;
> >>> +
> >>> + FrameBuffer *buffer = request->findBuffer(stream);
> >>> + if (buffer)
> >>> + stream->queueBuffer(buffer);
> >>> }
> >>> }
> >>>
> >>> @@ -1248,7 +1268,7 @@ void RPiCameraData::clearIncompleteRequests()
> >>> * Has the buffer already been handed back to the
> >>> * request? If not, do so now.
> >>> */
> >>> - if (buffer->request())
> >>> + if (buffer && buffer->request())
> >>> pipe_->completeBuffer(camera_, request, buffer);
> >>> }
> >>>
> >>> @@ -1260,30 +1280,24 @@ void RPiCameraData::clearIncompleteRequests()
> >>> void RPiCameraData::handleStreamBuffer(FrameBuffer *buffer, RPi::RPiStream *stream)
> >>> {
> >>> if (stream->isExternal()) {
> >>> - if (!dropFrameCount_) {
> >>> - Request *request = buffer->request();
> >>> + Request *request = requestQueue_.front();
> >>> +
> >>> + if (!dropFrameCount_ && request->findBuffer(stream) == buffer) {
> >>> + /*
> >>> + * Tag the buffer as completed, returning it to the
> >>> + * application.
> >>> + */
> >>> pipe_->completeBuffer(camera_, request, buffer);
> >>> + } else {
> >>> + /*
> >>> + * This buffer was not part of the Request, so we can
> >>> + * recycle it.
> >>> + */
> >>> + stream->returnBuffer(buffer);
> >>> }
> >>> } else {
> >>> - /* Special handling for RAW buffer Requests.
> >>> - *
> >>> - * The ISP input stream is alway an import stream, but if the
> >>> - * current Request has been made for a buffer on the stream,
> >>> - * simply memcpy to the Request buffer and requeue back to the
> >>> - * device.
> >>> - */
> >>> - if (stream == &unicam_[Unicam::Image] && !dropFrameCount_) {
> >>> - const Stream *rawStream = static_cast<const Stream *>(&isp_[Isp::Input]);
> >>> - Request *request = requestQueue_.front();
> >>> - FrameBuffer *raw = request->findBuffer(const_cast<Stream *>(rawStream));
> >>> - if (raw) {
> >>> - raw->copyFrom(buffer);
> >>> - pipe_->completeBuffer(camera_, request, raw);
> >>> - }
> >>> - }
> >>> -
> >>> - /* Simply requeue the buffer. */
> >>> - stream->dev()->queueBuffer(buffer);
> >>> + /* Simply re-queue the buffer to the requested stream. */
> >>> + stream->queueBuffer(buffer);
> >>> }
> >>> }
> >>>
> >>> @@ -1367,7 +1381,7 @@ void RPiCameraData::tryRunPipeline()
> >>> * current bayer buffer will be removed and re-queued to the driver.
> >>> */
> >>> embeddedBuffer = updateQueue(embeddedQueue_, bayerBuffer->metadata().timestamp,
> >>> - unicam_[Unicam::Embedded].dev());
> >>> + &unicam_[Unicam::Embedded]);
> >>>
> >>> if (!embeddedBuffer) {
> >>> LOG(RPI, Debug) << "Could not find matching embedded buffer";
> >>> @@ -1386,7 +1400,7 @@ void RPiCameraData::tryRunPipeline()
> >>>
> >>> embeddedBuffer = embeddedQueue_.front();
> >>> bayerBuffer = updateQueue(bayerQueue_, embeddedBuffer->metadata().timestamp,
> >>> - unicam_[Unicam::Image].dev());
> >>> + &unicam_[Unicam::Image]);
> >>>
> >>> if (!bayerBuffer) {
> >>> LOG(RPI, Debug) << "Could not find matching bayer buffer - ending.";
> >>> @@ -1394,11 +1408,7 @@ void RPiCameraData::tryRunPipeline()
> >>> }
> >>> }
> >>>
> >>> - /*
> >>> - * Take the first request from the queue and action the IPA.
> >>> - * Unicam buffers for the request have already been queued as they come
> >>> - * in.
> >>> - */
> >>> + /* Take the first request from the queue and action the IPA. */
> >>> Request *request = requestQueue_.front();
> >>>
> >>> /*
> >>> @@ -1410,12 +1420,6 @@ void RPiCameraData::tryRunPipeline()
> >>> op.controls = { request->controls() };
> >>> ipa_->processEvent(op);
> >>>
> >>> - /* Queue up any ISP buffers passed into the request. */
> >>> - for (auto &stream : isp_) {
> >>> - if (stream.isExternal())
> >>> - stream.dev()->queueBuffer(request->findBuffer(&stream));
> >>> - }
> >>> -
> >>> /* Ready to use the buffers, pop them off the queue. */
> >>> bayerQueue_.pop();
> >>> embeddedQueue_.pop();
> >>> @@ -1445,32 +1449,42 @@ void RPiCameraData::tryFlushQueues()
> >>> * and give a chance for the hardware to return to lock-step. We do have
> >>> * to drop all interim frames.
> >>> */
> >>> - if (unicam_[Unicam::Image].getBuffers()->size() == bayerQueue_.size() &&
> >>> - unicam_[Unicam::Embedded].getBuffers()->size() == embeddedQueue_.size()) {
> >>> + if (unicam_[Unicam::Image].getBuffers().size() == bayerQueue_.size() &&
> >>> + unicam_[Unicam::Embedded].getBuffers().size() == embeddedQueue_.size()) {
> >>> + /* This cannot happen when Unicam streams are external. */
> >>> + assert(!unicam_[Unicam::Image].isExternal());
> >>> +
> >>> LOG(RPI, Warning) << "Flushing all buffer queues!";
> >>>
> >>> while (!bayerQueue_.empty()) {
> >>> - unicam_[Unicam::Image].dev()->queueBuffer(bayerQueue_.front());
> >>> + unicam_[Unicam::Image].queueBuffer(bayerQueue_.front());
> >>> bayerQueue_.pop();
> >>> }
> >>>
> >>> while (!embeddedQueue_.empty()) {
> >>> - unicam_[Unicam::Embedded].dev()->queueBuffer(embeddedQueue_.front());
> >>> + unicam_[Unicam::Embedded].queueBuffer(embeddedQueue_.front());
> >>> embeddedQueue_.pop();
> >>> }
> >>> }
> >>> }
> >>>
> >>> FrameBuffer *RPiCameraData::updateQueue(std::queue<FrameBuffer *> &q, uint64_t timestamp,
> >>> - V4L2VideoDevice *dev)
> >>> + RPi::RPiStream *stream)
> >>> {
> >>> + /*
> >>> + * If the unicam streams are external (both have to the same), then we
> >>
> >> both have to 'be' the same?
> >>
> >>> + * can only return out the top buffer in the queue, and assume they have
> >>> + * been synced by queuing at the same time. We cannot drop these frames,
> >>> + * as they may have been provided externally.
> >>> + */
> >>> while (!q.empty()) {
> >>> FrameBuffer *b = q.front();
> >>> - if (b->metadata().timestamp < timestamp) {
> >>> + if (!stream->isExternal() && b->metadata().timestamp < timestamp) {
> >>> q.pop();
> >>> - dev->queueBuffer(b);
> >>> - LOG(RPI, Warning) << "Dropping input frame!";
> >>> - } else if (b->metadata().timestamp == timestamp) {
> >>> + stream->queueBuffer(b);
> >>> + LOG(RPI, Warning) << "Dropping unmatched input frame in stream "
> >>> + << stream->name();
> >>> + } else if (stream->isExternal() || b->metadata().timestamp == timestamp) {
> >>> /* The calling function will pop the item from the queue. */
> >>> return b;
> >>> } else {
> >>> diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp
> >>> index 2edb8b59..02f8d3e0 100644
> >>> --- a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp
> >>> +++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp
> >>> @@ -21,30 +21,20 @@ V4L2VideoDevice *RPiStream::dev() const
> >>>
> >>> void RPiStream::setExternal(bool external)
> >>> {
> >>> + /* Import streams cannot be external. */
> >>> + assert(!external || !importOnly_);
> >>> external_ = external;
> >>> }
> >>>
> >>> bool RPiStream::isExternal() const
> >>> {
> >>> - /*
> >>> - * Import streams cannot be external.
> >>> - *
> >>> - * RAW capture is a special case where we simply copy the RAW
> >>> - * buffer out of the request. All other buffer handling happens
> >>> - * as if the stream is internal.
> >>> - */
> >>> - return external_ && !importOnly_;
> >>> -}
> >>> -
> >>> -bool RPiStream::isImporter() const
> >>> -{
> >>> - return importOnly_;
> >>> + return external_;
> >>> }
> >>>
> >>> void RPiStream::reset()
> >>> {
> >>> external_ = false;
> >>> - internalBuffers_.clear();
> >>> + clearBuffers();
> >>> }
> >>>
> >>> std::string RPiStream::name() const
> >>> @@ -52,65 +42,123 @@ std::string RPiStream::name() const
> >>> return name_;
> >>> }
> >>>
> >>> -void RPiStream::setExternalBuffers(std::vector<std::unique_ptr<FrameBuffer>> *buffers)
> >>> +void RPiStream::setExportedBuffers(std::vector<std::unique_ptr<FrameBuffer>> *buffers)
> >>> {
> >>> - externalBuffers_ = buffers;
> >>> + std::transform(buffers->begin(), buffers->end(), std::back_inserter(bufferList_),
> >>> + [](std::unique_ptr<FrameBuffer> &b) { return b.get(); });
> >>> }
> >>>
> >>> -const std::vector<std::unique_ptr<FrameBuffer>> *RPiStream::getBuffers() const
> >>> +const std::vector<FrameBuffer *> &RPiStream::getBuffers() const
> >>> {
> >>> - return external_ ? externalBuffers_ : &internalBuffers_;
> >>> + return bufferList_;
> >>> }
> >>>
> >>> void RPiStream::releaseBuffers()
> >>> {
> >>> dev_->releaseBuffers();
> >>> - if (!external_ && !importOnly_)
> >>> - internalBuffers_.clear();
> >>> + clearBuffers();
> >>> }
> >>>
> >>> -int RPiStream::importBuffers(unsigned int count)
> >>> +int RPiStream::prepareBuffers(unsigned int count)
> >>> {
> >>> + int ret;
> >>> +
> >>> + if (!importOnly_) {
> >>> + if (count) {
> >>> + /* Export some frame buffers for internal use. */
> >>> + ret = dev_->exportBuffers(count, &internalBuffers_);
> >>> + if (ret < 0)
> >>> + return ret;
> >>> +
> >>> + /* Add these exported buffers to the internal/external buffer list. */
> >>> + setExportedBuffers(&internalBuffers_);
> >>> +
> >>> + /* Add these buffers to the queue of internal usable buffers. */
> >>> + for (auto const &buffer : internalBuffers_)
> >>> + availableBuffers_.push(buffer.get());
> >>> + }
> >>> +
> >>> + /* We must import all internal/external exported buffers. */
> >>> + count = bufferList_.size();
> >>> + }
> >>> +
> >>> return dev_->importBuffers(count);
> >>> }
> >>>
> >>> -int RPiStream::allocateBuffers(unsigned int count)
> >>> +int RPiStream::queueAllBuffers()
> >>> {
> >>> - return dev_->allocateBuffers(count, &internalBuffers_);
> >>> -}
> >>> + int ret;
> >>>
> >>> -int RPiStream::queueBuffers()
> >>> -{
> >>> if (external_)
> >>> return 0;
> >>>
> >>> - for (auto &b : internalBuffers_) {
> >>> - int ret = dev_->queueBuffer(b.get());
> >>> - if (ret) {
> >>> - LOG(RPISTREAM, Error) << "Failed to queue buffers for "
> >>> - << name_;
> >>> + while (!availableBuffers_.empty()) {
> >>> + ret = queueBuffer(availableBuffers_.front());
> >>> + if (ret < 0)
> >>> return ret;
> >>> - }
> >>> +
> >>> + availableBuffers_.pop();
> >>> }
> >>>
> >>> return 0;
> >>> }
> >>>
> >>> -bool RPiStream::findFrameBuffer(FrameBuffer *buffer) const
> >>> +int RPiStream::queueBuffer(FrameBuffer *buffer)
> >>> +{
> >>> + /*
> >>> + * A nullptr buffer implies an external stream, but no external
> >>> + * buffer has been supplied. So, pick one from the availableBuffers_
> >>> + * queue.
>
> Aha, now I see this here, maybe the comment addition I suggested above
> is possibly redundant, as it's explained here ...
>
> >>> + */
> >>> + if (!buffer) {
> >>> + if (availableBuffers_.empty()) {
> >>> + LOG(RPISTREAM, Warning) << "No buffers available for "
> >>> + << name_;
> >>> + return -EINVAL;
> >>> + }
> >>> +
> >>> + buffer = availableBuffers_.front();
> >>> + availableBuffers_.pop();
> >>> + }
> >>> +
> >>> + LOG(RPISTREAM, Debug) << "Queuing buffer " << buffer->cookie()
> >>> + << " for " << name_;
> >>> +
> >>> + int ret = dev_->queueBuffer(buffer);
> >>> + if (ret) {
> >>> + LOG(RPISTREAM, Error) << "Failed to queue buffer for "
> >>> + << name_;
> >>> + }
> >>> +
> >>> + return ret;
> >>> +}
> >>> +
> >>> +void RPiStream::returnBuffer(FrameBuffer *buffer)
> >>> {
> >>> - auto start = external_ ? externalBuffers_->begin() : internalBuffers_.begin();
> >>> - auto end = external_ ? externalBuffers_->end() : internalBuffers_.end();
> >>> + /* This can only be called for external streams. */
> >>> + assert(external_);
>
> Could this use ASSERT() from libcamera/internal/log.h ?
>
> >>> +
> >>> + availableBuffers_.push(buffer);
> >>> +}
> >>>
> >>> +bool RPiStream::findFrameBuffer(FrameBuffer *buffer) const
> >>> +{
> >>> if (importOnly_)
> >>> return false;
> >>>
> >>> - if (std::find_if(start, end,
> >>> - [buffer](std::unique_ptr<FrameBuffer> const &ref) { return ref.get() == buffer; }) != end)
> >>> + if (std::find(bufferList_.begin(), bufferList_.end(), buffer) != bufferList_.end())
> >>> return true;
> >>>
> >>> return false;
> >>> }
> >>>
> >>> +void RPiStream::clearBuffers()
> >>> +{
> >>> + availableBuffers_ = std::queue<FrameBuffer *>{};
> >>> + internalBuffers_.clear();
> >>> + bufferList_.clear();
> >>> +}
> >>> +
> >>> } /* namespace RPi */
> >>>
> >>> } /* namespace libcamera */
> >>> diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.h b/src/libcamera/pipeline/raspberrypi/rpi_stream.h
> >>> index 3957e342..af9c2ad2 100644
> >>> --- a/src/libcamera/pipeline/raspberrypi/rpi_stream.h
> >>> +++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.h
> >>> @@ -39,21 +39,22 @@ public:
> >>> V4L2VideoDevice *dev() const;
> >>> void setExternal(bool external);
> >>> bool isExternal() const;
> >>> - bool isImporter() const;
> >>> void reset();
> >>> std::string name() const;
> >>> - void setExternalBuffers(std::vector<std::unique_ptr<FrameBuffer>> *buffers);
> >>> - const std::vector<std::unique_ptr<FrameBuffer>> *getBuffers() const;
> >>> + void setExportedBuffers(std::vector<std::unique_ptr<FrameBuffer>> *buffers);
> >>> + const std::vector<FrameBuffer *> &getBuffers() const;
> >>> void releaseBuffers();
> >>> - int importBuffers(unsigned int count);
> >>> - int allocateBuffers(unsigned int count);
> >>> - int queueBuffers();
> >>> + int prepareBuffers(unsigned int count);
> >>> + int queueAllBuffers();
> >>> + int queueBuffer(FrameBuffer *buffer);
> >>> + void returnBuffer(FrameBuffer *buffer);
> >>> bool findFrameBuffer(FrameBuffer *buffer) const;
> >>
> >> Perhaps we should break those up into sections in patch 1/9. It's hard
> >> to see the wood-for-the-trees in the wall of function declarations.
> >
> > Not sure I follow what you mean?
>
> There is a lot of text all bunched up together, so it's hard to get my
> eyes around the class members.
>
> It would be nice to group functions with some line breaks to make it
> easier on the eye.
>
> But that should happen in patch 1/9, so I'll go comment on it there. I
> skipped it before because that patch just moves code around, but I thnik
> it can also improve formatting for readability.
>
> >>>
> >>> private:
> >>> + void clearBuffers();
> >>> /*
> >>> * Indicates that this stream is active externally, i.e. the buffers
> >>> - * are provided by the application.
> >>> + * might be provided by (and returned to) the application.
> >>> */
> >>> bool external_;
> >>> /* Indicates that this stream only imports buffers, e.g. ISP input. */
> >>> @@ -62,10 +63,19 @@ private:
> >>> std::string name_;
> >>> /* The actual device stream. */
> >>> std::unique_ptr<V4L2VideoDevice> dev_;
> >>> - /* Internally allocated framebuffers associated with this device stream. */
> >>> + /* All framebuffers associated with this device stream. */
> >>> + std::vector<FrameBuffer *> bufferList_;
> >>> + /*
> >>> + * List of frame buffer that we can use if none have been provided by
> >>> + * the application for external streams. This is populated by the
> >>> + * buffers exported internally.
> >>> + */
> >>> + std::queue<FrameBuffer *> availableBuffers_;
> >>> + /*
> >>> + * This is a list of buffers exported internally. Need to keep this around
> >>> + * as the stream needs to maintain ownership of these buffers.
> >>> + */
> >>> std::vector<std::unique_ptr<FrameBuffer>> internalBuffers_;
> >>> - /* Externally allocated framebuffers associated with this device stream. */
> >>> - std::vector<std::unique_ptr<FrameBuffer>> *externalBuffers_;
> >>
> >> Likewise here, some whitespace to break up these members would really
> >> help, as by the time I've got here I've got definite review fatigue and
> >> lots of lines close together just become a blur ... :S
> >
> > That's not a problem.
>
> But for this patch, I can't see anything wrong ... and I can see how
> it's helping manage buffers to allow optional buffers on a stream for
> RAW zero copy support.
>
> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>
> >>> };
> >>>
> >>> /*
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list