[libcamera-devel] android: camera_device: Only advertise RAW support if RAW16 is available

10792 diff mbox series

The HAL expects RAW16 support if the RAW capability is set, add a check
for this.

Suggested-by: Jacopo Mondi <jacopo@jmondi.org>
Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
---
 src/android/camera_device.cpp | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Hi Jacopo,

Thank you for the patch.

On Thu, Dec 31, 2020 at 11:02:18AM +0100, Niklas Söderlund wrote:
> The HAL expects RAW16 support if the RAW capability is set, add a check

Maybe "The Android camera service" instead of "The HAL" to emphasize
this is not an internal requirement of our implementation ? Or maybe we
should even expand the commit message to explain the requirement ? It
wasn't clear from the documentation (although re-reading it now, I
wonder why) and took us some time to figure it out, it's useful to
capture it. How about this ?

The Android camera2 API defines a RAW capture capability ([1]) for
devices that support "outputting RAW buffers and metadata for
interpreting them". This capability requires the camera device to
support RAW_SENSOR ([2]) as an output format. Despite what its name may
sound like, the RAW_SENSOR format is defined as a 16 bits RAW format,
not an opaque implementation-dependent format (which is instead called
RAW_PRIVATE). Devices may additionally support the RAW10 and RAW12
formats, but that isn't enough to claim RAW capture capability.

To comply with the API requirements, only report the
ANDROID_REQUEST_AVAILABLE_CAPABILITIES_RAW capability when 16-bit RAW is
supported.

[1] https://developer.android.com/reference/android/hardware/camera2/CameraMetadata#REQUEST_AVAILABLE_CAPABILITIES_RAW
[2] https://developer.android.com/reference/android/graphics/ImageFormat#RAW_SENSOR

> for this.
> 
> Suggested-by: Jacopo Mondi <jacopo@jmondi.org>
> Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>

Tested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  src/android/camera_device.cpp | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> index 7678d4485ce987b5..23be2a593a2aee5e 100644
> --- a/src/android/camera_device.cpp
> +++ b/src/android/camera_device.cpp
> @@ -1099,7 +1099,9 @@ const camera_metadata_t *CameraDevice::getStaticMetadata()
>  	if (cameraConfig && !cameraConfig->empty()) {
>  		const PixelFormatInfo &info =
>  			PixelFormatInfo::info(cameraConfig->at(0).pixelFormat);
> -		if (info.colourEncoding == PixelFormatInfo::ColourEncodingRAW) {
> +		/* Only advertise RAW support if RAW16 is possible. */
> +		if (info.colourEncoding == PixelFormatInfo::ColourEncodingRAW &&
> +		    info.bitsPerPixel == 16) {
>  			rawStreamAvailable = true;
>  			availableCapabilities.push_back(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_RAW);
>  		}

On Thu, Dec 31, 2020 at 05:16:49PM +0200, Laurent Pinchart wrote:
> Hi Jacopo,

Or rather Niklas. My fingers type Jacopo's name automatically when
replying to patches for the HAL :-)

> Thank you for the patch.
> 
> On Thu, Dec 31, 2020 at 11:02:18AM +0100, Niklas Söderlund wrote:
> > The HAL expects RAW16 support if the RAW capability is set, add a check
> 
> Maybe "The Android camera service" instead of "The HAL" to emphasize
> this is not an internal requirement of our implementation ? Or maybe we
> should even expand the commit message to explain the requirement ? It
> wasn't clear from the documentation (although re-reading it now, I
> wonder why) and took us some time to figure it out, it's useful to
> capture it. How about this ?
> 
> The Android camera2 API defines a RAW capture capability ([1]) for
> devices that support "outputting RAW buffers and metadata for
> interpreting them". This capability requires the camera device to
> support RAW_SENSOR ([2]) as an output format. Despite what its name may
> sound like, the RAW_SENSOR format is defined as a 16 bits RAW format,
> not an opaque implementation-dependent format (which is instead called
> RAW_PRIVATE). Devices may additionally support the RAW10 and RAW12
> formats, but that isn't enough to claim RAW capture capability.
> 
> To comply with the API requirements, only report the
> ANDROID_REQUEST_AVAILABLE_CAPABILITIES_RAW capability when 16-bit RAW is
> supported.
> 
> [1] https://developer.android.com/reference/android/hardware/camera2/CameraMetadata#REQUEST_AVAILABLE_CAPABILITIES_RAW
> [2] https://developer.android.com/reference/android/graphics/ImageFormat#RAW_SENSOR
> 
> > for this.
> > 
> > Suggested-by: Jacopo Mondi <jacopo@jmondi.org>
> > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> 
> Tested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> > ---
> >  src/android/camera_device.cpp | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> > index 7678d4485ce987b5..23be2a593a2aee5e 100644
> > --- a/src/android/camera_device.cpp
> > +++ b/src/android/camera_device.cpp
> > @@ -1099,7 +1099,9 @@ const camera_metadata_t *CameraDevice::getStaticMetadata()
> >  	if (cameraConfig && !cameraConfig->empty()) {
> >  		const PixelFormatInfo &info =
> >  			PixelFormatInfo::info(cameraConfig->at(0).pixelFormat);
> > -		if (info.colourEncoding == PixelFormatInfo::ColourEncodingRAW) {
> > +		/* Only advertise RAW support if RAW16 is possible. */
> > +		if (info.colourEncoding == PixelFormatInfo::ColourEncodingRAW &&
> > +		    info.bitsPerPixel == 16) {
> >  			rawStreamAvailable = true;
> >  			availableCapabilities.push_back(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_RAW);
> >  		}