[libcamera-devel] android: camera_device: Fix exposure time tag in exif and android

11044 diff mbox series

The exposure time libcamera control is in microseconds while android and
our exif component use nanoseconds. Convert it appropriately.

CTS also expects the exposure time exif tag to match the exposure time
set in the android result metadata. Fix it.

Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
---
 src/android/camera_device.cpp            | 2 +-
 src/android/jpeg/post_processor_jpeg.cpp | 3 ++-
 2 files changed, 3 insertions(+), 2 deletions(-)

Hi Paul,

On Thu, Jan 28, 2021 at 07:22:14PM +0900, Paul Elder wrote:
> The exposure time libcamera control is in microseconds while android and
> our exif component use nanoseconds. Convert it appropriately.

My bad! Sorry about overlooking this
(you can write ExposureTime as it's the actual control name)

>
> CTS also expects the exposure time exif tag to match the exposure time
> set in the android result metadata. Fix it.

Correct, looks like a separate patch though, but ok...

>
> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> ---
>  src/android/camera_device.cpp            | 2 +-
>  src/android/jpeg/post_processor_jpeg.cpp | 3 ++-
>  2 files changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> index f5066709..36382941 100644
> --- a/src/android/camera_device.cpp
> +++ b/src/android/camera_device.cpp
> @@ -2058,7 +2058,7 @@ CameraDevice::getResultMetadata(Camera3RequestDescriptor *descriptor,
>  	}
>
>  	if (metadata.contains(controls::ExposureTime)) {
> -		int32_t exposure = metadata.get(controls::ExposureTime);
> +		int64_t exposure = metadata.get(controls::ExposureTime) * 1000;
>  		resultMetadata->addEntry(ANDROID_SENSOR_EXPOSURE_TIME,
>  	}
> diff --git a/src/android/jpeg/post_processor_jpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp
> index cac0087b..6250b050 100644
> --- a/src/android/jpeg/post_processor_jpeg.cpp
> +++ b/src/android/jpeg/post_processor_jpeg.cpp
> @@ -113,7 +113,8 @@ int PostProcessorJpeg::process(const FrameBuffer &source,
>  	exif.setTimestamp(std::time(nullptr), 0ms);
>
>  	/* \todo Get this information from libcamera::Request::metadata */
> -	exif.setExposureTime(0);
> +	ret = resultMetadata->getEntry(ANDROID_SENSOR_EXPOSURE_TIME, &entry);
> +	exif.setExposureTime(ret ? *entry.data.i64 : 0);

If I'm not mistaken, this is the first getEntry() we call on the
result metadata, that has to be populated already by out
getResultMetadata().

If think it's fine, we need a single pass to convert libcamera
controls to Android format, and indeed the EXIF tags should reflect
the Android expected format.

Could you remove the \todo then ?

The patch content looks good
Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>

Thanks
  j

>  	ret = requestMetadata.getEntry(ANDROID_LENS_APERTURE, &entry);
>  	if (ret)
>  		exif.setAperture(*entry.data.f);
> --
> 2.27.0
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

Hi Paul,

Thank you for the patch.

On Thu, Jan 28, 2021 at 07:22:14PM +0900, Paul Elder wrote:
> The exposure time libcamera control is in microseconds while android and
> our exif component use nanoseconds. Convert it appropriately.
> 
> CTS also expects the exposure time exif tag to match the exposure time
> set in the android result metadata. Fix it.
> 
> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> ---
>  src/android/camera_device.cpp            | 2 +-
>  src/android/jpeg/post_processor_jpeg.cpp | 3 ++-
>  2 files changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> index f5066709..36382941 100644
> --- a/src/android/camera_device.cpp
> +++ b/src/android/camera_device.cpp
> @@ -2058,7 +2058,7 @@ CameraDevice::getResultMetadata(Camera3RequestDescriptor *descriptor,
>  	}
>  
>  	if (metadata.contains(controls::ExposureTime)) {
> -		int32_t exposure = metadata.get(controls::ExposureTime);
> +		int64_t exposure = metadata.get(controls::ExposureTime) * 1000;

The calculation will overflow for long exposure times.

		int64_t exposure = metadata.get(controls::ExposureTime) * 1000ULL;

After addressing Jacopo's comments,

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

>  		resultMetadata->addEntry(ANDROID_SENSOR_EXPOSURE_TIME,
>  					 &exposure, 1);
>  	}
> diff --git a/src/android/jpeg/post_processor_jpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp
> index cac0087b..6250b050 100644
> --- a/src/android/jpeg/post_processor_jpeg.cpp
> +++ b/src/android/jpeg/post_processor_jpeg.cpp
> @@ -113,7 +113,8 @@ int PostProcessorJpeg::process(const FrameBuffer &source,
>  	exif.setTimestamp(std::time(nullptr), 0ms);
>  
>  	/* \todo Get this information from libcamera::Request::metadata */
> -	exif.setExposureTime(0);
> +	ret = resultMetadata->getEntry(ANDROID_SENSOR_EXPOSURE_TIME, &entry);
> +	exif.setExposureTime(ret ? *entry.data.i64 : 0);
>  	ret = requestMetadata.getEntry(ANDROID_LENS_APERTURE, &entry);
>  	if (ret)
>  		exif.setAperture(*entry.data.f);