| Message ID | 20201020074229.119151-4-hiroh@chromium.org |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi Hiro, Thanks for your work. On 10/20/20 1:12 PM, Hirokazu Honda wrote: > CameraStream initializes several member variables in the > initializer list. Some of them are unchanged after. This makes > them constant. Especially, doing to |cameraDevice_| represents > CameraStream doesn't have the ownership of it. > > Signed-off-by: Hirokazu Honda <hiroh@chromium.org> > --- > src/android/camera_stream.cpp | 7 +++---- > src/android/camera_stream.h | 10 +++++----- > 2 files changed, 8 insertions(+), 9 deletions(-) > > diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp > index 28a6e09..9644b74 100644 > --- a/src/android/camera_stream.cpp > +++ b/src/android/camera_stream.cpp > @@ -38,13 +38,12 @@ LOG_DECLARE_CATEGORY(HAL); > * and buffer allocation. > */ > > -CameraStream::CameraStream(CameraDevice *cameraDevice, Type type, > +CameraStream::CameraStream(CameraDevice const *cameraDevice, Type type, > camera3_stream_t *camera3Stream, unsigned int index) > - : cameraDevice_(cameraDevice), type_(type), > + : cameraDevice_(cameraDevice), > + config_(cameraDevice->cameraConfiguration()), type_(type), > camera3Stream_(camera3Stream), index_(index) > { > - config_ = cameraDevice_->cameraConfiguration(); > - > if (type_ == Type::Internal || type_ == Type::Mapped) { > /* > * \todo There might be multiple post-processors. The logic > diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h > index c367a5f..23c139d 100644 > --- a/src/android/camera_stream.h > +++ b/src/android/camera_stream.h > @@ -109,7 +109,7 @@ public: > Internal, > Mapped, > }; > - CameraStream(CameraDevice *cameraDevice, Type type, > + CameraStream(CameraDevice const *cameraDevice, Type type, > camera3_stream_t *camera3Stream, unsigned int index); > > Type type() const { return type_; } > @@ -124,11 +124,11 @@ public: > void putBuffer(libcamera::FrameBuffer *buffer); > > private: > - CameraDevice *cameraDevice_; > - libcamera::CameraConfiguration *config_; > - Type type_; > + CameraDevice const *cameraDevice_; > + const libcamera::CameraConfiguration *config_; > + const Type type_; I am not really sure if type_ should be of const type. It is owned by the CameraStream but yes, I don't think type_ would be mysteriously assigned to some other Type somewhere underneath. Jacopo, can you please pitch in here if this seems good to you? Reviewed-by: Umang Jain <email@uajain.com> > camera3_stream_t *camera3Stream_; > - unsigned int index_; > + const unsigned int index_; > > std::unique_ptr<libcamera::FrameBufferAllocator> allocator_; > std::vector<libcamera::FrameBuffer *> buffers_;
Hi Hiro/Umang, On 20/10/2020 12:15, Umang Jain wrote: > Hi Hiro, > > Thanks for your work. > > On 10/20/20 1:12 PM, Hirokazu Honda wrote: >> CameraStream initializes several member variables in the >> initializer list. Some of them are unchanged after. This makes >> them constant. Especially, doing to |cameraDevice_| represents >> CameraStream doesn't have the ownership of it. >> >> Signed-off-by: Hirokazu Honda <hiroh@chromium.org> >> --- >> src/android/camera_stream.cpp | 7 +++---- >> src/android/camera_stream.h | 10 +++++----- >> 2 files changed, 8 insertions(+), 9 deletions(-) >> >> diff --git a/src/android/camera_stream.cpp >> b/src/android/camera_stream.cpp >> index 28a6e09..9644b74 100644 >> --- a/src/android/camera_stream.cpp >> +++ b/src/android/camera_stream.cpp >> @@ -38,13 +38,12 @@ LOG_DECLARE_CATEGORY(HAL); >> * and buffer allocation. >> */ >> -CameraStream::CameraStream(CameraDevice *cameraDevice, Type type, >> +CameraStream::CameraStream(CameraDevice const *cameraDevice, Type type, >> camera3_stream_t *camera3Stream, unsigned int index) >> - : cameraDevice_(cameraDevice), type_(type), >> + : cameraDevice_(cameraDevice), >> + config_(cameraDevice->cameraConfiguration()), type_(type), >> camera3Stream_(camera3Stream), index_(index) >> { >> - config_ = cameraDevice_->cameraConfiguration(); >> - >> if (type_ == Type::Internal || type_ == Type::Mapped) { >> /* >> * \todo There might be multiple post-processors. The logic >> diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h >> index c367a5f..23c139d 100644 >> --- a/src/android/camera_stream.h >> +++ b/src/android/camera_stream.h >> @@ -109,7 +109,7 @@ public: >> Internal, >> Mapped, >> }; >> - CameraStream(CameraDevice *cameraDevice, Type type, >> + CameraStream(CameraDevice const *cameraDevice, Type type, >> camera3_stream_t *camera3Stream, unsigned int index); >> Type type() const { return type_; } >> @@ -124,11 +124,11 @@ public: >> void putBuffer(libcamera::FrameBuffer *buffer); >> private: >> - CameraDevice *cameraDevice_; >> - libcamera::CameraConfiguration *config_; >> - Type type_; >> + CameraDevice const *cameraDevice_; >> + const libcamera::CameraConfiguration *config_; >> + const Type type_; > I am not really sure if type_ should be of const type. It is owned by > the CameraStream but yes, I don't think type_ would be mysteriously > assigned to some other Type somewhere underneath. Jacopo, can you please > pitch in here if this seems good to you? As it stands at the moment, this is fine. The type_ should not change once created. If this usage changes in the future, then we can update, so I think this is fine. Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > Reviewed-by: Umang Jain <email@uajain.com> >> camera3_stream_t *camera3Stream_; >> - unsigned int index_; >> + const unsigned int index_; >> std::unique_ptr<libcamera::FrameBufferAllocator> allocator_; >> std::vector<libcamera::FrameBuffer *> buffers_; > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hello, On Tue, Oct 20, 2020 at 04:28:01PM +0100, Kieran Bingham wrote: > On 20/10/2020 12:15, Umang Jain wrote: > > On 10/20/20 1:12 PM, Hirokazu Honda wrote: > >> CameraStream initializes several member variables in the > >> initializer list. Some of them are unchanged after. This makes > >> them constant. Especially, doing to |cameraDevice_| represents > >> CameraStream doesn't have the ownership of it. > >> > >> Signed-off-by: Hirokazu Honda <hiroh@chromium.org> > >> --- > >> src/android/camera_stream.cpp | 7 +++---- > >> src/android/camera_stream.h | 10 +++++----- > >> 2 files changed, 8 insertions(+), 9 deletions(-) > >> > >> diff --git a/src/android/camera_stream.cpp > >> b/src/android/camera_stream.cpp > >> index 28a6e09..9644b74 100644 > >> --- a/src/android/camera_stream.cpp > >> +++ b/src/android/camera_stream.cpp > >> @@ -38,13 +38,12 @@ LOG_DECLARE_CATEGORY(HAL); > >> * and buffer allocation. > >> */ > >> -CameraStream::CameraStream(CameraDevice *cameraDevice, Type type, > >> +CameraStream::CameraStream(CameraDevice const *cameraDevice, Type type, > >> camera3_stream_t *camera3Stream, unsigned int index) > >> - : cameraDevice_(cameraDevice), type_(type), > >> + : cameraDevice_(cameraDevice), > >> + config_(cameraDevice->cameraConfiguration()), type_(type), > >> camera3Stream_(camera3Stream), index_(index) > >> { > >> - config_ = cameraDevice_->cameraConfiguration(); > >> - > >> if (type_ == Type::Internal || type_ == Type::Mapped) { > >> /* > >> * \todo There might be multiple post-processors. The logic > >> diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h > >> index c367a5f..23c139d 100644 > >> --- a/src/android/camera_stream.h > >> +++ b/src/android/camera_stream.h > >> @@ -109,7 +109,7 @@ public: > >> Internal, > >> Mapped, > >> }; > >> - CameraStream(CameraDevice *cameraDevice, Type type, > >> + CameraStream(CameraDevice const *cameraDevice, Type type, > >> camera3_stream_t *camera3Stream, unsigned int index); > >> Type type() const { return type_; } > >> @@ -124,11 +124,11 @@ public: > >> void putBuffer(libcamera::FrameBuffer *buffer); > >> private: > >> - CameraDevice *cameraDevice_; > >> - libcamera::CameraConfiguration *config_; > >> - Type type_; > >> + CameraDevice const *cameraDevice_; > >> + const libcamera::CameraConfiguration *config_; > >> + const Type type_; > > I am not really sure if type_ should be of const type. It is owned by > > the CameraStream but yes, I don't think type_ would be mysteriously > > assigned to some other Type somewhere underneath. Jacopo, can you please > > pitch in here if this seems good to you? > > As it stands at the moment, this is fine. The type_ should not change > once created. > > If this usage changes in the future, then we can update, so I think this > is fine. Qualifying class members that are not meant to change during the lifetime of an object is not something we currently do, and we should. There's no need for tree-wide patches at this point, but let's remember this good coding practice. > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > Reviewed-by: Umang Jain <email@uajain.com> > > > >> camera3_stream_t *camera3Stream_; > >> - unsigned int index_; > >> + const unsigned int index_; > >> std::unique_ptr<libcamera::FrameBufferAllocator> allocator_; > >> std::vector<libcamera::FrameBuffer *> buffers_;
Hi Uman, Kierna and Laurent, On Wed, Oct 21, 2020 at 1:59 AM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hello, > > On Tue, Oct 20, 2020 at 04:28:01PM +0100, Kieran Bingham wrote: > > On 20/10/2020 12:15, Umang Jain wrote: > > > On 10/20/20 1:12 PM, Hirokazu Honda wrote: > > >> CameraStream initializes several member variables in the > > >> initializer list. Some of them are unchanged after. This makes > > >> them constant. Especially, doing to |cameraDevice_| represents > > >> CameraStream doesn't have the ownership of it. > > >> > > >> Signed-off-by: Hirokazu Honda <hiroh@chromium.org> > > >> --- > > >> src/android/camera_stream.cpp | 7 +++---- > > >> src/android/camera_stream.h | 10 +++++----- > > >> 2 files changed, 8 insertions(+), 9 deletions(-) > > >> > > >> diff --git a/src/android/camera_stream.cpp > > >> b/src/android/camera_stream.cpp > > >> index 28a6e09..9644b74 100644 > > >> --- a/src/android/camera_stream.cpp > > >> +++ b/src/android/camera_stream.cpp > > >> @@ -38,13 +38,12 @@ LOG_DECLARE_CATEGORY(HAL); > > >> * and buffer allocation. > > >> */ > > >> -CameraStream::CameraStream(CameraDevice *cameraDevice, Type type, > > >> +CameraStream::CameraStream(CameraDevice const *cameraDevice, Type type, > > >> camera3_stream_t *camera3Stream, unsigned int index) > > >> - : cameraDevice_(cameraDevice), type_(type), > > >> + : cameraDevice_(cameraDevice), > > >> + config_(cameraDevice->cameraConfiguration()), type_(type), > > >> camera3Stream_(camera3Stream), index_(index) > > >> { > > >> - config_ = cameraDevice_->cameraConfiguration(); > > >> - > > >> if (type_ == Type::Internal || type_ == Type::Mapped) { > > >> /* > > >> * \todo There might be multiple post-processors. The logic > > >> diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h > > >> index c367a5f..23c139d 100644 > > >> --- a/src/android/camera_stream.h > > >> +++ b/src/android/camera_stream.h > > >> @@ -109,7 +109,7 @@ public: > > >> Internal, > > >> Mapped, > > >> }; > > >> - CameraStream(CameraDevice *cameraDevice, Type type, > > >> + CameraStream(CameraDevice const *cameraDevice, Type type, > > >> camera3_stream_t *camera3Stream, unsigned int index); > > >> Type type() const { return type_; } > > >> @@ -124,11 +124,11 @@ public: > > >> void putBuffer(libcamera::FrameBuffer *buffer); > > >> private: > > >> - CameraDevice *cameraDevice_; > > >> - libcamera::CameraConfiguration *config_; > > >> - Type type_; > > >> + CameraDevice const *cameraDevice_; > > >> + const libcamera::CameraConfiguration *config_; > > >> + const Type type_; > > > I am not really sure if type_ should be of const type. It is owned by > > > the CameraStream but yes, I don't think type_ would be mysteriously > > > assigned to some other Type somewhere underneath. Jacopo, can you please > > > pitch in here if this seems good to you? > > > > As it stands at the moment, this is fine. The type_ should not change > > once created. > > > > If this usage changes in the future, then we can update, so I think this > > is fine. > > Qualifying class members that are not meant to change during the > lifetime of an object is not something we currently do, and we should. > There's no need for tree-wide patches at this point, but let's remember > this good coding practice. > Thanks for the comments. Ack. Best Regards, -Hiro > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > > > Reviewed-by: Umang Jain <email@uajain.com> > > > > > >> camera3_stream_t *camera3Stream_; > > >> - unsigned int index_; > > >> + const unsigned int index_; > > >> std::unique_ptr<libcamera::FrameBufferAllocator> allocator_; > > >> std::vector<libcamera::FrameBuffer *> buffers_; > > -- > Regards, > > Laurent Pinchart
diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp index 28a6e09..9644b74 100644 --- a/src/android/camera_stream.cpp +++ b/src/android/camera_stream.cpp @@ -38,13 +38,12 @@ LOG_DECLARE_CATEGORY(HAL); * and buffer allocation. */ -CameraStream::CameraStream(CameraDevice *cameraDevice, Type type, +CameraStream::CameraStream(CameraDevice const *cameraDevice, Type type, camera3_stream_t *camera3Stream, unsigned int index) - : cameraDevice_(cameraDevice), type_(type), + : cameraDevice_(cameraDevice), + config_(cameraDevice->cameraConfiguration()), type_(type), camera3Stream_(camera3Stream), index_(index) { - config_ = cameraDevice_->cameraConfiguration(); - if (type_ == Type::Internal || type_ == Type::Mapped) { /* * \todo There might be multiple post-processors. The logic diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h index c367a5f..23c139d 100644 --- a/src/android/camera_stream.h +++ b/src/android/camera_stream.h @@ -109,7 +109,7 @@ public: Internal, Mapped, }; - CameraStream(CameraDevice *cameraDevice, Type type, + CameraStream(CameraDevice const *cameraDevice, Type type, camera3_stream_t *camera3Stream, unsigned int index); Type type() const { return type_; } @@ -124,11 +124,11 @@ public: void putBuffer(libcamera::FrameBuffer *buffer); private: - CameraDevice *cameraDevice_; - libcamera::CameraConfiguration *config_; - Type type_; + CameraDevice const *cameraDevice_; + const libcamera::CameraConfiguration *config_; + const Type type_; camera3_stream_t *camera3Stream_; - unsigned int index_; + const unsigned int index_; std::unique_ptr<libcamera::FrameBufferAllocator> allocator_; std::vector<libcamera::FrameBuffer *> buffers_;
CameraStream initializes several member variables in the initializer list. Some of them are unchanged after. This makes them constant. Especially, doing to |cameraDevice_| represents CameraStream doesn't have the ownership of it. Signed-off-by: Hirokazu Honda <hiroh@chromium.org> --- src/android/camera_stream.cpp | 7 +++---- src/android/camera_stream.h | 10 +++++----- 2 files changed, 8 insertions(+), 9 deletions(-)