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

[Kieran Bingham]

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