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

[Kieran Bingham]

Hi Jacopo,

On 05/10/2020 11:46, Laurent Pinchart wrote:
> From: Jacopo Mondi <jacopo at 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 at 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 at 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
> 

-- 
Regards
--
Kieran