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

[Laurent Pinchart]

Hi Hiro,

On Wed, Mar 31, 2021 at 02:33:09PM +0900, Hirokazu Honda wrote:
> On Tue, Mar 30, 2021 at 7:37 PM Laurent Pinchart wrote:
> > 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().

I agree, this can cause problems.

With your "[PATCH 0/3] Handle an request error" series, an in particular
patch 2/3, I think you don't need to remove the request from the queue
anymore. Maybe this patch could be rebased on top of that series, and
then would be able to use std::queue ?

> > >       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);

By the way, I think this should be

		data->queuedRequests_.pop_back();

as it doesn't compile otherwise, but if you rebase the patch as proposed
above, it won't matter.

> > >
> > >       return ret;
> > >  }

-- 
Regards,

Laurent Pinchart