[libcamera-devel,v2,07/13] android: camera_stream: Fetch format and size from configuration

Message ID 20201006144432.22908-8-jacopo@jmondi.org
State Accepted
Headers show
Series
  • CameraStream refactor
Related show

Commit Message

Jacopo Mondi Oct. 6, 2020, 2:44 p.m. UTC
Fetch the format and size of the libcamera::StreamConfiguration
associated with a CameraStream by accessing the configuration by
index.

This removes the need to store the libcamera stream format and sizes
as class members and avoid duplicating information that might get out
of sync.

It also allows to remove the StreamConfiguration from the constructor
parameters list, as it can be identified by its index. While at it,
re-order the constructor parameters order.

Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
---
 src/android/camera_device.cpp |  8 +++-----
 src/android/camera_stream.cpp | 15 +++++----------
 src/android/camera_stream.h   | 18 ++++--------------
 3 files changed, 12 insertions(+), 29 deletions(-)

Comments

Kieran Bingham Oct. 6, 2020, 7:53 p.m. UTC | #1
Hi Jacopo,

On 06/10/2020 15:44, Jacopo Mondi wrote:
> Fetch the format and size of the libcamera::StreamConfiguration
> associated with a CameraStream by accessing the configuration by
> index.
> 
> This removes the need to store the libcamera stream format and sizes
> as class members and avoid duplicating information that might get out
> of sync.
> 
> It also allows to remove the StreamConfiguration from the constructor
> parameters list, as it can be identified by its index. While at it,
> re-order the constructor parameters order.
> 
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
>  src/android/camera_device.cpp |  8 +++-----
>  src/android/camera_stream.cpp | 15 +++++----------
>  src/android/camera_stream.h   | 18 ++++--------------
>  3 files changed, 12 insertions(+), 29 deletions(-)
> 
> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> index 47c423195796..678dca609003 100644
> --- a/src/android/camera_device.cpp
> +++ b/src/android/camera_device.cpp
> @@ -1206,9 +1206,8 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
>  		streamConfiguration.pixelFormat = format;
>  
>  		config_->addConfiguration(streamConfiguration);
> -		unsigned int index = config_->size() - 1;

Hrm ... personally, I think specifying that this is an index here
explicitly helps the readability below.

> -		streams_.emplace_back(this, stream, streamConfiguration,
> -				      CameraStream::Type::Direct, index);
> +		streams_.emplace_back(this, CameraStream::Type::Direct,
> +				      stream, config_->size() - 1);
>  		stream->priv = static_cast<void *>(&streams_.back());
>  	}
>  
> @@ -1262,8 +1261,7 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
>  			index = config_->size() - 1;
>  		}
>  
> -		StreamConfiguration &cfg = config_->at(index);
> -		streams_.emplace_back(this, jpegStream, cfg, type, index);
> +		streams_.emplace_back(this, type, jpegStream, index);
>  		jpegStream->priv = static_cast<void *>(&streams_.back());
>  	}
>  
> diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp
> index 9c7819efb679..3946a2cdf844 100644
> --- a/src/android/camera_stream.cpp
> +++ b/src/android/camera_stream.cpp
> @@ -17,18 +17,13 @@ using namespace libcamera;
>  
>  LOG_DECLARE_CATEGORY(HAL);
>  
> -CameraStream::CameraStream(CameraDevice *cameraDevice,
> -			   camera3_stream_t *camera3Stream,
> -			   const libcamera::StreamConfiguration &cfg,
> -			   Type type, unsigned int index)
> -	: cameraDevice_(cameraDevice), camera3Stream_(camera3Stream),
> -	  type_(type), index_(index)
> +CameraStream::CameraStream(CameraDevice *cameraDevice, Type type,
> +			   camera3_stream_t *camera3Stream, unsigned int index)
> +	: cameraDevice_(cameraDevice), type_(type),
> +	  camera3Stream_(camera3Stream), index_(index)
>  {
>  	config_ = cameraDevice_->cameraConfiguration();
>  
> -	format_ = cfg.pixelFormat;
> -	size_ = cfg.size;
> -
>  	if (type_ == Type::Internal || type_ == Type::Mapped)
>  		encoder_ = std::make_unique<EncoderLibJpeg>();
>  }
> @@ -63,7 +58,7 @@ int CameraStream::process(const libcamera::FrameBuffer &source,
>  	exif.setMake("libcamera");
>  	exif.setModel("cameraModel");
>  	exif.setOrientation(cameraDevice_->orientation());
> -	exif.setSize(size_);
> +	exif.setSize(configuration().size);
>  	/*
>  	 * We set the frame's EXIF timestamp as the time of encode.
>  	 * Since the precision we need for EXIF timestamp is only one
> diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h
> index d8d9d8c4b237..f46cfd605d03 100644
> --- a/src/android/camera_stream.h
> +++ b/src/android/camera_stream.h
> @@ -106,16 +106,11 @@ public:
>  		Internal,
>  		Mapped,
>  	};
> -	CameraStream(CameraDevice *cameraDevice,
> -		     camera3_stream_t *androidStream,
> -		     const libcamera::StreamConfiguration &cfg,
> -		     Type type, unsigned int index);
> +	CameraStream(CameraDevice *cameraDevice, Type type,
> +		     camera3_stream_t *camera3Stream, unsigned int index);
>  
> -	const camera3_stream_t &camera3Stream() const { return *camera3Stream_; }
> -	const libcamera::PixelFormat &format() const { return format_; }
> -	const libcamera::Size &size() const { return size_; }
>  	Type type() const { return type_; }
> -
> +	const camera3_stream_t &camera3Stream() const { return *camera3Stream_; }
>  	const libcamera::StreamConfiguration &configuration() const;
>  	libcamera::Stream *stream() const;
>  
> @@ -126,13 +121,8 @@ public:
>  private:
>  	CameraDevice *cameraDevice_;
>  	libcamera::CameraConfiguration *config_;
> -	camera3_stream_t *camera3Stream_;
>  	Type type_;
> -
> -	/* Libcamera facing format and sizes. */
> -	libcamera::PixelFormat format_;
> -	libcamera::Size size_;
> -
> +	camera3_stream_t *camera3Stream_;

a new line here?

But nothing blocking here.

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

>  	/*
>  	 * The index of the libcamera StreamConfiguration as added during
>  	 * configureStreams(). A single libcamera Stream may be used to deliver
>
Laurent Pinchart Oct. 7, 2020, 1:32 a.m. UTC | #2
Hi Jacopo,

Thank you for the patch.

On Tue, Oct 06, 2020 at 04:44:26PM +0200, Jacopo Mondi wrote:
> Fetch the format and size of the libcamera::StreamConfiguration
> associated with a CameraStream by accessing the configuration by
> index.
> 
> This removes the need to store the libcamera stream format and sizes
> as class members and avoid duplicating information that might get out
> of sync.
> 
> It also allows to remove the StreamConfiguration from the constructor
> parameters list, as it can be identified by its index. While at it,
> re-order the constructor parameters order.

I like the new order (along with the rest of the patch). Really nice
work on this series.

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

> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
>  src/android/camera_device.cpp |  8 +++-----
>  src/android/camera_stream.cpp | 15 +++++----------
>  src/android/camera_stream.h   | 18 ++++--------------
>  3 files changed, 12 insertions(+), 29 deletions(-)
> 
> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> index 47c423195796..678dca609003 100644
> --- a/src/android/camera_device.cpp
> +++ b/src/android/camera_device.cpp
> @@ -1206,9 +1206,8 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
>  		streamConfiguration.pixelFormat = format;
>  
>  		config_->addConfiguration(streamConfiguration);
> -		unsigned int index = config_->size() - 1;
> -		streams_.emplace_back(this, stream, streamConfiguration,
> -				      CameraStream::Type::Direct, index);
> +		streams_.emplace_back(this, CameraStream::Type::Direct,
> +				      stream, config_->size() - 1);
>  		stream->priv = static_cast<void *>(&streams_.back());
>  	}
>  
> @@ -1262,8 +1261,7 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
>  			index = config_->size() - 1;
>  		}
>  
> -		StreamConfiguration &cfg = config_->at(index);
> -		streams_.emplace_back(this, jpegStream, cfg, type, index);
> +		streams_.emplace_back(this, type, jpegStream, index);
>  		jpegStream->priv = static_cast<void *>(&streams_.back());
>  	}
>  
> diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp
> index 9c7819efb679..3946a2cdf844 100644
> --- a/src/android/camera_stream.cpp
> +++ b/src/android/camera_stream.cpp
> @@ -17,18 +17,13 @@ using namespace libcamera;
>  
>  LOG_DECLARE_CATEGORY(HAL);
>  
> -CameraStream::CameraStream(CameraDevice *cameraDevice,
> -			   camera3_stream_t *camera3Stream,
> -			   const libcamera::StreamConfiguration &cfg,
> -			   Type type, unsigned int index)
> -	: cameraDevice_(cameraDevice), camera3Stream_(camera3Stream),
> -	  type_(type), index_(index)
> +CameraStream::CameraStream(CameraDevice *cameraDevice, Type type,
> +			   camera3_stream_t *camera3Stream, unsigned int index)
> +	: cameraDevice_(cameraDevice), type_(type),
> +	  camera3Stream_(camera3Stream), index_(index)
>  {
>  	config_ = cameraDevice_->cameraConfiguration();
>  
> -	format_ = cfg.pixelFormat;
> -	size_ = cfg.size;
> -
>  	if (type_ == Type::Internal || type_ == Type::Mapped)
>  		encoder_ = std::make_unique<EncoderLibJpeg>();
>  }
> @@ -63,7 +58,7 @@ int CameraStream::process(const libcamera::FrameBuffer &source,
>  	exif.setMake("libcamera");
>  	exif.setModel("cameraModel");
>  	exif.setOrientation(cameraDevice_->orientation());
> -	exif.setSize(size_);
> +	exif.setSize(configuration().size);
>  	/*
>  	 * We set the frame's EXIF timestamp as the time of encode.
>  	 * Since the precision we need for EXIF timestamp is only one
> diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h
> index d8d9d8c4b237..f46cfd605d03 100644
> --- a/src/android/camera_stream.h
> +++ b/src/android/camera_stream.h
> @@ -106,16 +106,11 @@ public:
>  		Internal,
>  		Mapped,
>  	};
> -	CameraStream(CameraDevice *cameraDevice,
> -		     camera3_stream_t *androidStream,
> -		     const libcamera::StreamConfiguration &cfg,
> -		     Type type, unsigned int index);
> +	CameraStream(CameraDevice *cameraDevice, Type type,
> +		     camera3_stream_t *camera3Stream, unsigned int index);
>  
> -	const camera3_stream_t &camera3Stream() const { return *camera3Stream_; }
> -	const libcamera::PixelFormat &format() const { return format_; }
> -	const libcamera::Size &size() const { return size_; }
>  	Type type() const { return type_; }
> -
> +	const camera3_stream_t &camera3Stream() const { return *camera3Stream_; }
>  	const libcamera::StreamConfiguration &configuration() const;
>  	libcamera::Stream *stream() const;
>  
> @@ -126,13 +121,8 @@ public:
>  private:
>  	CameraDevice *cameraDevice_;
>  	libcamera::CameraConfiguration *config_;
> -	camera3_stream_t *camera3Stream_;
>  	Type type_;
> -
> -	/* Libcamera facing format and sizes. */
> -	libcamera::PixelFormat format_;
> -	libcamera::Size size_;
> -
> +	camera3_stream_t *camera3Stream_;
>  	/*
>  	 * The index of the libcamera StreamConfiguration as added during
>  	 * configureStreams(). A single libcamera Stream may be used to deliver
Jacopo Mondi Oct. 7, 2020, 8:08 a.m. UTC | #3
Hi Kieran

On Tue, Oct 06, 2020 at 08:53:11PM +0100, Kieran Bingham wrote:
> Hi Jacopo,
>
> On 06/10/2020 15:44, Jacopo Mondi wrote:
> > Fetch the format and size of the libcamera::StreamConfiguration
> > associated with a CameraStream by accessing the configuration by
> > index.
> >
> > This removes the need to store the libcamera stream format and sizes
> > as class members and avoid duplicating information that might get out
> > of sync.
> >
> > It also allows to remove the StreamConfiguration from the constructor
> > parameters list, as it can be identified by its index. While at it,
> > re-order the constructor parameters order.
> >
> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > ---
> >  src/android/camera_device.cpp |  8 +++-----
> >  src/android/camera_stream.cpp | 15 +++++----------
> >  src/android/camera_stream.h   | 18 ++++--------------
> >  3 files changed, 12 insertions(+), 29 deletions(-)
> >
> > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> > index 47c423195796..678dca609003 100644
> > --- a/src/android/camera_device.cpp
> > +++ b/src/android/camera_device.cpp
> > @@ -1206,9 +1206,8 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
> >  		streamConfiguration.pixelFormat = format;
> >
> >  		config_->addConfiguration(streamConfiguration);
> > -		unsigned int index = config_->size() - 1;
>
> Hrm ... personally, I think specifying that this is an index here
> explicitly helps the readability below.
>
> > -		streams_.emplace_back(this, stream, streamConfiguration,
> > -				      CameraStream::Type::Direct, index);
> > +		streams_.emplace_back(this, CameraStream::Type::Direct,
> > +				      stream, config_->size() - 1);
> >  		stream->priv = static_cast<void *>(&streams_.back());
> >  	}
> >
> > @@ -1262,8 +1261,7 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
> >  			index = config_->size() - 1;
> >  		}
> >
> > -		StreamConfiguration &cfg = config_->at(index);
> > -		streams_.emplace_back(this, jpegStream, cfg, type, index);
> > +		streams_.emplace_back(this, type, jpegStream, index);
> >  		jpegStream->priv = static_cast<void *>(&streams_.back());
> >  	}
> >
> > diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp
> > index 9c7819efb679..3946a2cdf844 100644
> > --- a/src/android/camera_stream.cpp
> > +++ b/src/android/camera_stream.cpp
> > @@ -17,18 +17,13 @@ using namespace libcamera;
> >
> >  LOG_DECLARE_CATEGORY(HAL);
> >
> > -CameraStream::CameraStream(CameraDevice *cameraDevice,
> > -			   camera3_stream_t *camera3Stream,
> > -			   const libcamera::StreamConfiguration &cfg,
> > -			   Type type, unsigned int index)
> > -	: cameraDevice_(cameraDevice), camera3Stream_(camera3Stream),
> > -	  type_(type), index_(index)
> > +CameraStream::CameraStream(CameraDevice *cameraDevice, Type type,
> > +			   camera3_stream_t *camera3Stream, unsigned int index)
> > +	: cameraDevice_(cameraDevice), type_(type),
> > +	  camera3Stream_(camera3Stream), index_(index)
> >  {
> >  	config_ = cameraDevice_->cameraConfiguration();
> >
> > -	format_ = cfg.pixelFormat;
> > -	size_ = cfg.size;
> > -
> >  	if (type_ == Type::Internal || type_ == Type::Mapped)
> >  		encoder_ = std::make_unique<EncoderLibJpeg>();
> >  }
> > @@ -63,7 +58,7 @@ int CameraStream::process(const libcamera::FrameBuffer &source,
> >  	exif.setMake("libcamera");
> >  	exif.setModel("cameraModel");
> >  	exif.setOrientation(cameraDevice_->orientation());
> > -	exif.setSize(size_);
> > +	exif.setSize(configuration().size);
> >  	/*
> >  	 * We set the frame's EXIF timestamp as the time of encode.
> >  	 * Since the precision we need for EXIF timestamp is only one
> > diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h
> > index d8d9d8c4b237..f46cfd605d03 100644
> > --- a/src/android/camera_stream.h
> > +++ b/src/android/camera_stream.h
> > @@ -106,16 +106,11 @@ public:
> >  		Internal,
> >  		Mapped,
> >  	};
> > -	CameraStream(CameraDevice *cameraDevice,
> > -		     camera3_stream_t *androidStream,
> > -		     const libcamera::StreamConfiguration &cfg,
> > -		     Type type, unsigned int index);
> > +	CameraStream(CameraDevice *cameraDevice, Type type,
> > +		     camera3_stream_t *camera3Stream, unsigned int index);
> >
> > -	const camera3_stream_t &camera3Stream() const { return *camera3Stream_; }
> > -	const libcamera::PixelFormat &format() const { return format_; }
> > -	const libcamera::Size &size() const { return size_; }
> >  	Type type() const { return type_; }
> > -
> > +	const camera3_stream_t &camera3Stream() const { return *camera3Stream_; }
> >  	const libcamera::StreamConfiguration &configuration() const;
> >  	libcamera::Stream *stream() const;
> >
> > @@ -126,13 +121,8 @@ public:
> >  private:
> >  	CameraDevice *cameraDevice_;
> >  	libcamera::CameraConfiguration *config_;
> > -	camera3_stream_t *camera3Stream_;
> >  	Type type_;
> > -
> > -	/* Libcamera facing format and sizes. */
> > -	libcamera::PixelFormat format_;
> > -	libcamera::Size size_;
> > -
> > +	camera3_stream_t *camera3Stream_;
>
> a new line here?

I considered that, but I had troubles identifying in which blocks to
place members:

        CameraDevice
        CameraConfiguration
        Type

        camera3

        /*
         * Big comment
         */
         index
         Encoder

Too sparse, why index and encoder in the same block ?

        CameraDevice
        CameraConfiguration
        Type
        camera3

        /*
         * Big comment
         */
         index
         encoder


Why index is different than others?

        CameraDevice
        CameraConfiguration
        Type
        camera3
        /*
         * Big comment
         */
         index
         encoder

This seems simpler.

>
> But nothing blocking here.
>
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

Thanks
  j

>
> >  	/*
> >  	 * The index of the libcamera StreamConfiguration as added during
> >  	 * configureStreams(). A single libcamera Stream may be used to deliver
> >
>
> --
> Regards
> --
> Kieran

Patch

diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
index 47c423195796..678dca609003 100644
--- a/src/android/camera_device.cpp
+++ b/src/android/camera_device.cpp
@@ -1206,9 +1206,8 @@  int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
 		streamConfiguration.pixelFormat = format;
 
 		config_->addConfiguration(streamConfiguration);
-		unsigned int index = config_->size() - 1;
-		streams_.emplace_back(this, stream, streamConfiguration,
-				      CameraStream::Type::Direct, index);
+		streams_.emplace_back(this, CameraStream::Type::Direct,
+				      stream, config_->size() - 1);
 		stream->priv = static_cast<void *>(&streams_.back());
 	}
 
@@ -1262,8 +1261,7 @@  int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
 			index = config_->size() - 1;
 		}
 
-		StreamConfiguration &cfg = config_->at(index);
-		streams_.emplace_back(this, jpegStream, cfg, type, index);
+		streams_.emplace_back(this, type, jpegStream, index);
 		jpegStream->priv = static_cast<void *>(&streams_.back());
 	}
 
diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp
index 9c7819efb679..3946a2cdf844 100644
--- a/src/android/camera_stream.cpp
+++ b/src/android/camera_stream.cpp
@@ -17,18 +17,13 @@  using namespace libcamera;
 
 LOG_DECLARE_CATEGORY(HAL);
 
-CameraStream::CameraStream(CameraDevice *cameraDevice,
-			   camera3_stream_t *camera3Stream,
-			   const libcamera::StreamConfiguration &cfg,
-			   Type type, unsigned int index)
-	: cameraDevice_(cameraDevice), camera3Stream_(camera3Stream),
-	  type_(type), index_(index)
+CameraStream::CameraStream(CameraDevice *cameraDevice, Type type,
+			   camera3_stream_t *camera3Stream, unsigned int index)
+	: cameraDevice_(cameraDevice), type_(type),
+	  camera3Stream_(camera3Stream), index_(index)
 {
 	config_ = cameraDevice_->cameraConfiguration();
 
-	format_ = cfg.pixelFormat;
-	size_ = cfg.size;
-
 	if (type_ == Type::Internal || type_ == Type::Mapped)
 		encoder_ = std::make_unique<EncoderLibJpeg>();
 }
@@ -63,7 +58,7 @@  int CameraStream::process(const libcamera::FrameBuffer &source,
 	exif.setMake("libcamera");
 	exif.setModel("cameraModel");
 	exif.setOrientation(cameraDevice_->orientation());
-	exif.setSize(size_);
+	exif.setSize(configuration().size);
 	/*
 	 * We set the frame's EXIF timestamp as the time of encode.
 	 * Since the precision we need for EXIF timestamp is only one
diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h
index d8d9d8c4b237..f46cfd605d03 100644
--- a/src/android/camera_stream.h
+++ b/src/android/camera_stream.h
@@ -106,16 +106,11 @@  public:
 		Internal,
 		Mapped,
 	};
-	CameraStream(CameraDevice *cameraDevice,
-		     camera3_stream_t *androidStream,
-		     const libcamera::StreamConfiguration &cfg,
-		     Type type, unsigned int index);
+	CameraStream(CameraDevice *cameraDevice, Type type,
+		     camera3_stream_t *camera3Stream, unsigned int index);
 
-	const camera3_stream_t &camera3Stream() const { return *camera3Stream_; }
-	const libcamera::PixelFormat &format() const { return format_; }
-	const libcamera::Size &size() const { return size_; }
 	Type type() const { return type_; }
-
+	const camera3_stream_t &camera3Stream() const { return *camera3Stream_; }
 	const libcamera::StreamConfiguration &configuration() const;
 	libcamera::Stream *stream() const;
 
@@ -126,13 +121,8 @@  public:
 private:
 	CameraDevice *cameraDevice_;
 	libcamera::CameraConfiguration *config_;
-	camera3_stream_t *camera3Stream_;
 	Type type_;
-
-	/* Libcamera facing format and sizes. */
-	libcamera::PixelFormat format_;
-	libcamera::Size size_;
-
+	camera3_stream_t *camera3Stream_;
 	/*
 	 * The index of the libcamera StreamConfiguration as added during
 	 * configureStreams(). A single libcamera Stream may be used to deliver