| Message ID | 20200720091311.805092-9-naush@raspberrypi.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Naush, On 20/07/2020 10:13, Naushir Patuck wrote: > Add further queueing into the RPiStream object to ensure that we always > follow the buffer ordering (be it internal or external) given by > incoming Requests. > > This is essential, otherwise we risk dropping frames that are meant to > be part of a Request, and can cause the pipeline to stall indefinitely. > This also prevents any possibility of mismatched frame buffers going > through the pipeline and out to the application. > Sounds helpful... some questions below ... but I suspect I've just misinterpreted something... > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > --- > .../pipeline/raspberrypi/rpi_stream.cpp | 70 ++++++++++++++++--- > .../pipeline/raspberrypi/rpi_stream.h | 12 +++- > 2 files changed, 71 insertions(+), 11 deletions(-) > > diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp > index 02f8d3e0..6635a751 100644 > --- a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp > +++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp > @@ -112,25 +112,34 @@ int RPiStream::queueBuffer(FrameBuffer *buffer) > */ > if (!buffer) { > if (availableBuffers_.empty()) { > - LOG(RPISTREAM, Warning) << "No buffers available for " > + LOG(RPISTREAM, Info) << "No buffers available for " > << name_; > - return -EINVAL; > + /* > + * Note that we need to requeue an internal buffer as soon > + * as one becomes available. > + */ > + requeueBuffers_.push(nullptr); > + return 0; > } > > buffer = availableBuffers_.front(); > availableBuffers_.pop(); > } > > - LOG(RPISTREAM, Debug) << "Queuing buffer " << buffer->cookie() > - << " for " << name_; > + /* > + * If no earlier requests are pending to be queued we can go ahead and > + * queue the buffer into the device. > + */ > + if (requeueBuffers_.empty()) > + return queueToDevice(buffer); > > - int ret = dev_->queueBuffer(buffer); > - if (ret) { > - LOG(RPISTREAM, Error) << "Failed to queue buffer for " > - << name_; > - } > + /* > + * There are earlier buffers to be queued, so this bufferm ust go on s/bufferm ust/buffer must/ That could be fixed up while applying. It's minor. > + * the waiting list. > + */ > + requeueBuffers_.push(buffer); > > - return ret; > + return 0; > } > > void RPiStream::returnBuffer(FrameBuffer *buffer) > @@ -138,7 +147,35 @@ void RPiStream::returnBuffer(FrameBuffer *buffer) > /* This can only be called for external streams. */ > assert(external_); > > + /* Push this buffer back into the queue to be used again. */ > availableBuffers_.push(buffer); > + > + /* > + * Do we have any buffers that are waiting to be queued? > + * If so, do it now as availableBuffers_ will not be empty. > + */ > + while (!requeueBuffers_.empty()) { > + FrameBuffer *buffer = requeueBuffers_.front(); > + > + if (!buffer) { > + /* > + * We want to queue an internal buffer, but none > + * are available. Can't do anything, quit the loop. > + */ > + if (availableBuffers_.empty()) > + break; Can this happen? This looks like: while(requeueBuffer exist) buffer = first buffer from requeueBuffers if (no buffer) { /* unreachable code ? */ } > + > + /* > + * We want to queue an internal buffer, and at least one > + * is available. > + */ > + buffer = availableBuffers_.front(); > + availableBuffers_.pop(); > + } > + > + requeueBuffers_.pop(); > + queueToDevice(buffer); > + } > } > > bool RPiStream::findFrameBuffer(FrameBuffer *buffer) const > @@ -155,10 +192,23 @@ bool RPiStream::findFrameBuffer(FrameBuffer *buffer) const > void RPiStream::clearBuffers() > { > availableBuffers_ = std::queue<FrameBuffer *>{}; > + requeueBuffers_ = std::queue<FrameBuffer *>{}; > internalBuffers_.clear(); > bufferList_.clear(); > } > > +int RPiStream::queueToDevice(FrameBuffer *buffer) > +{ > + 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; > +} > + > } /* namespace RPi */ > > } /* namespace libcamera */ > diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.h b/src/libcamera/pipeline/raspberrypi/rpi_stream.h > index af9c2ad2..6222c867 100644 > --- a/src/libcamera/pipeline/raspberrypi/rpi_stream.h > +++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.h > @@ -52,6 +52,7 @@ public: > > private: > void clearBuffers(); > + int queueToDevice(FrameBuffer *buffer); > /* > * Indicates that this stream is active externally, i.e. the buffers > * might be provided by (and returned to) the application. > @@ -63,7 +64,7 @@ private: > std::string name_; > /* The actual device stream. */ > std::unique_ptr<V4L2VideoDevice> dev_; > - /* All framebuffers associated with this device stream. */ > + /* All frame buffers associated with this device stream. */ > std::vector<FrameBuffer *> bufferList_; > /* > * List of frame buffer that we can use if none have been provided by > @@ -71,6 +72,15 @@ private: > * buffers exported internally. > */ > std::queue<FrameBuffer *> availableBuffers_; > + /* > + * List of frame buffer that are to be re-queued into the device. "List of frame buffers ..." > + * A nullptr indicates any internal buffer can be used (from availableBuffers_), > + * whereas a valid pointer indicates an external buffer to be queued. Oh ... are we pushing nullptrs on to the queues? ... perhaps that answers my unreachable code question above ... > + * > + * Ordering buffers to be queued is important here as it must match the > + * requests coming from the application. > + */ > + std::queue<FrameBuffer *> requeueBuffers_; > /* > * This is a list of buffers exported internally. Need to keep this around > * as the stream needs to maintain ownership of these buffers. >
Hi Kieran, On Tue, 21 Jul 2020 at 12:23, Kieran Bingham <kieran.bingham@ideasonboard.com> wrote: > > Hi Naush, > > On 20/07/2020 10:13, Naushir Patuck wrote: > > Add further queueing into the RPiStream object to ensure that we always > > follow the buffer ordering (be it internal or external) given by > > incoming Requests. > > > > This is essential, otherwise we risk dropping frames that are meant to > > be part of a Request, and can cause the pipeline to stall indefinitely. > > This also prevents any possibility of mismatched frame buffers going > > through the pipeline and out to the application. > > > > Sounds helpful... some questions below ... but I suspect I've just > misinterpreted something... > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > > --- > > .../pipeline/raspberrypi/rpi_stream.cpp | 70 ++++++++++++++++--- > > .../pipeline/raspberrypi/rpi_stream.h | 12 +++- > > 2 files changed, 71 insertions(+), 11 deletions(-) > > > > diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp > > index 02f8d3e0..6635a751 100644 > > --- a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp > > +++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp > > @@ -112,25 +112,34 @@ int RPiStream::queueBuffer(FrameBuffer *buffer) > > */ > > if (!buffer) { > > if (availableBuffers_.empty()) { > > - LOG(RPISTREAM, Warning) << "No buffers available for " > > + LOG(RPISTREAM, Info) << "No buffers available for " > > << name_; > > - return -EINVAL; > > + /* > > + * Note that we need to requeue an internal buffer as soon > > + * as one becomes available. > > + */ > > + requeueBuffers_.push(nullptr); > > + return 0; > > } > > > > buffer = availableBuffers_.front(); > > availableBuffers_.pop(); > > } > > > > - LOG(RPISTREAM, Debug) << "Queuing buffer " << buffer->cookie() > > - << " for " << name_; > > + /* > > + * If no earlier requests are pending to be queued we can go ahead and > > + * queue the buffer into the device. > > + */ > > + if (requeueBuffers_.empty()) > > + return queueToDevice(buffer); > > > > - int ret = dev_->queueBuffer(buffer); > > - if (ret) { > > - LOG(RPISTREAM, Error) << "Failed to queue buffer for " > > - << name_; > > - } > > + /* > > + * There are earlier buffers to be queued, so this bufferm ust go on > > > s/bufferm ust/buffer must/ > > That could be fixed up while applying. It's minor. > > > > > + * the waiting list. > > + */ > > + requeueBuffers_.push(buffer); > > > > - return ret; > > + return 0; > > } > > > > void RPiStream::returnBuffer(FrameBuffer *buffer) > > @@ -138,7 +147,35 @@ void RPiStream::returnBuffer(FrameBuffer *buffer) > > /* This can only be called for external streams. */ > > assert(external_); > > > > + /* Push this buffer back into the queue to be used again. */ > > availableBuffers_.push(buffer); > > + > > + /* > > + * Do we have any buffers that are waiting to be queued? > > + * If so, do it now as availableBuffers_ will not be empty. > > + */ > > + while (!requeueBuffers_.empty()) { > > + FrameBuffer *buffer = requeueBuffers_.front(); > > + > > + if (!buffer) { > > + /* > > + * We want to queue an internal buffer, but none > > + * are available. Can't do anything, quit the loop. > > + */ > > + if (availableBuffers_.empty()) > > + break; > > Can this happen? > > This looks like: > > while(requeueBuffer exist) > buffer = first buffer from requeueBuffers > if (no buffer) { > /* unreachable code ? */ > } > Yes it is possible, having a nullptr element in requeueBuffers_ means a Request did not provide a buffer for that stream, so we have to use a buffer from availableBuffers_ instead. This buffer will not be returned out to the applications. Hope that makes sense. Regards, Naush > > > > + > > + /* > > + * We want to queue an internal buffer, and at least one > > + * is available. > > + */ > > + buffer = availableBuffers_.front(); > > + availableBuffers_.pop(); > > + } > > + > > + requeueBuffers_.pop(); > > + queueToDevice(buffer); > > + } > > } > > > > bool RPiStream::findFrameBuffer(FrameBuffer *buffer) const > > @@ -155,10 +192,23 @@ bool RPiStream::findFrameBuffer(FrameBuffer *buffer) const > > void RPiStream::clearBuffers() > > { > > availableBuffers_ = std::queue<FrameBuffer *>{}; > > + requeueBuffers_ = std::queue<FrameBuffer *>{}; > > internalBuffers_.clear(); > > bufferList_.clear(); > > } > > > > +int RPiStream::queueToDevice(FrameBuffer *buffer) > > +{ > > + 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; > > +} > > + > > } /* namespace RPi */ > > > > } /* namespace libcamera */ > > diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.h b/src/libcamera/pipeline/raspberrypi/rpi_stream.h > > index af9c2ad2..6222c867 100644 > > --- a/src/libcamera/pipeline/raspberrypi/rpi_stream.h > > +++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.h > > @@ -52,6 +52,7 @@ public: > > > > private: > > void clearBuffers(); > > + int queueToDevice(FrameBuffer *buffer); > > /* > > * Indicates that this stream is active externally, i.e. the buffers > > * might be provided by (and returned to) the application. > > @@ -63,7 +64,7 @@ private: > > std::string name_; > > /* The actual device stream. */ > > std::unique_ptr<V4L2VideoDevice> dev_; > > - /* All framebuffers associated with this device stream. */ > > + /* All frame buffers associated with this device stream. */ > > std::vector<FrameBuffer *> bufferList_; > > /* > > * List of frame buffer that we can use if none have been provided by > > @@ -71,6 +72,15 @@ private: > > * buffers exported internally. > > */ > > std::queue<FrameBuffer *> availableBuffers_; > > + /* > > + * List of frame buffer that are to be re-queued into the device. > > "List of frame buffers ..." > > > + * A nullptr indicates any internal buffer can be used (from availableBuffers_), > > + * whereas a valid pointer indicates an external buffer to be queued. > > Oh ... are we pushing nullptrs on to the queues? ... perhaps that > answers my unreachable code question above ... > > > > + * > > + * Ordering buffers to be queued is important here as it must match the > > + * requests coming from the application. > > + */ > > + std::queue<FrameBuffer *> requeueBuffers_; > > /* > > * This is a list of buffers exported internally. Need to keep this around > > * as the stream needs to maintain ownership of these buffers. > > > > -- > Regards > -- > Kieran
Hi Naush, Thank you for the patch. On Tue, Jul 21, 2020 at 01:35:51PM +0100, Naushir Patuck wrote: > On Tue, 21 Jul 2020 at 12:23, Kieran Bingham wrote: > > On 20/07/2020 10:13, Naushir Patuck wrote: > > > Add further queueing into the RPiStream object to ensure that we always > > > follow the buffer ordering (be it internal or external) given by > > > incoming Requests. > > > > > > This is essential, otherwise we risk dropping frames that are meant to > > > be part of a Request, and can cause the pipeline to stall indefinitely. > > > This also prevents any possibility of mismatched frame buffers going > > > through the pipeline and out to the application. > > > > Sounds helpful... some questions below ... but I suspect I've just > > misinterpreted something... > > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > > Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > > > --- > > > .../pipeline/raspberrypi/rpi_stream.cpp | 70 ++++++++++++++++--- > > > .../pipeline/raspberrypi/rpi_stream.h | 12 +++- > > > 2 files changed, 71 insertions(+), 11 deletions(-) > > > > > > diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp > > > index 02f8d3e0..6635a751 100644 > > > --- a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp > > > +++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp > > > @@ -112,25 +112,34 @@ int RPiStream::queueBuffer(FrameBuffer *buffer) > > > */ > > > if (!buffer) { > > > if (availableBuffers_.empty()) { > > > - LOG(RPISTREAM, Warning) << "No buffers available for " > > > + LOG(RPISTREAM, Info) << "No buffers available for " > > > << name_; > > > - return -EINVAL; > > > + /* > > > + * Note that we need to requeue an internal buffer as soon > > > + * as one becomes available. > > > + */ > > > + requeueBuffers_.push(nullptr); I'm having a hard time parsing this patch. "requeue" sounds like the queue contains buffers that are completed and need to be given back to the device, and I think that's not what is happening here. Or is it ? It also seems that the buffers are queued to the device in returnBuffers(), which is meant to signal completion. This makes the code flow very opaque to me. I'm afraid I just can't provide meaningful review comments as I don't understand what's going on. > > > + return 0; > > > } > > > > > > buffer = availableBuffers_.front(); > > > availableBuffers_.pop(); > > > } > > > > > > - LOG(RPISTREAM, Debug) << "Queuing buffer " << buffer->cookie() > > > - << " for " << name_; > > > + /* > > > + * If no earlier requests are pending to be queued we can go ahead and > > > + * queue the buffer into the device. > > > + */ > > > + if (requeueBuffers_.empty()) > > > + return queueToDevice(buffer); > > > > > > - int ret = dev_->queueBuffer(buffer); > > > - if (ret) { > > > - LOG(RPISTREAM, Error) << "Failed to queue buffer for " > > > - << name_; > > > - } > > > + /* > > > + * There are earlier buffers to be queued, so this bufferm ust go on > > > > s/bufferm ust/buffer must/ > > > > That could be fixed up while applying. It's minor. > > > > > + * the waiting list. > > > + */ > > > + requeueBuffers_.push(buffer); > > > > > > - return ret; > > > + return 0; > > > } > > > > > > void RPiStream::returnBuffer(FrameBuffer *buffer) > > > @@ -138,7 +147,35 @@ void RPiStream::returnBuffer(FrameBuffer *buffer) > > > /* This can only be called for external streams. */ > > > assert(external_); > > > > > > + /* Push this buffer back into the queue to be used again. */ > > > availableBuffers_.push(buffer); > > > + > > > + /* > > > + * Do we have any buffers that are waiting to be queued? > > > + * If so, do it now as availableBuffers_ will not be empty. > > > + */ > > > + while (!requeueBuffers_.empty()) { > > > + FrameBuffer *buffer = requeueBuffers_.front(); > > > + > > > + if (!buffer) { > > > + /* > > > + * We want to queue an internal buffer, but none > > > + * are available. Can't do anything, quit the loop. > > > + */ > > > + if (availableBuffers_.empty()) > > > + break; > > > > Can this happen? > > > > This looks like: > > > > while(requeueBuffer exist) > > buffer = first buffer from requeueBuffers > > if (no buffer) { > > /* unreachable code ? */ > > } > > > > Yes it is possible, having a nullptr element in requeueBuffers_ means > a Request did not provide a buffer for that stream, so we have to use > a buffer from availableBuffers_ instead. This buffer will not be > returned out to the applications. Hope that makes sense. > > > > + > > > + /* > > > + * We want to queue an internal buffer, and at least one > > > + * is available. > > > + */ > > > + buffer = availableBuffers_.front(); > > > + availableBuffers_.pop(); > > > + } > > > + > > > + requeueBuffers_.pop(); > > > + queueToDevice(buffer); > > > + } > > > } > > > > > > bool RPiStream::findFrameBuffer(FrameBuffer *buffer) const > > > @@ -155,10 +192,23 @@ bool RPiStream::findFrameBuffer(FrameBuffer *buffer) const > > > void RPiStream::clearBuffers() > > > { > > > availableBuffers_ = std::queue<FrameBuffer *>{}; > > > + requeueBuffers_ = std::queue<FrameBuffer *>{}; > > > internalBuffers_.clear(); > > > bufferList_.clear(); > > > } > > > > > > +int RPiStream::queueToDevice(FrameBuffer *buffer) > > > +{ > > > + 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; > > > +} > > > + > > > } /* namespace RPi */ > > > > > > } /* namespace libcamera */ > > > diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.h b/src/libcamera/pipeline/raspberrypi/rpi_stream.h > > > index af9c2ad2..6222c867 100644 > > > --- a/src/libcamera/pipeline/raspberrypi/rpi_stream.h > > > +++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.h > > > @@ -52,6 +52,7 @@ public: > > > > > > private: > > > void clearBuffers(); > > > + int queueToDevice(FrameBuffer *buffer); > > > /* > > > * Indicates that this stream is active externally, i.e. the buffers > > > * might be provided by (and returned to) the application. > > > @@ -63,7 +64,7 @@ private: > > > std::string name_; > > > /* The actual device stream. */ > > > std::unique_ptr<V4L2VideoDevice> dev_; > > > - /* All framebuffers associated with this device stream. */ > > > + /* All frame buffers associated with this device stream. */ > > > std::vector<FrameBuffer *> bufferList_; > > > /* > > > * List of frame buffer that we can use if none have been provided by > > > @@ -71,6 +72,15 @@ private: > > > * buffers exported internally. > > > */ > > > std::queue<FrameBuffer *> availableBuffers_; > > > + /* > > > + * List of frame buffer that are to be re-queued into the device. > > > > "List of frame buffers ..." > > > > > + * A nullptr indicates any internal buffer can be used (from availableBuffers_), > > > + * whereas a valid pointer indicates an external buffer to be queued. > > > > Oh ... are we pushing nullptrs on to the queues? ... perhaps that > > answers my unreachable code question above ... > > > > > + * > > > + * Ordering buffers to be queued is important here as it must match the > > > + * requests coming from the application. > > > + */ > > > + std::queue<FrameBuffer *> requeueBuffers_; > > > /* > > > * This is a list of buffers exported internally. Need to keep this around > > > * as the stream needs to maintain ownership of these buffers.
Hi Laurent, Thank you for your feedback. On Wed, 22 Jul 2020 at 16:43, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Naush, > > Thank you for the patch. > > On Tue, Jul 21, 2020 at 01:35:51PM +0100, Naushir Patuck wrote: > > On Tue, 21 Jul 2020 at 12:23, Kieran Bingham wrote: > > > On 20/07/2020 10:13, Naushir Patuck wrote: > > > > Add further queueing into the RPiStream object to ensure that we always > > > > follow the buffer ordering (be it internal or external) given by > > > > incoming Requests. > > > > > > > > This is essential, otherwise we risk dropping frames that are meant to > > > > be part of a Request, and can cause the pipeline to stall indefinitely. > > > > This also prevents any possibility of mismatched frame buffers going > > > > through the pipeline and out to the application. > > > > > > Sounds helpful... some questions below ... but I suspect I've just > > > misinterpreted something... > > > > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > > > Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > > > > --- > > > > .../pipeline/raspberrypi/rpi_stream.cpp | 70 ++++++++++++++++--- > > > > .../pipeline/raspberrypi/rpi_stream.h | 12 +++- > > > > 2 files changed, 71 insertions(+), 11 deletions(-) > > > > > > > > diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp > > > > index 02f8d3e0..6635a751 100644 > > > > --- a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp > > > > +++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp > > > > @@ -112,25 +112,34 @@ int RPiStream::queueBuffer(FrameBuffer *buffer) > > > > */ > > > > if (!buffer) { > > > > if (availableBuffers_.empty()) { > > > > - LOG(RPISTREAM, Warning) << "No buffers available for " > > > > + LOG(RPISTREAM, Info) << "No buffers available for " > > > > << name_; > > > > - return -EINVAL; > > > > + /* > > > > + * Note that we need to requeue an internal buffer as soon > > > > + * as one becomes available. > > > > + */ > > > > + requeueBuffers_.push(nullptr); > > I'm having a hard time parsing this patch. "requeue" sounds like the > queue contains buffers that are completed and need to be given back to > the device, and I think that's not what is happening here. Or is it ? > > It also seems that the buffers are queued to the device in > returnBuffers(), which is meant to signal completion. This makes the > code flow very opaque to me. I'm afraid I just can't provide meaningful > review comments as I don't understand what's going on. > I must admit, I did hit my head on the wall trying to ensure I have covered all grounds - it all gets very complicated very quickly. And my variable/method names could possibly be chosen better. Here is what i am trying to achieve with this commit: 1) We must handle drop frames at the start of streaming correctly. By correctly I mean that we cannot queue Request provided buffers into the device and drop them, i.e. not return them to the application. We cannot rely on having as many internal buffers allocated to match the amount of drop frames requested. 2) Similarly we must account for the fact that not every request will contain a buffer for, say, a RAW image when RAW streams are exported to the application. In these cases, we still need to queue an internal buffer. But what happens if we have run out of internal buffers? This needs to be handled correctly. So the result is a queue (called, requeueBuffers) that stores requests for buffers needing to be queued to the device in the case we have run out of internal buffers, and the application has not provided one in the Request. This happens in queueBuffer(). Ordering is important here, as we must be in lock-step with the order Requests come in. When a buffer completes (and providing it was an internal buffer in the absence of a RAW buffer in the Request in our example above), we call returnBuffer() which then checks if we had anything to re-queue (hence the name requeueBuffers_) to the device now that an internal buffer has been returned to the stream. Hope this explanation makes the code a bit easier to understand. If not, I'm happy to chat or call to provide more clarity :) If you do have any suggestions for naming, please do let me know and I am open to changing these. Regards, Naush > > > > + return 0; > > > > } > > > > > > > > buffer = availableBuffers_.front(); > > > > availableBuffers_.pop(); > > > > } > > > > > > > > - LOG(RPISTREAM, Debug) << "Queuing buffer " << buffer->cookie() > > > > - << " for " << name_; > > > > + /* > > > > + * If no earlier requests are pending to be queued we can go ahead and > > > > + * queue the buffer into the device. > > > > + */ > > > > + if (requeueBuffers_.empty()) > > > > + return queueToDevice(buffer); > > > > > > > > - int ret = dev_->queueBuffer(buffer); > > > > - if (ret) { > > > > - LOG(RPISTREAM, Error) << "Failed to queue buffer for " > > > > - << name_; > > > > - } > > > > + /* > > > > + * There are earlier buffers to be queued, so this bufferm ust go on > > > > > > s/bufferm ust/buffer must/ > > > > > > That could be fixed up while applying. It's minor. > > > > > > > + * the waiting list. > > > > + */ > > > > + requeueBuffers_.push(buffer); > > > > > > > > - return ret; > > > > + return 0; > > > > } > > > > > > > > void RPiStream::returnBuffer(FrameBuffer *buffer) > > > > @@ -138,7 +147,35 @@ void RPiStream::returnBuffer(FrameBuffer *buffer) > > > > /* This can only be called for external streams. */ > > > > assert(external_); > > > > > > > > + /* Push this buffer back into the queue to be used again. */ > > > > availableBuffers_.push(buffer); > > > > + > > > > + /* > > > > + * Do we have any buffers that are waiting to be queued? > > > > + * If so, do it now as availableBuffers_ will not be empty. > > > > + */ > > > > + while (!requeueBuffers_.empty()) { > > > > + FrameBuffer *buffer = requeueBuffers_.front(); > > > > + > > > > + if (!buffer) { > > > > + /* > > > > + * We want to queue an internal buffer, but none > > > > + * are available. Can't do anything, quit the loop. > > > > + */ > > > > + if (availableBuffers_.empty()) > > > > + break; > > > > > > Can this happen? > > > > > > This looks like: > > > > > > while(requeueBuffer exist) > > > buffer = first buffer from requeueBuffers > > > if (no buffer) { > > > /* unreachable code ? */ > > > } > > > > > > > Yes it is possible, having a nullptr element in requeueBuffers_ means > > a Request did not provide a buffer for that stream, so we have to use > > a buffer from availableBuffers_ instead. This buffer will not be > > returned out to the applications. Hope that makes sense. > > > > > > + > > > > + /* > > > > + * We want to queue an internal buffer, and at least one > > > > + * is available. > > > > + */ > > > > + buffer = availableBuffers_.front(); > > > > + availableBuffers_.pop(); > > > > + } > > > > + > > > > + requeueBuffers_.pop(); > > > > + queueToDevice(buffer); > > > > + } > > > > } > > > > > > > > bool RPiStream::findFrameBuffer(FrameBuffer *buffer) const > > > > @@ -155,10 +192,23 @@ bool RPiStream::findFrameBuffer(FrameBuffer *buffer) const > > > > void RPiStream::clearBuffers() > > > > { > > > > availableBuffers_ = std::queue<FrameBuffer *>{}; > > > > + requeueBuffers_ = std::queue<FrameBuffer *>{}; > > > > internalBuffers_.clear(); > > > > bufferList_.clear(); > > > > } > > > > > > > > +int RPiStream::queueToDevice(FrameBuffer *buffer) > > > > +{ > > > > + 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; > > > > +} > > > > + > > > > } /* namespace RPi */ > > > > > > > > } /* namespace libcamera */ > > > > diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.h b/src/libcamera/pipeline/raspberrypi/rpi_stream.h > > > > index af9c2ad2..6222c867 100644 > > > > --- a/src/libcamera/pipeline/raspberrypi/rpi_stream.h > > > > +++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.h > > > > @@ -52,6 +52,7 @@ public: > > > > > > > > private: > > > > void clearBuffers(); > > > > + int queueToDevice(FrameBuffer *buffer); > > > > /* > > > > * Indicates that this stream is active externally, i.e. the buffers > > > > * might be provided by (and returned to) the application. > > > > @@ -63,7 +64,7 @@ private: > > > > std::string name_; > > > > /* The actual device stream. */ > > > > std::unique_ptr<V4L2VideoDevice> dev_; > > > > - /* All framebuffers associated with this device stream. */ > > > > + /* All frame buffers associated with this device stream. */ > > > > std::vector<FrameBuffer *> bufferList_; > > > > /* > > > > * List of frame buffer that we can use if none have been provided by > > > > @@ -71,6 +72,15 @@ private: > > > > * buffers exported internally. > > > > */ > > > > std::queue<FrameBuffer *> availableBuffers_; > > > > + /* > > > > + * List of frame buffer that are to be re-queued into the device. > > > > > > "List of frame buffers ..." > > > > > > > + * A nullptr indicates any internal buffer can be used (from availableBuffers_), > > > > + * whereas a valid pointer indicates an external buffer to be queued. > > > > > > Oh ... are we pushing nullptrs on to the queues? ... perhaps that > > > answers my unreachable code question above ... > > > > > > > + * > > > > + * Ordering buffers to be queued is important here as it must match the > > > > + * requests coming from the application. > > > > + */ > > > > + std::queue<FrameBuffer *> requeueBuffers_; > > > > /* > > > > * This is a list of buffers exported internally. Need to keep this around > > > > * as the stream needs to maintain ownership of these buffers. > > -- > Regards, > > Laurent Pinchart
diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp index 02f8d3e0..6635a751 100644 --- a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp +++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp @@ -112,25 +112,34 @@ int RPiStream::queueBuffer(FrameBuffer *buffer) */ if (!buffer) { if (availableBuffers_.empty()) { - LOG(RPISTREAM, Warning) << "No buffers available for " + LOG(RPISTREAM, Info) << "No buffers available for " << name_; - return -EINVAL; + /* + * Note that we need to requeue an internal buffer as soon + * as one becomes available. + */ + requeueBuffers_.push(nullptr); + return 0; } buffer = availableBuffers_.front(); availableBuffers_.pop(); } - LOG(RPISTREAM, Debug) << "Queuing buffer " << buffer->cookie() - << " for " << name_; + /* + * If no earlier requests are pending to be queued we can go ahead and + * queue the buffer into the device. + */ + if (requeueBuffers_.empty()) + return queueToDevice(buffer); - int ret = dev_->queueBuffer(buffer); - if (ret) { - LOG(RPISTREAM, Error) << "Failed to queue buffer for " - << name_; - } + /* + * There are earlier buffers to be queued, so this bufferm ust go on + * the waiting list. + */ + requeueBuffers_.push(buffer); - return ret; + return 0; } void RPiStream::returnBuffer(FrameBuffer *buffer) @@ -138,7 +147,35 @@ void RPiStream::returnBuffer(FrameBuffer *buffer) /* This can only be called for external streams. */ assert(external_); + /* Push this buffer back into the queue to be used again. */ availableBuffers_.push(buffer); + + /* + * Do we have any buffers that are waiting to be queued? + * If so, do it now as availableBuffers_ will not be empty. + */ + while (!requeueBuffers_.empty()) { + FrameBuffer *buffer = requeueBuffers_.front(); + + if (!buffer) { + /* + * We want to queue an internal buffer, but none + * are available. Can't do anything, quit the loop. + */ + if (availableBuffers_.empty()) + break; + + /* + * We want to queue an internal buffer, and at least one + * is available. + */ + buffer = availableBuffers_.front(); + availableBuffers_.pop(); + } + + requeueBuffers_.pop(); + queueToDevice(buffer); + } } bool RPiStream::findFrameBuffer(FrameBuffer *buffer) const @@ -155,10 +192,23 @@ bool RPiStream::findFrameBuffer(FrameBuffer *buffer) const void RPiStream::clearBuffers() { availableBuffers_ = std::queue<FrameBuffer *>{}; + requeueBuffers_ = std::queue<FrameBuffer *>{}; internalBuffers_.clear(); bufferList_.clear(); } +int RPiStream::queueToDevice(FrameBuffer *buffer) +{ + 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; +} + } /* namespace RPi */ } /* namespace libcamera */ diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.h b/src/libcamera/pipeline/raspberrypi/rpi_stream.h index af9c2ad2..6222c867 100644 --- a/src/libcamera/pipeline/raspberrypi/rpi_stream.h +++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.h @@ -52,6 +52,7 @@ public: private: void clearBuffers(); + int queueToDevice(FrameBuffer *buffer); /* * Indicates that this stream is active externally, i.e. the buffers * might be provided by (and returned to) the application. @@ -63,7 +64,7 @@ private: std::string name_; /* The actual device stream. */ std::unique_ptr<V4L2VideoDevice> dev_; - /* All framebuffers associated with this device stream. */ + /* All frame buffers associated with this device stream. */ std::vector<FrameBuffer *> bufferList_; /* * List of frame buffer that we can use if none have been provided by @@ -71,6 +72,15 @@ private: * buffers exported internally. */ std::queue<FrameBuffer *> availableBuffers_; + /* + * List of frame buffer that are to be re-queued into the device. + * A nullptr indicates any internal buffer can be used (from availableBuffers_), + * whereas a valid pointer indicates an external buffer to be queued. + * + * Ordering buffers to be queued is important here as it must match the + * requests coming from the application. + */ + std::queue<FrameBuffer *> requeueBuffers_; /* * This is a list of buffers exported internally. Need to keep this around * as the stream needs to maintain ownership of these buffers.