[libcamera-devel] [PATCH v2 4/4] android: camera_stream: Make some member variables constant

[Laurent Pinchart]

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 at 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 at ideasonboard.com>
>
> > Reviewed-by: Umang Jain <email at 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