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

[Hirokazu Honda]

Hi Uman, Kierna and Laurent,

On Wed, Oct 21, 2020 at 1:59 AM Laurent Pinchart
<laurent.pinchart at 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 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.
>

Thanks for the comments.
Ack.

Best Regards,
-Hiro


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