[libcamera-devel] [PATCH 4/4] android: camera_device: support multiple stream configurations

[Laurent Pinchart]

Hi Kieran,

On Tue, Jun 30, 2020 at 03:20:40PM +0100, Kieran Bingham wrote:
> On 29/06/2020 21:29, Laurent Pinchart wrote:
> > On Mon, Jun 29, 2020 at 05:39:16PM +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 at ideasonboard.com>
> >> ---
> >>  src/android/camera_device.cpp | 72 ++++++++++++++++++-----------------
> >>  1 file changed, 38 insertions(+), 34 deletions(-)
> >>
> >> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> >> index a6410f42fee8..568ba28067d0 100644
> >> --- a/src/android/camera_device.cpp
> >> +++ b/src/android/camera_device.cpp
> >> @@ -956,48 +956,52 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
> >>  			       << " (" << 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];
> >> -
> >> -	/* Translate Android format code to libcamera pixel format. */
> >> -	PixelFormat format = toPixelFormat(camera3Stream->format);
> >> -	if (!format.isValid())
> >> -		return -EINVAL;
> >> -
> >>  	/*
> >> -	 * Hardcode viewfinder role, replacing the generated configuration
> >> -	 * parameters with the ones requested by the Android framework.
> >> +	 * Generate an empty configuration, and construct a StreamConfiguration
> >> +	 * for each camera3_stream to add to it.
> >>  	 */
> >> -	StreamRoles roles = { StreamRole::Viewfinder };
> >> -	config_ = camera_->generateConfiguration(roles);
> >> -	if (!config_ || config_->empty()) {
> >> +	config_ = camera_->generateConfiguration();
> >> +	if (!config_) {
> >>  		LOG(HAL, Error) << "Failed to generate camera configuration";
> >>  		return -EINVAL;
> >>  	}
> > 
> > This should really really not fail, we could possibly drop the check (it
> > should be caught by pipeline handler tests, albeit we don't have them
> > yet).
> 
> Indeed, it shouldn't. I'm not adding a check here though, but I agree it
> could be removed if all the pipelines were guaranteed to successfully
> generate the configuration.
> 
> Perhaps an assert for developer time checks, which could be compiled out
> at release builds.

A LOG(Fatal) then ? :-)

> >>  
> >> -	StreamConfiguration *streamConfiguration = &config_->at(0);
> >> -	streamConfiguration->size.width = camera3Stream->width;
> >> -	streamConfiguration->size.height = camera3Stream->height;
> >> -	streamConfiguration->pixelFormat = format;
> >> +	for (unsigned int i = 0; i < stream_list->num_streams; ++i) {
> >> +		camera3_stream_t *stream = stream_list->streams[i];
> >>  
> >> -	switch (config_->validate()) {
> >> -	case CameraConfiguration::Valid:
> >> -		break;
> >> -	case CameraConfiguration::Adjusted:
> >> -		LOG(HAL, Info) << "Camera configuration adjusted";
> >> -		config_.reset();
> >> -		return -EINVAL;
> >> -	case CameraConfiguration::Invalid:
> >> -		LOG(HAL, Info) << "Camera configuration invalid";
> >> -		config_.reset();
> >> -		return -EINVAL;
> >> -	}
> >> +		PixelFormat format = toPixelFormat(stream->format);
> >> +		if (!format.isValid())
> >> +			return -EINVAL;
> >> +
> >> +		StreamConfiguration streamConfiguration;
> >> +
> >> +		streamConfiguration.size.width = stream->width;
> >> +		streamConfiguration.size.height = stream->height;
> >> +		streamConfiguration.pixelFormat = format;
> >> +
> >> +		StreamConfiguration &cfg = config_->addConfiguration(streamConfiguration);
> >> +
> >> +		/* 'cfg' can be modifed during the validation process */
> >> +		CameraConfiguration::Status status = config_->validate();
> > 
> > Is there really a need to validate at each step instead of at the end ?
> 
> Well I needed to be able to populate the stream->max_buffers from the
> cfg.bufferCount below.
> 
> But I do agree the validate should happen with all streams computed.
> It will require a second loop, and being able to map back the stream
> configurations to the camera3Stream - but we need that anyway, so I'll
> keep trying to fix that up.

For this patch you could just iterate over the camera3Stream again, the
StreamConfiguration instances in CameraConfiguration are stored in the
same order. Long term we'll need to map back from Stream to
camera3Stream, so a std::map (or something else) may already be useful.

> > If it's for debugging purpose, you could print the whole camera
> > configuration when the validation fails, so it van be compated with the
> > requested stream configurations that are printed earlier. Validation is
> > a potentially expensive operation (involving V4L2 ioctl calls, which,
> > for UVC for instance, even involve interactions with the hardware), so
> > a loop to create the camera configuration followed by a single
> > validation would be best I think. You then wouldn't need patch 2/4 (I'd
> > like to keep changes to CameraConfiguration and StreamConfiguration
> > minimal until we rework them, and I'm not sure the code above that
> > passes a StreamConfiguration to addConfiguration() to get another
> > StreamConfiguration out is any less confusing than what we have today).
> 
> We're lacking a clear way of mapping a StreamConfiguration back to the
> camera3stream. But I'll work on a way to map this correctly.
> 
> >> +
> >> +		LOG(HAL, Info) << "Stream #" << i << " (validated): " << cfg.toString();
> > 
> > Let's try to not make the log too verbose in normal cases.
> 
> That's probably debug while I'm developing ;-)
> 
> Thanks for the feedback, Particularly on a half baked early patch.
> 
> >>  
> >> -	camera3Stream->max_buffers = streamConfiguration->bufferCount;
> >> +		switch (status) {
> >> +		case CameraConfiguration::Valid:
> >> +			break;
> >> +		case CameraConfiguration::Adjusted:
> >> +			LOG(HAL, Info) << "Camera configuration adjusted";
> >> +			config_.reset();
> >> +			return -EINVAL;
> >> +		case CameraConfiguration::Invalid:
> >> +			LOG(HAL, Info) << "Camera configuration invalid";
> >> +			config_.reset();
> >> +			return -EINVAL;
> >> +		}
> >> +
> >> +		/* Use the bufferCount confirmed by the validation process. */
> >> +		stream->max_buffers = cfg.bufferCount;
> >> +	}
> >>  
> >>  	/*
> >>  	 * Once the CameraConfiguration has been adjusted/validated

-- 
Regards,

Laurent Pinchart