[libcamera-devel] [PATCH v4 6/9] libcamera: pipeline: raspberrypi: Rework stream buffer logic for zero-copy
Naushir Patuck
naush at raspberrypi.com
Wed Jul 22 17:33:08 CEST 2020
Hi Laurent,
Thank you for the review.
On Wed, 22 Jul 2020 at 16:25, Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> 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 ?
With this change, we can now advertise embedded data as well as isp
statistics available for the application to request. The only thing
missing is a way for the application to do so. In a separate
discussion we talked about libcamera::PixelFormat not being the right
place as these are data formats, not image formats. No conclusion has
been reached yet, but I will resurrect that discussion shortly :-)
>
> > >>> 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 ?
Oversight. Will fix.
>
> > >>> }
> > >>>
> > >>> /*
> > >>> @@ -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
Regards,
Naush
More information about the libcamera-devel
mailing list