[libcamera-devel] [PATCH] libcamera: CameraData: Change queuedRequests_ type to std::dequeue

[Hirokazu Honda]

Hi all, thanks for review!

On Tue, Mar 30, 2021 at 7:37 PM Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> Hi Hiro,
>
> Thank you for the patch.
>
> On Tue, Mar 30, 2021 at 03:27:36PM +0900, Hirokazu Honda wrote:
> > A more appropriate type is std::dequeue as requests are reported
> > and removed in the order of queuing.
> >
> > Signed-off-by: Hirokazu Honda <hiroh at chromium.org>
> > Reviewed-by: Sebastian Fricke <sebastian.fricke at posteo.net>
> > ---
> >  include/libcamera/internal/pipeline_handler.h | 4 ++--
> >  src/libcamera/pipeline_handler.cpp            | 8 ++++----
> >  2 files changed, 6 insertions(+), 6 deletions(-)
> >
> > diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h
> > index 9bdda8f3..ff9d88d8 100644
> > --- a/include/libcamera/internal/pipeline_handler.h
> > +++ b/include/libcamera/internal/pipeline_handler.h
> > @@ -7,9 +7,9 @@
> >  #ifndef __LIBCAMERA_INTERNAL_PIPELINE_HANDLER_H__
> >  #define __LIBCAMERA_INTERNAL_PIPELINE_HANDLER_H__
> >
> > -#include <list>
> >  #include <map>
> >  #include <memory>
> > +#include <queue>
> >  #include <set>
> >  #include <string>
> >  #include <sys/types.h>
> > @@ -44,7 +44,7 @@ public:
> >       virtual ~CameraData() = default;
> >
> >       PipelineHandler *pipe_;
> > -     std::list<Request *> queuedRequests_;
> > +     std::deque<Request *> queuedRequests_;
>
> Wouldn't std::queue actually convey the meaning better ? It's
> implemented on top of a double-ended queue (deque) by default, but
> conceptually, it exposes a FIFO interface.
>

The reason why I couldn't make it std::queue is a request is removed
on failure of queueRequestDevice and pop_back() is required in
queueRequest().
I wonder if we need to insert once before queueRequestDevice() call.
For example, is the following code fine?
int ret = queueRequestDevice(camera, request);
if (!ret)
   data->queuedRequests_.push(request);
return ret;

My concern to this code is that queueRequetDevice() might call
completeRequest(), and then the request isn't in
data->queuedRequests().

-Hiro

> >       ControlInfoMap controlInfo_;
> >       ControlList properties_;
> >
> > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
> > index e3d4975d..0fe913e9 100644
> > --- a/src/libcamera/pipeline_handler.cpp
> > +++ b/src/libcamera/pipeline_handler.cpp
> > @@ -72,10 +72,10 @@ LOG_DEFINE_CATEGORY(Pipeline)
> >
> >  /**
> >   * \var CameraData::queuedRequests_
> > - * \brief The list of queued and not yet completed request
> > + * \brief The queue of incomplete requests.
> >   *
> > - * The list of queued request is used to track requests queued in order to
> > - * ensure completion of all requests when the pipeline handler is stopped.
> > + * This queue is used to ensure that all requests are completed when the pipeline
> > + * handler is stopped.
> >   *
> >   * \sa PipelineHandler::queueRequest(), PipelineHandler::stop(),
> >   * PipelineHandler::completeRequest()
> > @@ -386,7 +386,7 @@ int PipelineHandler::queueRequest(Request *request)
> >
> >       int ret = queueRequestDevice(camera, request);
> >       if (ret)
> > -             data->queuedRequests_.remove(request);
> > +             data->queuedRequests_.pop_back(request);
> >
> >       return ret;
> >  }
>
> --
> Regards,
>
> Laurent Pinchart