[libcamera-devel,04/15] android: camera_stream: Construct with Android stream

9951 diff mbox series

From: Jacopo Mondi <jacopo@jmondi.org>

A CameraStream maps a stream requested by Android to a
libcamera::StreamConfiguration. It shall then be constructed with
those information clearly specified.

Change the CameraStream constructor to accept an Android
camera3_stream_t and a libcamera::StreamConfiguration to copy the
format and size information in the class members as no reference can be
taken to the StreamConfiguration instances as they're subject to
relocations.

Pass to the CameraStream constructor a pointer to the CameraDevice class
and make the CameraConfiguration accessible. It will be used to retrieve
the StreamConfiguration associated with the CameraStream.

Also change the format on which the CameraDevice performs checks to
decide if post-processing is required, as the libcamera facing format is
not meaningful anymore, but the Android required format should be used
instead.

Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
---
 src/android/camera_device.cpp | 11 +++++------
 src/android/camera_device.h   |  4 ++++
 src/android/camera_stream.cpp | 14 ++++++++++++--
 src/android/camera_stream.h   | 18 ++++++++++++++++--
 4 files changed, 37 insertions(+), 10 deletions(-)

Hi Jacopo,

On 05/10/2020 11:46, Laurent Pinchart wrote:
> From: Jacopo Mondi <jacopo@jmondi.org>
> 
> A CameraStream maps a stream requested by Android to a
> libcamera::StreamConfiguration. It shall then be constructed with
> those information clearly specified.
> > Change the CameraStream constructor to accept an Android
> camera3_stream_t and a libcamera::StreamConfiguration to copy the
> format and size information in the class members as no reference can be
> taken to the StreamConfiguration instances as they're subject to
> relocations.

I'm not sure we need to say all that, but could have just been "Pass the
android camera3_stream_t, and a libcamera::StreamConfiguration to
identify the source and destination parameters of this stream.

> 
> Pass to the CameraStream constructor a pointer to the CameraDevice class
> and make the CameraConfiguration accessible. It will be used to retrieve
> the StreamConfiguration associated with the CameraStream.

Pass a CameraDevice pointer to the CameraStream constructor to allow
retrieval of the StreamConfiguration associated with the CameraStream.

> Also change the format on which the CameraDevice performs checks to
> decide if post-processing is required, as the libcamera facing format is
> not meaningful anymore, but the Android required format should be used
> instead.
> 
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
>  src/android/camera_device.cpp | 11 +++++------
>  src/android/camera_device.h   |  4 ++++
>  src/android/camera_stream.cpp | 14 ++++++++++++--
>  src/android/camera_stream.h   | 18 ++++++++++++++++--
>  4 files changed, 37 insertions(+), 10 deletions(-)
> 
> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> index 9c9a5cfa3c2f..2c4dd4dee28c 100644
> --- a/src/android/camera_device.cpp
> +++ b/src/android/camera_device.cpp
> @@ -1216,8 +1216,8 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
>  
>  		config_->addConfiguration(streamConfiguration);
>  		unsigned int index = config_->size() - 1;
> -		streams_.emplace_back(format, size, CameraStream::Type::Direct,
> -				      index);
> +		streams_.emplace_back(this, stream, streamConfiguration,
> +				      CameraStream::Type::Direct, index);
>  		stream->priv = static_cast<void *>(&streams_.back());
>  	}
>  
> @@ -1272,8 +1272,7 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
>  		}
>  
>  		StreamConfiguration &cfg = config_->at(index);
> -
> -		streams_.emplace_back(formats::MJPEG, cfg.size, type, index);
> +		streams_.emplace_back(this, jpegStream, cfg, type, index);
>  		jpegStream->priv = static_cast<void *>(&streams_.back());
>  	}
>  
> @@ -1405,7 +1404,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
>  		descriptor->buffers[i].buffer = camera3Buffers[i].buffer;
>  
>  		/* Software streams are handled after hardware streams complete. */
> -		if (cameraStream->format() == formats::MJPEG)
> +		if (cameraStream->outputFormat() == HAL_PIXEL_FORMAT_BLOB)

I understand the premise for this change, it's just so unfortunate that
the android format is so ... well ... unrepresentative :-)


I haven't gone to check yet, but I wonder if there is a better boolean
logic to check ...

		if (format != NV12) perhaps ?

I guess it depends on what other combinations we'll see through here.

But no need to change this now

>  			continue;
>  
>  		/*
> @@ -1469,7 +1468,7 @@ void CameraDevice::requestComplete(Request *request)
>  		CameraStream *cameraStream =
>  			static_cast<CameraStream *>(descriptor->buffers[i].stream->priv);
>  
> -		if (cameraStream->format() != formats::MJPEG)
> +		if (cameraStream->outputFormat() != HAL_PIXEL_FORMAT_BLOB)
>  			continue;
>  
>  		Encoder *encoder = cameraStream->encoder();
> diff --git a/src/android/camera_device.h b/src/android/camera_device.h
> index 52923ec979a7..4e326fa3e1fb 100644
> --- a/src/android/camera_device.h
> +++ b/src/android/camera_device.h
> @@ -43,6 +43,10 @@ public:
>  	unsigned int id() const { return id_; }
>  	camera3_device_t *camera3Device() { return &camera3Device_; }
>  	const libcamera::Camera *camera() const { return camera_.get(); }
> +	libcamera::CameraConfiguration *cameraConfiguration() const
> +	{
> +		return config_.get();
> +	}
>  
>  	int facing() const { return facing_; }
>  	int orientation() const { return orientation_; }
> diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp
> index 5b2b625c563b..5c1f1d7da416 100644
> --- a/src/android/camera_stream.cpp
> +++ b/src/android/camera_stream.cpp
> @@ -7,13 +7,23 @@
>  
>  #include "camera_stream.h"
>  
> +#include "camera_device.h"
>  #include "jpeg/encoder_libjpeg.h"
>  
>  using namespace libcamera;
>  
> -CameraStream::CameraStream(PixelFormat format, Size size, Type type, unsigned int index)
> -	: format_(format), size_(size), type_(type), index_(index), encoder(nullptr)
> +CameraStream::CameraStream(CameraDevice *cameraDev,
> +			   camera3_stream_t *androidStream,
> +			   const libcamera::StreamConfiguration &cfg,
> +			   Type type, unsigned int index)
> +	: cameraDevice_(cameraDev), androidStream_(androidStream), type_(type),
> +	  index_(index), encoder_(nullptr)
>  {
> +	config_ = cameraDevice_->cameraConfiguration();
> +
> +	format_ = cfg.pixelFormat;
> +	size_ = cfg.size;
> +
>  	if (type_ == Type::Internal || type_ == Type::Mapped)
>  		encoder_.reset(new EncoderLibJpeg);
>  }
> diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h
> index d0dc40d81151..2d71a50c78a4 100644
> --- a/src/android/camera_stream.h
> +++ b/src/android/camera_stream.h
> @@ -9,11 +9,16 @@
>  
>  #include <memory>
>  
> +#include <hardware/camera3.h>
> +
> +#include <libcamera/camera.h>
>  #include <libcamera/geometry.h>
>  #include <libcamera/pixel_format.h>
>  
>  #include "jpeg/encoder.h"
>  
> +class CameraDevice;
> +
>  class CameraStream
>  {
>  public:
> @@ -99,9 +104,12 @@ public:
>  		Internal,
>  		Mapped,
>  	};
> -	CameraStream(libcamera::PixelFormat format, libcamera::Size size,
> +	CameraStream(CameraDevice *cameraDev,

I would have just used 'dev', or 'device', I don't think it conflicts
with anything else in this function does it ?

or as the class private member it goes into is cameraDevice_, it could
have been cameraDevice ;-) - But it's not important either.

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



> +		     camera3_stream_t *androidStream,
> +		     const libcamera::StreamConfiguration &cfg,
>  		     Type type, unsigned int index);
>  
> +	int outputFormat() const { return androidStream_->format; }
>  	const libcamera::PixelFormat &format() const { return format_; }
>  	const libcamera::Size &size() const { return size_; }
>  	Type type() const { return type_; }
> @@ -111,9 +119,15 @@ public:
>  	int configure(const libcamera::StreamConfiguration &cfg);
>  
>  private:
> +	CameraDevice *cameraDevice_;
> +	libcamera::CameraConfiguration *config_;
> +	camera3_stream_t *androidStream_;
> +	Type type_;
> +
> +	/* Libcamera facing format and sizes. */
>  	libcamera::PixelFormat format_;
>  	libcamera::Size size_;
> -	Type type_;
> +
>  	/*
>  	 * The index of the libcamera StreamConfiguration as added during
>  	 * configureStreams(). A single libcamera Stream may be used to deliver
>

On Mon, Oct 05, 2020 at 05:31:31PM +0100, Kieran Bingham wrote:
> On 05/10/2020 11:46, Laurent Pinchart wrote:
> > From: Jacopo Mondi <jacopo@jmondi.org>
> > 
> > A CameraStream maps a stream requested by Android to a
> > libcamera::StreamConfiguration. It shall then be constructed with
> > those information clearly specified.
> > > Change the CameraStream constructor to accept an Android
> > camera3_stream_t and a libcamera::StreamConfiguration to copy the
> > format and size information in the class members as no reference can be
> > taken to the StreamConfiguration instances as they're subject to
> > relocations.
> 
> I'm not sure we need to say all that, but could have just been "Pass the
> android camera3_stream_t, and a libcamera::StreamConfiguration to
> identify the source and destination parameters of this stream.
> 
> > Pass to the CameraStream constructor a pointer to the CameraDevice class
> > and make the CameraConfiguration accessible. It will be used to retrieve
> > the StreamConfiguration associated with the CameraStream.
> 
> Pass a CameraDevice pointer to the CameraStream constructor to allow
> retrieval of the StreamConfiguration associated with the CameraStream.
> 
> > Also change the format on which the CameraDevice performs checks to
> > decide if post-processing is required, as the libcamera facing format is
> > not meaningful anymore, but the Android required format should be used
> > instead.
> > 
> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > ---
> >  src/android/camera_device.cpp | 11 +++++------
> >  src/android/camera_device.h   |  4 ++++
> >  src/android/camera_stream.cpp | 14 ++++++++++++--
> >  src/android/camera_stream.h   | 18 ++++++++++++++++--
> >  4 files changed, 37 insertions(+), 10 deletions(-)
> > 
> > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> > index 9c9a5cfa3c2f..2c4dd4dee28c 100644
> > --- a/src/android/camera_device.cpp
> > +++ b/src/android/camera_device.cpp
> > @@ -1216,8 +1216,8 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
> >  
> >  		config_->addConfiguration(streamConfiguration);
> >  		unsigned int index = config_->size() - 1;
> > -		streams_.emplace_back(format, size, CameraStream::Type::Direct,
> > -				      index);
> > +		streams_.emplace_back(this, stream, streamConfiguration,
> > +				      CameraStream::Type::Direct, index);
> >  		stream->priv = static_cast<void *>(&streams_.back());
> >  	}
> >  
> > @@ -1272,8 +1272,7 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
> >  		}
> >  
> >  		StreamConfiguration &cfg = config_->at(index);
> > -
> > -		streams_.emplace_back(formats::MJPEG, cfg.size, type, index);
> > +		streams_.emplace_back(this, jpegStream, cfg, type, index);
> >  		jpegStream->priv = static_cast<void *>(&streams_.back());
> >  	}
> >  
> > @@ -1405,7 +1404,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
> >  		descriptor->buffers[i].buffer = camera3Buffers[i].buffer;
> >  
> >  		/* Software streams are handled after hardware streams complete. */
> > -		if (cameraStream->format() == formats::MJPEG)
> > +		if (cameraStream->outputFormat() == HAL_PIXEL_FORMAT_BLOB)
> 
> I understand the premise for this change, it's just so unfortunate that
> the android format is so ... well ... unrepresentative :-)
> 
> 
> I haven't gone to check yet, but I wonder if there is a better boolean
> logic to check ...
> 
> 		if (format != NV12) perhaps ?
> 
> I guess it depends on what other combinations we'll see through here.
> 
> But no need to change this now
> 
> >  			continue;
> >  
> >  		/*
> > @@ -1469,7 +1468,7 @@ void CameraDevice::requestComplete(Request *request)
> >  		CameraStream *cameraStream =
> >  			static_cast<CameraStream *>(descriptor->buffers[i].stream->priv);
> >  
> > -		if (cameraStream->format() != formats::MJPEG)
> > +		if (cameraStream->outputFormat() != HAL_PIXEL_FORMAT_BLOB)
> >  			continue;
> >  
> >  		Encoder *encoder = cameraStream->encoder();
> > diff --git a/src/android/camera_device.h b/src/android/camera_device.h
> > index 52923ec979a7..4e326fa3e1fb 100644
> > --- a/src/android/camera_device.h
> > +++ b/src/android/camera_device.h
> > @@ -43,6 +43,10 @@ public:
> >  	unsigned int id() const { return id_; }
> >  	camera3_device_t *camera3Device() { return &camera3Device_; }
> >  	const libcamera::Camera *camera() const { return camera_.get(); }
> > +	libcamera::CameraConfiguration *cameraConfiguration() const
> > +	{
> > +		return config_.get();
> > +	}
> >  
> >  	int facing() const { return facing_; }
> >  	int orientation() const { return orientation_; }
> > diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp
> > index 5b2b625c563b..5c1f1d7da416 100644
> > --- a/src/android/camera_stream.cpp
> > +++ b/src/android/camera_stream.cpp
> > @@ -7,13 +7,23 @@
> >  
> >  #include "camera_stream.h"
> >  
> > +#include "camera_device.h"
> >  #include "jpeg/encoder_libjpeg.h"
> >  
> >  using namespace libcamera;
> >  
> > -CameraStream::CameraStream(PixelFormat format, Size size, Type type, unsigned int index)
> > -	: format_(format), size_(size), type_(type), index_(index), encoder(nullptr)
> > +CameraStream::CameraStream(CameraDevice *cameraDev,
> > +			   camera3_stream_t *androidStream,
> > +			   const libcamera::StreamConfiguration &cfg,
> > +			   Type type, unsigned int index)
> > +	: cameraDevice_(cameraDev), androidStream_(androidStream), type_(type),
> > +	  index_(index), encoder_(nullptr)
> >  {
> > +	config_ = cameraDevice_->cameraConfiguration();
> > +
> > +	format_ = cfg.pixelFormat;
> > +	size_ = cfg.size;
> > +
> >  	if (type_ == Type::Internal || type_ == Type::Mapped)
> >  		encoder_.reset(new EncoderLibJpeg);
> >  }
> > diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h
> > index d0dc40d81151..2d71a50c78a4 100644
> > --- a/src/android/camera_stream.h
> > +++ b/src/android/camera_stream.h
> > @@ -9,11 +9,16 @@
> >  
> >  #include <memory>
> >  
> > +#include <hardware/camera3.h>
> > +
> > +#include <libcamera/camera.h>
> >  #include <libcamera/geometry.h>
> >  #include <libcamera/pixel_format.h>
> >  
> >  #include "jpeg/encoder.h"
> >  
> > +class CameraDevice;
> > +
> >  class CameraStream
> >  {
> >  public:
> > @@ -99,9 +104,12 @@ public:
> >  		Internal,
> >  		Mapped,
> >  	};
> > -	CameraStream(libcamera::PixelFormat format, libcamera::Size size,
> > +	CameraStream(CameraDevice *cameraDev,
> 
> I would have just used 'dev', or 'device', I don't think it conflicts
> with anything else in this function does it ?

I like short names when they're not ambiguous, and even more
importantly, when they're used consistently. If someone wants to go
through the HAL code at some point to propose a consistent naming scheme
(CameraStream and camera3_stream_t is a good candidate for instance) and
make sure it's consistently applied, I'll be happy to review it.

> or as the class private member it goes into is cameraDevice_, it could
> have been cameraDevice ;-) - But it's not important either.
> 
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> 
> > +		     camera3_stream_t *androidStream,
> > +		     const libcamera::StreamConfiguration &cfg,
> >  		     Type type, unsigned int index);
> >  
> > +	int outputFormat() const { return androidStream_->format; }
> >  	const libcamera::PixelFormat &format() const { return format_; }
> >  	const libcamera::Size &size() const { return size_; }
> >  	Type type() const { return type_; }
> > @@ -111,9 +119,15 @@ public:
> >  	int configure(const libcamera::StreamConfiguration &cfg);
> >  
> >  private:
> > +	CameraDevice *cameraDevice_;
> > +	libcamera::CameraConfiguration *config_;
> > +	camera3_stream_t *androidStream_;
> > +	Type type_;
> > +
> > +	/* Libcamera facing format and sizes. */
> >  	libcamera::PixelFormat format_;
> >  	libcamera::Size size_;
> > -	Type type_;
> > +
> >  	/*
> >  	 * The index of the libcamera StreamConfiguration as added during
> >  	 * configureStreams(). A single libcamera Stream may be used to deliver

Hi Kieran,

On Mon, Oct 05, 2020 at 05:31:31PM +0100, Kieran Bingham wrote:
> Hi Jacopo,
>
> On 05/10/2020 11:46, Laurent Pinchart wrote:
> > From: Jacopo Mondi <jacopo@jmondi.org>
> >
> > A CameraStream maps a stream requested by Android to a
> > libcamera::StreamConfiguration. It shall then be constructed with
> > those information clearly specified.
> > > Change the CameraStream constructor to accept an Android
> > camera3_stream_t and a libcamera::StreamConfiguration to copy the
> > format and size information in the class members as no reference can be
> > taken to the StreamConfiguration instances as they're subject to
> > relocations.
>
> I'm not sure we need to say all that, but could have just been "Pass the
> android camera3_stream_t, and a libcamera::StreamConfiguration to
> identify the source and destination parameters of this stream.
>
> >
> > Pass to the CameraStream constructor a pointer to the CameraDevice class
> > and make the CameraConfiguration accessible. It will be used to retrieve
> > the StreamConfiguration associated with the CameraStream.
>
> Pass a CameraDevice pointer to the CameraStream constructor to allow
> retrieval of the StreamConfiguration associated with the CameraStream.
>

Thanks, much better!

> > Also change the format on which the CameraDevice performs checks to
> > decide if post-processing is required, as the libcamera facing format is
> > not meaningful anymore, but the Android required format should be used
> > instead.
> >
> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > ---
> >  src/android/camera_device.cpp | 11 +++++------
> >  src/android/camera_device.h   |  4 ++++
> >  src/android/camera_stream.cpp | 14 ++++++++++++--
> >  src/android/camera_stream.h   | 18 ++++++++++++++++--
> >  4 files changed, 37 insertions(+), 10 deletions(-)
> >
> > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> > index 9c9a5cfa3c2f..2c4dd4dee28c 100644
> > --- a/src/android/camera_device.cpp
> > +++ b/src/android/camera_device.cpp
> > @@ -1216,8 +1216,8 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
> >
> >  		config_->addConfiguration(streamConfiguration);
> >  		unsigned int index = config_->size() - 1;
> > -		streams_.emplace_back(format, size, CameraStream::Type::Direct,
> > -				      index);
> > +		streams_.emplace_back(this, stream, streamConfiguration,
> > +				      CameraStream::Type::Direct, index);
> >  		stream->priv = static_cast<void *>(&streams_.back());
> >  	}
> >
> > @@ -1272,8 +1272,7 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
> >  		}
> >
> >  		StreamConfiguration &cfg = config_->at(index);
> > -
> > -		streams_.emplace_back(formats::MJPEG, cfg.size, type, index);
> > +		streams_.emplace_back(this, jpegStream, cfg, type, index);
> >  		jpegStream->priv = static_cast<void *>(&streams_.back());
> >  	}
> >
> > @@ -1405,7 +1404,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
> >  		descriptor->buffers[i].buffer = camera3Buffers[i].buffer;
> >
> >  		/* Software streams are handled after hardware streams complete. */
> > -		if (cameraStream->format() == formats::MJPEG)
> > +		if (cameraStream->outputFormat() == HAL_PIXEL_FORMAT_BLOB)
>
> I understand the premise for this change, it's just so unfortunate that
> the android format is so ... well ... unrepresentative :-)

More than this, I'm thinking of doing what I'll do with the libcamera
StreamConfiguration
                if (cameraStream->androidStream().format == ... )

And not provide a confusing CameraStream::outputFormat() helper at
all.

>
>
> I haven't gone to check yet, but I wonder if there is a better boolean
> logic to check ...
>
> 		if (format != NV12) perhaps ?

We can't bet to have only NV12 and MJPEG

>
> I guess it depends on what other combinations we'll see through here.
>
> But no need to change this now
>
> >  			continue;
> >
> >  		/*
> > @@ -1469,7 +1468,7 @@ void CameraDevice::requestComplete(Request *request)
> >  		CameraStream *cameraStream =
> >  			static_cast<CameraStream *>(descriptor->buffers[i].stream->priv);
> >
> > -		if (cameraStream->format() != formats::MJPEG)
> > +		if (cameraStream->outputFormat() != HAL_PIXEL_FORMAT_BLOB)
> >  			continue;
> >
> >  		Encoder *encoder = cameraStream->encoder();
> > diff --git a/src/android/camera_device.h b/src/android/camera_device.h
> > index 52923ec979a7..4e326fa3e1fb 100644
> > --- a/src/android/camera_device.h
> > +++ b/src/android/camera_device.h
> > @@ -43,6 +43,10 @@ public:
> >  	unsigned int id() const { return id_; }
> >  	camera3_device_t *camera3Device() { return &camera3Device_; }
> >  	const libcamera::Camera *camera() const { return camera_.get(); }
> > +	libcamera::CameraConfiguration *cameraConfiguration() const
> > +	{
> > +		return config_.get();
> > +	}
> >
> >  	int facing() const { return facing_; }
> >  	int orientation() const { return orientation_; }
> > diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp
> > index 5b2b625c563b..5c1f1d7da416 100644
> > --- a/src/android/camera_stream.cpp
> > +++ b/src/android/camera_stream.cpp
> > @@ -7,13 +7,23 @@
> >
> >  #include "camera_stream.h"
> >
> > +#include "camera_device.h"
> >  #include "jpeg/encoder_libjpeg.h"
> >
> >  using namespace libcamera;
> >
> > -CameraStream::CameraStream(PixelFormat format, Size size, Type type, unsigned int index)
> > -	: format_(format), size_(size), type_(type), index_(index), encoder(nullptr)
> > +CameraStream::CameraStream(CameraDevice *cameraDev,
> > +			   camera3_stream_t *androidStream,
> > +			   const libcamera::StreamConfiguration &cfg,
> > +			   Type type, unsigned int index)
> > +	: cameraDevice_(cameraDev), androidStream_(androidStream), type_(type),
> > +	  index_(index), encoder_(nullptr)
> >  {
> > +	config_ = cameraDevice_->cameraConfiguration();
> > +
> > +	format_ = cfg.pixelFormat;
> > +	size_ = cfg.size;
> > +
> >  	if (type_ == Type::Internal || type_ == Type::Mapped)
> >  		encoder_.reset(new EncoderLibJpeg);
> >  }
> > diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h
> > index d0dc40d81151..2d71a50c78a4 100644
> > --- a/src/android/camera_stream.h
> > +++ b/src/android/camera_stream.h
> > @@ -9,11 +9,16 @@
> >
> >  #include <memory>
> >
> > +#include <hardware/camera3.h>
> > +
> > +#include <libcamera/camera.h>
> >  #include <libcamera/geometry.h>
> >  #include <libcamera/pixel_format.h>
> >
> >  #include "jpeg/encoder.h"
> >
> > +class CameraDevice;
> > +
> >  class CameraStream
> >  {
> >  public:
> > @@ -99,9 +104,12 @@ public:
> >  		Internal,
> >  		Mapped,
> >  	};
> > -	CameraStream(libcamera::PixelFormat format, libcamera::Size size,
> > +	CameraStream(CameraDevice *cameraDev,
>
> I would have just used 'dev', or 'device', I don't think it conflicts
> with anything else in this function does it ?
>
> or as the class private member it goes into is cameraDevice_, it could
> have been cameraDevice ;-) - But it's not important either.

I thought about shortening the name too, but as it goes to
cameraDevice_ I kept it long.

And as Laurent said, if we could unify the naming schemes (ie.
everything android prefixed with camera3 and everything libcamera
prefixed with.... ) it would be nice

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

Thanks
  j

>
>
>
> > +		     camera3_stream_t *androidStream,
> > +		     const libcamera::StreamConfiguration &cfg,
> >  		     Type type, unsigned int index);
> >
> > +	int outputFormat() const { return androidStream_->format; }
> >  	const libcamera::PixelFormat &format() const { return format_; }
> >  	const libcamera::Size &size() const { return size_; }
> >  	Type type() const { return type_; }
> > @@ -111,9 +119,15 @@ public:
> >  	int configure(const libcamera::StreamConfiguration &cfg);
> >
> >  private:
> > +	CameraDevice *cameraDevice_;
> > +	libcamera::CameraConfiguration *config_;
> > +	camera3_stream_t *androidStream_;
> > +	Type type_;
> > +
> > +	/* Libcamera facing format and sizes. */
> >  	libcamera::PixelFormat format_;
> >  	libcamera::Size size_;
> > -	Type type_;
> > +
> >  	/*
> >  	 * The index of the libcamera StreamConfiguration as added during
> >  	 * configureStreams(). A single libcamera Stream may be used to deliver
> >
>
> --
> Regards
> --
> Kieran

Hi Jacopo,

On 06/10/2020 12:24, Jacopo Mondi wrote:
> Hi Kieran,
> 
> On Mon, Oct 05, 2020 at 05:31:31PM +0100, Kieran Bingham wrote:
>> Hi Jacopo,
>>
>> On 05/10/2020 11:46, Laurent Pinchart wrote:
>>> From: Jacopo Mondi <jacopo@jmondi.org>
>>>
>>> A CameraStream maps a stream requested by Android to a
>>> libcamera::StreamConfiguration. It shall then be constructed with
>>> those information clearly specified.
>>>> Change the CameraStream constructor to accept an Android
>>> camera3_stream_t and a libcamera::StreamConfiguration to copy the
>>> format and size information in the class members as no reference can be
>>> taken to the StreamConfiguration instances as they're subject to
>>> relocations.
>>
>> I'm not sure we need to say all that, but could have just been "Pass the
>> android camera3_stream_t, and a libcamera::StreamConfiguration to
>> identify the source and destination parameters of this stream.
>>
>>>
>>> Pass to the CameraStream constructor a pointer to the CameraDevice class
>>> and make the CameraConfiguration accessible. It will be used to retrieve
>>> the StreamConfiguration associated with the CameraStream.
>>
>> Pass a CameraDevice pointer to the CameraStream constructor to allow
>> retrieval of the StreamConfiguration associated with the CameraStream.
>>
> 
> Thanks, much better!
> 
>>> Also change the format on which the CameraDevice performs checks to
>>> decide if post-processing is required, as the libcamera facing format is
>>> not meaningful anymore, but the Android required format should be used
>>> instead.
>>>
>>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
>>> ---
>>>  src/android/camera_device.cpp | 11 +++++------
>>>  src/android/camera_device.h   |  4 ++++
>>>  src/android/camera_stream.cpp | 14 ++++++++++++--
>>>  src/android/camera_stream.h   | 18 ++++++++++++++++--
>>>  4 files changed, 37 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
>>> index 9c9a5cfa3c2f..2c4dd4dee28c 100644
>>> --- a/src/android/camera_device.cpp
>>> +++ b/src/android/camera_device.cpp
>>> @@ -1216,8 +1216,8 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
>>>
>>>  		config_->addConfiguration(streamConfiguration);
>>>  		unsigned int index = config_->size() - 1;
>>> -		streams_.emplace_back(format, size, CameraStream::Type::Direct,
>>> -				      index);
>>> +		streams_.emplace_back(this, stream, streamConfiguration,
>>> +				      CameraStream::Type::Direct, index);
>>>  		stream->priv = static_cast<void *>(&streams_.back());
>>>  	}
>>>
>>> @@ -1272,8 +1272,7 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
>>>  		}
>>>
>>>  		StreamConfiguration &cfg = config_->at(index);
>>> -
>>> -		streams_.emplace_back(formats::MJPEG, cfg.size, type, index);
>>> +		streams_.emplace_back(this, jpegStream, cfg, type, index);
>>>  		jpegStream->priv = static_cast<void *>(&streams_.back());
>>>  	}
>>>
>>> @@ -1405,7 +1404,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
>>>  		descriptor->buffers[i].buffer = camera3Buffers[i].buffer;
>>>
>>>  		/* Software streams are handled after hardware streams complete. */
>>> -		if (cameraStream->format() == formats::MJPEG)
>>> +		if (cameraStream->outputFormat() == HAL_PIXEL_FORMAT_BLOB)
>>
>> I understand the premise for this change, it's just so unfortunate that
>> the android format is so ... well ... unrepresentative :-)
> 
> More than this, I'm thinking of doing what I'll do with the libcamera
> StreamConfiguration
>                 if (cameraStream->androidStream().format == ... )
> 
> And not provide a confusing CameraStream::outputFormat() helper at
> all.

Great, that looks good!


>> I haven't gone to check yet, but I wonder if there is a better boolean
>> logic to check ...
>>
>> 		if (format != NV12) perhaps ?
> 
> We can't bet to have only NV12 and MJPEG

No, indeed - my premise was that we will (later) need to make the
decision somewhere about what a software stream is.

I.e. - if the Android stream is an NV12, and we can only generate a
YUYV, we will need a software(or opengl :D) format convertor (to support
UVC).

So there is going to be a more complex requirement here to decide what a
software stream is, which will really be
   "input format != output format"...

Anyway, this is fine for now.

The internal buffer handling will in fact make it much easier to look at
software format conversion anyway.


>> I guess it depends on what other combinations we'll see through here.
>>
>> But no need to change this now
>>
>>>  			continue;
>>>
>>>  		/*
>>> @@ -1469,7 +1468,7 @@ void CameraDevice::requestComplete(Request *request)
>>>  		CameraStream *cameraStream =
>>>  			static_cast<CameraStream *>(descriptor->buffers[i].stream->priv);
>>>
>>> -		if (cameraStream->format() != formats::MJPEG)
>>> +		if (cameraStream->outputFormat() != HAL_PIXEL_FORMAT_BLOB)
>>>  			continue;
>>>
>>>  		Encoder *encoder = cameraStream->encoder();
>>> diff --git a/src/android/camera_device.h b/src/android/camera_device.h
>>> index 52923ec979a7..4e326fa3e1fb 100644
>>> --- a/src/android/camera_device.h
>>> +++ b/src/android/camera_device.h
>>> @@ -43,6 +43,10 @@ public:
>>>  	unsigned int id() const { return id_; }
>>>  	camera3_device_t *camera3Device() { return &camera3Device_; }
>>>  	const libcamera::Camera *camera() const { return camera_.get(); }
>>> +	libcamera::CameraConfiguration *cameraConfiguration() const
>>> +	{
>>> +		return config_.get();
>>> +	}
>>>
>>>  	int facing() const { return facing_; }
>>>  	int orientation() const { return orientation_; }
>>> diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp
>>> index 5b2b625c563b..5c1f1d7da416 100644
>>> --- a/src/android/camera_stream.cpp
>>> +++ b/src/android/camera_stream.cpp
>>> @@ -7,13 +7,23 @@
>>>
>>>  #include "camera_stream.h"
>>>
>>> +#include "camera_device.h"
>>>  #include "jpeg/encoder_libjpeg.h"
>>>
>>>  using namespace libcamera;
>>>
>>> -CameraStream::CameraStream(PixelFormat format, Size size, Type type, unsigned int index)
>>> -	: format_(format), size_(size), type_(type), index_(index), encoder(nullptr)
>>> +CameraStream::CameraStream(CameraDevice *cameraDev,
>>> +			   camera3_stream_t *androidStream,
>>> +			   const libcamera::StreamConfiguration &cfg,
>>> +			   Type type, unsigned int index)
>>> +	: cameraDevice_(cameraDev), androidStream_(androidStream), type_(type),
>>> +	  index_(index), encoder_(nullptr)
>>>  {
>>> +	config_ = cameraDevice_->cameraConfiguration();
>>> +
>>> +	format_ = cfg.pixelFormat;
>>> +	size_ = cfg.size;
>>> +
>>>  	if (type_ == Type::Internal || type_ == Type::Mapped)
>>>  		encoder_.reset(new EncoderLibJpeg);
>>>  }
>>> diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h
>>> index d0dc40d81151..2d71a50c78a4 100644
>>> --- a/src/android/camera_stream.h
>>> +++ b/src/android/camera_stream.h
>>> @@ -9,11 +9,16 @@
>>>
>>>  #include <memory>
>>>
>>> +#include <hardware/camera3.h>
>>> +
>>> +#include <libcamera/camera.h>
>>>  #include <libcamera/geometry.h>
>>>  #include <libcamera/pixel_format.h>
>>>
>>>  #include "jpeg/encoder.h"
>>>
>>> +class CameraDevice;
>>> +
>>>  class CameraStream
>>>  {
>>>  public:
>>> @@ -99,9 +104,12 @@ public:
>>>  		Internal,
>>>  		Mapped,
>>>  	};
>>> -	CameraStream(libcamera::PixelFormat format, libcamera::Size size,
>>> +	CameraStream(CameraDevice *cameraDev,
>>
>> I would have just used 'dev', or 'device', I don't think it conflicts
>> with anything else in this function does it ?
>>
>> or as the class private member it goes into is cameraDevice_, it could
>> have been cameraDevice ;-) - But it's not important either.
> 
> I thought about shortening the name too, but as it goes to
> cameraDevice_ I kept it long.
> 
> And as Laurent said, if we could unify the naming schemes (ie.
> everything android prefixed with camera3 and everything libcamera
> prefixed with.... ) it would be nice
> 
>>
>> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> 
> Thanks
>   j
> 
>>
>>
>>
>>> +		     camera3_stream_t *androidStream,
>>> +		     const libcamera::StreamConfiguration &cfg,
>>>  		     Type type, unsigned int index);
>>>
>>> +	int outputFormat() const { return androidStream_->format; }
>>>  	const libcamera::PixelFormat &format() const { return format_; }
>>>  	const libcamera::Size &size() const { return size_; }
>>>  	Type type() const { return type_; }
>>> @@ -111,9 +119,15 @@ public:
>>>  	int configure(const libcamera::StreamConfiguration &cfg);
>>>
>>>  private:
>>> +	CameraDevice *cameraDevice_;
>>> +	libcamera::CameraConfiguration *config_;
>>> +	camera3_stream_t *androidStream_;
>>> +	Type type_;
>>> +
>>> +	/* Libcamera facing format and sizes. */
>>>  	libcamera::PixelFormat format_;
>>>  	libcamera::Size size_;
>>> -	Type type_;
>>> +
>>>  	/*
>>>  	 * The index of the libcamera StreamConfiguration as added during
>>>  	 * configureStreams(). A single libcamera Stream may be used to deliver
>>>
>>
>> --
>> Regards
>> --
>> Kieran

Hello,

On Tue, Oct 06, 2020 at 01:21:00PM +0100, Kieran Bingham wrote:
> On 06/10/2020 12:24, Jacopo Mondi wrote:
> > On Mon, Oct 05, 2020 at 05:31:31PM +0100, Kieran Bingham wrote:
> >> On 05/10/2020 11:46, Laurent Pinchart wrote:
> >>> From: Jacopo Mondi <jacopo@jmondi.org>
> >>>
> >>> A CameraStream maps a stream requested by Android to a
> >>> libcamera::StreamConfiguration. It shall then be constructed with
> >>> those information clearly specified.
> >>>> Change the CameraStream constructor to accept an Android
> >>> camera3_stream_t and a libcamera::StreamConfiguration to copy the
> >>> format and size information in the class members as no reference can be
> >>> taken to the StreamConfiguration instances as they're subject to
> >>> relocations.
> >>
> >> I'm not sure we need to say all that, but could have just been "Pass the
> >> android camera3_stream_t, and a libcamera::StreamConfiguration to
> >> identify the source and destination parameters of this stream.
> >>
> >>> Pass to the CameraStream constructor a pointer to the CameraDevice class
> >>> and make the CameraConfiguration accessible. It will be used to retrieve
> >>> the StreamConfiguration associated with the CameraStream.
> >>
> >> Pass a CameraDevice pointer to the CameraStream constructor to allow
> >> retrieval of the StreamConfiguration associated with the CameraStream.
> > 
> > Thanks, much better!
> > 
> >>> Also change the format on which the CameraDevice performs checks to
> >>> decide if post-processing is required, as the libcamera facing format is
> >>> not meaningful anymore, but the Android required format should be used
> >>> instead.
> >>>
> >>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> >>> ---
> >>>  src/android/camera_device.cpp | 11 +++++------
> >>>  src/android/camera_device.h   |  4 ++++
> >>>  src/android/camera_stream.cpp | 14 ++++++++++++--
> >>>  src/android/camera_stream.h   | 18 ++++++++++++++++--
> >>>  4 files changed, 37 insertions(+), 10 deletions(-)
> >>>
> >>> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> >>> index 9c9a5cfa3c2f..2c4dd4dee28c 100644
> >>> --- a/src/android/camera_device.cpp
> >>> +++ b/src/android/camera_device.cpp
> >>> @@ -1216,8 +1216,8 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
> >>>
> >>>  		config_->addConfiguration(streamConfiguration);
> >>>  		unsigned int index = config_->size() - 1;
> >>> -		streams_.emplace_back(format, size, CameraStream::Type::Direct,
> >>> -				      index);
> >>> +		streams_.emplace_back(this, stream, streamConfiguration,
> >>> +				      CameraStream::Type::Direct, index);
> >>>  		stream->priv = static_cast<void *>(&streams_.back());
> >>>  	}
> >>>
> >>> @@ -1272,8 +1272,7 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
> >>>  		}
> >>>
> >>>  		StreamConfiguration &cfg = config_->at(index);
> >>> -
> >>> -		streams_.emplace_back(formats::MJPEG, cfg.size, type, index);
> >>> +		streams_.emplace_back(this, jpegStream, cfg, type, index);
> >>>  		jpegStream->priv = static_cast<void *>(&streams_.back());
> >>>  	}
> >>>
> >>> @@ -1405,7 +1404,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
> >>>  		descriptor->buffers[i].buffer = camera3Buffers[i].buffer;
> >>>
> >>>  		/* Software streams are handled after hardware streams complete. */
> >>> -		if (cameraStream->format() == formats::MJPEG)
> >>> +		if (cameraStream->outputFormat() == HAL_PIXEL_FORMAT_BLOB)
> >>
> >> I understand the premise for this change, it's just so unfortunate that
> >> the android format is so ... well ... unrepresentative :-)
> > 
> > More than this, I'm thinking of doing what I'll do with the libcamera
> > StreamConfiguration
> >                 if (cameraStream->androidStream().format == ... )
> > 
> > And not provide a confusing CameraStream::outputFormat() helper at
> > all.
> 
> Great, that looks good!
> 
> >> I haven't gone to check yet, but I wonder if there is a better boolean
> >> logic to check ...
> >>
> >> 		if (format != NV12) perhaps ?
> > 
> > We can't bet to have only NV12 and MJPEG
> 
> No, indeed - my premise was that we will (later) need to make the
> decision somewhere about what a software stream is.
> 
> I.e. - if the Android stream is an NV12, and we can only generate a
> YUYV, we will need a software(or opengl :D) format convertor (to support
> UVC).
> 
> So there is going to be a more complex requirement here to decide what a
> software stream is, which will really be

Should we start calling them post-processed streams (we can bikeshed the
name), as the post-processing may be handled by hardware (hardware
scaler or format converter, JPEG encoder, GPU, ...) ?

>    "input format != output format"...
> 
> Anyway, this is fine for now.
> 
> The internal buffer handling will in fact make it much easier to look at
> software format conversion anyway.
>
> >> I guess it depends on what other combinations we'll see through here.
> >>
> >> But no need to change this now
> >>
> >>>  			continue;
> >>>
> >>>  		/*
> >>> @@ -1469,7 +1468,7 @@ void CameraDevice::requestComplete(Request *request)
> >>>  		CameraStream *cameraStream =
> >>>  			static_cast<CameraStream *>(descriptor->buffers[i].stream->priv);
> >>>
> >>> -		if (cameraStream->format() != formats::MJPEG)
> >>> +		if (cameraStream->outputFormat() != HAL_PIXEL_FORMAT_BLOB)
> >>>  			continue;
> >>>
> >>>  		Encoder *encoder = cameraStream->encoder();
> >>> diff --git a/src/android/camera_device.h b/src/android/camera_device.h
> >>> index 52923ec979a7..4e326fa3e1fb 100644
> >>> --- a/src/android/camera_device.h
> >>> +++ b/src/android/camera_device.h
> >>> @@ -43,6 +43,10 @@ public:
> >>>  	unsigned int id() const { return id_; }
> >>>  	camera3_device_t *camera3Device() { return &camera3Device_; }
> >>>  	const libcamera::Camera *camera() const { return camera_.get(); }
> >>> +	libcamera::CameraConfiguration *cameraConfiguration() const
> >>> +	{
> >>> +		return config_.get();
> >>> +	}
> >>>
> >>>  	int facing() const { return facing_; }
> >>>  	int orientation() const { return orientation_; }
> >>> diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp
> >>> index 5b2b625c563b..5c1f1d7da416 100644
> >>> --- a/src/android/camera_stream.cpp
> >>> +++ b/src/android/camera_stream.cpp
> >>> @@ -7,13 +7,23 @@
> >>>
> >>>  #include "camera_stream.h"
> >>>
> >>> +#include "camera_device.h"
> >>>  #include "jpeg/encoder_libjpeg.h"
> >>>
> >>>  using namespace libcamera;
> >>>
> >>> -CameraStream::CameraStream(PixelFormat format, Size size, Type type, unsigned int index)
> >>> -	: format_(format), size_(size), type_(type), index_(index), encoder(nullptr)
> >>> +CameraStream::CameraStream(CameraDevice *cameraDev,
> >>> +			   camera3_stream_t *androidStream,
> >>> +			   const libcamera::StreamConfiguration &cfg,
> >>> +			   Type type, unsigned int index)
> >>> +	: cameraDevice_(cameraDev), androidStream_(androidStream), type_(type),
> >>> +	  index_(index), encoder_(nullptr)
> >>>  {
> >>> +	config_ = cameraDevice_->cameraConfiguration();
> >>> +
> >>> +	format_ = cfg.pixelFormat;
> >>> +	size_ = cfg.size;
> >>> +
> >>>  	if (type_ == Type::Internal || type_ == Type::Mapped)
> >>>  		encoder_.reset(new EncoderLibJpeg);
> >>>  }
> >>> diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h
> >>> index d0dc40d81151..2d71a50c78a4 100644
> >>> --- a/src/android/camera_stream.h
> >>> +++ b/src/android/camera_stream.h
> >>> @@ -9,11 +9,16 @@
> >>>
> >>>  #include <memory>
> >>>
> >>> +#include <hardware/camera3.h>
> >>> +
> >>> +#include <libcamera/camera.h>
> >>>  #include <libcamera/geometry.h>
> >>>  #include <libcamera/pixel_format.h>
> >>>
> >>>  #include "jpeg/encoder.h"
> >>>
> >>> +class CameraDevice;
> >>> +
> >>>  class CameraStream
> >>>  {
> >>>  public:
> >>> @@ -99,9 +104,12 @@ public:
> >>>  		Internal,
> >>>  		Mapped,
> >>>  	};
> >>> -	CameraStream(libcamera::PixelFormat format, libcamera::Size size,
> >>> +	CameraStream(CameraDevice *cameraDev,
> >>
> >> I would have just used 'dev', or 'device', I don't think it conflicts
> >> with anything else in this function does it ?
> >>
> >> or as the class private member it goes into is cameraDevice_, it could
> >> have been cameraDevice ;-) - But it's not important either.
> > 
> > I thought about shortening the name too, but as it goes to
> > cameraDevice_ I kept it long.
> > 
> > And as Laurent said, if we could unify the naming schemes (ie.
> > everything android prefixed with camera3 and everything libcamera
> > prefixed with.... ) it would be nice

We could also use hal as a prefix for the android side to keep it
shorter.

> > 
> >> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > 
> > Thanks
> >
> >>> +		     camera3_stream_t *androidStream,
> >>> +		     const libcamera::StreamConfiguration &cfg,
> >>>  		     Type type, unsigned int index);
> >>>
> >>> +	int outputFormat() const { return androidStream_->format; }
> >>>  	const libcamera::PixelFormat &format() const { return format_; }
> >>>  	const libcamera::Size &size() const { return size_; }
> >>>  	Type type() const { return type_; }
> >>> @@ -111,9 +119,15 @@ public:
> >>>  	int configure(const libcamera::StreamConfiguration &cfg);
> >>>
> >>>  private:
> >>> +	CameraDevice *cameraDevice_;
> >>> +	libcamera::CameraConfiguration *config_;
> >>> +	camera3_stream_t *androidStream_;
> >>> +	Type type_;
> >>> +
> >>> +	/* Libcamera facing format and sizes. */
> >>>  	libcamera::PixelFormat format_;
> >>>  	libcamera::Size size_;
> >>> -	Type type_;
> >>> +
> >>>  	/*
> >>>  	 * The index of the libcamera StreamConfiguration as added during
> >>>  	 * configureStreams(). A single libcamera Stream may be used to deliver

Hi Laurent,

On 07/10/2020 01:57, Laurent Pinchart wrote:
> Hello,

<snip>:

>>
>>>> I haven't gone to check yet, but I wonder if there is a better boolean
>>>> logic to check ...
>>>>
>>>> 		if (format != NV12) perhaps ?
>>>
>>> We can't bet to have only NV12 and MJPEG
>>
>> No, indeed - my premise was that we will (later) need to make the
>> decision somewhere about what a software stream is.
>>
>> I.e. - if the Android stream is an NV12, and we can only generate a
>> YUYV, we will need a software(or opengl :D) format convertor (to support
>> UVC).
>>
>> So there is going to be a more complex requirement here to decide what a
>> software stream is, which will really be
> 
> Should we start calling them post-processed streams (we can bikeshed the
> name), as the post-processing may be handled by hardware (hardware
> scaler or format converter, JPEG encoder, GPU, ...) ?

Yes, indeed - we shouldn't be calling them software streams at all.


> 
>>    "input format != output format"...
>>
>> Anyway, this is fine for now.
>>
>> The internal buffer handling will in fact make it much easier to look at
>> software format conversion anyway.

<snip>

>>>>> -	CameraStream(libcamera::PixelFormat format, libcamera::Size size,
>>>>> +	CameraStream(CameraDevice *cameraDev,
>>>>
>>>> I would have just used 'dev', or 'device', I don't think it conflicts
>>>> with anything else in this function does it ?
>>>>
>>>> or as the class private member it goes into is cameraDevice_, it could
>>>> have been cameraDevice ;-) - But it's not important either.
>>>
>>> I thought about shortening the name too, but as it goes to
>>> cameraDevice_ I kept it long.
>>>
>>> And as Laurent said, if we could unify the naming schemes (ie.
>>> everything android prefixed with camera3 and everything libcamera
>>> prefixed with.... ) it would be nice
> 
> We could also use hal as a prefix for the android side to keep it
> shorter.

And keep libcamera objects without a prefix? Or lc ?

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

<snip>

Hi Kieran,

On Wed, Oct 07, 2020 at 09:47:33AM +0100, Kieran Bingham wrote:
> On 07/10/2020 01:57, Laurent Pinchart wrote:
> > Hello,
> 
> <snip>:
> 
> >>>> I haven't gone to check yet, but I wonder if there is a better boolean
> >>>> logic to check ...
> >>>>
> >>>> 		if (format != NV12) perhaps ?
> >>>
> >>> We can't bet to have only NV12 and MJPEG
> >>
> >> No, indeed - my premise was that we will (later) need to make the
> >> decision somewhere about what a software stream is.
> >>
> >> I.e. - if the Android stream is an NV12, and we can only generate a
> >> YUYV, we will need a software(or opengl :D) format convertor (to support
> >> UVC).
> >>
> >> So there is going to be a more complex requirement here to decide what a
> >> software stream is, which will really be
> > 
> > Should we start calling them post-processed streams (we can bikeshed the
> > name), as the post-processing may be handled by hardware (hardware
> > scaler or format converter, JPEG encoder, GPU, ...) ?
> 
> Yes, indeed - we shouldn't be calling them software streams at all.
> 
> >>    "input format != output format"...
> >>
> >> Anyway, this is fine for now.
> >>
> >> The internal buffer handling will in fact make it much easier to look at
> >> software format conversion anyway.
> 
> <snip>
> 
> >>>>> -	CameraStream(libcamera::PixelFormat format, libcamera::Size size,
> >>>>> +	CameraStream(CameraDevice *cameraDev,
> >>>>
> >>>> I would have just used 'dev', or 'device', I don't think it conflicts
> >>>> with anything else in this function does it ?
> >>>>
> >>>> or as the class private member it goes into is cameraDevice_, it could
> >>>> have been cameraDevice ;-) - But it's not important either.
> >>>
> >>> I thought about shortening the name too, but as it goes to
> >>> cameraDevice_ I kept it long.
> >>>
> >>> And as Laurent said, if we could unify the naming schemes (ie.
> >>> everything android prefixed with camera3 and everything libcamera
> >>> prefixed with.... ) it would be nice
> > 
> > We could also use hal as a prefix for the android side to keep it
> > shorter.
> 
> And keep libcamera objects without a prefix? Or lc ?

I'd go for no prefix, lc sounds really weird (but maybe I'd get use to
it).

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