| Message ID | 20210114104035.302968-4-paul.elder@ideasonboard.com |
|---|---|
| State | Superseded |
| Delegated to: | Paul Elder |
| Headers | show |
| Series |
|
| Related | show |
Hi Paul, Thank you for the patch. 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 let me know if you still think the current implementation is incorrect ? 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@ideasonboard.com> Sogned-off-by ? > --- > 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; > } >
Hi Laurent, On Fri, Jan 15, 2021 at 07:27:54PM +0200, Laurent Pinchart wrote: > Hi Paul, > > Thank you for the patch. > > 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. > 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@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; > > } > > Thanks, Paul
Hi Paul, On Tue, Jan 19, 2021 at 05:10:36PM +0900, paul.elder@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@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@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; > > > } > > >
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; }
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. Sogned-off-by: Paul Elder <paul.elder@ideasonboard.com> --- src/android/jpeg/exif.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)