[libcamera-devel,v3] android: jpeg: get ISO from SENSOR_SENSITIVITY
diff mbox series

Message ID 20210617045758.1432766-1-paul.elder@ideasonboard.com
State Superseded
Delegated to: Paul Elder
Headers show
Series
  • [libcamera-devel,v3] android: jpeg: get ISO from SENSOR_SENSITIVITY
Related show

Commit Message

Paul Elder June 17, 2021, 4:57 a.m. UTC 
The data for the exif ISO tag needs to come from SENSOR_SENSITIVITY. Set
it.

Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

---
This on its own doesn't fix any CTS tests, but it prevents a test
failure later on when we add the proper static metadata for the FULL
hardware level.
---
 src/android/jpeg/post_processor_jpeg.cpp | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

Comments

Hirokazu Honda June 17, 2021, 5:31 a.m. UTC | #1 
HI Paul, thank you for the patch.

On Thu, Jun 17, 2021 at 1:58 PM Paul Elder <paul.elder@ideasonboard.com>
wrote:

> The data for the exif ISO tag needs to come from SENSOR_SENSITIVITY. Set
> it.
>
> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>
> ---
> This on its own doesn't fix any CTS tests, but it prevents a test
> failure later on when we add the proper static metadata for the FULL
> hardware level.
> ---
>  src/android/jpeg/post_processor_jpeg.cpp | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/src/android/jpeg/post_processor_jpeg.cpp
> b/src/android/jpeg/post_processor_jpeg.cpp
> index 058ccc99..921ac823 100644
> --- a/src/android/jpeg/post_processor_jpeg.cpp
> +++ b/src/android/jpeg/post_processor_jpeg.cpp
> @@ -119,7 +119,10 @@ int PostProcessorJpeg::process(const FrameBuffer
> &source,
>         ret = requestMetadata.getEntry(ANDROID_LENS_APERTURE, &entry);
>         if (ret)
>                 exif.setAperture(*entry.data.f);
> -       exif.setISO(100);
> +
> +       ret = resultMetadata->getEntry(ANDROID_SENSOR_SENSITIVITY, &entry);
> +       exif.setISO(ret ? *entry.data.i32 : 100);
>

Noob: where does 100 come from?

Reviewed-by: Hirokazu Honda <hiroh@chromium.org>

> +
>         exif.setFlash(Exif::Flash::FlashNotPresent);
>         exif.setWhiteBalance(Exif::WhiteBalance::Auto);
>
> --
> 2.27.0
>
>
Paul Elder June 17, 2021, 5:44 a.m. UTC | #2 
Hi Hiro,

Thank you for the review.

On Thu, Jun 17, 2021 at 02:31:11PM +0900, Hirokazu Honda wrote:
> HI Paul, thank you for the patch.
> 
> On Thu, Jun 17, 2021 at 1:58 PM Paul Elder <paul.elder@ideasonboard.com> wrote:
> 
>     The data for the exif ISO tag needs to come from SENSOR_SENSITIVITY. Set
>     it.
> 
>     Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
>     Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
>     ---
>     This on its own doesn't fix any CTS tests, but it prevents a test
>     failure later on when we add the proper static metadata for the FULL
>     hardware level.
>     ---
>      src/android/jpeg/post_processor_jpeg.cpp | 5 ++++-
>      1 file changed, 4 insertions(+), 1 deletion(-)
> 
>     diff --git a/src/android/jpeg/post_processor_jpeg.cpp b/src/android/jpeg/
>     post_processor_jpeg.cpp
>     index 058ccc99..921ac823 100644
>     --- a/src/android/jpeg/post_processor_jpeg.cpp
>     +++ b/src/android/jpeg/post_processor_jpeg.cpp
>     @@ -119,7 +119,10 @@ int PostProcessorJpeg::process(const FrameBuffer &
>     source,
>             ret = requestMetadata.getEntry(ANDROID_LENS_APERTURE, &entry);
>             if (ret)
>                     exif.setAperture(*entry.data.f);
>     -       exif.setISO(100);
>     +
>     +       ret = resultMetadata->getEntry(ANDROID_SENSOR_SENSITIVITY, &entry);
>     +       exif.setISO(ret ? *entry.data.i32 : 100);
> 
> 
> Noob: where does 100 come from?

It was an arbitrary number that passed CTS LIMITED :D


Paul

> 
> Reviewed-by: Hirokazu Honda <hiroh@chromium.org> 
> 
>     +
>             exif.setFlash(Exif::Flash::FlashNotPresent);
>             exif.setWhiteBalance(Exif::WhiteBalance::Auto);
> 
>     --
>     2.27.0
> 
>
Hirokazu Honda June 17, 2021, 5:47 a.m. UTC | #3 
Hi Paul,

On Thu, Jun 17, 2021 at 2:44 PM <paul.elder@ideasonboard.com> wrote:

> Hi Hiro,
>
> Thank you for the review.
>
> On Thu, Jun 17, 2021 at 02:31:11PM +0900, Hirokazu Honda wrote:
> > HI Paul, thank you for the patch.
> >
> > On Thu, Jun 17, 2021 at 1:58 PM Paul Elder <paul.elder@ideasonboard.com>
> wrote:
> >
> >     The data for the exif ISO tag needs to come from SENSOR_SENSITIVITY.
> Set
> >     it.
> >
> >     Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> >     Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >
> >     ---
> >     This on its own doesn't fix any CTS tests, but it prevents a test
> >     failure later on when we add the proper static metadata for the FULL
> >     hardware level.
> >     ---
> >      src/android/jpeg/post_processor_jpeg.cpp | 5 ++++-
> >      1 file changed, 4 insertions(+), 1 deletion(-)
> >
> >     diff --git a/src/android/jpeg/post_processor_jpeg.cpp
> b/src/android/jpeg/
> >     post_processor_jpeg.cpp
> >     index 058ccc99..921ac823 100644
> >     --- a/src/android/jpeg/post_processor_jpeg.cpp
> >     +++ b/src/android/jpeg/post_processor_jpeg.cpp
> >     @@ -119,7 +119,10 @@ int PostProcessorJpeg::process(const
> FrameBuffer &
> >     source,
> >             ret = requestMetadata.getEntry(ANDROID_LENS_APERTURE,
> &entry);
> >             if (ret)
> >                     exif.setAperture(*entry.data.f);
> >     -       exif.setISO(100);
> >     +
> >     +       ret = resultMetadata->getEntry(ANDROID_SENSOR_SENSITIVITY,
> &entry);
> >     +       exif.setISO(ret ? *entry.data.i32 : 100);
> >
> >
> > Noob: where does 100 come from?
>
> It was an arbitrary number that passed CTS LIMITED :D
>
>
I see.
Shall we add a comment about it?

Thanks,
-Hiro

>
> Paul
>
> >
> > Reviewed-by: Hirokazu Honda <hiroh@chromium.org>
> >
> >     +
> >             exif.setFlash(Exif::Flash::FlashNotPresent);
> >             exif.setWhiteBalance(Exif::WhiteBalance::Auto);
> >
> >     --
> >     2.27.0
> >
> >
>
Paul Elder June 17, 2021, 6:05 a.m. UTC | #4 
Hi Hiro,

On Thu, Jun 17, 2021 at 02:47:55PM +0900, Hirokazu Honda wrote:
> Hi Paul,
> 
> On Thu, Jun 17, 2021 at 2:44 PM <paul.elder@ideasonboard.com> wrote:
> 
>     Hi Hiro,
> 
>     Thank you for the review.
> 
>     On Thu, Jun 17, 2021 at 02:31:11PM +0900, Hirokazu Honda wrote:
>     > HI Paul, thank you for the patch.
>     >
>     > On Thu, Jun 17, 2021 at 1:58 PM Paul Elder <paul.elder@ideasonboard.com>
>     wrote:
>     >
>     >     The data for the exif ISO tag needs to come from SENSOR_SENSITIVITY.
>     Set
>     >     it.
>     >
>     >     Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
>     >     Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>     >
>     >     ---
>     >     This on its own doesn't fix any CTS tests, but it prevents a test
>     >     failure later on when we add the proper static metadata for the FULL
>     >     hardware level.
>     >     ---
>     >      src/android/jpeg/post_processor_jpeg.cpp | 5 ++++-
>     >      1 file changed, 4 insertions(+), 1 deletion(-)
>     >
>     >     diff --git a/src/android/jpeg/post_processor_jpeg.cpp b/src/android/
>     jpeg/
>     >     post_processor_jpeg.cpp
>     >     index 058ccc99..921ac823 100644
>     >     --- a/src/android/jpeg/post_processor_jpeg.cpp
>     >     +++ b/src/android/jpeg/post_processor_jpeg.cpp
>     >     @@ -119,7 +119,10 @@ int PostProcessorJpeg::process(const FrameBuffer
>     &
>     >     source,
>     >             ret = requestMetadata.getEntry(ANDROID_LENS_APERTURE, &
>     entry);
>     >             if (ret)
>     >                     exif.setAperture(*entry.data.f);
>     >     -       exif.setISO(100);
>     >     +
>     >     +       ret = resultMetadata->getEntry(ANDROID_SENSOR_SENSITIVITY, &
>     entry);
>     >     +       exif.setISO(ret ? *entry.data.i32 : 100);
>     >
>     >
>     > Noob: where does 100 come from?
> 
>     It was an arbitrary number that passed CTS LIMITED :D
> 
> 
> 
> I see.
> Shall we add a comment about it?

Yeah we probably should.

Also I checked the EXIF documentation [1] and it says this field is for
"Indicates the ISO Speed and ISO Latitude of the camera or input device
as specified in ISO 12232", so that doesn't seem to match with
SENSOR_SENSITIVITY [2]...

But also this allowed CTS FULL to pass when I tested it a long time
ago... I'll have to check again.

[1] https://www.exif.org/Exif2-2.PDF
[2] https://developer.android.com/reference/android/hardware/camera2/CaptureRequest#SENSOR_SENSITIVITY


Paul

> 
>     >
>     > Reviewed-by: Hirokazu Honda <hiroh@chromium.org> 
>     >
>     >     +
>     >             exif.setFlash(Exif::Flash::FlashNotPresent);
>     >             exif.setWhiteBalance(Exif::WhiteBalance::Auto);
>     >
>     >     --
>     >     2.27.0
>     >
>     >
>

Patch
diff mbox series 

diff --git a/src/android/jpeg/post_processor_jpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp
index 058ccc99..921ac823 100644
--- a/src/android/jpeg/post_processor_jpeg.cpp
+++ b/src/android/jpeg/post_processor_jpeg.cpp
@@ -119,7 +119,10 @@  int PostProcessorJpeg::process(const FrameBuffer &source,
 	ret = requestMetadata.getEntry(ANDROID_LENS_APERTURE, &entry);
 	if (ret)
 		exif.setAperture(*entry.data.f);
-	exif.setISO(100);
+
+	ret = resultMetadata->getEntry(ANDROID_SENSOR_SENSITIVITY, &entry);
+	exif.setISO(ret ? *entry.data.i32 : 100);
+
 	exif.setFlash(Exif::Flash::FlashNotPresent);
 	exif.setWhiteBalance(Exif::WhiteBalance::Auto);