[libcamera-devel] [PATCH v2 4/4] android: camera_stream: Make some member variables constant
Kieran Bingham
kieran.bingham at ideasonboard.com
Tue Oct 20 17:28:01 CEST 2020
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 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.
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_;
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
--
Regards
--
Kieran
More information about the libcamera-devel
mailing list