[libcamera-devel,RFC,1/2] libcamera: CameraData: Change queuedRequests_ type to std::dequeue
diff mbox series

Message ID 20210329022604.110201-2-hiroh@chromium.org
State Changes Requested
Headers show
Series
  • Enable handling a number of capture requests
Related show

Commit Message

Hirokazu Honda March 29, 2021, 2:26 a.m. UTC
The type of CameraData::queuedRequests_ is std::list. The more
appropriate type is std::dequeue as the requests is reported as
complete in the order of queuing and are needed to be removed in
the first and last ones.

Signed-off-by: Hirokazu Honda <hiroh@chromium.org>
---
 include/libcamera/internal/pipeline_handler.h | 4 ++--
 src/libcamera/pipeline_handler.cpp            | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

Comments

Sebastian Fricke March 29, 2021, 4:22 a.m. UTC | #1
Hey Hirokazu,

Thank you for the patch.

On 29.03.2021 11:26, Hirokazu Honda wrote:
>The type of CameraData::queuedRequests_ is std::list. The more
>appropriate type is std::dequeue as the requests is reported as
>complete in the order of queuing and are needed to be removed in
>the first and last ones.

This sentence is a little hard to read and contains some incorrect
English grammar, how about:

```
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>
>---
> include/libcamera/internal/pipeline_handler.h | 4 ++--
> src/libcamera/pipeline_handler.cpp            | 4 ++--
> 2 files changed, 4 insertions(+), 4 deletions(-)
>
>diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h
>index 093b94c6..763da63e 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 0228d7b2..4cb58084 100644
>--- a/src/libcamera/pipeline_handler.cpp
>+++ b/src/libcamera/pipeline_handler.cpp
>@@ -72,9 +72,9 @@ LOG_DEFINE_CATEGORY(Pipeline)
>
> /**
>  * \var CameraData::queuedRequests_
>- * \brief The list of queued and not yet completed request
>+ * \brief The queue of queued and not yet completed request

s/The queue of queued and not yet completed request/
   The queue of incomplete requests/

>  *
>- * The list of queued request is used to track requests queued in order to
>+ * The queue of queued request is used to track requests queued in order to
>  * ensure completion of all requests when the pipeline handler is stopped.

I think that this sentence contains a lot of redundant phrasing. How
about this:

```
This queue is used to ensure that all requests are completed when the pipeline
handler is stopped.
```


Besides those little improvements of the documentation and commit
description:

Reviewed-by: Sebastian Fricke <sebastian.fricke@posteo.net>

>  *
>  * \sa PipelineHandler::queueRequest(), PipelineHandler::stop(),
>-- 
>2.31.0.291.g576ba9dcdaf-goog
>
>_______________________________________________
>libcamera-devel mailing list
>libcamera-devel@lists.libcamera.org
>https://lists.libcamera.org/listinfo/libcamera-devel

Patch
diff mbox series

diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h
index 093b94c6..763da63e 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 0228d7b2..4cb58084 100644
--- a/src/libcamera/pipeline_handler.cpp
+++ b/src/libcamera/pipeline_handler.cpp
@@ -72,9 +72,9 @@  LOG_DEFINE_CATEGORY(Pipeline)
 
 /**
  * \var CameraData::queuedRequests_
- * \brief The list of queued and not yet completed request
+ * \brief The queue of queued and not yet completed request
  *
- * The list of queued request is used to track requests queued in order to
+ * The queue of queued request is used to track requests queued in order to
  * ensure completion of all requests when the pipeline handler is stopped.
  *
  * \sa PipelineHandler::queueRequest(), PipelineHandler::stop(),