[libcamera-devel] [PATCH 2/5] android: camera_device: Generate JPEG sizes

[Kieran Bingham]

Hi Jacopo,

On 02/09/2020 14:15, Jacopo Mondi wrote:
> Hi Kieran,
> 
> On Wed, Sep 02, 2020 at 01:55:49PM +0100, Kieran Bingham wrote:
>> Hi Jacopo,
>>
>> On 02/09/2020 11:47, 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>
>>> ---
>>>  src/android/camera_device.cpp | 43 +++++++++++++++++++++++------------
>>>  1 file changed, 28 insertions(+), 15 deletions(-)
>>>
>>> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
>>> index ad0d7fd15d90..8a8072123961 100644
>>> --- a/src/android/camera_device.cpp
>>> +++ b/src/android/camera_device.cpp
>>> @@ -363,17 +363,27 @@ int CameraDevice::initializeStreamConfigurations()
>>>  		const std::vector<PixelFormat> &libcameraFormats =
>>>  			camera3Format.libcameraFormats;
>>>
>>> +		/*
>>> +		 * Fixed format mapping for JPEG.
>>> +		 *
>>> +		 * The list of supported JPEG resolutions is generated
>>> +		 * from the list of resolutions supported by
>>> +		 * HAL_PIXEL_FORMAT_YCbCr_420_888 from which JPEG is produced.
>>> +		 *
>>> +		 * \todo Wire the JPEG encoder interface to query the list
>>> +		 * of supported resolutions.
>>
>> As we require NV12 for android, I think this is fine.
>> The encoder 'could' encode other formats, but it only needs to encode
>> NV12 - as there must always be a frame of that format right?
>>
>>
>> (I envisage we'll have to add a pixel-convertor to take YUYV webcams to
>> NV12, so even then, we'll still have an NV12 frame to encode).
>>
>>
>>> +		 */
>>> +		if (androidFormat == HAL_PIXEL_FORMAT_BLOB) {
>>> +			formatsMap_[androidFormat] = formats::MJPEG;
>>> +			continue;
>>> +		}
>>> +
>>>  		/*
>>>  		 * Test the libcamera formats that can produce images
>>>  		 * compatible with the format defined by Android.
>>>  		 */
>>>  		PixelFormat mappedFormat;
>>>  		for (const PixelFormat &pixelFormat : libcameraFormats) {
>>> -			/* \todo Fixed mapping for JPEG. */
>>> -			if (androidFormat == HAL_PIXEL_FORMAT_BLOB) {
>>> -				mappedFormat = formats::MJPEG;
>>> -				break;
>>> -			}
>>>
>>>  			/*
>>>  			 * The stream configuration size can be adjusted,
>>> @@ -416,19 +426,22 @@ int CameraDevice::initializeStreamConfigurations()
>>>  			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 (status != CameraConfiguration::Valid)
>>>  				continue;
>>>
>>>  			streamConfigurations_.push_back({ res, androidFormat });
>>> +
>>> +			/*
>>> +			 * If the format is HAL_PIXEL_FORMAT_YCbCr_420_888
>>> +			 * from which JPEG is produced, add an entry for
>>> +			 * the JPEG stream.
>>> +			 *
>>> +			 * \todo Wire the JPEG encoder to query the supported
>>> +			 * sizes provided a list of formats it can encode.
>>
>> I don't understand this comment.
>>
>> What do you need from the JPEG encoder?
>>
>> The supported sizes are whatever it's given.... It shouldn't change the
>> size.
> 
> Currently we create the encoder at configureStream() time, as we only
> have one. Going forward I suppose we'll have multiple classes
> implementing the (currently minimal) Encoder interface, as on some
> platforms we could have some HW accelerated implementations.
> 
> I also assumed other implementation might have requirements in terms
> of alignment, maximum and minimum supported sizes and so on, and
> supported formats the JPEG images can be produced from.
> 
> The idea here is to be able to instantiate Encoders early, or at least
> identify the one in use earlier, and query it to learn about the above
> mentioned constraints, which should be reflected in the available
> formats reported to the framework.
> 
> Does this make any sense to you ?


Yes, thanks that helps a lot - I was in the wrong mindset - of only
thinking about the existing software jpeg encoder.

Seems I missed this:

Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>

> 
> Thanks
>   j
> 
>>
>>> +			 */
>>> +			if (androidFormat == HAL_PIXEL_FORMAT_YCbCr_420_888)
>>> +				streamConfigurations_.push_back(
>>> +					{ res, HAL_PIXEL_FORMAT_BLOB });
>>>  		}
>>>  	}
>>>
>>>
>>
>> --
>> Regards
>> --
>> Kieran

-- 
Regards
--
Kieran