[libcamera-devel] [PATCH v2 2/9] android: camera_device: Provide a toPixelFormat helper

[Kieran Bingham]

Hi Jacopo,


On 03/07/2020 10:04, Jacopo Mondi wrote:
> Sorry, one nit
> 
> On Thu, Jul 02, 2020 at 10:36:47PM +0100, Kieran Bingham wrote:
>> Rather than converting pixelformats through the map, and then
>> dereferencing the iterator later, create a helper to explicitly return a
>> PixelFormat type.
>>
>> Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
>> ---
>>  src/android/camera_device.cpp | 28 ++++++++++++++++++++--------
>>  src/android/camera_device.h   |  1 +
>>  2 files changed, 21 insertions(+), 8 deletions(-)
>>
>> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
>> index f788c11e7254..b9031ff0c2a4 100644
>> --- a/src/android/camera_device.cpp
>> +++ b/src/android/camera_device.cpp
>> @@ -923,6 +923,19 @@ const camera_metadata_t *CameraDevice::constructDefaultRequestSettings(int type)
>>  	return requestTemplate->get();
>>  }
>>
>> +PixelFormat CameraDevice::toPixelFormat(int format)
>> +{
>> +	/* Translate Android format code to libcamera pixel format. */
>> +	auto it = formatsMap_.find(format);
>> +	if (it == formatsMap_.end()) {
>> +		LOG(HAL, Error) << "Requested format " << utils::hex(format)
>> +				<< " not supported";
>> +		return PixelFormat();
>> +	}
>> +
>> +	return it->second;
>> +}
>> +
>>  /*
>>   * Inspect the stream_list to produce a list of StreamConfiguration to
>>   * be use to configure the Camera.
>> @@ -932,11 +945,14 @@ 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];
>>
>> +		PixelFormat format = toPixelFormat(stream->format);
>> +
>>  		LOG(HAL, Info) << "Stream #" << i
>>  			       << ", direction: " << stream->stream_type
>>  			       << ", width: " << stream->width
>>  			       << ", height: " << stream->height
>> -			       << ", format: " << utils::hex(stream->format);
>> +			       << ", format: " << utils::hex(stream->format)
>> +			       << " (" << format.toString() << ")";
> 
> Is it a good idea to report the mapped format when priting information
> about the android requested streams ? Just wondering...
> 

It certainly helps to know what the stream configurations are:

Without:

> <line-prefix-snip> Stream #0, direction: 0, width: 2560, height: 1920, format: 0x00000023
> <line-prefix-snip> Stream #1, direction: 0, width: 2560, height: 1920, format: 0x00000021

With:

> <line-prefix-snip> Stream #0, direction: 0, width: 2560, height: 1920, format: 0x00000023 (NV12)
> <line-prefix-snip> Stream #1, direction: 0, width: 2560, height: 1920, format: 0x00000021 (MJPEG)


Otherwise, we can add a toString() on the android formats of course,

But it also really helps in the cros-camera-tests too to see when a
format is requested that we don't yet handle:

> <line-prefix-snip> Stream #0, direction: 0, width: 320, height: 240, format: 0x20203859 (<INVALID>)

--
Kieran



>>  	}
>>
>>  	/* Only one stream is supported. */
>> @@ -947,13 +963,9 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
>>  	camera3_stream_t *camera3Stream = stream_list->streams[0];
>>
>>  	/* Translate Android format code to libcamera pixel format. */
>> -	auto it = formatsMap_.find(camera3Stream->format);
>> -	if (it == formatsMap_.end()) {
>> -		LOG(HAL, Error) << "Requested format "
>> -				<< utils::hex(camera3Stream->format)
>> -				<< " not supported";
>> +	PixelFormat format = toPixelFormat(camera3Stream->format);
>> +	if (!format.isValid())
>>  		return -EINVAL;
>> -	}
>>
>>  	/*
>>  	 * Hardcode viewfinder role, replacing the generated configuration
>> @@ -969,7 +981,7 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
>>  	StreamConfiguration *streamConfiguration = &config_->at(0);
>>  	streamConfiguration->size.width = camera3Stream->width;
>>  	streamConfiguration->size.height = camera3Stream->height;
>> -	streamConfiguration->pixelFormat = it->second;
>> +	streamConfiguration->pixelFormat = format;
>>
>>  	switch (config_->validate()) {
>>  	case CameraConfiguration::Valid:
>> diff --git a/src/android/camera_device.h b/src/android/camera_device.h
>> index ed11410a5577..5bd6cf416156 100644
>> --- a/src/android/camera_device.h
>> +++ b/src/android/camera_device.h
>> @@ -72,6 +72,7 @@ private:
>>  	std::tuple<uint32_t, uint32_t> calculateStaticMetadataSize();
>>  	void notifyShutter(uint32_t frameNumber, uint64_t timestamp);
>>  	void notifyError(uint32_t frameNumber, camera3_stream_t *stream);
>> +	libcamera::PixelFormat toPixelFormat(int format);
>>  	std::unique_ptr<CameraMetadata> getResultMetadata(int frame_number,
>>  							  int64_t timestamp);
>>
>> --
>> 2.25.1
>>
>> _______________________________________________
>> libcamera-devel mailing list
>> libcamera-devel at lists.libcamera.org
>> https://lists.libcamera.org/listinfo/libcamera-devel

-- 
Regards
--
Kieran