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

[Jacopo Mondi]

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.

>
>
> 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 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