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

[Kieran Bingham]

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 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.
>>
> 
> 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 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 :-)
> 
> 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 at 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

-- 
Regards
--
Kieran