[libcamera-devel,v2,3/9] android: camera_device: Support multiple stream configurations
diff mbox series

Message ID 20200702213654.2129054-4-kieran.bingham@ideasonboard.com
State Superseded
Headers show
Series
  • android: Multi-stream support
Related show

Commit Message

Kieran Bingham July 2, 2020, 9:36 p.m. UTC 
Create an initial Camera Configuration using an empty role set, and
populate the StreamConfigurations manually from each of the streams
given by the Android camera3_stream_configuration_t stream_list.

Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
---
 src/android/camera_device.cpp | 51 +++++++++++++++++------------------
 1 file changed, 25 insertions(+), 26 deletions(-)

Comments

Niklas Söderlund July 2, 2020, 11:22 p.m. UTC | #1 
Hi Kieran,

Thanks for your work.

On 2020-07-02 22:36:48 +0100, Kieran Bingham wrote:
> Create an initial Camera Configuration using an empty role set, and
> populate the StreamConfigurations manually from each of the streams
> given by the Android camera3_stream_configuration_t stream_list.
> 
> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> ---
>  src/android/camera_device.cpp | 51 +++++++++++++++++------------------
>  1 file changed, 25 insertions(+), 26 deletions(-)
> 
> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> index b9031ff0c2a4..03dcdd520794 100644
> --- a/src/android/camera_device.cpp
> +++ b/src/android/camera_device.cpp
> @@ -942,6 +942,16 @@ PixelFormat CameraDevice::toPixelFormat(int format)
>   */
>  int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
>  {
> +	/*
> +	 * Generate an empty configuration, and construct a StreamConfiguration
> +	 * for each camera3_stream to add to it.
> +	 */
> +	config_ = camera_->generateConfiguration();
> +	if (!config_) {
> +		LOG(HAL, Error) << "Failed to generate camera configuration";
> +		return -EINVAL;
> +	}
> +
>  	for (unsigned int i = 0; i < stream_list->num_streams; ++i) {
>  		camera3_stream_t *stream = stream_list->streams[i];
>  
> @@ -953,35 +963,18 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
>  			       << ", height: " << stream->height
>  			       << ", format: " << utils::hex(stream->format)
>  			       << " (" << format.toString() << ")";
> -	}
>  
> -	/* Only one stream is supported. */
> -	if (stream_list->num_streams != 1) {
> -		LOG(HAL, Error) << "Only one stream supported";
> -		return -EINVAL;
> -	}
> -	camera3_stream_t *camera3Stream = stream_list->streams[0];
> +		if (!format.isValid())
> +			return -EINVAL;
>  
> -	/* Translate Android format code to libcamera pixel format. */
> -	PixelFormat format = toPixelFormat(camera3Stream->format);
> -	if (!format.isValid())
> -		return -EINVAL;
> +		StreamConfiguration streamConfiguration;
>  
> -	/*
> -	 * Hardcode viewfinder role, replacing the generated configuration
> -	 * parameters with the ones requested by the Android framework.
> -	 */
> -	StreamRoles roles = { StreamRole::Viewfinder };
> -	config_ = camera_->generateConfiguration(roles);
> -	if (!config_ || config_->empty()) {
> -		LOG(HAL, Error) << "Failed to generate camera configuration";
> -		return -EINVAL;
> -	}
> +		streamConfiguration.size.width = stream->width;
> +		streamConfiguration.size.height = stream->height;
> +		streamConfiguration.pixelFormat = format;

This is not related to this patch but blindly translating a HAL format 
using a lookup table to a libcamera format will not work for RAW. I'm 
just curious if it is sufficient for JPEG?

>  
> -	StreamConfiguration *streamConfiguration = &config_->at(0);
> -	streamConfiguration->size.width = camera3Stream->width;
> -	streamConfiguration->size.height = camera3Stream->height;
> -	streamConfiguration->pixelFormat = format;
> +		config_->addConfiguration(streamConfiguration);
> +	}
>  
>  	switch (config_->validate()) {
>  	case CameraConfiguration::Valid:
> @@ -996,7 +989,13 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
>  		return -EINVAL;
>  	}
>  
> -	camera3Stream->max_buffers = streamConfiguration->bufferCount;
> +	for (unsigned int i = 0; i < stream_list->num_streams; ++i) {

Small nit, would it make sens to iterate over config_ instead of 
stream_list->num_streams? If we have a discrepancy we would end up with 
less streams then requested but if we use ->at(i) an exception.

This is a small thing that poped out at me, with or without this 
changed,

Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>

> +		camera3_stream_t *stream = stream_list->streams[i];
> +		StreamConfiguration &streamConfiguration = config_->at(i);
> +
> +		/* Use the bufferCount confirmed by the validation process. */
> +		stream->max_buffers = streamConfiguration.bufferCount;
> +	}
>  
>  	/*
>  	 * Once the CameraConfiguration has been adjusted/validated
> -- 
> 2.25.1
> 
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
Laurent Pinchart July 3, 2020, 12:22 a.m. UTC | #2 
Hi Kieran,

Thank you for the patch.

On Thu, Jul 02, 2020 at 10:36:48PM +0100, Kieran Bingham wrote:
> Create an initial Camera Configuration using an empty role set, and
> populate the StreamConfigurations manually from each of the streams
> given by the Android camera3_stream_configuration_t stream_list.
> 
> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

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

> ---
>  src/android/camera_device.cpp | 51 +++++++++++++++++------------------
>  1 file changed, 25 insertions(+), 26 deletions(-)
> 
> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> index b9031ff0c2a4..03dcdd520794 100644
> --- a/src/android/camera_device.cpp
> +++ b/src/android/camera_device.cpp
> @@ -942,6 +942,16 @@ PixelFormat CameraDevice::toPixelFormat(int format)
>   */
>  int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
>  {
> +	/*
> +	 * Generate an empty configuration, and construct a StreamConfiguration
> +	 * for each camera3_stream to add to it.
> +	 */
> +	config_ = camera_->generateConfiguration();
> +	if (!config_) {
> +		LOG(HAL, Error) << "Failed to generate camera configuration";
> +		return -EINVAL;
> +	}
> +
>  	for (unsigned int i = 0; i < stream_list->num_streams; ++i) {
>  		camera3_stream_t *stream = stream_list->streams[i];
>  
> @@ -953,35 +963,18 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
>  			       << ", height: " << stream->height
>  			       << ", format: " << utils::hex(stream->format)
>  			       << " (" << format.toString() << ")";
> -	}
>  
> -	/* Only one stream is supported. */
> -	if (stream_list->num_streams != 1) {
> -		LOG(HAL, Error) << "Only one stream supported";
> -		return -EINVAL;
> -	}
> -	camera3_stream_t *camera3Stream = stream_list->streams[0];
> +		if (!format.isValid())
> +			return -EINVAL;
>  
> -	/* Translate Android format code to libcamera pixel format. */
> -	PixelFormat format = toPixelFormat(camera3Stream->format);
> -	if (!format.isValid())
> -		return -EINVAL;
> +		StreamConfiguration streamConfiguration;
>  
> -	/*
> -	 * Hardcode viewfinder role, replacing the generated configuration
> -	 * parameters with the ones requested by the Android framework.
> -	 */
> -	StreamRoles roles = { StreamRole::Viewfinder };
> -	config_ = camera_->generateConfiguration(roles);
> -	if (!config_ || config_->empty()) {
> -		LOG(HAL, Error) << "Failed to generate camera configuration";
> -		return -EINVAL;
> -	}
> +		streamConfiguration.size.width = stream->width;
> +		streamConfiguration.size.height = stream->height;
> +		streamConfiguration.pixelFormat = format;
>  
> -	StreamConfiguration *streamConfiguration = &config_->at(0);
> -	streamConfiguration->size.width = camera3Stream->width;
> -	streamConfiguration->size.height = camera3Stream->height;
> -	streamConfiguration->pixelFormat = format;
> +		config_->addConfiguration(streamConfiguration);
> +	}
>  
>  	switch (config_->validate()) {
>  	case CameraConfiguration::Valid:
> @@ -996,7 +989,13 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
>  		return -EINVAL;
>  	}
>  
> -	camera3Stream->max_buffers = streamConfiguration->bufferCount;
> +	for (unsigned int i = 0; i < stream_list->num_streams; ++i) {
> +		camera3_stream_t *stream = stream_list->streams[i];
> +		StreamConfiguration &streamConfiguration = config_->at(i);
> +
> +		/* Use the bufferCount confirmed by the validation process. */
> +		stream->max_buffers = streamConfiguration.bufferCount;
> +	}
>  
>  	/*
>  	 * Once the CameraConfiguration has been adjusted/validated
Jacopo Mondi July 3, 2020, 9:08 a.m. UTC | #3 
Hi Kieran

On Thu, Jul 02, 2020 at 10:36:48PM +0100, Kieran Bingham wrote:
> Create an initial Camera Configuration using an empty role set, and
> populate the StreamConfigurations manually from each of the streams
> given by the Android camera3_stream_configuration_t stream_list.
>
> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>

Thanks
  j

> ---
>  src/android/camera_device.cpp | 51 +++++++++++++++++------------------
>  1 file changed, 25 insertions(+), 26 deletions(-)
>
> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> index b9031ff0c2a4..03dcdd520794 100644
> --- a/src/android/camera_device.cpp
> +++ b/src/android/camera_device.cpp
> @@ -942,6 +942,16 @@ PixelFormat CameraDevice::toPixelFormat(int format)
>   */
>  int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
>  {
> +	/*
> +	 * Generate an empty configuration, and construct a StreamConfiguration
> +	 * for each camera3_stream to add to it.
> +	 */
> +	config_ = camera_->generateConfiguration();
> +	if (!config_) {
> +		LOG(HAL, Error) << "Failed to generate camera configuration";
> +		return -EINVAL;
> +	}
> +
>  	for (unsigned int i = 0; i < stream_list->num_streams; ++i) {
>  		camera3_stream_t *stream = stream_list->streams[i];
>
> @@ -953,35 +963,18 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
>  			       << ", height: " << stream->height
>  			       << ", format: " << utils::hex(stream->format)
>  			       << " (" << format.toString() << ")";
> -	}
>
> -	/* Only one stream is supported. */
> -	if (stream_list->num_streams != 1) {
> -		LOG(HAL, Error) << "Only one stream supported";
> -		return -EINVAL;
> -	}
> -	camera3_stream_t *camera3Stream = stream_list->streams[0];
> +		if (!format.isValid())
> +			return -EINVAL;
>
> -	/* Translate Android format code to libcamera pixel format. */
> -	PixelFormat format = toPixelFormat(camera3Stream->format);
> -	if (!format.isValid())
> -		return -EINVAL;
> +		StreamConfiguration streamConfiguration;
>
> -	/*
> -	 * Hardcode viewfinder role, replacing the generated configuration
> -	 * parameters with the ones requested by the Android framework.
> -	 */
> -	StreamRoles roles = { StreamRole::Viewfinder };
> -	config_ = camera_->generateConfiguration(roles);
> -	if (!config_ || config_->empty()) {
> -		LOG(HAL, Error) << "Failed to generate camera configuration";
> -		return -EINVAL;
> -	}
> +		streamConfiguration.size.width = stream->width;
> +		streamConfiguration.size.height = stream->height;
> +		streamConfiguration.pixelFormat = format;
>
> -	StreamConfiguration *streamConfiguration = &config_->at(0);
> -	streamConfiguration->size.width = camera3Stream->width;
> -	streamConfiguration->size.height = camera3Stream->height;
> -	streamConfiguration->pixelFormat = format;
> +		config_->addConfiguration(streamConfiguration);
> +	}
>
>  	switch (config_->validate()) {
>  	case CameraConfiguration::Valid:
> @@ -996,7 +989,13 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
>  		return -EINVAL;
>  	}
>
> -	camera3Stream->max_buffers = streamConfiguration->bufferCount;
> +	for (unsigned int i = 0; i < stream_list->num_streams; ++i) {
> +		camera3_stream_t *stream = stream_list->streams[i];
> +		StreamConfiguration &streamConfiguration = config_->at(i);
> +
> +		/* Use the bufferCount confirmed by the validation process. */
> +		stream->max_buffers = streamConfiguration.bufferCount;
> +	}
>
>  	/*
>  	 * Once the CameraConfiguration has been adjusted/validated
> --
> 2.25.1
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
Kieran Bingham July 3, 2020, 9:30 a.m. UTC | #4 
Hi Niklas,

On 03/07/2020 00:22, Niklas Söderlund wrote:
> Hi Kieran,
> 
> Thanks for your work.
> 
> On 2020-07-02 22:36:48 +0100, Kieran Bingham wrote:
>> Create an initial Camera Configuration using an empty role set, and
>> populate the StreamConfigurations manually from each of the streams
>> given by the Android camera3_stream_configuration_t stream_list.
>>
>> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>> ---
>>  src/android/camera_device.cpp | 51 +++++++++++++++++------------------
>>  1 file changed, 25 insertions(+), 26 deletions(-)
>>
>> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
>> index b9031ff0c2a4..03dcdd520794 100644
>> --- a/src/android/camera_device.cpp
>> +++ b/src/android/camera_device.cpp
>> @@ -942,6 +942,16 @@ PixelFormat CameraDevice::toPixelFormat(int format)
>>   */
>>  int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
>>  {
>> +	/*
>> +	 * Generate an empty configuration, and construct a StreamConfiguration
>> +	 * for each camera3_stream to add to it.
>> +	 */
>> +	config_ = camera_->generateConfiguration();
>> +	if (!config_) {
>> +		LOG(HAL, Error) << "Failed to generate camera configuration";
>> +		return -EINVAL;
>> +	}
>> +
>>  	for (unsigned int i = 0; i < stream_list->num_streams; ++i) {
>>  		camera3_stream_t *stream = stream_list->streams[i];
>>  
>> @@ -953,35 +963,18 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
>>  			       << ", height: " << stream->height
>>  			       << ", format: " << utils::hex(stream->format)
>>  			       << " (" << format.toString() << ")";
>> -	}
>>  
>> -	/* Only one stream is supported. */
>> -	if (stream_list->num_streams != 1) {
>> -		LOG(HAL, Error) << "Only one stream supported";
>> -		return -EINVAL;
>> -	}
>> -	camera3_stream_t *camera3Stream = stream_list->streams[0];
>> +		if (!format.isValid())
>> +			return -EINVAL;
>>  
>> -	/* Translate Android format code to libcamera pixel format. */
>> -	PixelFormat format = toPixelFormat(camera3Stream->format);
>> -	if (!format.isValid())
>> -		return -EINVAL;
>> +		StreamConfiguration streamConfiguration;
>>  
>> -	/*
>> -	 * Hardcode viewfinder role, replacing the generated configuration
>> -	 * parameters with the ones requested by the Android framework.
>> -	 */
>> -	StreamRoles roles = { StreamRole::Viewfinder };
>> -	config_ = camera_->generateConfiguration(roles);
>> -	if (!config_ || config_->empty()) {
>> -		LOG(HAL, Error) << "Failed to generate camera configuration";
>> -		return -EINVAL;
>> -	}
>> +		streamConfiguration.size.width = stream->width;
>> +		streamConfiguration.size.height = stream->height;
>> +		streamConfiguration.pixelFormat = format;
> 
> This is not related to this patch but blindly translating a HAL format 
> using a lookup table to a libcamera format will not work for RAW. I'm 
> just curious if it is sufficient for JPEG?
> 

No - it is not, but that's not tackled by this patch.

Lets consider this patch as supporting multiple streams of types that
are already supported ;-)

I would expect this patch to now support multiple YUV streams.


>>  
>> -	StreamConfiguration *streamConfiguration = &config_->at(0);
>> -	streamConfiguration->size.width = camera3Stream->width;
>> -	streamConfiguration->size.height = camera3Stream->height;
>> -	streamConfiguration->pixelFormat = format;
>> +		config_->addConfiguration(streamConfiguration);
>> +	}
>>  
>>  	switch (config_->validate()) {
>>  	case CameraConfiguration::Valid:
>> @@ -996,7 +989,13 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
>>  		return -EINVAL;
>>  	}
>>  
>> -	camera3Stream->max_buffers = streamConfiguration->bufferCount;
>> +	for (unsigned int i = 0; i < stream_list->num_streams; ++i) {
> 
> Small nit, would it make sens to iterate over config_ instead of 
> stream_list->num_streams? If we have a discrepancy we would end up with 
> less streams then requested but if we use ->at(i) an exception.

No... or at least - not in the /very/ near future.

There will very soon be an explicit discrepency between the number of
camera3 streams and the number of libcamera streams.

Adding a JPEG stream will not add a new stream, but we must still
support it.

Later I add a mapping which stores the correct libcamera index to
reference for this stream (for which multiple camera3 streams might
reference a single libcamera stream index).

If I iterate over config_ here, I would still need an index to map
/back/ to the camera3_stream index, so either way an explicit for loop
seems better here currently.



> 
> This is a small thing that poped out at me, with or without this 
> changed,
> 
> Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>

Thanks,

I hope the above makes you happier with this path...


> 
>> +		camera3_stream_t *stream = stream_list->streams[i];
>> +		StreamConfiguration &streamConfiguration = config_->at(i);
>> +
>> +		/* Use the bufferCount confirmed by the validation process. */
>> +		stream->max_buffers = streamConfiguration.bufferCount;
>> +	}
>>  
>>  	/*
>>  	 * Once the CameraConfiguration has been adjusted/validated
>> -- 
>> 2.25.1
>>
>> _______________________________________________
>> libcamera-devel mailing list
>> libcamera-devel@lists.libcamera.org
>> https://lists.libcamera.org/listinfo/libcamera-devel
>
Niklas Söderlund July 3, 2020, 10:14 a.m. UTC | #5 
Hi Kieran,

On 2020-07-03 10:30:17 +0100, Kieran Bingham wrote:
> Hi Niklas,
> 
> On 03/07/2020 00:22, Niklas Söderlund wrote:
> > Hi Kieran,
> > 
> > Thanks for your work.
> > 
> > On 2020-07-02 22:36:48 +0100, Kieran Bingham wrote:
> >> Create an initial Camera Configuration using an empty role set, and
> >> populate the StreamConfigurations manually from each of the streams
> >> given by the Android camera3_stream_configuration_t stream_list.
> >>
> >> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> >> ---
> >>  src/android/camera_device.cpp | 51 +++++++++++++++++------------------
> >>  1 file changed, 25 insertions(+), 26 deletions(-)
> >>
> >> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> >> index b9031ff0c2a4..03dcdd520794 100644
> >> --- a/src/android/camera_device.cpp
> >> +++ b/src/android/camera_device.cpp
> >> @@ -942,6 +942,16 @@ PixelFormat CameraDevice::toPixelFormat(int format)
> >>   */
> >>  int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
> >>  {
> >> +	/*
> >> +	 * Generate an empty configuration, and construct a StreamConfiguration
> >> +	 * for each camera3_stream to add to it.
> >> +	 */
> >> +	config_ = camera_->generateConfiguration();
> >> +	if (!config_) {
> >> +		LOG(HAL, Error) << "Failed to generate camera configuration";
> >> +		return -EINVAL;
> >> +	}
> >> +
> >>  	for (unsigned int i = 0; i < stream_list->num_streams; ++i) {
> >>  		camera3_stream_t *stream = stream_list->streams[i];
> >>  
> >> @@ -953,35 +963,18 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
> >>  			       << ", height: " << stream->height
> >>  			       << ", format: " << utils::hex(stream->format)
> >>  			       << " (" << format.toString() << ")";
> >> -	}
> >>  
> >> -	/* Only one stream is supported. */
> >> -	if (stream_list->num_streams != 1) {
> >> -		LOG(HAL, Error) << "Only one stream supported";
> >> -		return -EINVAL;
> >> -	}
> >> -	camera3_stream_t *camera3Stream = stream_list->streams[0];
> >> +		if (!format.isValid())
> >> +			return -EINVAL;
> >>  
> >> -	/* Translate Android format code to libcamera pixel format. */
> >> -	PixelFormat format = toPixelFormat(camera3Stream->format);
> >> -	if (!format.isValid())
> >> -		return -EINVAL;
> >> +		StreamConfiguration streamConfiguration;
> >>  
> >> -	/*
> >> -	 * Hardcode viewfinder role, replacing the generated configuration
> >> -	 * parameters with the ones requested by the Android framework.
> >> -	 */
> >> -	StreamRoles roles = { StreamRole::Viewfinder };
> >> -	config_ = camera_->generateConfiguration(roles);
> >> -	if (!config_ || config_->empty()) {
> >> -		LOG(HAL, Error) << "Failed to generate camera configuration";
> >> -		return -EINVAL;
> >> -	}
> >> +		streamConfiguration.size.width = stream->width;
> >> +		streamConfiguration.size.height = stream->height;
> >> +		streamConfiguration.pixelFormat = format;
> > 
> > This is not related to this patch but blindly translating a HAL format 
> > using a lookup table to a libcamera format will not work for RAW. I'm 
> > just curious if it is sufficient for JPEG?
> > 
> 
> No - it is not, but that's not tackled by this patch.
> 
> Lets consider this patch as supporting multiple streams of types that
> are already supported ;-)
> 
> I would expect this patch to now support multiple YUV streams.
> 
> 
> >>  
> >> -	StreamConfiguration *streamConfiguration = &config_->at(0);
> >> -	streamConfiguration->size.width = camera3Stream->width;
> >> -	streamConfiguration->size.height = camera3Stream->height;
> >> -	streamConfiguration->pixelFormat = format;
> >> +		config_->addConfiguration(streamConfiguration);
> >> +	}
> >>  
> >>  	switch (config_->validate()) {
> >>  	case CameraConfiguration::Valid:
> >> @@ -996,7 +989,13 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
> >>  		return -EINVAL;
> >>  	}
> >>  
> >> -	camera3Stream->max_buffers = streamConfiguration->bufferCount;
> >> +	for (unsigned int i = 0; i < stream_list->num_streams; ++i) {
> > 
> > Small nit, would it make sens to iterate over config_ instead of 
> > stream_list->num_streams? If we have a discrepancy we would end up with 
> > less streams then requested but if we use ->at(i) an exception.
> 
> No... or at least - not in the /very/ near future.
> 
> There will very soon be an explicit discrepency between the number of
> camera3 streams and the number of libcamera streams.
> 
> Adding a JPEG stream will not add a new stream, but we must still
> support it.
> 
> Later I add a mapping which stores the correct libcamera index to
> reference for this stream (for which multiple camera3 streams might
> reference a single libcamera stream index).
> 
> If I iterate over config_ here, I would still need an index to map
> /back/ to the camera3_stream index, so either way an explicit for loop
> seems better here currently.
> 
> 
> 
> > 
> > This is a small thing that poped out at me, with or without this 
> > changed,
> > 
> > Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> 
> Thanks,
> 
> I hope the above makes you happier with this path...

It does, thanks for the explanation.

> 
> 
> > 
> >> +		camera3_stream_t *stream = stream_list->streams[i];
> >> +		StreamConfiguration &streamConfiguration = config_->at(i);
> >> +
> >> +		/* Use the bufferCount confirmed by the validation process. */
> >> +		stream->max_buffers = streamConfiguration.bufferCount;
> >> +	}
> >>  
> >>  	/*
> >>  	 * Once the CameraConfiguration has been adjusted/validated
> >> -- 
> >> 2.25.1
> >>
> >> _______________________________________________
> >> libcamera-devel mailing list
> >> libcamera-devel@lists.libcamera.org
> >> https://lists.libcamera.org/listinfo/libcamera-devel
> > 
> 
> -- 
> Regards
> --
> Kieran

Patch
diff mbox series 

diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
index b9031ff0c2a4..03dcdd520794 100644
--- a/src/android/camera_device.cpp
+++ b/src/android/camera_device.cpp
@@ -942,6 +942,16 @@  PixelFormat CameraDevice::toPixelFormat(int format)
  */
 int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
 {
+	/*
+	 * Generate an empty configuration, and construct a StreamConfiguration
+	 * for each camera3_stream to add to it.
+	 */
+	config_ = camera_->generateConfiguration();
+	if (!config_) {
+		LOG(HAL, Error) << "Failed to generate camera configuration";
+		return -EINVAL;
+	}
+
 	for (unsigned int i = 0; i < stream_list->num_streams; ++i) {
 		camera3_stream_t *stream = stream_list->streams[i];
 
@@ -953,35 +963,18 @@  int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
 			       << ", height: " << stream->height
 			       << ", format: " << utils::hex(stream->format)
 			       << " (" << format.toString() << ")";
-	}
 
-	/* Only one stream is supported. */
-	if (stream_list->num_streams != 1) {
-		LOG(HAL, Error) << "Only one stream supported";
-		return -EINVAL;
-	}
-	camera3_stream_t *camera3Stream = stream_list->streams[0];
+		if (!format.isValid())
+			return -EINVAL;
 
-	/* Translate Android format code to libcamera pixel format. */
-	PixelFormat format = toPixelFormat(camera3Stream->format);
-	if (!format.isValid())
-		return -EINVAL;
+		StreamConfiguration streamConfiguration;
 
-	/*
-	 * Hardcode viewfinder role, replacing the generated configuration
-	 * parameters with the ones requested by the Android framework.
-	 */
-	StreamRoles roles = { StreamRole::Viewfinder };
-	config_ = camera_->generateConfiguration(roles);
-	if (!config_ || config_->empty()) {
-		LOG(HAL, Error) << "Failed to generate camera configuration";
-		return -EINVAL;
-	}
+		streamConfiguration.size.width = stream->width;
+		streamConfiguration.size.height = stream->height;
+		streamConfiguration.pixelFormat = format;
 
-	StreamConfiguration *streamConfiguration = &config_->at(0);
-	streamConfiguration->size.width = camera3Stream->width;
-	streamConfiguration->size.height = camera3Stream->height;
-	streamConfiguration->pixelFormat = format;
+		config_->addConfiguration(streamConfiguration);
+	}
 
 	switch (config_->validate()) {
 	case CameraConfiguration::Valid:
@@ -996,7 +989,13 @@  int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
 		return -EINVAL;
 	}
 
-	camera3Stream->max_buffers = streamConfiguration->bufferCount;
+	for (unsigned int i = 0; i < stream_list->num_streams; ++i) {
+		camera3_stream_t *stream = stream_list->streams[i];
+		StreamConfiguration &streamConfiguration = config_->at(i);
+
+		/* Use the bufferCount confirmed by the validation process. */
+		stream->max_buffers = streamConfiguration.bufferCount;
+	}
 
 	/*
 	 * Once the CameraConfiguration has been adjusted/validated