[libcamera-devel] android: exif: Contain IMAGE_WIDTH and IMAGE_LENGTH data
diff mbox series

Message ID 20210323080231.3389137-1-hiroh@chromium.org
State Deferred
Headers show
Series
  • [libcamera-devel] android: exif: Contain IMAGE_WIDTH and IMAGE_LENGTH data
Related show

Commit Message

Hirokazu Honda March 23, 2021, 8:02 a.m. UTC 
ChromeOS camera test checks if exif data has the IMAGE_WIDTH and
IMAGE_LENGTH and they are the same as the requested jpeg size.
This adds the resolution data to exif.

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

---
 src/android/jpeg/exif.cpp | 2 ++
 1 file changed, 2 insertions(+)

--
2.31.0.rc2.261.g7f71774620-goog

Comments

Umang Jain March 23, 2021, 9:04 a.m. UTC | #1 
Hi Hiro,

On 3/23/21 1:32 PM, Hirokazu Honda wrote:
> ChromeOS camera test checks if exif data has the IMAGE_WIDTH and
> IMAGE_LENGTH and they are the same as the requested jpeg size.
> This adds the resolution data to exif.
>
> Signed-off-by: Hirokazu Honda <hiroh@chromium.org>
>
> ---
>   src/android/jpeg/exif.cpp | 2 ++
>   1 file changed, 2 insertions(+)
>
> diff --git a/src/android/jpeg/exif.cpp b/src/android/jpeg/exif.cpp
> index 922086cd..29c7be0f 100644
> --- a/src/android/jpeg/exif.cpp
> +++ b/src/android/jpeg/exif.cpp
> @@ -286,6 +286,8 @@ void Exif::setModel(const std::string &model)
>
>   void Exif::setSize(const Size &size)
>   {
> +	setLong(EXIF_IFD_0, EXIF_TAG_IMAGE_LENGTH, size.height);
> +	setLong(EXIF_IFD_0, EXIF_TAG_IMAGE_WIDTH, size.width);
I am reading the EXIF spec and for ImageLength and ImageWidth, it states:

```
ImageWidth
The number of columns of image data, equal to the number of pixels per 
row. In JPEG compressed
data, this tag shall not be used because a JPEG marker is used instead 
of it.
```

Same for ImageLength.

We compress the image using JPEG post-processor right? Hence, I think 
these tags shouldn't be applicable (as per spec's point of view).

>   	setLong(EXIF_IFD_EXIF, EXIF_TAG_PIXEL_Y_DIMENSION, size.height);
>   	setLong(EXIF_IFD_EXIF, EXIF_TAG_PIXEL_X_DIMENSION, size.width);
>   }
> --
> 2.31.0.rc2.261.g7f71774620-goog
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
--
Regards,
Umang Jain
Hirokazu Honda March 25, 2021, 6:18 a.m. UTC | #2 
Hi Umang, thanks for reviewing,

On Tue, Mar 23, 2021 at 6:04 PM Umang Jain <umang.jain@ideasonboard.com> wrote:
>
> Hi Hiro,
>
> On 3/23/21 1:32 PM, Hirokazu Honda wrote:
> > ChromeOS camera test checks if exif data has the IMAGE_WIDTH and
> > IMAGE_LENGTH and they are the same as the requested jpeg size.
> > This adds the resolution data to exif.
> >
> > Signed-off-by: Hirokazu Honda <hiroh@chromium.org>
> >
> > ---
> >   src/android/jpeg/exif.cpp | 2 ++
> >   1 file changed, 2 insertions(+)
> >
> > diff --git a/src/android/jpeg/exif.cpp b/src/android/jpeg/exif.cpp
> > index 922086cd..29c7be0f 100644
> > --- a/src/android/jpeg/exif.cpp
> > +++ b/src/android/jpeg/exif.cpp
> > @@ -286,6 +286,8 @@ void Exif::setModel(const std::string &model)
> >
> >   void Exif::setSize(const Size &size)
> >   {
> > +     setLong(EXIF_IFD_0, EXIF_TAG_IMAGE_LENGTH, size.height);
> > +     setLong(EXIF_IFD_0, EXIF_TAG_IMAGE_WIDTH, size.width);
> I am reading the EXIF spec and for ImageLength and ImageWidth, it states:
>
> ```
> ImageWidth
> The number of columns of image data, equal to the number of pixels per
> row. In JPEG compressed
> data, this tag shall not be used because a JPEG marker is used instead
> of it.
> ```
>
> Same for ImageLength.
>
> We compress the image using JPEG post-processor right? Hence, I think
> these tags shouldn't be applicable (as per spec's point of view).
>

I think you're right.
Our test code [1] seems to come from Android CTS test code [2].
I am asking Android camera people if the check is correct.
I will update here their response.

[1] https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/platform2/camera/camera3_test/camera3_exif_validator.cc;l=337;drc=82fcae1960c0283214607107037c747c0e545617
[2] https://cs.android.com/android/platform/superproject/+/master:cts/tests/camera/utils/src/android/hardware/camera2/cts/CameraTestUtils.java;l=2669;drc=556fa09af46eeb2e8abe418d0f049c154bc4181a

Regards,
-Hiro

> >       setLong(EXIF_IFD_EXIF, EXIF_TAG_PIXEL_Y_DIMENSION, size.height);
> >       setLong(EXIF_IFD_EXIF, EXIF_TAG_PIXEL_X_DIMENSION, size.width);
> >   }
> > --
> > 2.31.0.rc2.261.g7f71774620-goog
> > _______________________________________________
> > libcamera-devel mailing list
> > libcamera-devel@lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel
> --
> Regards,
> Umang Jain
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
Hirokazu Honda April 1, 2021, 3:27 p.m. UTC | #3 
Hi,

On Thu, Mar 25, 2021 at 3:18 PM Hirokazu Honda <hiroh@chromium.org> wrote:
>
> Hi Umang, thanks for reviewing,
>
> On Tue, Mar 23, 2021 at 6:04 PM Umang Jain <umang.jain@ideasonboard.com> wrote:
> >
> > Hi Hiro,
> >
> > On 3/23/21 1:32 PM, Hirokazu Honda wrote:
> > > ChromeOS camera test checks if exif data has the IMAGE_WIDTH and
> > > IMAGE_LENGTH and they are the same as the requested jpeg size.
> > > This adds the resolution data to exif.
> > >
> > > Signed-off-by: Hirokazu Honda <hiroh@chromium.org>
> > >
> > > ---
> > >   src/android/jpeg/exif.cpp | 2 ++
> > >   1 file changed, 2 insertions(+)
> > >
> > > diff --git a/src/android/jpeg/exif.cpp b/src/android/jpeg/exif.cpp
> > > index 922086cd..29c7be0f 100644
> > > --- a/src/android/jpeg/exif.cpp
> > > +++ b/src/android/jpeg/exif.cpp
> > > @@ -286,6 +286,8 @@ void Exif::setModel(const std::string &model)
> > >
> > >   void Exif::setSize(const Size &size)
> > >   {
> > > +     setLong(EXIF_IFD_0, EXIF_TAG_IMAGE_LENGTH, size.height);
> > > +     setLong(EXIF_IFD_0, EXIF_TAG_IMAGE_WIDTH, size.width);
> > I am reading the EXIF spec and for ImageLength and ImageWidth, it states:
> >
> > ```
> > ImageWidth
> > The number of columns of image data, equal to the number of pixels per
> > row. In JPEG compressed
> > data, this tag shall not be used because a JPEG marker is used instead
> > of it.
> > ```
> >
> > Same for ImageLength.
> >
> > We compress the image using JPEG post-processor right? Hence, I think
> > these tags shouldn't be applicable (as per spec's point of view).
> >
>
> I think you're right.
> Our test code [1] seems to come from Android CTS test code [2].
> I am asking Android camera people if the check is correct.
> I will update here their response.
>
> [1] https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/platform2/camera/camera3_test/camera3_exif_validator.cc;l=337;drc=82fcae1960c0283214607107037c747c0e545617
> [2] https://cs.android.com/android/platform/superproject/+/master:cts/tests/camera/utils/src/android/hardware/camera2/cts/CameraTestUtils.java;l=2669;drc=556fa09af46eeb2e8abe418d0f049c154bc4181a
>

I found, if there is no exif tags of IMAGE_WIDTH and IMAGE_LENGTH,
Android sets them to ones of JPEG markers. [1]
It is because some CTS tests that checks the exif tags passed albeit
we don't set the tags.
I reached out Android developers about the tests and the code.
They agree the EXIF spec states these tags are not needed.
However, they said it was pretty common that these tags exist in pract
and their CTS tries to make sure common tags are included in EXIF.
So they are reluctant to change it and apps may be expecting them to
exif on JPEGS from Android devices now as the code exists since 2016.
FWIW, CDD doesn't include required EXIF tags unfortunately.

There are two solutions for us.
1.) Don't add these tags in libcamera
This is correct in terms of EXIF spec.
Even if we don't set the tags, they are set by Android to ones of JPEG markers.
The problem is that chromeos camera test fails due to the wrong check
[2]. We remove the checks on this solution.

2.) Add these tags in libcamera
Android and chromeos camera tests will pass.
Although this is wrong in terms of EXIF spec, it might be practical to
do this because some Android HAL clients (wrongly) expect the
behavior.

How do you think?

[1] https://cs.android.com/android/platform/superproject/+/master:frameworks/base/media/java/android/media/ExifInterface.java;l=2907
[2] https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/platform2/camera/camera3_test/camera3_exif_validator.cc;l=337;drc=82fcae1960c0283214607107037c747c0e545617

Best Regards,
-Hiro
> Regards,
> -Hiro
>
> > >       setLong(EXIF_IFD_EXIF, EXIF_TAG_PIXEL_Y_DIMENSION, size.height);
> > >       setLong(EXIF_IFD_EXIF, EXIF_TAG_PIXEL_X_DIMENSION, size.width);
> > >   }
> > > --
> > > 2.31.0.rc2.261.g7f71774620-goog
> > > _______________________________________________
> > > libcamera-devel mailing list
> > > libcamera-devel@lists.libcamera.org
> > > https://lists.libcamera.org/listinfo/libcamera-devel
> > --
> > Regards,
> > Umang Jain
> > _______________________________________________
> > libcamera-devel mailing list
> > libcamera-devel@lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel
Laurent Pinchart April 3, 2021, 1:47 a.m. UTC | #4 
Hi Hiro,

On Fri, Apr 02, 2021 at 12:27:05AM +0900, Hirokazu Honda wrote:
> On Thu, Mar 25, 2021 at 3:18 PM Hirokazu Honda wrote:
> > On Tue, Mar 23, 2021 at 6:04 PM Umang Jain wrote:
> > > On 3/23/21 1:32 PM, Hirokazu Honda wrote:
> > > > ChromeOS camera test checks if exif data has the IMAGE_WIDTH and
> > > > IMAGE_LENGTH and they are the same as the requested jpeg size.
> > > > This adds the resolution data to exif.
> > > >
> > > > Signed-off-by: Hirokazu Honda <hiroh@chromium.org>
> > > >
> > > > ---
> > > >   src/android/jpeg/exif.cpp | 2 ++
> > > >   1 file changed, 2 insertions(+)
> > > >
> > > > diff --git a/src/android/jpeg/exif.cpp b/src/android/jpeg/exif.cpp
> > > > index 922086cd..29c7be0f 100644
> > > > --- a/src/android/jpeg/exif.cpp
> > > > +++ b/src/android/jpeg/exif.cpp
> > > > @@ -286,6 +286,8 @@ void Exif::setModel(const std::string &model)
> > > >
> > > >   void Exif::setSize(const Size &size)
> > > >   {
> > > > +     setLong(EXIF_IFD_0, EXIF_TAG_IMAGE_LENGTH, size.height);
> > > > +     setLong(EXIF_IFD_0, EXIF_TAG_IMAGE_WIDTH, size.width);
> > > I am reading the EXIF spec and for ImageLength and ImageWidth, it states:
> > >
> > > ```
> > > ImageWidth
> > > The number of columns of image data, equal to the number of pixels per
> > > row. In JPEG compressed
> > > data, this tag shall not be used because a JPEG marker is used instead
> > > of it.
> > > ```
> > >
> > > Same for ImageLength.
> > >
> > > We compress the image using JPEG post-processor right? Hence, I think
> > > these tags shouldn't be applicable (as per spec's point of view).
> >
> > I think you're right.
> > Our test code [1] seems to come from Android CTS test code [2].
> > I am asking Android camera people if the check is correct.
> > I will update here their response.
> >
> > [1] https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/platform2/camera/camera3_test/camera3_exif_validator.cc;l=337;drc=82fcae1960c0283214607107037c747c0e545617
> > [2] https://cs.android.com/android/platform/superproject/+/master:cts/tests/camera/utils/src/android/hardware/camera2/cts/CameraTestUtils.java;l=2669;drc=556fa09af46eeb2e8abe418d0f049c154bc4181a
> 
> I found, if there is no exif tags of IMAGE_WIDTH and IMAGE_LENGTH,
> Android sets them to ones of JPEG markers. [1]
> It is because some CTS tests that checks the exif tags passed albeit
> we don't set the tags.
> I reached out Android developers about the tests and the code.
> They agree the EXIF spec states these tags are not needed.
> However, they said it was pretty common that these tags exist in pract

No wonder they're pretty common, if Android adds them erroneously :-)

> and their CTS tries to make sure common tags are included in EXIF.
> So they are reluctant to change it and apps may be expecting them to
> exif on JPEGS from Android devices now as the code exists since 2016.
> FWIW, CDD doesn't include required EXIF tags unfortunately.
> 
> There are two solutions for us.
> 1.) Don't add these tags in libcamera
> This is correct in terms of EXIF spec.
> Even if we don't set the tags, they are set by Android to ones of JPEG markers.
> The problem is that chromeos camera test fails due to the wrong check
> [2]. We remove the checks on this solution.
> 
> 2.) Add these tags in libcamera
> Android and chromeos camera tests will pass.
> Although this is wrong in terms of EXIF spec, it might be practical to
> do this because some Android HAL clients (wrongly) expect the
> behavior.
> 
> How do you think?

I really dislike when we're forced to do the wrong thing because someone
made a mistake and won't fix it :-(

Our HAL implementation is meant to support both Chrome OS and Android.
On Android, if the camera service adds the Exif tags, it doesn't matter
if we include them or not, applications will always see the same
(incorrect) behaviour with the tags included. I'd say that relying on
the Android camera service to do so is likely best: the incorrect
behaviour is an Android requirement due to bug-to-bug backward
compatibility, so letting the Android camera service handle it would
make sense. They could then decide to change the behaviour later (which
is unlikely though), without needing a modification in libcamera.

On Chrome OS, if there's no worry about possibly issues with
applications, I think it would be better to not include the tags, to be
compliant with the Exif specification, and dropping the corresponding
check in [2]. If there's a risk of breakage with existing applications,
however, that can be reconsidered.

One additional question is how widespread applications relying on those
tags are. If they're mostly related to Android, the above should hold
true. If non-Android applications have started including and/or
consuming those tags in JPEG images, then it's a different story as it
would mean that the whole world isn't compliant with the specification,
and it wouldn't make sense to be pedantic.

What do you think ?

> [1] https://cs.android.com/android/platform/superproject/+/master:frameworks/base/media/java/android/media/ExifInterface.java;l=2907
> [2] https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/platform2/camera/camera3_test/camera3_exif_validator.cc;l=337;drc=82fcae1960c0283214607107037c747c0e545617
> 
> > > >       setLong(EXIF_IFD_EXIF, EXIF_TAG_PIXEL_Y_DIMENSION, size.height);
> > > >       setLong(EXIF_IFD_EXIF, EXIF_TAG_PIXEL_X_DIMENSION, size.width);
> > > >   }
Hirokazu Honda April 3, 2021, 11:17 a.m. UTC | #5 
HI Laurent,
thanks for commenting.

On Sat, Apr 3, 2021 at 10:48 AM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Hiro,
>
> On Fri, Apr 02, 2021 at 12:27:05AM +0900, Hirokazu Honda wrote:
> > On Thu, Mar 25, 2021 at 3:18 PM Hirokazu Honda wrote:
> > > On Tue, Mar 23, 2021 at 6:04 PM Umang Jain wrote:
> > > > On 3/23/21 1:32 PM, Hirokazu Honda wrote:
> > > > > ChromeOS camera test checks if exif data has the IMAGE_WIDTH and
> > > > > IMAGE_LENGTH and they are the same as the requested jpeg size.
> > > > > This adds the resolution data to exif.
> > > > >
> > > > > Signed-off-by: Hirokazu Honda <hiroh@chromium.org>
> > > > >
> > > > > ---
> > > > >   src/android/jpeg/exif.cpp | 2 ++
> > > > >   1 file changed, 2 insertions(+)
> > > > >
> > > > > diff --git a/src/android/jpeg/exif.cpp b/src/android/jpeg/exif.cpp
> > > > > index 922086cd..29c7be0f 100644
> > > > > --- a/src/android/jpeg/exif.cpp
> > > > > +++ b/src/android/jpeg/exif.cpp
> > > > > @@ -286,6 +286,8 @@ void Exif::setModel(const std::string &model)
> > > > >
> > > > >   void Exif::setSize(const Size &size)
> > > > >   {
> > > > > +     setLong(EXIF_IFD_0, EXIF_TAG_IMAGE_LENGTH, size.height);
> > > > > +     setLong(EXIF_IFD_0, EXIF_TAG_IMAGE_WIDTH, size.width);
> > > > I am reading the EXIF spec and for ImageLength and ImageWidth, it states:
> > > >
> > > > ```
> > > > ImageWidth
> > > > The number of columns of image data, equal to the number of pixels per
> > > > row. In JPEG compressed
> > > > data, this tag shall not be used because a JPEG marker is used instead
> > > > of it.
> > > > ```
> > > >
> > > > Same for ImageLength.
> > > >
> > > > We compress the image using JPEG post-processor right? Hence, I think
> > > > these tags shouldn't be applicable (as per spec's point of view).
> > >
> > > I think you're right.
> > > Our test code [1] seems to come from Android CTS test code [2].
> > > I am asking Android camera people if the check is correct.
> > > I will update here their response.
> > >
> > > [1] https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/platform2/camera/camera3_test/camera3_exif_validator.cc;l=337;drc=82fcae1960c0283214607107037c747c0e545617
> > > [2] https://cs.android.com/android/platform/superproject/+/master:cts/tests/camera/utils/src/android/hardware/camera2/cts/CameraTestUtils.java;l=2669;drc=556fa09af46eeb2e8abe418d0f049c154bc4181a
> >
> > I found, if there is no exif tags of IMAGE_WIDTH and IMAGE_LENGTH,
> > Android sets them to ones of JPEG markers. [1]
> > It is because some CTS tests that checks the exif tags passed albeit
> > we don't set the tags.
> > I reached out Android developers about the tests and the code.
> > They agree the EXIF spec states these tags are not needed.
> > However, they said it was pretty common that these tags exist in pract
>
> No wonder they're pretty common, if Android adds them erroneously :-)
>
> > and their CTS tries to make sure common tags are included in EXIF.
> > So they are reluctant to change it and apps may be expecting them to
> > exif on JPEGS from Android devices now as the code exists since 2016.
> > FWIW, CDD doesn't include required EXIF tags unfortunately.
> >
> > There are two solutions for us.
> > 1.) Don't add these tags in libcamera
> > This is correct in terms of EXIF spec.
> > Even if we don't set the tags, they are set by Android to ones of JPEG markers.
> > The problem is that chromeos camera test fails due to the wrong check
> > [2]. We remove the checks on this solution.
> >
> > 2.) Add these tags in libcamera
> > Android and chromeos camera tests will pass.
> > Although this is wrong in terms of EXIF spec, it might be practical to
> > do this because some Android HAL clients (wrongly) expect the
> > behavior.
> >
> > How do you think?
>
> I really dislike when we're forced to do the wrong thing because someone
> made a mistake and won't fix it :-(
>
> Our HAL implementation is meant to support both Chrome OS and Android.
> On Android, if the camera service adds the Exif tags, it doesn't matter
> if we include them or not, applications will always see the same
> (incorrect) behaviour with the tags included. I'd say that relying on
> the Android camera service to do so is likely best: the incorrect
> behaviour is an Android requirement due to bug-to-bug backward
> compatibility, so letting the Android camera service handle it would
> make sense. They could then decide to change the behaviour later (which
> is unlikely though), without needing a modification in libcamera.
>
> On Chrome OS, if there's no worry about possibly issues with
> applications, I think it would be better to not include the tags, to be
> compliant with the Exif specification, and dropping the corresponding
> check in [2]. If there's a risk of breakage with existing applications,
> however, that can be reconsidered.
>

I understand your opinion. I think there is no issue if HAL doesn't
produce a JPEG image with the tags.
I am going to remove the check in the chromeos camera test.

> One additional question is how widespread applications relying on those
> tags are. If they're mostly related to Android, the above should hold
> true. If non-Android applications have started including and/or
> consuming those tags in JPEG images, then it's a different story as it
> would mean that the whole world isn't compliant with the specification,
> and it wouldn't make sense to be pedantic.
>
> What do you think ?
>

I have no idea to be honest.
If we get an issue with some apps, let's be re-back to this patch.
-Hiro

> > [1] https://cs.android.com/android/platform/superproject/+/master:frameworks/base/media/java/android/media/ExifInterface.java;l=2907
> > [2] https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/platform2/camera/camera3_test/camera3_exif_validator.cc;l=337;drc=82fcae1960c0283214607107037c747c0e545617
> >
> > > > >       setLong(EXIF_IFD_EXIF, EXIF_TAG_PIXEL_Y_DIMENSION, size.height);
> > > > >       setLong(EXIF_IFD_EXIF, EXIF_TAG_PIXEL_X_DIMENSION, size.width);
> > > > >   }
>
> --
> Regards,
>
> Laurent Pinchart

Patch
diff mbox series 

diff --git a/src/android/jpeg/exif.cpp b/src/android/jpeg/exif.cpp
index 922086cd..29c7be0f 100644
--- a/src/android/jpeg/exif.cpp
+++ b/src/android/jpeg/exif.cpp
@@ -286,6 +286,8 @@  void Exif::setModel(const std::string &model)

 void Exif::setSize(const Size &size)
 {
+	setLong(EXIF_IFD_0, EXIF_TAG_IMAGE_LENGTH, size.height);
+	setLong(EXIF_IFD_0, EXIF_TAG_IMAGE_WIDTH, size.width);
 	setLong(EXIF_IFD_EXIF, EXIF_TAG_PIXEL_Y_DIMENSION, size.height);
 	setLong(EXIF_IFD_EXIF, EXIF_TAG_PIXEL_X_DIMENSION, size.width);
 }