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

[Kieran Bingham]

Hi Jacopo,

On 03/07/2020 10:40, Jacopo Mondi wrote:
> Hi Kieran
> 
> On Fri, Jul 03, 2020 at 10:19:58AM +0100, Kieran Bingham wrote:
>> 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>)
>>
> 
> All good, it was an honest question as the mapping is something we
> implement, not something provided by the framework, and initially the
> printout was meant to indentify what we receive

I understand, I hope the () help identify that distinction.

I feel it's very useful to know at the point we print all the requested
streams what format /we/ interpret that to be though.

I'd probably like to see the 0x20203859 converted to the relevant
android string too, but still keep the libcamera conversion (we have to
convert it anyway) so we know if we were able to map successfully or not.

Still, that can happen later if we need to go that far.


> 
>> --
>> 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

-- 
Regards
--
Kieran