[libcamera-devel,02/11] android: camera_request: Turn struct into a class
diff mbox series

Message ID 20211018132923.476242-3-umang.jain@ideasonboard.com
State Superseded
Delegated to: Umang Jain
Headers show
Series
  • android: Overhaul request handling
Related show

Commit Message

Umang Jain Oct. 18, 2021, 1:29 p.m. UTC 
From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

The Camera3RequestDescriptor structure is growing into an object with
member functions. Turn it into a class, uninline the destructor to
reduce code size, explicitly disable copy as requests are not copyable,
and delete the default constructor to force all instances to be fully
constructed.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 src/android/camera_device.h    |  2 +-
 src/android/camera_request.cpp |  8 +++++---
 src/android/camera_request.h   | 13 +++++++++----
 3 files changed, 15 insertions(+), 8 deletions(-)

Comments

Jacopo Mondi Oct. 18, 2021, 3:29 p.m. UTC | #1 
Hi Laurent,

On Mon, Oct 18, 2021 at 06:59:14PM +0530, Umang Jain wrote:
> From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>
> The Camera3RequestDescriptor structure is growing into an object with
> member functions. Turn it into a class, uninline the destructor to
> reduce code size, explicitly disable copy as requests are not copyable,
> and delete the default constructor to force all instances to be fully
> constructed.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  src/android/camera_device.h    |  2 +-
>  src/android/camera_request.cpp |  8 +++++---
>  src/android/camera_request.h   | 13 +++++++++----
>  3 files changed, 15 insertions(+), 8 deletions(-)
>
> diff --git a/src/android/camera_device.h b/src/android/camera_device.h
> index 86224aa1..863cf414 100644
> --- a/src/android/camera_device.h
> +++ b/src/android/camera_device.h
> @@ -33,7 +33,7 @@
>  #include "camera_worker.h"
>  #include "jpeg/encoder.h"
>
> -struct Camera3RequestDescriptor;
> +class Camera3RequestDescriptor;
>  struct CameraConfigData;
>
>  class CameraDevice : protected libcamera::Loggable
> diff --git a/src/android/camera_request.cpp b/src/android/camera_request.cpp
> index 93e546bf..16a632b3 100644
> --- a/src/android/camera_request.cpp
> +++ b/src/android/camera_request.cpp
> @@ -10,10 +10,10 @@
>  using namespace libcamera;
>
>  /*
> - * \struct Camera3RequestDescriptor
> + * \class Camera3RequestDescriptor
>   *
> - * A utility structure that groups information about a capture request to be
> - * later re-used at request complete time to notify the framework.
> + * A utility class that groups information about a capture request to be later
> + * reused at request complete time to notify the framework.
>   */
>
>  Camera3RequestDescriptor::Camera3RequestDescriptor(
> @@ -43,3 +43,5 @@ Camera3RequestDescriptor::Camera3RequestDescriptor(
>  	request_ = std::make_unique<CaptureRequest>(camera,
>  						    reinterpret_cast<uint64_t>(this));
>  }
> +
> +Camera3RequestDescriptor::~Camera3RequestDescriptor() = default;

We should have done this for most classes probably...

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

Thanks
  j

> diff --git a/src/android/camera_request.h b/src/android/camera_request.h
> index 1346f6fa..79dfdb58 100644
> --- a/src/android/camera_request.h
> +++ b/src/android/camera_request.h
> @@ -10,6 +10,8 @@
>  #include <memory>
>  #include <vector>
>
> +#include <libcamera/base/class.h>
> +
>  #include <libcamera/camera.h>
>  #include <libcamera/framebuffer.h>
>
> @@ -18,18 +20,18 @@
>  #include "camera_metadata.h"
>  #include "camera_worker.h"
>
> -struct Camera3RequestDescriptor {
> +class Camera3RequestDescriptor
> +{
> +public:
>  	enum class Status {
>  		Pending,
>  		Success,
>  		Error,
>  	};
>
> -	Camera3RequestDescriptor() = default;
> -	~Camera3RequestDescriptor() = default;
>  	Camera3RequestDescriptor(libcamera::Camera *camera,
>  				 const camera3_capture_request_t *camera3Request);
> -	Camera3RequestDescriptor &operator=(Camera3RequestDescriptor &&) = default;
> +	~Camera3RequestDescriptor();
>
>  	bool isPending() const { return status_ == Status::Pending; }
>
> @@ -41,6 +43,9 @@ struct Camera3RequestDescriptor {
>
>  	camera3_capture_result_t captureResult_ = {};
>  	Status status_ = Status::Pending;
> +
> +private:
> +	LIBCAMERA_DISABLE_COPY(Camera3RequestDescriptor)
>  };
>
>  #endif /* __ANDROID_CAMERA_REQUEST_H__ */
> --
> 2.31.0
>
Hirokazu Honda Oct. 19, 2021, 4:03 a.m. UTC | #2 
HI Umang and Laurent, thank you for the patch.

On Mon, Oct 18, 2021 at 10:29 PM Umang Jain <umang.jain@ideasonboard.com> wrote:
>
> From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>
> The Camera3RequestDescriptor structure is growing into an object with
> member functions. Turn it into a class, uninline the destructor to
> reduce code size, explicitly disable copy as requests are not copyable,
> and delete the default constructor to force all instances to be fully
> constructed.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  src/android/camera_device.h    |  2 +-
>  src/android/camera_request.cpp |  8 +++++---
>  src/android/camera_request.h   | 13 +++++++++----
>  3 files changed, 15 insertions(+), 8 deletions(-)
>
> diff --git a/src/android/camera_device.h b/src/android/camera_device.h
> index 86224aa1..863cf414 100644
> --- a/src/android/camera_device.h
> +++ b/src/android/camera_device.h
> @@ -33,7 +33,7 @@
>  #include "camera_worker.h"
>  #include "jpeg/encoder.h"
>
> -struct Camera3RequestDescriptor;
> +class Camera3RequestDescriptor;
>  struct CameraConfigData;
>
>  class CameraDevice : protected libcamera::Loggable
> diff --git a/src/android/camera_request.cpp b/src/android/camera_request.cpp
> index 93e546bf..16a632b3 100644
> --- a/src/android/camera_request.cpp
> +++ b/src/android/camera_request.cpp
> @@ -10,10 +10,10 @@
>  using namespace libcamera;
>
>  /*
> - * \struct Camera3RequestDescriptor
> + * \class Camera3RequestDescriptor
>   *
> - * A utility structure that groups information about a capture request to be
> - * later re-used at request complete time to notify the framework.
> + * A utility class that groups information about a capture request to be later
> + * reused at request complete time to notify the framework.
>   */
>
>  Camera3RequestDescriptor::Camera3RequestDescriptor(
> @@ -43,3 +43,5 @@ Camera3RequestDescriptor::Camera3RequestDescriptor(
>         request_ = std::make_unique<CaptureRequest>(camera,
>                                                     reinterpret_cast<uint64_t>(this));
>  }
> +
> +Camera3RequestDescriptor::~Camera3RequestDescriptor() = default;
> diff --git a/src/android/camera_request.h b/src/android/camera_request.h
> index 1346f6fa..79dfdb58 100644
> --- a/src/android/camera_request.h
> +++ b/src/android/camera_request.h
> @@ -10,6 +10,8 @@
>  #include <memory>
>  #include <vector>
>
> +#include <libcamera/base/class.h>
> +
>  #include <libcamera/camera.h>
>  #include <libcamera/framebuffer.h>
>
> @@ -18,18 +20,18 @@
>  #include "camera_metadata.h"
>  #include "camera_worker.h"
>
> -struct Camera3RequestDescriptor {
> +class Camera3RequestDescriptor
> +{
> +public:
>         enum class Status {
>                 Pending,
>                 Success,
>                 Error,
>         };
>
> -       Camera3RequestDescriptor() = default;
> -       ~Camera3RequestDescriptor() = default;
>         Camera3RequestDescriptor(libcamera::Camera *camera,
>                                  const camera3_capture_request_t *camera3Request);
> -       Camera3RequestDescriptor &operator=(Camera3RequestDescriptor &&) = default;
> +       ~Camera3RequestDescriptor();
>
>         bool isPending() const { return status_ == Status::Pending; }
>
> @@ -41,6 +43,9 @@ struct Camera3RequestDescriptor {
>
>         camera3_capture_result_t captureResult_ = {};
>         Status status_ = Status::Pending;
> +
> +private:

This private is unnecessary.

Reviewed-by: Hirokazu Honda <hiroh@chromium.org>

> +       LIBCAMERA_DISABLE_COPY(Camera3RequestDescriptor)
>  };
>
>  #endif /* __ANDROID_CAMERA_REQUEST_H__ */
> --
> 2.31.0
>
Laurent Pinchart Oct. 19, 2021, 6:44 a.m. UTC | #3 
Hi Hiro,

On Tue, Oct 19, 2021 at 01:03:45PM +0900, Hirokazu Honda wrote:
> On Mon, Oct 18, 2021 at 10:29 PM Umang Jain wrote:
> >
> > From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >
> > The Camera3RequestDescriptor structure is growing into an object with
> > member functions. Turn it into a class, uninline the destructor to
> > reduce code size, explicitly disable copy as requests are not copyable,
> > and delete the default constructor to force all instances to be fully
> > constructed.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> >  src/android/camera_device.h    |  2 +-
> >  src/android/camera_request.cpp |  8 +++++---
> >  src/android/camera_request.h   | 13 +++++++++----
> >  3 files changed, 15 insertions(+), 8 deletions(-)
> >
> > diff --git a/src/android/camera_device.h b/src/android/camera_device.h
> > index 86224aa1..863cf414 100644
> > --- a/src/android/camera_device.h
> > +++ b/src/android/camera_device.h
> > @@ -33,7 +33,7 @@
> >  #include "camera_worker.h"
> >  #include "jpeg/encoder.h"
> >
> > -struct Camera3RequestDescriptor;
> > +class Camera3RequestDescriptor;
> >  struct CameraConfigData;
> >
> >  class CameraDevice : protected libcamera::Loggable
> > diff --git a/src/android/camera_request.cpp b/src/android/camera_request.cpp
> > index 93e546bf..16a632b3 100644
> > --- a/src/android/camera_request.cpp
> > +++ b/src/android/camera_request.cpp
> > @@ -10,10 +10,10 @@
> >  using namespace libcamera;
> >
> >  /*
> > - * \struct Camera3RequestDescriptor
> > + * \class Camera3RequestDescriptor
> >   *
> > - * A utility structure that groups information about a capture request to be
> > - * later re-used at request complete time to notify the framework.
> > + * A utility class that groups information about a capture request to be later
> > + * reused at request complete time to notify the framework.
> >   */
> >
> >  Camera3RequestDescriptor::Camera3RequestDescriptor(
> > @@ -43,3 +43,5 @@ Camera3RequestDescriptor::Camera3RequestDescriptor(
> >         request_ = std::make_unique<CaptureRequest>(camera,
> >                                                     reinterpret_cast<uint64_t>(this));
> >  }
> > +
> > +Camera3RequestDescriptor::~Camera3RequestDescriptor() = default;
> > diff --git a/src/android/camera_request.h b/src/android/camera_request.h
> > index 1346f6fa..79dfdb58 100644
> > --- a/src/android/camera_request.h
> > +++ b/src/android/camera_request.h
> > @@ -10,6 +10,8 @@
> >  #include <memory>
> >  #include <vector>
> >
> > +#include <libcamera/base/class.h>
> > +
> >  #include <libcamera/camera.h>
> >  #include <libcamera/framebuffer.h>
> >
> > @@ -18,18 +20,18 @@
> >  #include "camera_metadata.h"
> >  #include "camera_worker.h"
> >
> > -struct Camera3RequestDescriptor {
> > +class Camera3RequestDescriptor
> > +{
> > +public:
> >         enum class Status {
> >                 Pending,
> >                 Success,
> >                 Error,
> >         };
> >
> > -       Camera3RequestDescriptor() = default;
> > -       ~Camera3RequestDescriptor() = default;
> >         Camera3RequestDescriptor(libcamera::Camera *camera,
> >                                  const camera3_capture_request_t *camera3Request);
> > -       Camera3RequestDescriptor &operator=(Camera3RequestDescriptor &&) = default;
> > +       ~Camera3RequestDescriptor();
> >
> >         bool isPending() const { return status_ == Status::Pending; }
> >
> > @@ -41,6 +43,9 @@ struct Camera3RequestDescriptor {
> >
> >         camera3_capture_result_t captureResult_ = {};
> >         Status status_ = Status::Pending;
> > +
> > +private:
> 
> This private is unnecessary.

It's not strictly necessary indeed, but we put the
LIBCAMERA_DISABLE_COPY macro in a private block everywhere, mostly as a
coding convention.

> Reviewed-by: Hirokazu Honda <hiroh@chromium.org>
> 
> > +       LIBCAMERA_DISABLE_COPY(Camera3RequestDescriptor)
> >  };
> >
> >  #endif /* __ANDROID_CAMERA_REQUEST_H__ */

Patch
diff mbox series 

diff --git a/src/android/camera_device.h b/src/android/camera_device.h
index 86224aa1..863cf414 100644
--- a/src/android/camera_device.h
+++ b/src/android/camera_device.h
@@ -33,7 +33,7 @@ 
 #include "camera_worker.h"
 #include "jpeg/encoder.h"
 
-struct Camera3RequestDescriptor;
+class Camera3RequestDescriptor;
 struct CameraConfigData;
 
 class CameraDevice : protected libcamera::Loggable
diff --git a/src/android/camera_request.cpp b/src/android/camera_request.cpp
index 93e546bf..16a632b3 100644
--- a/src/android/camera_request.cpp
+++ b/src/android/camera_request.cpp
@@ -10,10 +10,10 @@ 
 using namespace libcamera;
 
 /*
- * \struct Camera3RequestDescriptor
+ * \class Camera3RequestDescriptor
  *
- * A utility structure that groups information about a capture request to be
- * later re-used at request complete time to notify the framework.
+ * A utility class that groups information about a capture request to be later
+ * reused at request complete time to notify the framework.
  */
 
 Camera3RequestDescriptor::Camera3RequestDescriptor(
@@ -43,3 +43,5 @@  Camera3RequestDescriptor::Camera3RequestDescriptor(
 	request_ = std::make_unique<CaptureRequest>(camera,
 						    reinterpret_cast<uint64_t>(this));
 }
+
+Camera3RequestDescriptor::~Camera3RequestDescriptor() = default;
diff --git a/src/android/camera_request.h b/src/android/camera_request.h
index 1346f6fa..79dfdb58 100644
--- a/src/android/camera_request.h
+++ b/src/android/camera_request.h
@@ -10,6 +10,8 @@ 
 #include <memory>
 #include <vector>
 
+#include <libcamera/base/class.h>
+
 #include <libcamera/camera.h>
 #include <libcamera/framebuffer.h>
 
@@ -18,18 +20,18 @@ 
 #include "camera_metadata.h"
 #include "camera_worker.h"
 
-struct Camera3RequestDescriptor {
+class Camera3RequestDescriptor
+{
+public:
 	enum class Status {
 		Pending,
 		Success,
 		Error,
 	};
 
-	Camera3RequestDescriptor() = default;
-	~Camera3RequestDescriptor() = default;
 	Camera3RequestDescriptor(libcamera::Camera *camera,
 				 const camera3_capture_request_t *camera3Request);
-	Camera3RequestDescriptor &operator=(Camera3RequestDescriptor &&) = default;
+	~Camera3RequestDescriptor();
 
 	bool isPending() const { return status_ == Status::Pending; }
 
@@ -41,6 +43,9 @@  struct Camera3RequestDescriptor {
 
 	camera3_capture_result_t captureResult_ = {};
 	Status status_ = Status::Pending;
+
+private:
+	LIBCAMERA_DISABLE_COPY(Camera3RequestDescriptor)
 };
 
 #endif /* __ANDROID_CAMERA_REQUEST_H__ */