[libcamera-devel,v3,09/11] android: camera_device: Rework CameraStream handling

9540 diff mbox series

The CameraDevice::streams_ vector of CameraStream instances is
currently mostly accessed by index. The current implementation
creates all the CameraStream during the first loop that inspects the
camera3_stream instances, and the update the index of the
StreamConfiguration associated with the CameraStream during a second
loop that inspects MJPEG streams. A third loop creates the JPEG encoder
associated with CameraStreams that produce MJPEG format.

As the index-based association is hard to follow and rather fragile,
rework the creation and handling of CameraStream:

1) Make the StreamConfiguration index a constructor parameter and a
   private struct member.  This disallow the creation of CameraStream
   without a StreamConfiguration index assigned.

2) Create CameraStream only after the associated StreamConfiguration
   has been identified. The first loop creates CameraStream for non-JPEG
   streams, the second for the JPEG ones after having identified the
   associated StreamConfiguration. Since we have just created the
   CameraStream, create the JPEG encoder at the same time instead of
   deferring it.

This change removes all accesses by index to the CameraDevice::streams_
vector.

No functional changes intended, but this change aims to make the code
easier to follow and more robust.

Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
---
 src/android/camera_device.cpp | 91 ++++++++++++++++++-----------------
 src/android/camera_device.h   | 18 ++++---
 2 files changed, 57 insertions(+), 52 deletions(-)

Hi Jacopo,

Thanks for your work.

On 2020-09-08 15:41:40 +0200, Jacopo Mondi wrote:
> The CameraDevice::streams_ vector of CameraStream instances is
> currently mostly accessed by index. The current implementation
> creates all the CameraStream during the first loop that inspects the
> camera3_stream instances, and the update the index of the
> StreamConfiguration associated with the CameraStream during a second
> loop that inspects MJPEG streams. A third loop creates the JPEG encoder
> associated with CameraStreams that produce MJPEG format.
> 
> As the index-based association is hard to follow and rather fragile,
> rework the creation and handling of CameraStream:
> 
> 1) Make the StreamConfiguration index a constructor parameter and a
>    private struct member.  This disallow the creation of CameraStream
>    without a StreamConfiguration index assigned.
> 
> 2) Create CameraStream only after the associated StreamConfiguration
>    has been identified. The first loop creates CameraStream for non-JPEG
>    streams, the second for the JPEG ones after having identified the
>    associated StreamConfiguration. Since we have just created the
>    CameraStream, create the JPEG encoder at the same time instead of
>    deferring it.
> 
> This change removes all accesses by index to the CameraDevice::streams_
> vector.
> 
> No functional changes intended, but this change aims to make the code
> easier to follow and more robust.
> 
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
>  src/android/camera_device.cpp | 91 ++++++++++++++++++-----------------
>  src/android/camera_device.h   | 18 ++++---
>  2 files changed, 57 insertions(+), 52 deletions(-)
> 
> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> index eab01d808917..e0260c92246c 100644
> --- a/src/android/camera_device.cpp
> +++ b/src/android/camera_device.cpp
> @@ -168,8 +168,8 @@ MappedCamera3Buffer::MappedCamera3Buffer(const buffer_handle_t camera3buffer,
>  	}
>  }
>  
> -CameraStream::CameraStream(PixelFormat f, Size s)
> -	: index(-1), format(f), size(s), jpeg(nullptr)
> +CameraStream::CameraStream(PixelFormat f, Size s, unsigned int i)
> +	: format(f), size(s), jpeg(nullptr), index_(i)
>  {
>  }
>  
> @@ -1190,6 +1190,7 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
>  	streams_.reserve(stream_list->num_streams);
>  
>  	/* First handle all non-MJPEG streams. */
> +	camera3_stream_t *jpegStream = nullptr;
>  	for (unsigned int i = 0; i < stream_list->num_streams; ++i) {
>  		camera3_stream_t *stream = stream_list->streams[i];
>  		Size size(stream->width, stream->height);
> @@ -1206,52 +1207,50 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
>  		if (!format.isValid())
>  			return -EINVAL;
>  
> -		streams_.emplace_back(format, size);
> -		stream->priv = static_cast<void *>(&streams_[i]);
> -
>  		/* Defer handling of MJPEG streams until all others are known. */
> -		if (stream->format == HAL_PIXEL_FORMAT_BLOB)
> +		if (stream->format == HAL_PIXEL_FORMAT_BLOB) {
> +			jpegStream = stream;
>  			continue;
> +		}
>  
>  		StreamConfiguration streamConfiguration;
> -
>  		streamConfiguration.size = size;
>  		streamConfiguration.pixelFormat = format;
>  
>  		config_->addConfiguration(streamConfiguration);
> -		streams_[i].index = config_->size() - 1;
> +		unsigned int index = config_->size() - 1;
> +		streams_.emplace_back(format, size, index);
> +		stream->priv = static_cast<void *>(&streams_.back());

Is this not dangerous? If the stremas_ vector is modified in any way the 
members may be reallocated and the pointer becomes invalid. I think this 
is what boost::container::stable_vector tries to solve for example.

>  	}
>  
>  	/* Now handle MJPEG streams, adding a new stream if required. */
> -	for (unsigned int i = 0; i < stream_list->num_streams; ++i) {
> -		camera3_stream_t *stream = stream_list->streams[i];
> -		bool match = false;
> -
> -		if (stream->format != HAL_PIXEL_FORMAT_BLOB)
> -			continue;
> +	if (jpegStream) {
> +		int index = -1;
>  
> -		/* Search for a compatible stream */
> -		for (unsigned int j = 0; j < config_->size(); j++) {
> -			StreamConfiguration &cfg = config_->at(j);
> +		/* Search for a compatible stream in the non-JPEG ones. */
> +		for (unsigned int i = 0; i < config_->size(); i++) {
> +			StreamConfiguration &cfg = config_->at(i);
>  
>  			/*
>  			 * \todo The PixelFormat must also be compatible with
>  			 * the encoder.
>  			 */
> -			if (cfg.size == streams_[i].size) {
> -				LOG(HAL, Info) << "Stream " << i
> -					       << " using libcamera stream " << j;
> +			if (cfg.size.width != jpegStream->width ||
> +			    cfg.size.height != jpegStream->height)
> +				continue;
>  
> -				match = true;
> -				streams_[i].index = j;
> -			}
> +			LOG(HAL, Info)
> +				<< "Android JPEG stream mapped on stream " << i;
> +
> +			index = i;
> +			break;
>  		}
>  
>  		/*
>  		 * Without a compatible match for JPEG encoding we must
>  		 * introduce a new stream to satisfy the request requirements.
>  		 */
> -		if (!match) {
> +		if (index < 0) {
>  			StreamConfiguration streamConfiguration;
>  
>  			/*
> @@ -1260,15 +1259,31 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
>  			 * handled, and should be considered as part of any
>  			 * stream configuration reworks.
>  			 */
> -			streamConfiguration.size.width = stream->width;
> -			streamConfiguration.size.height = stream->height;
> +			streamConfiguration.size.width = jpegStream->width;
> +			streamConfiguration.size.height = jpegStream->height;
>  			streamConfiguration.pixelFormat = formats::NV12;
>  
>  			LOG(HAL, Info) << "Adding " << streamConfiguration.toString()
>  				       << " for MJPEG support";
>  
>  			config_->addConfiguration(streamConfiguration);
> -			streams_[i].index = config_->size() - 1;
> +			index = config_->size() - 1;
> +		}
> +
> +		StreamConfiguration &cfg = config_->at(index);
> +		CameraStream &cameraStream =
> +			streams_.emplace_back(formats::MJPEG, cfg.size, index);
> +		jpegStream->priv = static_cast<void *>(&cameraStream);
> +
> +		/*
> +		 * Construct a software encoder for MJPEG streams from the
> +		 * chosen libcamera source stream.
> +		 */
> +		cameraStream.jpeg = new EncoderLibJpeg();
> +		int ret = cameraStream.jpeg->configure(cfg);
> +		if (ret) {
> +			LOG(HAL, Error) << "Failed to configure encoder";
> +			return ret;
>  		}
>  	}
>  
> @@ -1291,25 +1306,11 @@ 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];
> -		CameraStream *cameraStream = &streams_[i];
> -		StreamConfiguration &cfg = config_->at(cameraStream->index);
> +		CameraStream *cameraStream = static_cast<CameraStream *>(stream->priv);
> +		StreamConfiguration &cfg = config_->at(cameraStream->index());
>  
>  		/* Use the bufferCount confirmed by the validation process. */
>  		stream->max_buffers = cfg.bufferCount;
> -
> -		/*
> -		 * Construct a software encoder for MJPEG streams from the
> -		 * chosen libcamera source stream.
> -		 */
> -		if (cameraStream->format == formats::MJPEG) {
> -			cameraStream->jpeg = new EncoderLibJpeg();
> -			int ret = cameraStream->jpeg->configure(cfg);
> -			if (ret) {
> -				LOG(HAL, Error)
> -					<< "Failed to configure encoder";
> -				return ret;
> -			}
> -		}
>  	}
>  
>  	/*
> @@ -1423,7 +1424,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
>  		}
>  		descriptor->frameBuffers.emplace_back(buffer);
>  
> -		StreamConfiguration *streamConfiguration = &config_->at(cameraStream->index);
> +		StreamConfiguration *streamConfiguration = &config_->at(cameraStream->index());
>  		Stream *stream = streamConfiguration->stream();
>  
>  		request->addBuffer(stream, buffer);
> @@ -1478,7 +1479,7 @@ void CameraDevice::requestComplete(Request *request)
>  			continue;
>  		}
>  
> -		StreamConfiguration *streamConfiguration = &config_->at(cameraStream->index);
> +		StreamConfiguration *streamConfiguration = &config_->at(cameraStream->index());
>  		Stream *stream = streamConfiguration->stream();
>  		FrameBuffer *buffer = request->findBuffer(stream);
>  		if (!buffer) {
> diff --git a/src/android/camera_device.h b/src/android/camera_device.h
> index dc0ee664d443..f8f237203ce9 100644
> --- a/src/android/camera_device.h
> +++ b/src/android/camera_device.h
> @@ -28,20 +28,24 @@
>  class CameraMetadata;
>  
>  struct CameraStream {
> -	CameraStream(libcamera::PixelFormat, libcamera::Size);
> +public:
> +	CameraStream(libcamera::PixelFormat, libcamera::Size, unsigned int i);
>  	~CameraStream();
>  
> -	/*
> -	 * The index of the libcamera StreamConfiguration as added during
> -	 * configureStreams(). A single libcamera Stream may be used to deliver
> -	 * one or more streams to the Android framework.
> -	 */
> -	unsigned int index;
> +	unsigned int index() const { return index_; }
>  
>  	libcamera::PixelFormat format;
>  	libcamera::Size size;
>  
>  	Encoder *jpeg;
> +
> +private:
> +	/*
> +	 * The index of the libcamera StreamConfiguration as added during
> +	 * configureStreams(). A single libcamera Stream may be used to deliver
> +	 * one or more streams to the Android framework.
> +	 */
> +	unsigned int index_;
>  };
>  
>  class CameraDevice : protected libcamera::Loggable
> -- 
> 2.28.0
> 
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

Hi Niklas,

On Thu, Sep 10, 2020 at 01:00:06PM +0200, Niklas Söderlund wrote:
> Hi Jacopo,
>
> Thanks for your work.
>
> On 2020-09-08 15:41:40 +0200, Jacopo Mondi wrote:
> > The CameraDevice::streams_ vector of CameraStream instances is
> > currently mostly accessed by index. The current implementation
> > creates all the CameraStream during the first loop that inspects the
> > camera3_stream instances, and the update the index of the
> > StreamConfiguration associated with the CameraStream during a second
> > loop that inspects MJPEG streams. A third loop creates the JPEG encoder
> > associated with CameraStreams that produce MJPEG format.
> >
> > As the index-based association is hard to follow and rather fragile,
> > rework the creation and handling of CameraStream:
> >
> > 1) Make the StreamConfiguration index a constructor parameter and a
> >    private struct member.  This disallow the creation of CameraStream
> >    without a StreamConfiguration index assigned.
> >
> > 2) Create CameraStream only after the associated StreamConfiguration
> >    has been identified. The first loop creates CameraStream for non-JPEG
> >    streams, the second for the JPEG ones after having identified the
> >    associated StreamConfiguration. Since we have just created the
> >    CameraStream, create the JPEG encoder at the same time instead of
> >    deferring it.
> >
> > This change removes all accesses by index to the CameraDevice::streams_
> > vector.
> >
> > No functional changes intended, but this change aims to make the code
> > easier to follow and more robust.
> >
> > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > ---
> >  src/android/camera_device.cpp | 91 ++++++++++++++++++-----------------
> >  src/android/camera_device.h   | 18 ++++---
> >  2 files changed, 57 insertions(+), 52 deletions(-)
> >
> > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> > index eab01d808917..e0260c92246c 100644
> > --- a/src/android/camera_device.cpp
> > +++ b/src/android/camera_device.cpp
> > @@ -168,8 +168,8 @@ MappedCamera3Buffer::MappedCamera3Buffer(const buffer_handle_t camera3buffer,
> >  	}
> >  }
> >
> > -CameraStream::CameraStream(PixelFormat f, Size s)
> > -	: index(-1), format(f), size(s), jpeg(nullptr)
> > +CameraStream::CameraStream(PixelFormat f, Size s, unsigned int i)
> > +	: format(f), size(s), jpeg(nullptr), index_(i)
> >  {
> >  }
> >
> > @@ -1190,6 +1190,7 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
> >  	streams_.reserve(stream_list->num_streams);
> >
> >  	/* First handle all non-MJPEG streams. */
> > +	camera3_stream_t *jpegStream = nullptr;
> >  	for (unsigned int i = 0; i < stream_list->num_streams; ++i) {
> >  		camera3_stream_t *stream = stream_list->streams[i];
> >  		Size size(stream->width, stream->height);
> > @@ -1206,52 +1207,50 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
> >  		if (!format.isValid())
> >  			return -EINVAL;
> >
> > -		streams_.emplace_back(format, size);
> > -		stream->priv = static_cast<void *>(&streams_[i]);
> > -
> >  		/* Defer handling of MJPEG streams until all others are known. */
> > -		if (stream->format == HAL_PIXEL_FORMAT_BLOB)
> > +		if (stream->format == HAL_PIXEL_FORMAT_BLOB) {
> > +			jpegStream = stream;
> >  			continue;
> > +		}
> >
> >  		StreamConfiguration streamConfiguration;
> > -
> >  		streamConfiguration.size = size;
> >  		streamConfiguration.pixelFormat = format;
> >
> >  		config_->addConfiguration(streamConfiguration);
> > -		streams_[i].index = config_->size() - 1;
> > +		unsigned int index = config_->size() - 1;
> > +		streams_.emplace_back(format, size, index);
> > +		stream->priv = static_cast<void *>(&streams_.back());
>
> Is this not dangerous? If the stremas_ vector is modified in any way the
> members may be reallocated and the pointer becomes invalid. I think this
> is what boost::container::stable_vector tries to solve for example.

We reserve space for all possible entries in advance to avoid
relocations.

>
> >  	}
> >
> >  	/* Now handle MJPEG streams, adding a new stream if required. */
> > -	for (unsigned int i = 0; i < stream_list->num_streams; ++i) {
> > -		camera3_stream_t *stream = stream_list->streams[i];
> > -		bool match = false;
> > -
> > -		if (stream->format != HAL_PIXEL_FORMAT_BLOB)
> > -			continue;
> > +	if (jpegStream) {
> > +		int index = -1;
> >
> > -		/* Search for a compatible stream */
> > -		for (unsigned int j = 0; j < config_->size(); j++) {
> > -			StreamConfiguration &cfg = config_->at(j);
> > +		/* Search for a compatible stream in the non-JPEG ones. */
> > +		for (unsigned int i = 0; i < config_->size(); i++) {
> > +			StreamConfiguration &cfg = config_->at(i);
> >
> >  			/*
> >  			 * \todo The PixelFormat must also be compatible with
> >  			 * the encoder.
> >  			 */
> > -			if (cfg.size == streams_[i].size) {
> > -				LOG(HAL, Info) << "Stream " << i
> > -					       << " using libcamera stream " << j;
> > +			if (cfg.size.width != jpegStream->width ||
> > +			    cfg.size.height != jpegStream->height)
> > +				continue;
> >
> > -				match = true;
> > -				streams_[i].index = j;
> > -			}
> > +			LOG(HAL, Info)
> > +				<< "Android JPEG stream mapped on stream " << i;
> > +
> > +			index = i;
> > +			break;
> >  		}
> >
> >  		/*
> >  		 * Without a compatible match for JPEG encoding we must
> >  		 * introduce a new stream to satisfy the request requirements.
> >  		 */
> > -		if (!match) {
> > +		if (index < 0) {
> >  			StreamConfiguration streamConfiguration;
> >
> >  			/*
> > @@ -1260,15 +1259,31 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
> >  			 * handled, and should be considered as part of any
> >  			 * stream configuration reworks.
> >  			 */
> > -			streamConfiguration.size.width = stream->width;
> > -			streamConfiguration.size.height = stream->height;
> > +			streamConfiguration.size.width = jpegStream->width;
> > +			streamConfiguration.size.height = jpegStream->height;
> >  			streamConfiguration.pixelFormat = formats::NV12;
> >
> >  			LOG(HAL, Info) << "Adding " << streamConfiguration.toString()
> >  				       << " for MJPEG support";
> >
> >  			config_->addConfiguration(streamConfiguration);
> > -			streams_[i].index = config_->size() - 1;
> > +			index = config_->size() - 1;
> > +		}
> > +
> > +		StreamConfiguration &cfg = config_->at(index);
> > +		CameraStream &cameraStream =
> > +			streams_.emplace_back(formats::MJPEG, cfg.size, index);
> > +		jpegStream->priv = static_cast<void *>(&cameraStream);
> > +
> > +		/*
> > +		 * Construct a software encoder for MJPEG streams from the
> > +		 * chosen libcamera source stream.
> > +		 */
> > +		cameraStream.jpeg = new EncoderLibJpeg();
> > +		int ret = cameraStream.jpeg->configure(cfg);
> > +		if (ret) {
> > +			LOG(HAL, Error) << "Failed to configure encoder";
> > +			return ret;
> >  		}
> >  	}
> >
> > @@ -1291,25 +1306,11 @@ 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];
> > -		CameraStream *cameraStream = &streams_[i];
> > -		StreamConfiguration &cfg = config_->at(cameraStream->index);
> > +		CameraStream *cameraStream = static_cast<CameraStream *>(stream->priv);
> > +		StreamConfiguration &cfg = config_->at(cameraStream->index());
> >
> >  		/* Use the bufferCount confirmed by the validation process. */
> >  		stream->max_buffers = cfg.bufferCount;
> > -
> > -		/*
> > -		 * Construct a software encoder for MJPEG streams from the
> > -		 * chosen libcamera source stream.
> > -		 */
> > -		if (cameraStream->format == formats::MJPEG) {
> > -			cameraStream->jpeg = new EncoderLibJpeg();
> > -			int ret = cameraStream->jpeg->configure(cfg);
> > -			if (ret) {
> > -				LOG(HAL, Error)
> > -					<< "Failed to configure encoder";
> > -				return ret;
> > -			}
> > -		}
> >  	}
> >
> >  	/*
> > @@ -1423,7 +1424,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
> >  		}
> >  		descriptor->frameBuffers.emplace_back(buffer);
> >
> > -		StreamConfiguration *streamConfiguration = &config_->at(cameraStream->index);
> > +		StreamConfiguration *streamConfiguration = &config_->at(cameraStream->index());
> >  		Stream *stream = streamConfiguration->stream();
> >
> >  		request->addBuffer(stream, buffer);
> > @@ -1478,7 +1479,7 @@ void CameraDevice::requestComplete(Request *request)
> >  			continue;
> >  		}
> >
> > -		StreamConfiguration *streamConfiguration = &config_->at(cameraStream->index);
> > +		StreamConfiguration *streamConfiguration = &config_->at(cameraStream->index());
> >  		Stream *stream = streamConfiguration->stream();
> >  		FrameBuffer *buffer = request->findBuffer(stream);
> >  		if (!buffer) {
> > diff --git a/src/android/camera_device.h b/src/android/camera_device.h
> > index dc0ee664d443..f8f237203ce9 100644
> > --- a/src/android/camera_device.h
> > +++ b/src/android/camera_device.h
> > @@ -28,20 +28,24 @@
> >  class CameraMetadata;
> >
> >  struct CameraStream {
> > -	CameraStream(libcamera::PixelFormat, libcamera::Size);
> > +public:
> > +	CameraStream(libcamera::PixelFormat, libcamera::Size, unsigned int i);
> >  	~CameraStream();
> >
> > -	/*
> > -	 * The index of the libcamera StreamConfiguration as added during
> > -	 * configureStreams(). A single libcamera Stream may be used to deliver
> > -	 * one or more streams to the Android framework.
> > -	 */
> > -	unsigned int index;
> > +	unsigned int index() const { return index_; }
> >
> >  	libcamera::PixelFormat format;
> >  	libcamera::Size size;
> >
> >  	Encoder *jpeg;
> > +
> > +private:
> > +	/*
> > +	 * The index of the libcamera StreamConfiguration as added during
> > +	 * configureStreams(). A single libcamera Stream may be used to deliver
> > +	 * one or more streams to the Android framework.
> > +	 */
> > +	unsigned int index_;
> >  };
> >
> >  class CameraDevice : protected libcamera::Loggable
> > --
> > 2.28.0
> >
> > _______________________________________________
> > libcamera-devel mailing list
> > libcamera-devel@lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel
>
> --
> Regards,
> Niklas Söderlund

Hi Jacopo, Thanks for the patch.

On Thu, Sep 10, 2020 at 8:11 PM Jacopo Mondi <jacopo@jmondi.org> wrote:
>
> Hi Niklas,
>
> On Thu, Sep 10, 2020 at 01:00:06PM +0200, Niklas Söderlund wrote:
> > Hi Jacopo,
> >
> > Thanks for your work.
> >
> > On 2020-09-08 15:41:40 +0200, Jacopo Mondi wrote:
> > > The CameraDevice::streams_ vector of CameraStream instances is
> > > currently mostly accessed by index. The current implementation
> > > creates all the CameraStream during the first loop that inspects the
> > > camera3_stream instances, and the update the index of the
> > > StreamConfiguration associated with the CameraStream during a second
> > > loop that inspects MJPEG streams. A third loop creates the JPEG encoder
> > > associated with CameraStreams that produce MJPEG format.
> > >
> > > As the index-based association is hard to follow and rather fragile,
> > > rework the creation and handling of CameraStream:
> > >
> > > 1) Make the StreamConfiguration index a constructor parameter and a
> > >    private struct member.  This disallow the creation of CameraStream
> > >    without a StreamConfiguration index assigned.
> > >
> > > 2) Create CameraStream only after the associated StreamConfiguration
> > >    has been identified. The first loop creates CameraStream for non-JPEG
> > >    streams, the second for the JPEG ones after having identified the
> > >    associated StreamConfiguration. Since we have just created the
> > >    CameraStream, create the JPEG encoder at the same time instead of
> > >    deferring it.
> > >
> > > This change removes all accesses by index to the CameraDevice::streams_
> > > vector.
> > >
> > > No functional changes intended, but this change aims to make the code
> > > easier to follow and more robust.
> > >
> > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>

Reviewed-by: Hirokazu Honda <hiroh@chromium.org>

> > > ---
> > >  src/android/camera_device.cpp | 91 ++++++++++++++++++-----------------
> > >  src/android/camera_device.h   | 18 ++++---
> > >  2 files changed, 57 insertions(+), 52 deletions(-)
> > >
> > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> > > index eab01d808917..e0260c92246c 100644
> > > --- a/src/android/camera_device.cpp
> > > +++ b/src/android/camera_device.cpp
> > > @@ -168,8 +168,8 @@ MappedCamera3Buffer::MappedCamera3Buffer(const buffer_handle_t camera3buffer,
> > >     }
> > >  }
> > >
> > > -CameraStream::CameraStream(PixelFormat f, Size s)
> > > -   : index(-1), format(f), size(s), jpeg(nullptr)
> > > +CameraStream::CameraStream(PixelFormat f, Size s, unsigned int i)
> > > +   : format(f), size(s), jpeg(nullptr), index_(i)
> > >  {
> > >  }
> > >
> > > @@ -1190,6 +1190,7 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
> > >     streams_.reserve(stream_list->num_streams);
> > >
> > >     /* First handle all non-MJPEG streams. */
> > > +   camera3_stream_t *jpegStream = nullptr;
> > >     for (unsigned int i = 0; i < stream_list->num_streams; ++i) {
> > >             camera3_stream_t *stream = stream_list->streams[i];
> > >             Size size(stream->width, stream->height);
> > > @@ -1206,52 +1207,50 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
> > >             if (!format.isValid())
> > >                     return -EINVAL;
> > >
> > > -           streams_.emplace_back(format, size);
> > > -           stream->priv = static_cast<void *>(&streams_[i]);
> > > -
> > >             /* Defer handling of MJPEG streams until all others are known. */
> > > -           if (stream->format == HAL_PIXEL_FORMAT_BLOB)
> > > +           if (stream->format == HAL_PIXEL_FORMAT_BLOB) {
> > > +                   jpegStream = stream;
> > >                     continue;
> > > +           }
> > >
> > >             StreamConfiguration streamConfiguration;
> > > -
> > >             streamConfiguration.size = size;
> > >             streamConfiguration.pixelFormat = format;
> > >
> > >             config_->addConfiguration(streamConfiguration);
> > > -           streams_[i].index = config_->size() - 1;
> > > +           unsigned int index = config_->size() - 1;
> > > +           streams_.emplace_back(format, size, index);
> > > +           stream->priv = static_cast<void *>(&streams_.back());
> >
> > Is this not dangerous? If the stremas_ vector is modified in any way the
> > members may be reallocated and the pointer becomes invalid. I think this
> > is what boost::container::stable_vector tries to solve for example.
>
> We reserve space for all possible entries in advance to avoid
> relocations.
>
> >
> > >     }
> > >
> > >     /* Now handle MJPEG streams, adding a new stream if required. */
> > > -   for (unsigned int i = 0; i < stream_list->num_streams; ++i) {
> > > -           camera3_stream_t *stream = stream_list->streams[i];
> > > -           bool match = false;
> > > -
> > > -           if (stream->format != HAL_PIXEL_FORMAT_BLOB)
> > > -                   continue;
> > > +   if (jpegStream) {
> > > +           int index = -1;
> > >
> > > -           /* Search for a compatible stream */
> > > -           for (unsigned int j = 0; j < config_->size(); j++) {
> > > -                   StreamConfiguration &cfg = config_->at(j);
> > > +           /* Search for a compatible stream in the non-JPEG ones. */
> > > +           for (unsigned int i = 0; i < config_->size(); i++) {
> > > +                   StreamConfiguration &cfg = config_->at(i);
> > >
> > >                     /*
> > >                      * \todo The PixelFormat must also be compatible with
> > >                      * the encoder.
> > >                      */
> > > -                   if (cfg.size == streams_[i].size) {
> > > -                           LOG(HAL, Info) << "Stream " << i
> > > -                                          << " using libcamera stream " << j;
> > > +                   if (cfg.size.width != jpegStream->width ||
> > > +                       cfg.size.height != jpegStream->height)
> > > +                           continue;
> > >
> > > -                           match = true;
> > > -                           streams_[i].index = j;
> > > -                   }
> > > +                   LOG(HAL, Info)
> > > +                           << "Android JPEG stream mapped on stream " << i;
> > > +
> > > +                   index = i;
> > > +                   break;
> > >             }
> > >
> > >             /*
> > >              * Without a compatible match for JPEG encoding we must
> > >              * introduce a new stream to satisfy the request requirements.
> > >              */
> > > -           if (!match) {
> > > +           if (index < 0) {
> > >                     StreamConfiguration streamConfiguration;
> > >
> > >                     /*
> > > @@ -1260,15 +1259,31 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
> > >                      * handled, and should be considered as part of any
> > >                      * stream configuration reworks.
> > >                      */
> > > -                   streamConfiguration.size.width = stream->width;
> > > -                   streamConfiguration.size.height = stream->height;
> > > +                   streamConfiguration.size.width = jpegStream->width;
> > > +                   streamConfiguration.size.height = jpegStream->height;
> > >                     streamConfiguration.pixelFormat = formats::NV12;
> > >
> > >                     LOG(HAL, Info) << "Adding " << streamConfiguration.toString()
> > >                                    << " for MJPEG support";
> > >
> > >                     config_->addConfiguration(streamConfiguration);
> > > -                   streams_[i].index = config_->size() - 1;
> > > +                   index = config_->size() - 1;
> > > +           }
> > > +
> > > +           StreamConfiguration &cfg = config_->at(index);
> > > +           CameraStream &cameraStream =
> > > +                   streams_.emplace_back(formats::MJPEG, cfg.size, index);
> > > +           jpegStream->priv = static_cast<void *>(&cameraStream);
> > > +
> > > +           /*
> > > +            * Construct a software encoder for MJPEG streams from the
> > > +            * chosen libcamera source stream.
> > > +            */
> > > +           cameraStream.jpeg = new EncoderLibJpeg();
> > > +           int ret = cameraStream.jpeg->configure(cfg);
> > > +           if (ret) {
> > > +                   LOG(HAL, Error) << "Failed to configure encoder";
> > > +                   return ret;
> > >             }
> > >     }
> > >
> > > @@ -1291,25 +1306,11 @@ 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];
> > > -           CameraStream *cameraStream = &streams_[i];
> > > -           StreamConfiguration &cfg = config_->at(cameraStream->index);
> > > +           CameraStream *cameraStream = static_cast<CameraStream *>(stream->priv);
> > > +           StreamConfiguration &cfg = config_->at(cameraStream->index());
> > >
> > >             /* Use the bufferCount confirmed by the validation process. */
> > >             stream->max_buffers = cfg.bufferCount;
> > > -
> > > -           /*
> > > -            * Construct a software encoder for MJPEG streams from the
> > > -            * chosen libcamera source stream.
> > > -            */
> > > -           if (cameraStream->format == formats::MJPEG) {
> > > -                   cameraStream->jpeg = new EncoderLibJpeg();
> > > -                   int ret = cameraStream->jpeg->configure(cfg);
> > > -                   if (ret) {
> > > -                           LOG(HAL, Error)
> > > -                                   << "Failed to configure encoder";
> > > -                           return ret;
> > > -                   }
> > > -           }
> > >     }
> > >
> > >     /*
> > > @@ -1423,7 +1424,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
> > >             }
> > >             descriptor->frameBuffers.emplace_back(buffer);
> > >
> > > -           StreamConfiguration *streamConfiguration = &config_->at(cameraStream->index);
> > > +           StreamConfiguration *streamConfiguration = &config_->at(cameraStream->index());
> > >             Stream *stream = streamConfiguration->stream();
> > >
> > >             request->addBuffer(stream, buffer);
> > > @@ -1478,7 +1479,7 @@ void CameraDevice::requestComplete(Request *request)
> > >                     continue;
> > >             }
> > >
> > > -           StreamConfiguration *streamConfiguration = &config_->at(cameraStream->index);
> > > +           StreamConfiguration *streamConfiguration = &config_->at(cameraStream->index());
> > >             Stream *stream = streamConfiguration->stream();
> > >             FrameBuffer *buffer = request->findBuffer(stream);
> > >             if (!buffer) {
> > > diff --git a/src/android/camera_device.h b/src/android/camera_device.h
> > > index dc0ee664d443..f8f237203ce9 100644
> > > --- a/src/android/camera_device.h
> > > +++ b/src/android/camera_device.h
> > > @@ -28,20 +28,24 @@
> > >  class CameraMetadata;
> > >
> > >  struct CameraStream {
> > > -   CameraStream(libcamera::PixelFormat, libcamera::Size);
> > > +public:
> > > +   CameraStream(libcamera::PixelFormat, libcamera::Size, unsigned int i);
> > >     ~CameraStream();
> > >
> > > -   /*
> > > -    * The index of the libcamera StreamConfiguration as added during
> > > -    * configureStreams(). A single libcamera Stream may be used to deliver
> > > -    * one or more streams to the Android framework.
> > > -    */
> > > -   unsigned int index;
> > > +   unsigned int index() const { return index_; }
> > >
> > >     libcamera::PixelFormat format;
> > >     libcamera::Size size;
> > >
> > >     Encoder *jpeg;
> > > +
> > > +private:
> > > +   /*
> > > +    * The index of the libcamera StreamConfiguration as added during
> > > +    * configureStreams(). A single libcamera Stream may be used to deliver
> > > +    * one or more streams to the Android framework.
> > > +    */
> > > +   unsigned int index_;
> > >  };
> > >
> > >  class CameraDevice : protected libcamera::Loggable
> > > --
> > > 2.28.0
> > >
> > > _______________________________________________
> > > libcamera-devel mailing list
> > > libcamera-devel@lists.libcamera.org
> > > https://lists.libcamera.org/listinfo/libcamera-devel
> >
> > --
> > Regards,
> > Niklas Söderlund
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel