[libcamera-devel] [PATCH] android: camera_device: Report supported JPEG sizes

[Kieran Bingham]

Hi Jacopo,

On 05/08/2020 22:33, Jacopo Mondi wrote:
> Hi Laurent,
>    +Tomasz, Hiro and Han-Lin as there's a cros question at the end :)
>    +Kieran for the Compressor interface part
> 
> On Wed, Aug 05, 2020 at 07:53:41PM +0300, Laurent Pinchart wrote:
>> Hi Jacopo,
>>
>> On Wed, Aug 05, 2020 at 05:37:45PM +0200, Jacopo Mondi wrote:
>>> When producing the list of image resolution to claim as supported by the
>>> camera HAL, the JPEG stream was assumed to be 'always valid' as, at the
>>> time, there was no JPEG support in place at all.
>>>
>>> With the introduction of support for JPEG compression, reporting
>>> non-valid sizes as supported obviously causes troubles.
>>>
>>> In order to avoid reporting non-supported resolutions as supported,
>>> produce the list of available JPEG sizes by using the ones supported
>>> by the YCbCr_420_888 format, from which the JPEG stream is encoded.
>>>
>>> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
>>> ---
>>>
>>> Patch to be applied on top of Kieran's JPEG work.
>>> ---
>>>  src/android/camera_device.cpp | 38 +++++++++++++++++++++++++----------
>>>  1 file changed, 27 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
>>> index ec8ca934842a..6a9a038a2b53 100644
>>> --- a/src/android/camera_device.cpp
>>> +++ b/src/android/camera_device.cpp
>>> @@ -398,27 +398,43 @@ int CameraDevice::initializeStreamConfigurations()
>>>  		 */
>>>  		formatsMap_[androidFormat] = mappedFormat;
>>>
>>> +		/*
>>> +		 * Stop here for JPEG streams: the JPEG supported sizes will
>>> +		 * be tested later using the here recorded non-blob stream sizes.
>>> +		 */
>>> +		if (androidFormat == HAL_PIXEL_FORMAT_BLOB)
>>> +			continue;
>>> +
>>>  		for (const Size &res : cameraResolutions) {
>>>  			cfg.pixelFormat = mappedFormat;
>>>  			cfg.size = res;
>>>
>>> -			CameraConfiguration::Status status = cameraConfig->validate();
>>> -			/*
>>> -			 * Unconditionally report we can produce JPEG.
>>> -			 *
>>> -			 * \todo The JPEG stream will be implemented as an
>>> -			 * HAL-only stream, but some cameras can produce it
>>> -			 * directly. As of now, claim support for JPEG without
>>> -			 * inspecting where the JPEG stream is produced.
>>> -			 */
>>> -			if (androidFormat != HAL_PIXEL_FORMAT_BLOB &&
>>> -			    status != CameraConfiguration::Valid)
>>> +			if (cameraConfig->validate() != CameraConfiguration::Valid)
>>>  				continue;
>>>
>>>  			streamConfigurations_.push_back({ res, androidFormat });
>>>  		}
>>>  	}
>>>
>>> +	/*
>>> +	 * Insert the JPEG sizes by using the ones recorded for YUV streams
>>> +	 * from which JPEG is produced.
>>> +	 */
>>> +	std::vector<Camera3StreamConfiguration> jpegConfigurations;
>>> +	jpegConfigurations.reserve(cameraResolutions.size());
>>> +
>>> +	for (const auto &config : streamConfigurations_) {
>>> +		/* \todo JPEG can be produced from other formats too! */
>>
>> Another todo item, the android.scaler.availableStreamConfigurations
>> documentation lists required resolutions (see
>> https://android.googlesource.com/platform/system/media/+/refs/heads/master/camera/docs/docs.html
>> which very annoyingly googlesource.com can't display as html in a web browser).
>>
>> JPEG 	android.sensor.info.activeArraySize 	Any
>> JPEG 	1920x1080 (1080p) 			Any 	if 1080p <= activeArraySize
>> JPEG 	1280x720 (720) 				Any 	if 720p <= activeArraySize
>> JPEG 	640x480 (480p) 				Any 	if 480p <= activeArraySize
>> JPEG 	320x240 (240p) 				Any 	if 240p <= activeArraySize
>>
> 
> Those are already mandatory to be supported if I'm not mistaken.
> 
> we feed cameraResolutions for all the non-jpeg formats here:
> 
> 		for (const Size &res : cameraResolutions) {
> 			cfg.pixelFormat = mappedFormat;
> 			cfg.size = res;
> 
> 			if (cameraConfig->validate() != CameraConfiguration::Valid)
> 				continue;
> 
> 			streamConfigurations_.push_back({ res, androidFormat });
> 		}
> 
> Then we use the sizes associated with the mandatory
> HAL_PIXEL_FORMAT_YCbCr_420_888 to add entries for jpeg. Have I missed
> something on this part ?
> 
> Looking at the code now, we could create an initJpeg() functions, as
> all the jpeg streams are basically just ignored now and
> initialized at the end of the function.
> 
> Maybe it's really time to break this file apart, it's already quite
> big and terse. What if we delegate the jpeg initalization part to the
> Compressor interface (looking at it now, we could have gone for
> Encoder as a name maybe).

I certainly agree we need to split camera_device up.

But for me, I would instead consider that the compressor(/encoder)
shouldn't know about anything except doing that job.

Management of the stream should be handled by a CameraStream interface
(i.e. the struct CameraStream promoted to a full class, with it's own
camera_stream.cpp).

That's the layer that should be dealing with the interface between
android streams and libcamera streams IMO. It should then be up to any
instance of a CameraStream to decide if and how to handle the
requirements for that stream, which may be to use a libcamera stream, or
to use/implement a more configurable stream which might include
scaler/convertor/compressors as required.



As for using the name Encoder, yes that's fine by me.

I'll rename throughout my series now, I don't yet know if that will
require a repost, but I had hoped to push already.

I'll see if it warrants a v5 posting to the list or not depending on the
churn.

--
Kieran



> 
> As of now it would basically require two
> more methods: a way to get the a metadata pack with the values of
> ANDROID_JPEG_AVAILABLE_THUMBNAIL_SIZES and ANDROID_JPEG_MAX_SIZE
> initalized, and a vector of supported sizes given the vector of the
> ones so far collected.
> 
> Then we'll probably need a factory to create the right Compressor, but
> that's for later and we should pobably first discuss how to decide
> "which" encoder to initialize before getting to the "how".
> 
> In example, how does it work today in CrOS and what are the
> requirements going forward ?
> 
> Thanks
>   j
> 
>>> +		if (config.androidFormat != HAL_PIXEL_FORMAT_YCbCr_420_888)
>>> +			continue;
>>> +
>>> +		jpegConfigurations.push_back({ config.resolution,
>>> +					       HAL_PIXEL_FORMAT_BLOB });
>>> +	}
>>> +
>>> +	for (auto const jpegConfig : jpegConfigurations)
>>> +		streamConfigurations_.push_back(jpegConfig);
>>> +
>>>  	LOG(HAL, Debug) << "Collected stream configuration map: ";
>>>  	for (const auto &entry : streamConfigurations_)
>>>  		LOG(HAL, Debug) << "{ " << entry.resolution.toString() << " - "
>>
>> --
>> Regards,
>>
>> Laurent Pinchart