[libcamera-devel,v2,10/12] android: camera_device: Rework CameraStream handling

9471 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 | 75 +++++++++++++++++++----------------
 src/android/camera_device.h   | 18 +++++----
 2 files changed, 51 insertions(+), 42 deletions(-)

Hi Jacopo,

Thank you for the patch.

On Wed, Sep 02, 2020 at 05:22:34PM +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 | 75 +++++++++++++++++++----------------
>  src/android/camera_device.h   | 18 +++++----
>  2 files changed, 51 insertions(+), 42 deletions(-)
> 
> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> index 3630e87e8814..9bcd1d993c17 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)
>  {
>  }
>  
> @@ -1212,30 +1212,28 @@ 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)
>  			continue;
>  
>  		StreamConfiguration streamConfiguration;
> -
>  		streamConfiguration.size = size;
>  		streamConfiguration.pixelFormat = format;
>  
> -		streams_[i].index = config_->addConfiguration(streamConfiguration);
> +		unsigned int index = config_->addConfiguration(streamConfiguration);
> +		CameraStream &cameraStream = streams_.emplace_back(format, size, index);
> +		stream->priv = static_cast<void *>(&cameraStream);

This is problematic. std::vector::emplace_back may cause a reallocation,
which invalidates all iterators, rendering the pointer invalid. One
option would be to turn streams_ into an std::list, another option to
fill the priv pointers in a separate loop after populating streams_.

>  	}
>  
>  	/* 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;

Drive-by comment for a possible future simplification, can't we rely on
the fact that only a single JPEG stream will be requested ? If so, we
could store a pointer to the JPEG camera3_stream_configuration_t in the
previous loop, and have a

	if (jpegStream) {
		...
	}

here instead of a loop.

>  
> -		/* Search for a compatible stream */
> +		/* Search for a compatible stream in the non-JPEG ones. */
> +		bool match = false;
> +		unsigned int index;
>  		for (unsigned int j = 0; j < config_->size(); j++) {
>  			StreamConfiguration &cfg = config_->at(j);
>  
> @@ -1243,13 +1241,15 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
>  			 * \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 != stream->width ||
> +			    cfg.size.height != stream->height)
> +				continue;
>  
> -				match = true;
> -				streams_[i].index = j;
> -			}
> +			LOG(HAL, Info) << "Stream " << i

s/Stream/Android stream/ ?

> +				       << " using libcamera stream " << j;
> +
> +			index = j;
> +			match = true;

Should you break ?

>  		}
>  
>  		/*
> @@ -1272,7 +1272,26 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
>  			LOG(HAL, Info) << "Adding " << streamConfiguration.toString()
>  				       << " for MJPEG support";
>  
> -			streams_[i].index = config_->addConfiguration(streamConfiguration);
> +			index = config_->addConfiguration(streamConfiguration);
> +		}
> +
> +		StreamConfiguration &cfg = config_->at(index);
> +		PixelFormat format = formats::MJPEG;
> +		Size size = cfg.size;
> +
> +		CameraStream &cameraStream = streams_.emplace_back(format, size, index);

No need for the format and size local variables, you can write

		CameraStream &cameraStream =
			streams_.emplace_back(formats::MJPEG, cfg.size, index);

> +		stream->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";

This holds on a single line.

			LOG(HAL, Error) << "Failed to configure encoder";

> +			return ret;
>  		}
>  	}
>  
> @@ -1295,25 +1314,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;
> -			}
> -		}
>  	}
>  
>  	/*
> @@ -1427,7 +1432,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);
> @@ -1482,7 +1487,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 {

Can you turn it into a class while at it ?

Also clearly not a candidate for this patch, do you think it would be
useful to rename the HAL-related classes with a HAL prefix (e.g.
HALCamera instead of CameraDevice, HALStream instead of CameraStream) to
avoid confusing them with the libcamera classes ? We're familiar with
this code so it's probably not too confusing for us, but it may be for a
newcomer.

> -	CameraStream(libcamera::PixelFormat, libcamera::Size);
> +public:
> +	CameraStream(libcamera::PixelFormat, libcamera::Size, unsigned int i);

i is not a very clear name, index would be better. I would also name the
existing parameters (that could be a separate change).

>  	~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.
> +	 */

Should this be moved to the .cpp file ? Documentation rules are a bit
more relaxed here though, as it's internal doc.

> +	unsigned int index_;
>  };
>  
>  class CameraDevice : protected libcamera::Loggable

Hi Laurent,

On Sun, Sep 06, 2020 at 01:05:24AM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Wed, Sep 02, 2020 at 05:22:34PM +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 | 75 +++++++++++++++++++----------------
> >  src/android/camera_device.h   | 18 +++++----
> >  2 files changed, 51 insertions(+), 42 deletions(-)
> >
> > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> > index 3630e87e8814..9bcd1d993c17 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)
> >  {
> >  }
> >
> > @@ -1212,30 +1212,28 @@ 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)
> >  			continue;
> >
> >  		StreamConfiguration streamConfiguration;
> > -
> >  		streamConfiguration.size = size;
> >  		streamConfiguration.pixelFormat = format;
> >
> > -		streams_[i].index = config_->addConfiguration(streamConfiguration);
> > +		unsigned int index = config_->addConfiguration(streamConfiguration);
> > +		CameraStream &cameraStream = streams_.emplace_back(format, size, index);
> > +		stream->priv = static_cast<void *>(&cameraStream);
>
> This is problematic. std::vector::emplace_back may cause a reallocation,

We have reserved space for all the possible cameraStream instances
before entering this loop, didn't we ? It should save all relocations,
or don't you think it's enough ?

> which invalidates all iterators, rendering the pointer invalid. One
> option would be to turn streams_ into an std::list, another option to
> fill the priv pointers in a separate loop after populating streams_.
>
> >  	}
> >
> >  	/* 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;
>
> Drive-by comment for a possible future simplification, can't we rely on
> the fact that only a single JPEG stream will be requested ? If so, we
> could store a pointer to the JPEG camera3_stream_configuration_t in the
> previous loop, and have a
>
> 	if (jpegStream) {
> 		...
> 	}
>
> here instead of a loop.
>

Ah yes, we can save this external loop indeed!
Thanks ;)

> >
> > -		/* Search for a compatible stream */
> > +		/* Search for a compatible stream in the non-JPEG ones. */
> > +		bool match = false;
> > +		unsigned int index;
> >  		for (unsigned int j = 0; j < config_->size(); j++) {
> >  			StreamConfiguration &cfg = config_->at(j);
> >
> > @@ -1243,13 +1241,15 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
> >  			 * \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 != stream->width ||
> > +			    cfg.size.height != stream->height)
> > +				continue;
> >
> > -				match = true;
> > -				streams_[i].index = j;
> > -			}
> > +			LOG(HAL, Info) << "Stream " << i
>
> s/Stream/Android stream/ ?
>
> > +				       << " using libcamera stream " << j;
> > +
> > +			index = j;
> > +			match = true;
>
> Should you break ?
>

Ups, I should indeed

> >  		}
> >
> >  		/*
> > @@ -1272,7 +1272,26 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
> >  			LOG(HAL, Info) << "Adding " << streamConfiguration.toString()
> >  				       << " for MJPEG support";
> >
> > -			streams_[i].index = config_->addConfiguration(streamConfiguration);
> > +			index = config_->addConfiguration(streamConfiguration);
> > +		}
> > +
> > +		StreamConfiguration &cfg = config_->at(index);
> > +		PixelFormat format = formats::MJPEG;
> > +		Size size = cfg.size;
> > +
> > +		CameraStream &cameraStream = streams_.emplace_back(format, size, index);
>
> No need for the format and size local variables, you can write
>
> 		CameraStream &cameraStream =
> 			streams_.emplace_back(formats::MJPEG, cfg.size, index);
>

Kieran had a similar comment iirc. I'll change this!

> > +		stream->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";
>
> This holds on a single line.
>
> 			LOG(HAL, Error) << "Failed to configure encoder";

Right!

>
> > +			return ret;
> >  		}
> >  	}
> >
> > @@ -1295,25 +1314,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;
> > -			}
> > -		}
> >  	}
> >
> >  	/*
> > @@ -1427,7 +1432,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);
> > @@ -1482,7 +1487,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 {
>
> Can you turn it into a class while at it ?
>

See the end of the series  :)

> Also clearly not a candidate for this patch, do you think it would be
> useful to rename the HAL-related classes with a HAL prefix (e.g.
> HALCamera instead of CameraDevice, HALStream instead of CameraStream) to
> avoid confusing them with the libcamera classes ? We're familiar with
> this code so it's probably not too confusing for us, but it may be for a
> newcomer.

Indeed, I'm looking forward to get to a point where we can break
camera_device.cpp in multiple files, and possible re-name them as
well.

I tried using the camera3 prefix for all Android-things to keep them
separate from our ones, but indeed we have confusing names for some
classes.

>
> > -	CameraStream(libcamera::PixelFormat, libcamera::Size);
> > +public:
> > +	CameraStream(libcamera::PixelFormat, libcamera::Size, unsigned int i);
>
> i is not a very clear name, index would be better. I would also name the
> existing parameters (that could be a separate change).
>

I think this happens later all in one go

> >  	~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.
> > +	 */
>
> Should this be moved to the .cpp file ? Documentation rules are a bit
> more relaxed here though, as it's internal doc.
>

well, having them in the cpp file makes most sense for the purpose of
generating documentation, which doesn't happen for the HAL. This
comment alone, not anchored to the field declaration loses a bit of
value.

I thinks going forward we want to document more formally this part
though.

> > +	unsigned int index_;
> >  };
> >
> >  class CameraDevice : protected libcamera::Loggable
>
> --
> Regards,
>
> Laurent Pinchart

Hi Jacopo,

On Mon, Sep 07, 2020 at 10:53:33AM +0200, Jacopo Mondi wrote:
> On Sun, Sep 06, 2020 at 01:05:24AM +0300, Laurent Pinchart wrote:
> > On Wed, Sep 02, 2020 at 05:22:34PM +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 | 75 +++++++++++++++++++----------------
> > >  src/android/camera_device.h   | 18 +++++----
> > >  2 files changed, 51 insertions(+), 42 deletions(-)
> > >
> > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> > > index 3630e87e8814..9bcd1d993c17 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)
> > >  {
> > >  }
> > >
> > > @@ -1212,30 +1212,28 @@ 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)
> > >  			continue;
> > >
> > >  		StreamConfiguration streamConfiguration;
> > > -
> > >  		streamConfiguration.size = size;
> > >  		streamConfiguration.pixelFormat = format;
> > >
> > > -		streams_[i].index = config_->addConfiguration(streamConfiguration);
> > > +		unsigned int index = config_->addConfiguration(streamConfiguration);
> > > +		CameraStream &cameraStream = streams_.emplace_back(format, size, index);
> > > +		stream->priv = static_cast<void *>(&cameraStream);
> >
> > This is problematic. std::vector::emplace_back may cause a reallocation,
> 
> We have reserved space for all the possible cameraStream instances
> before entering this loop, didn't we ? It should save all relocations,
> or don't you think it's enough ?

You're right, I had forgotten about that.

> > which invalidates all iterators, rendering the pointer invalid. One
> > option would be to turn streams_ into an std::list, another option to
> > fill the priv pointers in a separate loop after populating streams_.
> >
> > >  	}
> > >
> > >  	/* 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;
> >
> > Drive-by comment for a possible future simplification, can't we rely on
> > the fact that only a single JPEG stream will be requested ? If so, we
> > could store a pointer to the JPEG camera3_stream_configuration_t in the
> > previous loop, and have a
> >
> > 	if (jpegStream) {
> > 		...
> > 	}
> >
> > here instead of a loop.
> 
> Ah yes, we can save this external loop indeed!
> Thanks ;)
> 
> > > -		/* Search for a compatible stream */
> > > +		/* Search for a compatible stream in the non-JPEG ones. */
> > > +		bool match = false;
> > > +		unsigned int index;
> > >  		for (unsigned int j = 0; j < config_->size(); j++) {
> > >  			StreamConfiguration &cfg = config_->at(j);
> > >
> > > @@ -1243,13 +1241,15 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
> > >  			 * \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 != stream->width ||
> > > +			    cfg.size.height != stream->height)
> > > +				continue;
> > >
> > > -				match = true;
> > > -				streams_[i].index = j;
> > > -			}
> > > +			LOG(HAL, Info) << "Stream " << i
> >
> > s/Stream/Android stream/ ?
> >
> > > +				       << " using libcamera stream " << j;
> > > +
> > > +			index = j;
> > > +			match = true;
> >
> > Should you break ?
> 
> Ups, I should indeed
> 
> > >  		}
> > >
> > >  		/*
> > > @@ -1272,7 +1272,26 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
> > >  			LOG(HAL, Info) << "Adding " << streamConfiguration.toString()
> > >  				       << " for MJPEG support";
> > >
> > > -			streams_[i].index = config_->addConfiguration(streamConfiguration);
> > > +			index = config_->addConfiguration(streamConfiguration);
> > > +		}
> > > +
> > > +		StreamConfiguration &cfg = config_->at(index);
> > > +		PixelFormat format = formats::MJPEG;
> > > +		Size size = cfg.size;
> > > +
> > > +		CameraStream &cameraStream = streams_.emplace_back(format, size, index);
> >
> > No need for the format and size local variables, you can write
> >
> > 		CameraStream &cameraStream =
> > 			streams_.emplace_back(formats::MJPEG, cfg.size, index);
> 
> Kieran had a similar comment iirc. I'll change this!
> 
> > > +		stream->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";
> >
> > This holds on a single line.
> >
> > 			LOG(HAL, Error) << "Failed to configure encoder";
> 
> Right!
> 
> > > +			return ret;
> > >  		}
> > >  	}
> > >
> > > @@ -1295,25 +1314,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;
> > > -			}
> > > -		}
> > >  	}
> > >
> > >  	/*
> > > @@ -1427,7 +1432,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);
> > > @@ -1482,7 +1487,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 {
> >
> > Can you turn it into a class while at it ?
> 
> See the end of the series  :)
> 
> > Also clearly not a candidate for this patch, do you think it would be
> > useful to rename the HAL-related classes with a HAL prefix (e.g.
> > HALCamera instead of CameraDevice, HALStream instead of CameraStream) to
> > avoid confusing them with the libcamera classes ? We're familiar with
> > this code so it's probably not too confusing for us, but it may be for a
> > newcomer.
> 
> Indeed, I'm looking forward to get to a point where we can break
> camera_device.cpp in multiple files, and possible re-name them as
> well.
> 
> I tried using the camera3 prefix for all Android-things to keep them
> separate from our ones, but indeed we have confusing names for some
> classes.
> 
> > > -	CameraStream(libcamera::PixelFormat, libcamera::Size);
> > > +public:
> > > +	CameraStream(libcamera::PixelFormat, libcamera::Size, unsigned int i);
> >
> > i is not a very clear name, index would be better. I would also name the
> > existing parameters (that could be a separate change).
> 
> I think this happens later all in one go
> 
> > >  	~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.
> > > +	 */
> >
> > Should this be moved to the .cpp file ? Documentation rules are a bit
> > more relaxed here though, as it's internal doc.
> 
> well, having them in the cpp file makes most sense for the purpose of
> generating documentation, which doesn't happen for the HAL. This
> comment alone, not anchored to the field declaration loses a bit of
> value.
> 
> I thinks going forward we want to document more formally this part
> though.
> 
> > > +	unsigned int index_;
> > >  };
> > >
> > >  class CameraDevice : protected libcamera::Loggable

On Mon, Sep 7, 2020 at 10:28 PM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Jacopo,
>
> On Mon, Sep 07, 2020 at 10:53:33AM +0200, Jacopo Mondi wrote:
> > On Sun, Sep 06, 2020 at 01:05:24AM +0300, Laurent Pinchart wrote:
> > > On Wed, Sep 02, 2020 at 05:22:34PM +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 | 75 +++++++++++++++++++----------------
> > > >  src/android/camera_device.h   | 18 +++++----
> > > >  2 files changed, 51 insertions(+), 42 deletions(-)
> > > >
> > > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> > > > index 3630e87e8814..9bcd1d993c17 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)
> > > >  {
> > > >  }
> > > >
> > > > @@ -1212,30 +1212,28 @@ 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)
> > > >                   continue;
> > > >
> > > >           StreamConfiguration streamConfiguration;
> > > > -
> > > >           streamConfiguration.size = size;
> > > >           streamConfiguration.pixelFormat = format;
> > > >
> > > > -         streams_[i].index = config_->addConfiguration(streamConfiguration);
> > > > +         unsigned int index = config_->addConfiguration(streamConfiguration);
> > > > +         CameraStream &cameraStream = streams_.emplace_back(format, size, index);
> > > > +         stream->priv = static_cast<void *>(&cameraStream);
> > >
> > > This is problematic. std::vector::emplace_back may cause a reallocation,
> >
> > We have reserved space for all the possible cameraStream instances
> > before entering this loop, didn't we ? It should save all relocations,
> > or don't you think it's enough ?
>
> You're right, I had forgotten about that.
>

Huge nit: [This is my preference, please feel free to ignore]
I would write
streams_.push_back(format, size, index);
stream->priv = static_cast<void*>(&streams_.back());

> > > which invalidates all iterators, rendering the pointer invalid. One
> > > option would be to turn streams_ into an std::list, another option to
> > > fill the priv pointers in a separate loop after populating streams_.
> > >
> > > >   }
> > > >
> > > >   /* 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;
> > >
> > > Drive-by comment for a possible future simplification, can't we rely on
> > > the fact that only a single JPEG stream will be requested ? If so, we
> > > could store a pointer to the JPEG camera3_stream_configuration_t in the
> > > previous loop, and have a
> > >
> > >     if (jpegStream) {
> > >             ...
> > >     }
> > >
> > > here instead of a loop.
> >
> > Ah yes, we can save this external loop indeed!
> > Thanks ;)
> >
> > > > -         /* Search for a compatible stream */
> > > > +         /* Search for a compatible stream in the non-JPEG ones. */
> > > > +         bool match = false;
> > > > +         unsigned int index;
> > > >           for (unsigned int j = 0; j < config_->size(); j++) {
> > > >                   StreamConfiguration &cfg = config_->at(j);
> > > >
> > > > @@ -1243,13 +1241,15 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
> > > >                    * \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 != stream->width ||
> > > > +                     cfg.size.height != stream->height)
> > > > +                         continue;
> > > >
> > > > -                         match = true;
> > > > -                         streams_[i].index = j;
> > > > -                 }
> > > > +                 LOG(HAL, Info) << "Stream " << i
> > >
> > > s/Stream/Android stream/ ?
> > >
> > > > +                                << " using libcamera stream " << j;
> > > > +
> > > > +                 index = j;
> > > > +                 match = true;
> > >
> > > Should you break ?
> >
> > Ups, I should indeed
> >

Nit: I would save match by setting index to -1 initially, or use std::optional.

> > > >           }
> > > >
> > > >           /*
> > > > @@ -1272,7 +1272,26 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
> > > >                   LOG(HAL, Info) << "Adding " << streamConfiguration.toString()
> > > >                                  << " for MJPEG support";
> > > >
> > > > -                 streams_[i].index = config_->addConfiguration(streamConfiguration);
> > > > +                 index = config_->addConfiguration(streamConfiguration);
> > > > +         }
> > > > +
> > > > +         StreamConfiguration &cfg = config_->at(index);
> > > > +         PixelFormat format = formats::MJPEG;
> > > > +         Size size = cfg.size;
> > > > +
> > > > +         CameraStream &cameraStream = streams_.emplace_back(format, size, index);
> > >
> > > No need for the format and size local variables, you can write
> > >
> > >             CameraStream &cameraStream =
> > >                     streams_.emplace_back(formats::MJPEG, cfg.size, index);
> >
> > Kieran had a similar comment iirc. I'll change this!
> >
> > > > +         stream->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";
> > >
> > > This holds on a single line.
> > >
> > >                     LOG(HAL, Error) << "Failed to configure encoder";
> >
> > Right!
> >
> > > > +                 return ret;
> > > >           }
> > > >   }
> > > >
> > > > @@ -1295,25 +1314,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;
> > > > -                 }
> > > > -         }
> > > >   }
> > > >
> > > >   /*
> > > > @@ -1427,7 +1432,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);
> > > > @@ -1482,7 +1487,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 {
> > >
> > > Can you turn it into a class while at it ?
> >
> > See the end of the series  :)
> >
> > > Also clearly not a candidate for this patch, do you think it would be
> > > useful to rename the HAL-related classes with a HAL prefix (e.g.
> > > HALCamera instead of CameraDevice, HALStream instead of CameraStream) to
> > > avoid confusing them with the libcamera classes ? We're familiar with
> > > this code so it's probably not too confusing for us, but it may be for a
> > > newcomer.
> >
> > Indeed, I'm looking forward to get to a point where we can break
> > camera_device.cpp in multiple files, and possible re-name them as
> > well.
> >
> > I tried using the camera3 prefix for all Android-things to keep them
> > separate from our ones, but indeed we have confusing names for some
> > classes.
> >
> > > > - CameraStream(libcamera::PixelFormat, libcamera::Size);
> > > > +public:
> > > > + CameraStream(libcamera::PixelFormat, libcamera::Size, unsigned int i);
> > >
> > > i is not a very clear name, index would be better. I would also name the
> > > existing parameters (that could be a separate change).
> >
> > I think this happens later all in one go
> >
> > > >   ~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.
> > > > +  */
> > >
> > > Should this be moved to the .cpp file ? Documentation rules are a bit
> > > more relaxed here though, as it's internal doc.
> >
> > well, having them in the cpp file makes most sense for the purpose of
> > generating documentation, which doesn't happen for the HAL. This
> > comment alone, not anchored to the field declaration loses a bit of
> > value.
> >
> > I thinks going forward we want to document more formally this part
> > though.
> >
> > > > + unsigned int index_;
> > > >  };
> > > >
> > > >  class CameraDevice : protected libcamera::Loggable
>
> --
> Regards,
>
> Laurent Pinchart
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel