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

[Jacopo Mondi]

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

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