Message ID | 20211018132923.476242-3-umang.jain@ideasonboard.com |
---|---|
State | Superseded |
Delegated to: | Umang Jain |
Headers | show |
Series |
|
Related | show |
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 >
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 >
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__ */
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__ */