[libcamera-devel,01/15] android: camera_stream: Break out CameraStream

Message ID 20201005104649.10812-2-laurent.pinchart@ideasonboard.com
State Accepted
Headers show
Series
  • CameraStream refactoring
Related show

Commit Message

Laurent Pinchart Oct. 5, 2020, 10:46 a.m. UTC
From: Jacopo Mondi <jacopo@jmondi.org>

Break CameraStream out of the CameraDevice class.

No functional changes, only the code is moved.

Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
---
 src/android/camera_device.cpp |  6 ------
 src/android/camera_device.h   | 24 +--------------------
 src/android/camera_stream.cpp | 16 ++++++++++++++
 src/android/camera_stream.h   | 40 +++++++++++++++++++++++++++++++++++
 src/android/meson.build       |  1 +
 5 files changed, 58 insertions(+), 29 deletions(-)
 create mode 100644 src/android/camera_stream.cpp
 create mode 100644 src/android/camera_stream.h

Comments

Kieran Bingham Oct. 5, 2020, 10:54 a.m. UTC | #1
Hi Jacopo

I am incredibly happy to see this happen first ;-)

All of your work in progress prevented me from doing the same, so I'm
really happy to see this move now, and your other work based on top.

Thank you!

On 05/10/2020 11:46, Laurent Pinchart wrote:
> From: Jacopo Mondi <jacopo@jmondi.org>
> 
> Break CameraStream out of the CameraDevice class.
> 
> No functional changes, only the code is moved.
> 
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>

I think this will really help with the clarity of the design as it comes
up. I should have done this straight away when the CameraStream was
introduced.

Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>


> ---
>  src/android/camera_device.cpp |  6 ------
>  src/android/camera_device.h   | 24 +--------------------
>  src/android/camera_stream.cpp | 16 ++++++++++++++
>  src/android/camera_stream.h   | 40 +++++++++++++++++++++++++++++++++++
>  src/android/meson.build       |  1 +
>  5 files changed, 58 insertions(+), 29 deletions(-)
>  create mode 100644 src/android/camera_stream.cpp
>  create mode 100644 src/android/camera_stream.h
> 
> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> index 751699cd2113..bbc692fe109f 100644
> --- a/src/android/camera_device.cpp
> +++ b/src/android/camera_device.cpp
> @@ -169,12 +169,6 @@ MappedCamera3Buffer::MappedCamera3Buffer(const buffer_handle_t camera3buffer,
>  	}
>  }
>  
> -CameraStream::CameraStream(PixelFormat format, Size size,
> -			   unsigned int index, Encoder *encoder)
> -	: format_(format), size_(size), index_(index), encoder_(encoder)
> -{
> -}
> -
>  /*
>   * \struct Camera3RequestDescriptor
>   *
> diff --git a/src/android/camera_device.h b/src/android/camera_device.h
> index 1837748d2efc..52923ec979a7 100644
> --- a/src/android/camera_device.h
> +++ b/src/android/camera_device.h
> @@ -23,33 +23,11 @@
>  #include "libcamera/internal/log.h"
>  #include "libcamera/internal/message.h"
>  
> +#include "camera_stream.h"
>  #include "jpeg/encoder.h"
>  
>  class CameraMetadata;
>  
> -class CameraStream
> -{
> -public:
> -	CameraStream(libcamera::PixelFormat format, libcamera::Size size,
> -		     unsigned int index, Encoder *encoder = nullptr);
> -
> -	const libcamera::PixelFormat &format() const { return format_; }
> -	const libcamera::Size &size() const { return size_; }
> -	unsigned int index() const { return index_; }
> -	Encoder *encoder() const { return encoder_.get(); }
> -
> -private:
> -	libcamera::PixelFormat format_;
> -	libcamera::Size size_;
> -	/*
> -	 * The index of the libcamera StreamConfiguration as added during
> -	 * configureStreams(). A single libcamera Stream may be used to deliver
> -	 * one or more streams to the Android framework.
> -	 */
> -	unsigned int index_;
> -	std::unique_ptr<Encoder> encoder_;
> -};
> -
>  class CameraDevice : protected libcamera::Loggable
>  {
>  public:
> diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp
> new file mode 100644
> index 000000000000..01c62978ca3a
> --- /dev/null
> +++ b/src/android/camera_stream.cpp
> @@ -0,0 +1,16 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2020, Google Inc.
> + *
> + * camera_stream.cpp - Camera HAL stream
> + */
> +
> +#include "camera_stream.h"
> +
> +using namespace libcamera;
> +
> +CameraStream::CameraStream(PixelFormat format, Size size,
> +			   unsigned int index, Encoder *encoder)
> +	: format_(format), size_(size), index_(index), encoder_(encoder)
> +{
> +}
> diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h
> new file mode 100644
> index 000000000000..10dece7beb69
> --- /dev/null
> +++ b/src/android/camera_stream.h
> @@ -0,0 +1,40 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2020, Google Inc.
> + *
> + * camera_stream.h - Camera HAL stream
> + */
> +#ifndef __ANDROID_CAMERA_STREAM_H__
> +#define __ANDROID_CAMERA_STREAM_H__
> +
> +#include <memory>
> +
> +#include <libcamera/geometry.h>
> +#include <libcamera/pixel_format.h>
> +
> +#include "jpeg/encoder.h"
> +
> +class CameraStream
> +{
> +public:
> +	CameraStream(libcamera::PixelFormat format, libcamera::Size size,
> +		     unsigned int index, Encoder *encoder = nullptr);
> +
> +	const libcamera::PixelFormat &format() const { return format_; }
> +	const libcamera::Size &size() const { return size_; }
> +	unsigned int index() const { return index_; }
> +	Encoder *encoder() const { return encoder_.get(); }
> +
> +private:
> +	libcamera::PixelFormat format_;
> +	libcamera::Size size_;
> +	/*
> +	 * The index of the libcamera StreamConfiguration as added during
> +	 * configureStreams(). A single libcamera Stream may be used to deliver
> +	 * one or more streams to the Android framework.
> +	 */
> +	unsigned int index_;
> +	std::unique_ptr<Encoder> encoder_;
> +};
> +
> +#endif /* __ANDROID_CAMERA_STREAM__ */
> diff --git a/src/android/meson.build b/src/android/meson.build
> index 0293c2036561..802bb89afe57 100644
> --- a/src/android/meson.build
> +++ b/src/android/meson.build
> @@ -20,6 +20,7 @@ android_hal_sources = files([
>      'camera_device.cpp',
>      'camera_metadata.cpp',
>      'camera_ops.cpp',
> +    'camera_stream.cpp',
>      'jpeg/encoder_libjpeg.cpp',
>      'jpeg/exif.cpp',
>  ])
>
Laurent Pinchart Oct. 5, 2020, 11:19 a.m. UTC | #2
Hi Jacopo,

Thank you for the patch.

On Mon, Oct 05, 2020 at 01:46:35PM +0300, Laurent Pinchart wrote:
> From: Jacopo Mondi <jacopo@jmondi.org>
> 
> Break CameraStream out of the CameraDevice class.
> 
> No functional changes, only the code is moved.
> 
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
>  src/android/camera_device.cpp |  6 ------
>  src/android/camera_device.h   | 24 +--------------------
>  src/android/camera_stream.cpp | 16 ++++++++++++++
>  src/android/camera_stream.h   | 40 +++++++++++++++++++++++++++++++++++
>  src/android/meson.build       |  1 +
>  5 files changed, 58 insertions(+), 29 deletions(-)
>  create mode 100644 src/android/camera_stream.cpp
>  create mode 100644 src/android/camera_stream.h
> 
> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> index 751699cd2113..bbc692fe109f 100644
> --- a/src/android/camera_device.cpp
> +++ b/src/android/camera_device.cpp
> @@ -169,12 +169,6 @@ MappedCamera3Buffer::MappedCamera3Buffer(const buffer_handle_t camera3buffer,
>  	}
>  }
>  
> -CameraStream::CameraStream(PixelFormat format, Size size,
> -			   unsigned int index, Encoder *encoder)
> -	: format_(format), size_(size), index_(index), encoder_(encoder)
> -{
> -}
> -
>  /*
>   * \struct Camera3RequestDescriptor
>   *
> diff --git a/src/android/camera_device.h b/src/android/camera_device.h
> index 1837748d2efc..52923ec979a7 100644
> --- a/src/android/camera_device.h
> +++ b/src/android/camera_device.h
> @@ -23,33 +23,11 @@
>  #include "libcamera/internal/log.h"
>  #include "libcamera/internal/message.h"
>  
> +#include "camera_stream.h"
>  #include "jpeg/encoder.h"
>  
>  class CameraMetadata;
>  
> -class CameraStream
> -{
> -public:
> -	CameraStream(libcamera::PixelFormat format, libcamera::Size size,
> -		     unsigned int index, Encoder *encoder = nullptr);
> -
> -	const libcamera::PixelFormat &format() const { return format_; }
> -	const libcamera::Size &size() const { return size_; }
> -	unsigned int index() const { return index_; }
> -	Encoder *encoder() const { return encoder_.get(); }
> -
> -private:
> -	libcamera::PixelFormat format_;
> -	libcamera::Size size_;
> -	/*
> -	 * The index of the libcamera StreamConfiguration as added during
> -	 * configureStreams(). A single libcamera Stream may be used to deliver
> -	 * one or more streams to the Android framework.
> -	 */
> -	unsigned int index_;
> -	std::unique_ptr<Encoder> encoder_;
> -};
> -
>  class CameraDevice : protected libcamera::Loggable
>  {
>  public:
> diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp
> new file mode 100644
> index 000000000000..01c62978ca3a
> --- /dev/null
> +++ b/src/android/camera_stream.cpp
> @@ -0,0 +1,16 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2020, Google Inc.
> + *
> + * camera_stream.cpp - Camera HAL stream
> + */
> +
> +#include "camera_stream.h"
> +
> +using namespace libcamera;
> +
> +CameraStream::CameraStream(PixelFormat format, Size size,
> +			   unsigned int index, Encoder *encoder)
> +	: format_(format), size_(size), index_(index), encoder_(encoder)
> +{
> +}
> diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h
> new file mode 100644
> index 000000000000..10dece7beb69
> --- /dev/null
> +++ b/src/android/camera_stream.h
> @@ -0,0 +1,40 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2020, Google Inc.
> + *
> + * camera_stream.h - Camera HAL stream
> + */
> +#ifndef __ANDROID_CAMERA_STREAM_H__
> +#define __ANDROID_CAMERA_STREAM_H__
> +
> +#include <memory>
> +
> +#include <libcamera/geometry.h>
> +#include <libcamera/pixel_format.h>
> +
> +#include "jpeg/encoder.h"

You could forward-declare Encoder instead.

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

> +
> +class CameraStream
> +{
> +public:
> +	CameraStream(libcamera::PixelFormat format, libcamera::Size size,
> +		     unsigned int index, Encoder *encoder = nullptr);
> +
> +	const libcamera::PixelFormat &format() const { return format_; }
> +	const libcamera::Size &size() const { return size_; }
> +	unsigned int index() const { return index_; }
> +	Encoder *encoder() const { return encoder_.get(); }
> +
> +private:
> +	libcamera::PixelFormat format_;
> +	libcamera::Size size_;
> +	/*
> +	 * The index of the libcamera StreamConfiguration as added during
> +	 * configureStreams(). A single libcamera Stream may be used to deliver
> +	 * one or more streams to the Android framework.
> +	 */
> +	unsigned int index_;
> +	std::unique_ptr<Encoder> encoder_;
> +};
> +
> +#endif /* __ANDROID_CAMERA_STREAM__ */
> diff --git a/src/android/meson.build b/src/android/meson.build
> index 0293c2036561..802bb89afe57 100644
> --- a/src/android/meson.build
> +++ b/src/android/meson.build
> @@ -20,6 +20,7 @@ android_hal_sources = files([
>      'camera_device.cpp',
>      'camera_metadata.cpp',
>      'camera_ops.cpp',
> +    'camera_stream.cpp',
>      'jpeg/encoder_libjpeg.cpp',
>      'jpeg/exif.cpp',
>  ])
Jacopo Mondi Oct. 5, 2020, 2:56 p.m. UTC | #3
Hi Laurent,

On Mon, Oct 05, 2020 at 02:19:40PM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Mon, Oct 05, 2020 at 01:46:35PM +0300, Laurent Pinchart wrote:
> > From: Jacopo Mondi <jacopo@jmondi.org>
> >
> > Break CameraStream out of the CameraDevice class.
> >
> > No functional changes, only the code is moved.
> >
> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > ---
> >  src/android/camera_device.cpp |  6 ------
> >  src/android/camera_device.h   | 24 +--------------------
> >  src/android/camera_stream.cpp | 16 ++++++++++++++
> >  src/android/camera_stream.h   | 40 +++++++++++++++++++++++++++++++++++
> >  src/android/meson.build       |  1 +
> >  5 files changed, 58 insertions(+), 29 deletions(-)
> >  create mode 100644 src/android/camera_stream.cpp
> >  create mode 100644 src/android/camera_stream.h
> >
> > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> > index 751699cd2113..bbc692fe109f 100644
> > --- a/src/android/camera_device.cpp
> > +++ b/src/android/camera_device.cpp
> > @@ -169,12 +169,6 @@ MappedCamera3Buffer::MappedCamera3Buffer(const buffer_handle_t camera3buffer,
> >  	}
> >  }
> >
> > -CameraStream::CameraStream(PixelFormat format, Size size,
> > -			   unsigned int index, Encoder *encoder)
> > -	: format_(format), size_(size), index_(index), encoder_(encoder)
> > -{
> > -}
> > -
> >  /*
> >   * \struct Camera3RequestDescriptor
> >   *
> > diff --git a/src/android/camera_device.h b/src/android/camera_device.h
> > index 1837748d2efc..52923ec979a7 100644
> > --- a/src/android/camera_device.h
> > +++ b/src/android/camera_device.h
> > @@ -23,33 +23,11 @@
> >  #include "libcamera/internal/log.h"
> >  #include "libcamera/internal/message.h"
> >
> > +#include "camera_stream.h"
> >  #include "jpeg/encoder.h"
> >
> >  class CameraMetadata;
> >
> > -class CameraStream
> > -{
> > -public:
> > -	CameraStream(libcamera::PixelFormat format, libcamera::Size size,
> > -		     unsigned int index, Encoder *encoder = nullptr);
> > -
> > -	const libcamera::PixelFormat &format() const { return format_; }
> > -	const libcamera::Size &size() const { return size_; }
> > -	unsigned int index() const { return index_; }
> > -	Encoder *encoder() const { return encoder_.get(); }
> > -
> > -private:
> > -	libcamera::PixelFormat format_;
> > -	libcamera::Size size_;
> > -	/*
> > -	 * The index of the libcamera StreamConfiguration as added during
> > -	 * configureStreams(). A single libcamera Stream may be used to deliver
> > -	 * one or more streams to the Android framework.
> > -	 */
> > -	unsigned int index_;
> > -	std::unique_ptr<Encoder> encoder_;
> > -};
> > -
> >  class CameraDevice : protected libcamera::Loggable
> >  {
> >  public:
> > diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp
> > new file mode 100644
> > index 000000000000..01c62978ca3a
> > --- /dev/null
> > +++ b/src/android/camera_stream.cpp
> > @@ -0,0 +1,16 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2020, Google Inc.
> > + *
> > + * camera_stream.cpp - Camera HAL stream
> > + */
> > +
> > +#include "camera_stream.h"
> > +
> > +using namespace libcamera;
> > +
> > +CameraStream::CameraStream(PixelFormat format, Size size,
> > +			   unsigned int index, Encoder *encoder)
> > +	: format_(format), size_(size), index_(index), encoder_(encoder)
> > +{
> > +}
> > diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h
> > new file mode 100644
> > index 000000000000..10dece7beb69
> > --- /dev/null
> > +++ b/src/android/camera_stream.h
> > @@ -0,0 +1,40 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2020, Google Inc.
> > + *
> > + * camera_stream.h - Camera HAL stream
> > + */
> > +#ifndef __ANDROID_CAMERA_STREAM_H__
> > +#define __ANDROID_CAMERA_STREAM_H__
> > +
> > +#include <memory>
> > +
> > +#include <libcamera/geometry.h>
> > +#include <libcamera/pixel_format.h>
> > +
> > +#include "jpeg/encoder.h"
>
> You could forward-declare Encoder instead.

Apparently not
error: invalid application of 'sizeof' to an incomplete type 'Encoder'

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

Thanks
  j

>
> > +
> > +class CameraStream
> > +{
> > +public:
> > +	CameraStream(libcamera::PixelFormat format, libcamera::Size size,
> > +		     unsigned int index, Encoder *encoder = nullptr);
> > +
> > +	const libcamera::PixelFormat &format() const { return format_; }
> > +	const libcamera::Size &size() const { return size_; }
> > +	unsigned int index() const { return index_; }
> > +	Encoder *encoder() const { return encoder_.get(); }
> > +
> > +private:
> > +	libcamera::PixelFormat format_;
> > +	libcamera::Size size_;
> > +	/*
> > +	 * The index of the libcamera StreamConfiguration as added during
> > +	 * configureStreams(). A single libcamera Stream may be used to deliver
> > +	 * one or more streams to the Android framework.
> > +	 */
> > +	unsigned int index_;
> > +	std::unique_ptr<Encoder> encoder_;
> > +};
> > +
> > +#endif /* __ANDROID_CAMERA_STREAM__ */
> > diff --git a/src/android/meson.build b/src/android/meson.build
> > index 0293c2036561..802bb89afe57 100644
> > --- a/src/android/meson.build
> > +++ b/src/android/meson.build
> > @@ -20,6 +20,7 @@ android_hal_sources = files([
> >      'camera_device.cpp',
> >      'camera_metadata.cpp',
> >      'camera_ops.cpp',
> > +    'camera_stream.cpp',
> >      'jpeg/encoder_libjpeg.cpp',
> >      'jpeg/exif.cpp',
> >  ])
>
> --
> Regards,
>
> Laurent Pinchart
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
Laurent Pinchart Oct. 5, 2020, 4:19 p.m. UTC | #4
Hi Jacopo,

On Mon, Oct 05, 2020 at 04:56:31PM +0200, Jacopo Mondi wrote:
> On Mon, Oct 05, 2020 at 02:19:40PM +0300, Laurent Pinchart wrote:
> > On Mon, Oct 05, 2020 at 01:46:35PM +0300, Laurent Pinchart wrote:
> > > From: Jacopo Mondi <jacopo@jmondi.org>
> > >
> > > Break CameraStream out of the CameraDevice class.
> > >
> > > No functional changes, only the code is moved.
> > >
> > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > > ---
> > >  src/android/camera_device.cpp |  6 ------
> > >  src/android/camera_device.h   | 24 +--------------------
> > >  src/android/camera_stream.cpp | 16 ++++++++++++++
> > >  src/android/camera_stream.h   | 40 +++++++++++++++++++++++++++++++++++
> > >  src/android/meson.build       |  1 +
> > >  5 files changed, 58 insertions(+), 29 deletions(-)
> > >  create mode 100644 src/android/camera_stream.cpp
> > >  create mode 100644 src/android/camera_stream.h
> > >
> > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> > > index 751699cd2113..bbc692fe109f 100644
> > > --- a/src/android/camera_device.cpp
> > > +++ b/src/android/camera_device.cpp
> > > @@ -169,12 +169,6 @@ MappedCamera3Buffer::MappedCamera3Buffer(const buffer_handle_t camera3buffer,
> > >  	}
> > >  }
> > >
> > > -CameraStream::CameraStream(PixelFormat format, Size size,
> > > -			   unsigned int index, Encoder *encoder)
> > > -	: format_(format), size_(size), index_(index), encoder_(encoder)
> > > -{
> > > -}
> > > -
> > >  /*
> > >   * \struct Camera3RequestDescriptor
> > >   *
> > > diff --git a/src/android/camera_device.h b/src/android/camera_device.h
> > > index 1837748d2efc..52923ec979a7 100644
> > > --- a/src/android/camera_device.h
> > > +++ b/src/android/camera_device.h
> > > @@ -23,33 +23,11 @@
> > >  #include "libcamera/internal/log.h"
> > >  #include "libcamera/internal/message.h"
> > >
> > > +#include "camera_stream.h"
> > >  #include "jpeg/encoder.h"
> > >
> > >  class CameraMetadata;
> > >
> > > -class CameraStream
> > > -{
> > > -public:
> > > -	CameraStream(libcamera::PixelFormat format, libcamera::Size size,
> > > -		     unsigned int index, Encoder *encoder = nullptr);
> > > -
> > > -	const libcamera::PixelFormat &format() const { return format_; }
> > > -	const libcamera::Size &size() const { return size_; }
> > > -	unsigned int index() const { return index_; }
> > > -	Encoder *encoder() const { return encoder_.get(); }
> > > -
> > > -private:
> > > -	libcamera::PixelFormat format_;
> > > -	libcamera::Size size_;
> > > -	/*
> > > -	 * The index of the libcamera StreamConfiguration as added during
> > > -	 * configureStreams(). A single libcamera Stream may be used to deliver
> > > -	 * one or more streams to the Android framework.
> > > -	 */
> > > -	unsigned int index_;
> > > -	std::unique_ptr<Encoder> encoder_;
> > > -};
> > > -
> > >  class CameraDevice : protected libcamera::Loggable
> > >  {
> > >  public:
> > > diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp
> > > new file mode 100644
> > > index 000000000000..01c62978ca3a
> > > --- /dev/null
> > > +++ b/src/android/camera_stream.cpp
> > > @@ -0,0 +1,16 @@
> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > +/*
> > > + * Copyright (C) 2020, Google Inc.
> > > + *
> > > + * camera_stream.cpp - Camera HAL stream
> > > + */
> > > +
> > > +#include "camera_stream.h"
> > > +
> > > +using namespace libcamera;
> > > +
> > > +CameraStream::CameraStream(PixelFormat format, Size size,
> > > +			   unsigned int index, Encoder *encoder)
> > > +	: format_(format), size_(size), index_(index), encoder_(encoder)
> > > +{
> > > +}
> > > diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h
> > > new file mode 100644
> > > index 000000000000..10dece7beb69
> > > --- /dev/null
> > > +++ b/src/android/camera_stream.h
> > > @@ -0,0 +1,40 @@
> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > +/*
> > > + * Copyright (C) 2020, Google Inc.
> > > + *
> > > + * camera_stream.h - Camera HAL stream
> > > + */
> > > +#ifndef __ANDROID_CAMERA_STREAM_H__
> > > +#define __ANDROID_CAMERA_STREAM_H__
> > > +
> > > +#include <memory>
> > > +
> > > +#include <libcamera/geometry.h>
> > > +#include <libcamera/pixel_format.h>
> > > +
> > > +#include "jpeg/encoder.h"
> >
> > You could forward-declare Encoder instead.
> 
> Apparently not
> error: invalid application of 'sizeof' to an incomplete type 'Encoder'

You need to include the header in camera_stream.cpp :-)

> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >
> > > +
> > > +class CameraStream
> > > +{
> > > +public:
> > > +	CameraStream(libcamera::PixelFormat format, libcamera::Size size,
> > > +		     unsigned int index, Encoder *encoder = nullptr);
> > > +
> > > +	const libcamera::PixelFormat &format() const { return format_; }
> > > +	const libcamera::Size &size() const { return size_; }
> > > +	unsigned int index() const { return index_; }
> > > +	Encoder *encoder() const { return encoder_.get(); }
> > > +
> > > +private:
> > > +	libcamera::PixelFormat format_;
> > > +	libcamera::Size size_;
> > > +	/*
> > > +	 * The index of the libcamera StreamConfiguration as added during
> > > +	 * configureStreams(). A single libcamera Stream may be used to deliver
> > > +	 * one or more streams to the Android framework.
> > > +	 */
> > > +	unsigned int index_;
> > > +	std::unique_ptr<Encoder> encoder_;
> > > +};
> > > +
> > > +#endif /* __ANDROID_CAMERA_STREAM__ */
> > > diff --git a/src/android/meson.build b/src/android/meson.build
> > > index 0293c2036561..802bb89afe57 100644
> > > --- a/src/android/meson.build
> > > +++ b/src/android/meson.build
> > > @@ -20,6 +20,7 @@ android_hal_sources = files([
> > >      'camera_device.cpp',
> > >      'camera_metadata.cpp',
> > >      'camera_ops.cpp',
> > > +    'camera_stream.cpp',
> > >      'jpeg/encoder_libjpeg.cpp',
> > >      'jpeg/exif.cpp',
> > >  ])
> >
Jacopo Mondi Oct. 5, 2020, 4:36 p.m. UTC | #5
iHi Laurent

On Mon, Oct 05, 2020 at 07:19:32PM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> On Mon, Oct 05, 2020 at 04:56:31PM +0200, Jacopo Mondi wrote:
> > On Mon, Oct 05, 2020 at 02:19:40PM +0300, Laurent Pinchart wrote:
> > > On Mon, Oct 05, 2020 at 01:46:35PM +0300, Laurent Pinchart wrote:
> > > > From: Jacopo Mondi <jacopo@jmondi.org>
> > > >
> > > > Break CameraStream out of the CameraDevice class.
> > > >
> > > > No functional changes, only the code is moved.
> > > >
> > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > > > ---
> > > >  src/android/camera_device.cpp |  6 ------
> > > >  src/android/camera_device.h   | 24 +--------------------
> > > >  src/android/camera_stream.cpp | 16 ++++++++++++++
> > > >  src/android/camera_stream.h   | 40 +++++++++++++++++++++++++++++++++++
> > > >  src/android/meson.build       |  1 +
> > > >  5 files changed, 58 insertions(+), 29 deletions(-)
> > > >  create mode 100644 src/android/camera_stream.cpp
> > > >  create mode 100644 src/android/camera_stream.h
> > > >
> > > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> > > > index 751699cd2113..bbc692fe109f 100644
> > > > --- a/src/android/camera_device.cpp
> > > > +++ b/src/android/camera_device.cpp
> > > > @@ -169,12 +169,6 @@ MappedCamera3Buffer::MappedCamera3Buffer(const buffer_handle_t camera3buffer,
> > > >  	}
> > > >  }
> > > >
> > > > -CameraStream::CameraStream(PixelFormat format, Size size,
> > > > -			   unsigned int index, Encoder *encoder)
> > > > -	: format_(format), size_(size), index_(index), encoder_(encoder)
> > > > -{
> > > > -}
> > > > -
> > > >  /*
> > > >   * \struct Camera3RequestDescriptor
> > > >   *
> > > > diff --git a/src/android/camera_device.h b/src/android/camera_device.h
> > > > index 1837748d2efc..52923ec979a7 100644
> > > > --- a/src/android/camera_device.h
> > > > +++ b/src/android/camera_device.h
> > > > @@ -23,33 +23,11 @@
> > > >  #include "libcamera/internal/log.h"
> > > >  #include "libcamera/internal/message.h"
> > > >
> > > > +#include "camera_stream.h"
> > > >  #include "jpeg/encoder.h"
> > > >
> > > >  class CameraMetadata;
> > > >
> > > > -class CameraStream
> > > > -{
> > > > -public:
> > > > -	CameraStream(libcamera::PixelFormat format, libcamera::Size size,
> > > > -		     unsigned int index, Encoder *encoder = nullptr);
> > > > -
> > > > -	const libcamera::PixelFormat &format() const { return format_; }
> > > > -	const libcamera::Size &size() const { return size_; }
> > > > -	unsigned int index() const { return index_; }
> > > > -	Encoder *encoder() const { return encoder_.get(); }
> > > > -
> > > > -private:
> > > > -	libcamera::PixelFormat format_;
> > > > -	libcamera::Size size_;
> > > > -	/*
> > > > -	 * The index of the libcamera StreamConfiguration as added during
> > > > -	 * configureStreams(). A single libcamera Stream may be used to deliver
> > > > -	 * one or more streams to the Android framework.
> > > > -	 */
> > > > -	unsigned int index_;
> > > > -	std::unique_ptr<Encoder> encoder_;
> > > > -};
> > > > -
> > > >  class CameraDevice : protected libcamera::Loggable
> > > >  {
> > > >  public:
> > > > diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp
> > > > new file mode 100644
> > > > index 000000000000..01c62978ca3a
> > > > --- /dev/null
> > > > +++ b/src/android/camera_stream.cpp
> > > > @@ -0,0 +1,16 @@
> > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > > +/*
> > > > + * Copyright (C) 2020, Google Inc.
> > > > + *
> > > > + * camera_stream.cpp - Camera HAL stream
> > > > + */
> > > > +
> > > > +#include "camera_stream.h"
> > > > +
> > > > +using namespace libcamera;
> > > > +
> > > > +CameraStream::CameraStream(PixelFormat format, Size size,
> > > > +			   unsigned int index, Encoder *encoder)
> > > > +	: format_(format), size_(size), index_(index), encoder_(encoder)
> > > > +{
> > > > +}
> > > > diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h
> > > > new file mode 100644
> > > > index 000000000000..10dece7beb69
> > > > --- /dev/null
> > > > +++ b/src/android/camera_stream.h
> > > > @@ -0,0 +1,40 @@
> > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > > +/*
> > > > + * Copyright (C) 2020, Google Inc.
> > > > + *
> > > > + * camera_stream.h - Camera HAL stream
> > > > + */
> > > > +#ifndef __ANDROID_CAMERA_STREAM_H__
> > > > +#define __ANDROID_CAMERA_STREAM_H__
> > > > +
> > > > +#include <memory>
> > > > +
> > > > +#include <libcamera/geometry.h>
> > > > +#include <libcamera/pixel_format.h>
> > > > +
> > > > +#include "jpeg/encoder.h"
> > >
> > > You could forward-declare Encoder instead.
> >
> > Apparently not
> > error: invalid application of 'sizeof' to an incomplete type 'Encoder'
>
> You need to include the header in camera_stream.cpp :-)
>

AH, my bad, I thought the error was in the .h file because of the
unique_ptr<> wrapping Encoder.

Thanks!

> > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > >
> > > > +
> > > > +class CameraStream
> > > > +{
> > > > +public:
> > > > +	CameraStream(libcamera::PixelFormat format, libcamera::Size size,
> > > > +		     unsigned int index, Encoder *encoder = nullptr);
> > > > +
> > > > +	const libcamera::PixelFormat &format() const { return format_; }
> > > > +	const libcamera::Size &size() const { return size_; }
> > > > +	unsigned int index() const { return index_; }
> > > > +	Encoder *encoder() const { return encoder_.get(); }
> > > > +
> > > > +private:
> > > > +	libcamera::PixelFormat format_;
> > > > +	libcamera::Size size_;
> > > > +	/*
> > > > +	 * The index of the libcamera StreamConfiguration as added during
> > > > +	 * configureStreams(). A single libcamera Stream may be used to deliver
> > > > +	 * one or more streams to the Android framework.
> > > > +	 */
> > > > +	unsigned int index_;
> > > > +	std::unique_ptr<Encoder> encoder_;
> > > > +};
> > > > +
> > > > +#endif /* __ANDROID_CAMERA_STREAM__ */
> > > > diff --git a/src/android/meson.build b/src/android/meson.build
> > > > index 0293c2036561..802bb89afe57 100644
> > > > --- a/src/android/meson.build
> > > > +++ b/src/android/meson.build
> > > > @@ -20,6 +20,7 @@ android_hal_sources = files([
> > > >      'camera_device.cpp',
> > > >      'camera_metadata.cpp',
> > > >      'camera_ops.cpp',
> > > > +    'camera_stream.cpp',
> > > >      'jpeg/encoder_libjpeg.cpp',
> > > >      'jpeg/exif.cpp',
> > > >  ])
> > >
>
> --
> Regards,
>
> Laurent Pinchart
Umang Jain Oct. 6, 2020, 11:08 a.m. UTC | #6
Hi Jacopo,

On 10/5/20 4:16 PM, Laurent Pinchart wrote:
> From: Jacopo Mondi <jacopo@jmondi.org>
>
> Break CameraStream out of the CameraDevice class.
>
> No functional changes, only the code is moved.
>
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
Reviewed-by: Umang Jain <email@uajain.com>
> ---
>   src/android/camera_device.cpp |  6 ------
>   src/android/camera_device.h   | 24 +--------------------
>   src/android/camera_stream.cpp | 16 ++++++++++++++
>   src/android/camera_stream.h   | 40 +++++++++++++++++++++++++++++++++++
>   src/android/meson.build       |  1 +
>   5 files changed, 58 insertions(+), 29 deletions(-)
>   create mode 100644 src/android/camera_stream.cpp
>   create mode 100644 src/android/camera_stream.h
>
> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> index 751699cd2113..bbc692fe109f 100644
> --- a/src/android/camera_device.cpp
> +++ b/src/android/camera_device.cpp
> @@ -169,12 +169,6 @@ MappedCamera3Buffer::MappedCamera3Buffer(const buffer_handle_t camera3buffer,
>   	}
>   }
>   
> -CameraStream::CameraStream(PixelFormat format, Size size,
> -			   unsigned int index, Encoder *encoder)
> -	: format_(format), size_(size), index_(index), encoder_(encoder)
> -{
> -}
> -
>   /*
>    * \struct Camera3RequestDescriptor
>    *
> diff --git a/src/android/camera_device.h b/src/android/camera_device.h
> index 1837748d2efc..52923ec979a7 100644
> --- a/src/android/camera_device.h
> +++ b/src/android/camera_device.h
> @@ -23,33 +23,11 @@
>   #include "libcamera/internal/log.h"
>   #include "libcamera/internal/message.h"
>   
> +#include "camera_stream.h"
>   #include "jpeg/encoder.h"
>   
>   class CameraMetadata;
>   
> -class CameraStream
> -{
> -public:
> -	CameraStream(libcamera::PixelFormat format, libcamera::Size size,
> -		     unsigned int index, Encoder *encoder = nullptr);
> -
> -	const libcamera::PixelFormat &format() const { return format_; }
> -	const libcamera::Size &size() const { return size_; }
> -	unsigned int index() const { return index_; }
> -	Encoder *encoder() const { return encoder_.get(); }
> -
> -private:
> -	libcamera::PixelFormat format_;
> -	libcamera::Size size_;
> -	/*
> -	 * The index of the libcamera StreamConfiguration as added during
> -	 * configureStreams(). A single libcamera Stream may be used to deliver
> -	 * one or more streams to the Android framework.
> -	 */
> -	unsigned int index_;
> -	std::unique_ptr<Encoder> encoder_;
> -};
> -
>   class CameraDevice : protected libcamera::Loggable
>   {
>   public:
> diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp
> new file mode 100644
> index 000000000000..01c62978ca3a
> --- /dev/null
> +++ b/src/android/camera_stream.cpp
> @@ -0,0 +1,16 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2020, Google Inc.
> + *
> + * camera_stream.cpp - Camera HAL stream
> + */
> +
> +#include "camera_stream.h"
> +
> +using namespace libcamera;
> +
> +CameraStream::CameraStream(PixelFormat format, Size size,
> +			   unsigned int index, Encoder *encoder)
> +	: format_(format), size_(size), index_(index), encoder_(encoder)
> +{
> +}
> diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h
> new file mode 100644
> index 000000000000..10dece7beb69
> --- /dev/null
> +++ b/src/android/camera_stream.h
> @@ -0,0 +1,40 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2020, Google Inc.
> + *
> + * camera_stream.h - Camera HAL stream
> + */
> +#ifndef __ANDROID_CAMERA_STREAM_H__
> +#define __ANDROID_CAMERA_STREAM_H__
> +
> +#include <memory>
> +
> +#include <libcamera/geometry.h>
> +#include <libcamera/pixel_format.h>
> +
> +#include "jpeg/encoder.h"
> +
> +class CameraStream
> +{
> +public:
> +	CameraStream(libcamera::PixelFormat format, libcamera::Size size,
> +		     unsigned int index, Encoder *encoder = nullptr);
> +
> +	const libcamera::PixelFormat &format() const { return format_; }
> +	const libcamera::Size &size() const { return size_; }
> +	unsigned int index() const { return index_; }
> +	Encoder *encoder() const { return encoder_.get(); }
> +
> +private:
> +	libcamera::PixelFormat format_;
> +	libcamera::Size size_;
> +	/*
> +	 * The index of the libcamera StreamConfiguration as added during
> +	 * configureStreams(). A single libcamera Stream may be used to deliver
> +	 * one or more streams to the Android framework.
> +	 */
> +	unsigned int index_;
> +	std::unique_ptr<Encoder> encoder_;
> +};
> +
> +#endif /* __ANDROID_CAMERA_STREAM__ */
> diff --git a/src/android/meson.build b/src/android/meson.build
> index 0293c2036561..802bb89afe57 100644
> --- a/src/android/meson.build
> +++ b/src/android/meson.build
> @@ -20,6 +20,7 @@ android_hal_sources = files([
>       'camera_device.cpp',
>       'camera_metadata.cpp',
>       'camera_ops.cpp',
> +    'camera_stream.cpp',
>       'jpeg/encoder_libjpeg.cpp',
>       'jpeg/exif.cpp',
>   ])

Patch

diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
index 751699cd2113..bbc692fe109f 100644
--- a/src/android/camera_device.cpp
+++ b/src/android/camera_device.cpp
@@ -169,12 +169,6 @@  MappedCamera3Buffer::MappedCamera3Buffer(const buffer_handle_t camera3buffer,
 	}
 }
 
-CameraStream::CameraStream(PixelFormat format, Size size,
-			   unsigned int index, Encoder *encoder)
-	: format_(format), size_(size), index_(index), encoder_(encoder)
-{
-}
-
 /*
  * \struct Camera3RequestDescriptor
  *
diff --git a/src/android/camera_device.h b/src/android/camera_device.h
index 1837748d2efc..52923ec979a7 100644
--- a/src/android/camera_device.h
+++ b/src/android/camera_device.h
@@ -23,33 +23,11 @@ 
 #include "libcamera/internal/log.h"
 #include "libcamera/internal/message.h"
 
+#include "camera_stream.h"
 #include "jpeg/encoder.h"
 
 class CameraMetadata;
 
-class CameraStream
-{
-public:
-	CameraStream(libcamera::PixelFormat format, libcamera::Size size,
-		     unsigned int index, Encoder *encoder = nullptr);
-
-	const libcamera::PixelFormat &format() const { return format_; }
-	const libcamera::Size &size() const { return size_; }
-	unsigned int index() const { return index_; }
-	Encoder *encoder() const { return encoder_.get(); }
-
-private:
-	libcamera::PixelFormat format_;
-	libcamera::Size size_;
-	/*
-	 * The index of the libcamera StreamConfiguration as added during
-	 * configureStreams(). A single libcamera Stream may be used to deliver
-	 * one or more streams to the Android framework.
-	 */
-	unsigned int index_;
-	std::unique_ptr<Encoder> encoder_;
-};
-
 class CameraDevice : protected libcamera::Loggable
 {
 public:
diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp
new file mode 100644
index 000000000000..01c62978ca3a
--- /dev/null
+++ b/src/android/camera_stream.cpp
@@ -0,0 +1,16 @@ 
+/* SPDX-License-Identifier: LGPL-2.1-or-later */
+/*
+ * Copyright (C) 2020, Google Inc.
+ *
+ * camera_stream.cpp - Camera HAL stream
+ */
+
+#include "camera_stream.h"
+
+using namespace libcamera;
+
+CameraStream::CameraStream(PixelFormat format, Size size,
+			   unsigned int index, Encoder *encoder)
+	: format_(format), size_(size), index_(index), encoder_(encoder)
+{
+}
diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h
new file mode 100644
index 000000000000..10dece7beb69
--- /dev/null
+++ b/src/android/camera_stream.h
@@ -0,0 +1,40 @@ 
+/* SPDX-License-Identifier: LGPL-2.1-or-later */
+/*
+ * Copyright (C) 2020, Google Inc.
+ *
+ * camera_stream.h - Camera HAL stream
+ */
+#ifndef __ANDROID_CAMERA_STREAM_H__
+#define __ANDROID_CAMERA_STREAM_H__
+
+#include <memory>
+
+#include <libcamera/geometry.h>
+#include <libcamera/pixel_format.h>
+
+#include "jpeg/encoder.h"
+
+class CameraStream
+{
+public:
+	CameraStream(libcamera::PixelFormat format, libcamera::Size size,
+		     unsigned int index, Encoder *encoder = nullptr);
+
+	const libcamera::PixelFormat &format() const { return format_; }
+	const libcamera::Size &size() const { return size_; }
+	unsigned int index() const { return index_; }
+	Encoder *encoder() const { return encoder_.get(); }
+
+private:
+	libcamera::PixelFormat format_;
+	libcamera::Size size_;
+	/*
+	 * The index of the libcamera StreamConfiguration as added during
+	 * configureStreams(). A single libcamera Stream may be used to deliver
+	 * one or more streams to the Android framework.
+	 */
+	unsigned int index_;
+	std::unique_ptr<Encoder> encoder_;
+};
+
+#endif /* __ANDROID_CAMERA_STREAM__ */
diff --git a/src/android/meson.build b/src/android/meson.build
index 0293c2036561..802bb89afe57 100644
--- a/src/android/meson.build
+++ b/src/android/meson.build
@@ -20,6 +20,7 @@  android_hal_sources = files([
     'camera_device.cpp',
     'camera_metadata.cpp',
     'camera_ops.cpp',
+    'camera_stream.cpp',
     'jpeg/encoder_libjpeg.cpp',
     'jpeg/exif.cpp',
 ])