[libcamera-devel] [PATCH v4 8/9] libcamera: pipeline: raspberrypi: Add more robust stream buffer logic

[Naushir Patuck]

Hi Kieran,

On Tue, 21 Jul 2020 at 12:23, Kieran Bingham
<kieran.bingham at 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 at raspberrypi.com>
> > Reviewed-by: Niklas Söderlund <niklas.soderlund at 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