[libcamera-devel,1/2] android: CameraDevice: Mark getResultMetadata() const function
diff mbox series

Message ID 20210325111357.3862847-1-hiroh@chromium.org
State Accepted
Headers show
Series
  • [libcamera-devel,1/2] android: CameraDevice: Mark getResultMetadata() const function
Related show

Commit Message

Hirokazu Honda March 25, 2021, 11:13 a.m. UTC 
CameraDevice::getResultMetadata() doesn't change either
|descriptor| and member variables. It should be marked as a
const function and |descriptor| should be passed with const
lvalue reference.

Signed-off-by: Hirokazu Honda <hiroh@chromium.org>
---
 src/android/camera_device.cpp | 10 +++++-----
 src/android/camera_device.h   |  3 ++-
 2 files changed, 7 insertions(+), 6 deletions(-)

--
2.31.0.291.g576ba9dcdaf-goog

Comments

Laurent Pinchart March 25, 2021, 2:52 p.m. UTC | #1 
Hi Hiro,

Thank you for the patch.

On Thu, Mar 25, 2021 at 08:13:56PM +0900, Hirokazu Honda wrote:
> CameraDevice::getResultMetadata() doesn't change either
> |descriptor| and member variables. It should be marked as a
> const function and |descriptor| should be passed with const
> lvalue reference.

The patch itself looks good to me. I'd however like Jacopo's feedback,
as he has designed these classes, regarding whether he foresees a need
to modify the descriptor in that function in the near future.

If Jacopo is fine with the patch, you can also add my

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> Signed-off-by: Hirokazu Honda <hiroh@chromium.org>
> ---
>  src/android/camera_device.cpp | 10 +++++-----
>  src/android/camera_device.h   |  3 ++-
>  2 files changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> index 5fbf6f82..ae693664 100644
> --- a/src/android/camera_device.cpp
> +++ b/src/android/camera_device.cpp
> @@ -1924,7 +1924,7 @@ void CameraDevice::requestComplete(Request *request)
>  	 * pipeline handlers) timestamp in the Request itself.
>  	 */
>  	uint64_t timestamp = buffers.begin()->second->metadata().timestamp;
> -	resultMetadata = getResultMetadata(descriptor, timestamp);
> +	resultMetadata = getResultMetadata(*descriptor, timestamp);
> 
>  	/* Handle any JPEG compression. */
>  	for (camera3_stream_buffer_t &buffer : descriptor->buffers_) {
> @@ -2030,11 +2030,11 @@ void CameraDevice::notifyError(uint32_t frameNumber, camera3_stream_t *stream)
>   * Produce a set of fixed result metadata.
>   */
>  std::unique_ptr<CameraMetadata>
> -CameraDevice::getResultMetadata(Camera3RequestDescriptor *descriptor,
> -				int64_t timestamp)
> +CameraDevice::getResultMetadata(const Camera3RequestDescriptor &descriptor,
> +				int64_t timestamp) const
>  {
> -	const ControlList &metadata = descriptor->request_->metadata();
> -	const CameraMetadata &settings = descriptor->settings_;
> +	const ControlList &metadata = descriptor.request_->metadata();
> +	const CameraMetadata &settings = descriptor.settings_;
>  	camera_metadata_ro_entry_t entry;
>  	bool found;
> 
> diff --git a/src/android/camera_device.h b/src/android/camera_device.h
> index 09c395ff..11bdfec8 100644
> --- a/src/android/camera_device.h
> +++ b/src/android/camera_device.h
> @@ -102,7 +102,8 @@ private:
>  	libcamera::PixelFormat toPixelFormat(int format) const;
>  	int processControls(Camera3RequestDescriptor *descriptor);
>  	std::unique_ptr<CameraMetadata> getResultMetadata(
> -		Camera3RequestDescriptor *descriptor, int64_t timestamp);
> +		const Camera3RequestDescriptor &descriptor,
> +		int64_t timestamp) const;
> 
>  	unsigned int id_;
>  	camera3_device_t camera3Device_;
Jacopo Mondi March 25, 2021, 4:04 p.m. UTC | #2 
Hi Laurent,

On Thu, Mar 25, 2021 at 04:52:23PM +0200, Laurent Pinchart wrote:
> Hi Hiro,
>
> Thank you for the patch.
>
> On Thu, Mar 25, 2021 at 08:13:56PM +0900, Hirokazu Honda wrote:
> > CameraDevice::getResultMetadata() doesn't change either
> > |descriptor| and member variables. It should be marked as a
> > const function and |descriptor| should be passed with const
> > lvalue reference.
>
> The patch itself looks good to me. I'd however like Jacopo's feedback,
> as he has designed these classes, regarding whether he foresees a need
> to modify the descriptor in that function in the near future.

As far as I can tell this should not happen.
In any case we can rollback if that need arise in future.

>
> If Jacopo is fine with the patch, you can also add my
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>

Thanks
  j

>
> > Signed-off-by: Hirokazu Honda <hiroh@chromium.org>
> > ---
> >  src/android/camera_device.cpp | 10 +++++-----
> >  src/android/camera_device.h   |  3 ++-
> >  2 files changed, 7 insertions(+), 6 deletions(-)
> >
> > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> > index 5fbf6f82..ae693664 100644
> > --- a/src/android/camera_device.cpp
> > +++ b/src/android/camera_device.cpp
> > @@ -1924,7 +1924,7 @@ void CameraDevice::requestComplete(Request *request)
> >  	 * pipeline handlers) timestamp in the Request itself.
> >  	 */
> >  	uint64_t timestamp = buffers.begin()->second->metadata().timestamp;
> > -	resultMetadata = getResultMetadata(descriptor, timestamp);
> > +	resultMetadata = getResultMetadata(*descriptor, timestamp);
> >
> >  	/* Handle any JPEG compression. */
> >  	for (camera3_stream_buffer_t &buffer : descriptor->buffers_) {
> > @@ -2030,11 +2030,11 @@ void CameraDevice::notifyError(uint32_t frameNumber, camera3_stream_t *stream)
> >   * Produce a set of fixed result metadata.
> >   */
> >  std::unique_ptr<CameraMetadata>
> > -CameraDevice::getResultMetadata(Camera3RequestDescriptor *descriptor,
> > -				int64_t timestamp)
> > +CameraDevice::getResultMetadata(const Camera3RequestDescriptor &descriptor,
> > +				int64_t timestamp) const
> >  {
> > -	const ControlList &metadata = descriptor->request_->metadata();
> > -	const CameraMetadata &settings = descriptor->settings_;
> > +	const ControlList &metadata = descriptor.request_->metadata();
> > +	const CameraMetadata &settings = descriptor.settings_;
> >  	camera_metadata_ro_entry_t entry;
> >  	bool found;
> >
> > diff --git a/src/android/camera_device.h b/src/android/camera_device.h
> > index 09c395ff..11bdfec8 100644
> > --- a/src/android/camera_device.h
> > +++ b/src/android/camera_device.h
> > @@ -102,7 +102,8 @@ private:
> >  	libcamera::PixelFormat toPixelFormat(int format) const;
> >  	int processControls(Camera3RequestDescriptor *descriptor);
> >  	std::unique_ptr<CameraMetadata> getResultMetadata(
> > -		Camera3RequestDescriptor *descriptor, int64_t timestamp);
> > +		const Camera3RequestDescriptor &descriptor,
> > +		int64_t timestamp) const;
> >
> >  	unsigned int id_;
> >  	camera3_device_t camera3Device_;
>
> --
> Regards,
>
> Laurent Pinchart
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

Patch
diff mbox series 

diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
index 5fbf6f82..ae693664 100644
--- a/src/android/camera_device.cpp
+++ b/src/android/camera_device.cpp
@@ -1924,7 +1924,7 @@  void CameraDevice::requestComplete(Request *request)
 	 * pipeline handlers) timestamp in the Request itself.
 	 */
 	uint64_t timestamp = buffers.begin()->second->metadata().timestamp;
-	resultMetadata = getResultMetadata(descriptor, timestamp);
+	resultMetadata = getResultMetadata(*descriptor, timestamp);

 	/* Handle any JPEG compression. */
 	for (camera3_stream_buffer_t &buffer : descriptor->buffers_) {
@@ -2030,11 +2030,11 @@  void CameraDevice::notifyError(uint32_t frameNumber, camera3_stream_t *stream)
  * Produce a set of fixed result metadata.
  */
 std::unique_ptr<CameraMetadata>
-CameraDevice::getResultMetadata(Camera3RequestDescriptor *descriptor,
-				int64_t timestamp)
+CameraDevice::getResultMetadata(const Camera3RequestDescriptor &descriptor,
+				int64_t timestamp) const
 {
-	const ControlList &metadata = descriptor->request_->metadata();
-	const CameraMetadata &settings = descriptor->settings_;
+	const ControlList &metadata = descriptor.request_->metadata();
+	const CameraMetadata &settings = descriptor.settings_;
 	camera_metadata_ro_entry_t entry;
 	bool found;

diff --git a/src/android/camera_device.h b/src/android/camera_device.h
index 09c395ff..11bdfec8 100644
--- a/src/android/camera_device.h
+++ b/src/android/camera_device.h
@@ -102,7 +102,8 @@  private:
 	libcamera::PixelFormat toPixelFormat(int format) const;
 	int processControls(Camera3RequestDescriptor *descriptor);
 	std::unique_ptr<CameraMetadata> getResultMetadata(
-		Camera3RequestDescriptor *descriptor, int64_t timestamp);
+		const Camera3RequestDescriptor &descriptor,
+		int64_t timestamp) const;

 	unsigned int id_;
 	camera3_device_t camera3Device_;