[libcamera-devel,v2,11/12] android: camera_stream: Define explicit move constructor and destructors
diff mbox series

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

Commit Message

Umang Jain Oct. 19, 2021, 11:48 a.m. UTC 
From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

There's no need for the move constructor and the destructor to be
inline. Define them explicitly, with default implementations. This
allows usage of the CameraStream class without a complete definition of
the PostProcessor class.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Umang Jain<umang.jain@ideasonboard.com>
Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
---
 src/android/camera_stream.cpp | 4 ++++
 src/android/camera_stream.h   | 2 ++
 2 files changed, 6 insertions(+)

Comments

Hirokazu Honda Oct. 20, 2021, 2:18 a.m. UTC | #1 
Hi Laurent and Umang, thank you for the patch.


On Tue, Oct 19, 2021 at 8:48 PM Umang Jain <umang.jain@ideasonboard.com> wrote:
>
> From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>
> There's no need for the move constructor and the destructor to be
> inline. Define them explicitly, with default implementations. This
> allows usage of the CameraStream class without a complete definition of
> the PostProcessor class.
>

What is complete definition meant here?

-Hiro
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Reviewed-by: Umang Jain<umang.jain@ideasonboard.com>
> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
>  src/android/camera_stream.cpp | 4 ++++
>  src/android/camera_stream.h   | 2 ++
>  2 files changed, 6 insertions(+)
>
> diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp
> index 8e6ccb83..f44a2717 100644
> --- a/src/android/camera_stream.cpp
> +++ b/src/android/camera_stream.cpp
> @@ -56,6 +56,10 @@ CameraStream::CameraStream(CameraDevice *const cameraDevice,
>  {
>  }
>
> +CameraStream::CameraStream(CameraStream &&other) = default;
> +
> +CameraStream::~CameraStream() = default;
> +
>  const StreamConfiguration &CameraStream::configuration() const
>  {
>         return config_->at(index_);
> diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h
> index 85064268..f242336e 100644
> --- a/src/android/camera_stream.h
> +++ b/src/android/camera_stream.h
> @@ -112,6 +112,8 @@ public:
>         CameraStream(CameraDevice *const cameraDevice,
>                      libcamera::CameraConfiguration *config, Type type,
>                      camera3_stream_t *camera3Stream, unsigned int index);
> +       CameraStream(CameraStream &&other);
> +       ~CameraStream();
>
>         Type type() const { return type_; }
>         camera3_stream_t *camera3Stream() const { return camera3Stream_; }
> --
> 2.31.0
>
Laurent Pinchart Oct. 20, 2021, 2:40 a.m. UTC | #2 
Hi Hiro,

On Wed, Oct 20, 2021 at 11:18:42AM +0900, Hirokazu Honda wrote:
> On Tue, Oct 19, 2021 at 8:48 PM Umang Jain <umang.jain@ideasonboard.com> wrote:
> >
> > From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >
> > There's no need for the move constructor and the destructor to be
> > inline. Define them explicitly, with default implementations. This
> > allows usage of the CameraStream class without a complete definition of
> > the PostProcessor class.
> 
> What is complete definition meant here?

It means a full definition of the class (as obtained by including
post_processor.h), as opposed to a forward declaration.

> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > Reviewed-by: Umang Jain<umang.jain@ideasonboard.com>
> > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
> > ---
> >  src/android/camera_stream.cpp | 4 ++++
> >  src/android/camera_stream.h   | 2 ++
> >  2 files changed, 6 insertions(+)
> >
> > diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp
> > index 8e6ccb83..f44a2717 100644
> > --- a/src/android/camera_stream.cpp
> > +++ b/src/android/camera_stream.cpp
> > @@ -56,6 +56,10 @@ CameraStream::CameraStream(CameraDevice *const cameraDevice,
> >  {
> >  }
> >
> > +CameraStream::CameraStream(CameraStream &&other) = default;
> > +
> > +CameraStream::~CameraStream() = default;
> > +
> >  const StreamConfiguration &CameraStream::configuration() const
> >  {
> >         return config_->at(index_);
> > diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h
> > index 85064268..f242336e 100644
> > --- a/src/android/camera_stream.h
> > +++ b/src/android/camera_stream.h
> > @@ -112,6 +112,8 @@ public:
> >         CameraStream(CameraDevice *const cameraDevice,
> >                      libcamera::CameraConfiguration *config, Type type,
> >                      camera3_stream_t *camera3Stream, unsigned int index);
> > +       CameraStream(CameraStream &&other);
> > +       ~CameraStream();
> >
> >         Type type() const { return type_; }
> >         camera3_stream_t *camera3Stream() const { return camera3Stream_; }
Hirokazu Honda Oct. 20, 2021, 3:08 a.m. UTC | #3 
Hi Laurent,

On Wed, Oct 20, 2021 at 11:40 AM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Hiro,
>
> On Wed, Oct 20, 2021 at 11:18:42AM +0900, Hirokazu Honda wrote:
> > On Tue, Oct 19, 2021 at 8:48 PM Umang Jain <umang.jain@ideasonboard.com> wrote:
> > >
> > > From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > >
> > > There's no need for the move constructor and the destructor to be
> > > inline. Define them explicitly, with default implementations. This
> > > allows usage of the CameraStream class without a complete definition of
> > > the PostProcessor class.
> >
> > What is complete definition meant here?
>
> It means a full definition of the class (as obtained by including
> post_processor.h), as opposed to a forward declaration.
>

Hmm, sorry I don't understand..
Could you tell me what concretely becomes possible?

-Hiro
> > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > Reviewed-by: Umang Jain<umang.jain@ideasonboard.com>
> > > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
> > > ---
> > >  src/android/camera_stream.cpp | 4 ++++
> > >  src/android/camera_stream.h   | 2 ++
> > >  2 files changed, 6 insertions(+)
> > >
> > > diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp
> > > index 8e6ccb83..f44a2717 100644
> > > --- a/src/android/camera_stream.cpp
> > > +++ b/src/android/camera_stream.cpp
> > > @@ -56,6 +56,10 @@ CameraStream::CameraStream(CameraDevice *const cameraDevice,
> > >  {
> > >  }
> > >
> > > +CameraStream::CameraStream(CameraStream &&other) = default;
> > > +
> > > +CameraStream::~CameraStream() = default;
> > > +
> > >  const StreamConfiguration &CameraStream::configuration() const
> > >  {
> > >         return config_->at(index_);
> > > diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h
> > > index 85064268..f242336e 100644
> > > --- a/src/android/camera_stream.h
> > > +++ b/src/android/camera_stream.h
> > > @@ -112,6 +112,8 @@ public:
> > >         CameraStream(CameraDevice *const cameraDevice,
> > >                      libcamera::CameraConfiguration *config, Type type,
> > >                      camera3_stream_t *camera3Stream, unsigned int index);
> > > +       CameraStream(CameraStream &&other);
> > > +       ~CameraStream();
> > >
> > >         Type type() const { return type_; }
> > >         camera3_stream_t *camera3Stream() const { return camera3Stream_; }
>
> --
> Regards,
>
> Laurent Pinchart
Laurent Pinchart Oct. 20, 2021, 3:10 a.m. UTC | #4 
On Wed, Oct 20, 2021 at 12:08:21PM +0900, Hirokazu Honda wrote:
> On Wed, Oct 20, 2021 at 11:40 AM Laurent Pinchart wrote:
> > On Wed, Oct 20, 2021 at 11:18:42AM +0900, Hirokazu Honda wrote:
> > > On Tue, Oct 19, 2021 at 8:48 PM Umang Jain <umang.jain@ideasonboard.com> wrote:
> > > >
> > > > From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > >
> > > > There's no need for the move constructor and the destructor to be
> > > > inline. Define them explicitly, with default implementations. This
> > > > allows usage of the CameraStream class without a complete definition of
> > > > the PostProcessor class.
> > >
> > > What is complete definition meant here?
> >
> > It means a full definition of the class (as obtained by including
> > post_processor.h), as opposed to a forward declaration.
> 
> Hmm, sorry I don't understand..
> Could you tell me what concretely becomes possible?

It's now possible to use the CameraStream class without having to
include post_processor.h. The PostProcessor class can be
forward-declared in camera_stream.h instead.

> > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > > Reviewed-by: Umang Jain<umang.jain@ideasonboard.com>
> > > > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
> > > > ---
> > > >  src/android/camera_stream.cpp | 4 ++++
> > > >  src/android/camera_stream.h   | 2 ++
> > > >  2 files changed, 6 insertions(+)
> > > >
> > > > diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp
> > > > index 8e6ccb83..f44a2717 100644
> > > > --- a/src/android/camera_stream.cpp
> > > > +++ b/src/android/camera_stream.cpp
> > > > @@ -56,6 +56,10 @@ CameraStream::CameraStream(CameraDevice *const cameraDevice,
> > > >  {
> > > >  }
> > > >
> > > > +CameraStream::CameraStream(CameraStream &&other) = default;
> > > > +
> > > > +CameraStream::~CameraStream() = default;
> > > > +
> > > >  const StreamConfiguration &CameraStream::configuration() const
> > > >  {
> > > >         return config_->at(index_);
> > > > diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h
> > > > index 85064268..f242336e 100644
> > > > --- a/src/android/camera_stream.h
> > > > +++ b/src/android/camera_stream.h
> > > > @@ -112,6 +112,8 @@ public:
> > > >         CameraStream(CameraDevice *const cameraDevice,
> > > >                      libcamera::CameraConfiguration *config, Type type,
> > > >                      camera3_stream_t *camera3Stream, unsigned int index);
> > > > +       CameraStream(CameraStream &&other);
> > > > +       ~CameraStream();
> > > >
> > > >         Type type() const { return type_; }
> > > >         camera3_stream_t *camera3Stream() const { return camera3Stream_; }
Hirokazu Honda Oct. 20, 2021, 3:17 a.m. UTC | #5 
Hi Laurent,

On Wed, Oct 20, 2021 at 12:10 PM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> On Wed, Oct 20, 2021 at 12:08:21PM +0900, Hirokazu Honda wrote:
> > On Wed, Oct 20, 2021 at 11:40 AM Laurent Pinchart wrote:
> > > On Wed, Oct 20, 2021 at 11:18:42AM +0900, Hirokazu Honda wrote:
> > > > On Tue, Oct 19, 2021 at 8:48 PM Umang Jain <umang.jain@ideasonboard.com> wrote:
> > > > >
> > > > > From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > > >
> > > > > There's no need for the move constructor and the destructor to be
> > > > > inline. Define them explicitly, with default implementations. This
> > > > > allows usage of the CameraStream class without a complete definition of
> > > > > the PostProcessor class.
> > > >
> > > > What is complete definition meant here?
> > >
> > > It means a full definition of the class (as obtained by including
> > > post_processor.h), as opposed to a forward declaration.
> >
> > Hmm, sorry I don't understand..
> > Could you tell me what concretely becomes possible?
>
> It's now possible to use the CameraStream class without having to
> include post_processor.h. The PostProcessor class can be
> forward-declared in camera_stream.h instead.
>

Ah, I see. Thanks.
So shall we remove post_processor.h in camera_device.h?

> > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > > > Reviewed-by: Umang Jain<umang.jain@ideasonboard.com>
> > > > > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
> > > > > ---
> > > > >  src/android/camera_stream.cpp | 4 ++++
> > > > >  src/android/camera_stream.h   | 2 ++
> > > > >  2 files changed, 6 insertions(+)
> > > > >
> > > > > diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp
> > > > > index 8e6ccb83..f44a2717 100644
> > > > > --- a/src/android/camera_stream.cpp
> > > > > +++ b/src/android/camera_stream.cpp
> > > > > @@ -56,6 +56,10 @@ CameraStream::CameraStream(CameraDevice *const cameraDevice,
> > > > >  {
> > > > >  }
> > > > >
> > > > > +CameraStream::CameraStream(CameraStream &&other) = default;
> > > > > +
> > > > > +CameraStream::~CameraStream() = default;
> > > > > +
> > > > >  const StreamConfiguration &CameraStream::configuration() const
> > > > >  {
> > > > >         return config_->at(index_);
> > > > > diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h
> > > > > index 85064268..f242336e 100644
> > > > > --- a/src/android/camera_stream.h
> > > > > +++ b/src/android/camera_stream.h
> > > > > @@ -112,6 +112,8 @@ public:
> > > > >         CameraStream(CameraDevice *const cameraDevice,
> > > > >                      libcamera::CameraConfiguration *config, Type type,
> > > > >                      camera3_stream_t *camera3Stream, unsigned int index);
> > > > > +       CameraStream(CameraStream &&other);
> > > > > +       ~CameraStream();
> > > > >
> > > > >         Type type() const { return type_; }
> > > > >         camera3_stream_t *camera3Stream() const { return camera3Stream_; }
>
> --
> Regards,
>
> Laurent Pinchart
Laurent Pinchart Oct. 20, 2021, 3:29 a.m. UTC | #6 
Hi Hiro,

On Wed, Oct 20, 2021 at 12:17:02PM +0900, Hirokazu Honda wrote:
> On Wed, Oct 20, 2021 at 12:10 PM Laurent Pinchart wrote:
> > On Wed, Oct 20, 2021 at 12:08:21PM +0900, Hirokazu Honda wrote:
> > > On Wed, Oct 20, 2021 at 11:40 AM Laurent Pinchart wrote:
> > > > On Wed, Oct 20, 2021 at 11:18:42AM +0900, Hirokazu Honda wrote:
> > > > > On Tue, Oct 19, 2021 at 8:48 PM Umang Jain wrote:
> > > > > >
> > > > > > From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > > > >
> > > > > > There's no need for the move constructor and the destructor to be
> > > > > > inline. Define them explicitly, with default implementations. This
> > > > > > allows usage of the CameraStream class without a complete definition of
> > > > > > the PostProcessor class.
> > > > >
> > > > > What is complete definition meant here?
> > > >
> > > > It means a full definition of the class (as obtained by including
> > > > post_processor.h), as opposed to a forward declaration.
> > >
> > > Hmm, sorry I don't understand..
> > > Could you tell me what concretely becomes possible?
> >
> > It's now possible to use the CameraStream class without having to
> > include post_processor.h. The PostProcessor class can be
> > forward-declared in camera_stream.h instead.
> 
> Ah, I see. Thanks.
> So shall we remove post_processor.h in camera_device.h?

post_processor.h isn't included in camera_device.h. If you meant
camera_device.cpp, yes, I think it should be removed from there. I've
just sent a patch.

> > > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > > > > Reviewed-by: Umang Jain<umang.jain@ideasonboard.com>
> > > > > > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
> > > > > > ---
> > > > > >  src/android/camera_stream.cpp | 4 ++++
> > > > > >  src/android/camera_stream.h   | 2 ++
> > > > > >  2 files changed, 6 insertions(+)
> > > > > >
> > > > > > diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp
> > > > > > index 8e6ccb83..f44a2717 100644
> > > > > > --- a/src/android/camera_stream.cpp
> > > > > > +++ b/src/android/camera_stream.cpp
> > > > > > @@ -56,6 +56,10 @@ CameraStream::CameraStream(CameraDevice *const cameraDevice,
> > > > > >  {
> > > > > >  }
> > > > > >
> > > > > > +CameraStream::CameraStream(CameraStream &&other) = default;
> > > > > > +
> > > > > > +CameraStream::~CameraStream() = default;
> > > > > > +
> > > > > >  const StreamConfiguration &CameraStream::configuration() const
> > > > > >  {
> > > > > >         return config_->at(index_);
> > > > > > diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h
> > > > > > index 85064268..f242336e 100644
> > > > > > --- a/src/android/camera_stream.h
> > > > > > +++ b/src/android/camera_stream.h
> > > > > > @@ -112,6 +112,8 @@ public:
> > > > > >         CameraStream(CameraDevice *const cameraDevice,
> > > > > >                      libcamera::CameraConfiguration *config, Type type,
> > > > > >                      camera3_stream_t *camera3Stream, unsigned int index);
> > > > > > +       CameraStream(CameraStream &&other);
> > > > > > +       ~CameraStream();
> > > > > >
> > > > > >         Type type() const { return type_; }
> > > > > >         camera3_stream_t *camera3Stream() const { return camera3Stream_; }
Hirokazu Honda Oct. 20, 2021, 4:04 a.m. UTC | #7 
Hi Laurent,

On Wed, Oct 20, 2021 at 12:29 PM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Hiro,
>
> On Wed, Oct 20, 2021 at 12:17:02PM +0900, Hirokazu Honda wrote:
> > On Wed, Oct 20, 2021 at 12:10 PM Laurent Pinchart wrote:
> > > On Wed, Oct 20, 2021 at 12:08:21PM +0900, Hirokazu Honda wrote:
> > > > On Wed, Oct 20, 2021 at 11:40 AM Laurent Pinchart wrote:
> > > > > On Wed, Oct 20, 2021 at 11:18:42AM +0900, Hirokazu Honda wrote:
> > > > > > On Tue, Oct 19, 2021 at 8:48 PM Umang Jain wrote:
> > > > > > >
> > > > > > > From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > > > > >
> > > > > > > There's no need for the move constructor and the destructor to be
> > > > > > > inline. Define them explicitly, with default implementations. This
> > > > > > > allows usage of the CameraStream class without a complete definition of
> > > > > > > the PostProcessor class.
> > > > > >
> > > > > > What is complete definition meant here?
> > > > >
> > > > > It means a full definition of the class (as obtained by including
> > > > > post_processor.h), as opposed to a forward declaration.
> > > >
> > > > Hmm, sorry I don't understand..
> > > > Could you tell me what concretely becomes possible?
> > >
> > > It's now possible to use the CameraStream class without having to
> > > include post_processor.h. The PostProcessor class can be
> > > forward-declared in camera_stream.h instead.
> >
> > Ah, I see. Thanks.
> > So shall we remove post_processor.h in camera_device.h?
>
> post_processor.h isn't included in camera_device.h. If you meant
> camera_device.cpp, yes, I think it should be removed from there. I've
> just sent a patch.
>

Yes, I meant camera_device.cpp :p
Reviewed. Thanks for the patch.

> > > > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > > > > > Reviewed-by: Umang Jain<umang.jain@ideasonboard.com>
> > > > > > > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
> > > > > > > ---
> > > > > > >  src/android/camera_stream.cpp | 4 ++++
> > > > > > >  src/android/camera_stream.h   | 2 ++
> > > > > > >  2 files changed, 6 insertions(+)
> > > > > > >
> > > > > > > diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp
> > > > > > > index 8e6ccb83..f44a2717 100644
> > > > > > > --- a/src/android/camera_stream.cpp
> > > > > > > +++ b/src/android/camera_stream.cpp
> > > > > > > @@ -56,6 +56,10 @@ CameraStream::CameraStream(CameraDevice *const cameraDevice,
> > > > > > >  {
> > > > > > >  }
> > > > > > >
> > > > > > > +CameraStream::CameraStream(CameraStream &&other) = default;
> > > > > > > +
> > > > > > > +CameraStream::~CameraStream() = default;
> > > > > > > +
> > > > > > >  const StreamConfiguration &CameraStream::configuration() const
> > > > > > >  {
> > > > > > >         return config_->at(index_);
> > > > > > > diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h
> > > > > > > index 85064268..f242336e 100644
> > > > > > > --- a/src/android/camera_stream.h
> > > > > > > +++ b/src/android/camera_stream.h
> > > > > > > @@ -112,6 +112,8 @@ public:
> > > > > > >         CameraStream(CameraDevice *const cameraDevice,
> > > > > > >                      libcamera::CameraConfiguration *config, Type type,
> > > > > > >                      camera3_stream_t *camera3Stream, unsigned int index);
> > > > > > > +       CameraStream(CameraStream &&other);
> > > > > > > +       ~CameraStream();
> > > > > > >
> > > > > > >         Type type() const { return type_; }
> > > > > > >         camera3_stream_t *camera3Stream() const { return camera3Stream_; }
>
> --
> Regards,
>
> Laurent Pinchart

Patch
diff mbox series 

diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp
index 8e6ccb83..f44a2717 100644
--- a/src/android/camera_stream.cpp
+++ b/src/android/camera_stream.cpp
@@ -56,6 +56,10 @@  CameraStream::CameraStream(CameraDevice *const cameraDevice,
 {
 }
 
+CameraStream::CameraStream(CameraStream &&other) = default;
+
+CameraStream::~CameraStream() = default;
+
 const StreamConfiguration &CameraStream::configuration() const
 {
 	return config_->at(index_);
diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h
index 85064268..f242336e 100644
--- a/src/android/camera_stream.h
+++ b/src/android/camera_stream.h
@@ -112,6 +112,8 @@  public:
 	CameraStream(CameraDevice *const cameraDevice,
 		     libcamera::CameraConfiguration *config, Type type,
 		     camera3_stream_t *camera3Stream, unsigned int index);
+	CameraStream(CameraStream &&other);
+	~CameraStream();
 
 	Type type() const { return type_; }
 	camera3_stream_t *camera3Stream() const { return camera3Stream_; }