[libcamera-devel] [PATCH 3/6] android: jpeg: exif: Fix setOrientation EXIF values

[Laurent Pinchart]

Hi Paul,

On Tue, Jan 19, 2021 at 05:10:36PM +0900, paul.elder at ideasonboard.com wrote:
> On Fri, Jan 15, 2021 at 07:27:54PM +0200, Laurent Pinchart wrote:
> > On Thu, Jan 14, 2021 at 07:40:32PM +0900, Paul Elder wrote:
> > > The input to setOrientation is angle clockwise from 12 o'clock, while
> > > the EXIF output values were swapped for 90 and 270 degrees.
> > > 
> > > From the EXIF spec:
> > > 
> > > 6 = The 0th row is the visual right-hand side of the image, and the
> > >     0th column is the visual top.
> > > 8 = The 0th row is the visual left-hand side of the image, and the
> > >     0th column is the visual bottom.
> > > 
> > > 6 should be 90 degrees clockwise, while 8 should 270 degrees clockwise.
> > > Fix this.
> > 
> > https://lists.libcamera.org/pipermail/libcamera-devel/2020-September/012647.html
> > 
> > The important part is that "Android defines the rotation as the
> > clockwise angle by which the image needs to be rotated to appear in the
> > correct orientation on the device screen". Could you read the thread and
> 
> I see that both android jpeg orientation and sensor orientation share
> this same definition.
> 
> In the tests, in verifyJpegExifExtraTags() (CameraTestUtils.java), the
> android result metadata JPEG orientation is compared with the EXIF
> orientation tag:
> 
> int exifOrientation = exif.getAttributeInt(ExifInterface.TAG_ORIENTATION, -1)
> ...
> int requestedOrientation = result.get(CaptureResult.JPEG_ORIENTATION);
> ...
> collector.expectEquals("Exif orientaiton should match requested orientation",
> 	requestedOrientation, getExifOrientationInDegree(exifOrientation,
> 	collector));
> 
> In getExifOrientationInDegree() (CameraTestUtils.java), there's a nice
> switch-case on exifOrientation:
> 
> case ExifInterface.ORIENTATION_NORMAL:
> 	return 0;
> case ExifInterface.ORIENTATION_ROTATE_90:
> 	return 90;
> case ExifInterface.ORIENTATION_ROTATE_180:
> 	return 180;
> case ExifInterface.ORIENTATION_ROTATE_270:
> 	return 270;
> 
> And the definitions of ExifInterface.ORIENTATION_ROTATE_{90,270}
> according to [1]:
> 
> ORIENTATION_ROTATE_90
> Constant Value: 6 (0x00000006) 
> 
> ORIENTATION_ROTATE_270
> Constant Value: 8 (0x00000008) 
> 
> Which matches up with the test results, which prompted this patch.
> 
> [1] https://developer.android.com/reference/android/media/ExifInterface
> 
> > let me know if you still think the current implementation is incorrect ?
> 
> So according to the android properties documentation on jpeg and sensor
> orientation, the current implementation should be correct, but CTS
> disagrees.

As discussed on IRC, this patch is correct, and the original code is
correct too :-) This patch is needed because of another bug, which you
fix by replacing the camera device orientation with the
android.jpeg.orientation value taken from the request to set the Exif
orientation.

With an updated commit message to explain this,

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

> > Does this patch fix an issue you've noticed, or does it stem from
> > reading the code only ?
> > 
> > > Sogned-off-by: Paul Elder <paul.elder at ideasonboard.com>
> > 
> > Sogned-off-by ?
> 
> Whoops :p
> 
> > > ---
> > >  src/android/jpeg/exif.cpp | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/src/android/jpeg/exif.cpp b/src/android/jpeg/exif.cpp
> > > index 33b3fa7f..b19cb4cd 100644
> > > --- a/src/android/jpeg/exif.cpp
> > > +++ b/src/android/jpeg/exif.cpp
> > > @@ -263,13 +263,13 @@ void Exif::setOrientation(int orientation)
> > >  		value = 1;
> > >  		break;
> > >  	case 90:
> > > -		value = 8;
> > > +		value = 6;
> > >  		break;
> > >  	case 180:
> > >  		value = 3;
> > >  		break;
> > >  	case 270:
> > > -		value = 6;
> > > +		value = 8;
> > >  		break;
> > >  	}
> > >  

-- 
Regards,

Laurent Pinchart