[libcamera-devel] [PATCH v2 6/9] android: camera_device: Maintain a vector of CameraStream

[Laurent Pinchart]

Hi Kieran,

Thank you for the patch.

On Fri, Jul 03, 2020 at 01:31:09AM +0200, Niklas Söderlund wrote:
> On 2020-07-02 22:36:51 +0100, Kieran Bingham wrote:
> > Introduce a vector storing a CameraStream to track and maintain
> > state between an Android stream (camera3_stream_t) and a libcamera
> > Stream.
> > 
> > Only the index of the libcamera stream is stored, to facilitate identifying
> > the correct index for both the StreamConfiguration and Stream vectors.
> > 
> > Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> > ---
> >  src/android/camera_device.cpp | 18 ++++++++++++++++--
> >  src/android/camera_device.h   |  6 ++++++
> >  2 files changed, 22 insertions(+), 2 deletions(-)
> > 
> > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> > index 77083219d8a1..fc3962dac230 100644
> > --- a/src/android/camera_device.cpp
> > +++ b/src/android/camera_device.cpp
> > @@ -952,6 +952,14 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
> >  		return -EINVAL;
> >  	}
> >  
> > +	streams_.reserve(stream_list->num_streams);
> 
> Should you not also empty the vector before reserving memory? I'm 
> thinking of if something below fails and exits we could be stuch with a 
> streams_ that is half of B and half of A. Or maybe streams_ should be 
> emptied on error instead?

reserve() will only reserve memory to avoid a memory allocation for
every entry added to the vector, entries are still populated when adding
them. Said different, reserve() doesn't change size().

> > +
> > +	/*
> > +	 * Track actually created streams, as there may not be a 1:1 mapping of
> > +	 * camera3 streams to libcamera streams.
> > +	 */
> > +	unsigned int streamIndex = 0;
> > +
> >  	for (unsigned int i = 0; i < stream_list->num_streams; ++i) {
> >  		camera3_stream_t *stream = stream_list->streams[i];
> >  
> > @@ -967,6 +975,9 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
> >  		if (!format.isValid())
> >  			return -EINVAL;
> >  
> > +		/* Maintain internal state of all stream mappings. */
> > +		streams_[i].androidStream = stream;
> > +
> >  		StreamConfiguration streamConfiguration;
> >  
> >  		streamConfiguration.size.width = stream->width;
> > @@ -974,6 +985,7 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
> >  		streamConfiguration.pixelFormat = format;
> >  
> >  		config_->addConfiguration(streamConfiguration);
> > +		streams_[i].libcameraIndex = streamIndex++;
> >  	}
> >  
> >  	switch (config_->validate()) {
> > @@ -991,10 +1003,12 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
> >  
> >  	for (unsigned int i = 0; i < stream_list->num_streams; ++i) {
> >  		camera3_stream_t *stream = stream_list->streams[i];
> > -		StreamConfiguration &streamConfiguration = config_->at(i);
> > +		CameraStream *cameraStream = &streams_[i];
> > +
> 
> nit: I would drop the extra newline, with or without fixed,
> 
> > +		StreamConfiguration &cfg = config_->at(cameraStream->libcameraIndex);
> >  
> >  		/* Use the bufferCount confirmed by the validation process. */
> > -		stream->max_buffers = streamConfiguration.bufferCount;
> > +		stream->max_buffers = cfg.bufferCount;
> >  	}
> >  
> >  	/*
> > diff --git a/src/android/camera_device.h b/src/android/camera_device.h
> > index 5bd6cf416156..275760f0aa26 100644
> > --- a/src/android/camera_device.h
> > +++ b/src/android/camera_device.h
> > @@ -25,6 +25,11 @@
> >  
> >  class CameraMetadata;
> >  
> > +struct CameraStream {
> > +	camera3_stream *androidStream;

I'd name this halStream, the rationale being that prefixing names with
hal instead of android will keep them shorter.

> > +	unsigned int libcameraIndex;

And I'd simply use index here (no prefix for the libcamera side), but I
won't insist too much. The rationale is that otherwise I fear a
libcamera prefix will spread in many places. A short comment along the
lines of "The index of the stream in the CameraConfiguration" could also
possibly be useful.

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

> > +};
> > +
> >  class CameraDevice : protected libcamera::Loggable
> >  {
> >  public:
> > @@ -89,6 +94,7 @@ private:
> >  
> >  	std::vector<Camera3StreamConfiguration> streamConfigurations_;
> >  	std::map<int, libcamera::PixelFormat> formatsMap_;
> > +	std::vector<CameraStream> streams_;
> >  
> >  	int facing_;
> >  	int orientation_;

-- 
Regards,

Laurent Pinchart