[libcamera-devel] [PATCH] android: exif: Contain IMAGE_WIDTH and IMAGE_LENGTH data

[Hirokazu Honda]

HI Laurent,
thanks for commenting.

On Sat, Apr 3, 2021 at 10:48 AM Laurent Pinchart
<laurent.pinchart at 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 at 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