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

[Laurent Pinchart]

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.

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);
> > > >   }

-- 
Regards,

Laurent Pinchart