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

11787 diff mbox series

A more appropriate type is std::dequeue as requests are reported
and removed in the order of queuing.

Signed-off-by: Hirokazu Honda <hiroh@chromium.org>
Reviewed-by: Sebastian Fricke <sebastian.fricke@posteo.net>
---
 include/libcamera/internal/pipeline_handler.h | 4 ++--
 src/libcamera/pipeline_handler.cpp            | 8 ++++----
 2 files changed, 6 insertions(+), 6 deletions(-)

Hi Hiro,

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@chromium.org>
> Reviewed-by: Sebastian Fricke <sebastian.fricke@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_;
>  	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.

we don't use . at the end

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

The nit about the full stop can be fixed when applying
Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>

Thanks
  j
>
>  	return ret;
>  }
> --
> 2.31.0.291.g576ba9dcdaf-goog
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

Hey Jacopo,

On 30.03.2021 12:07, Jacopo Mondi wrote:
>Hi Hiro,
>
>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@chromium.org>
>> Reviewed-by: Sebastian Fricke <sebastian.fricke@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_;
>>  	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.
>
>we don't use . at the end

That was my fault, I suggested that change :S
Thank you.

>
>>   *
>> - * 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);
>
>The nit about the full stop can be fixed when applying
>Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
>
>Thanks
>  j
>>
>>  	return ret;
>>  }
>> --
>> 2.31.0.291.g576ba9dcdaf-goog
>>
>> _______________________________________________
>> libcamera-devel mailing list
>> libcamera-devel@lists.libcamera.org
>> https://lists.libcamera.org/listinfo/libcamera-devel
>_______________________________________________
>libcamera-devel mailing list
>libcamera-devel@lists.libcamera.org
>https://lists.libcamera.org/listinfo/libcamera-devel

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@chromium.org>
> Reviewed-by: Sebastian Fricke <sebastian.fricke@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.

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

Hi all, thanks for review!

On Tue, Mar 30, 2021 at 7:37 PM Laurent Pinchart
<laurent.pinchart@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@chromium.org>
> > Reviewed-by: Sebastian Fricke <sebastian.fricke@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

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@chromium.org>
> > > Reviewed-by: Sebastian Fricke <sebastian.fricke@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;
> > >  }