[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:48:27 CEST 2020
Hi Naush,
On Wed, Jul 22, 2020 at 04:33:08PM +0100, Naushir Patuck wrote:
> On Wed, 22 Jul 2020 at 16:25, Laurent Pinchart wrote:
> > 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.
Could we avoid exposing non-image streams to applications in the
meantime though ?
> 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 :-)
Take your time :-) Jokes aside, I'm working on support for
memory-to-memory processing (a.k.a. reprocessing), and I think that
should be merged first as a base to provide statistics to applications.
I expect it will take at least a couple of months until the reprocessing
API is ready, so there's no hurry at all.
> >>>>> 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
More information about the libcamera-devel
mailing list