[{"id":14555,"web_url":"https://patchwork.libcamera.org/comment/14555/","msgid":"","date":"2021-01-15T17:27:54","subject":"Re: [libcamera-devel] [PATCH 3/6] android: jpeg: exif: Fix\n\tsetOrientation EXIF values","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Paul,\n\nThank you for the patch.\n\nOn Thu, Jan 14, 2021 at 07:40:32PM +0900, Paul Elder wrote:\n> The input to setOrientation is angle clockwise from 12 o'clock, while\n> the EXIF output values were swapped for 90 and 270 degrees.\n> \n> From the EXIF spec:\n> \n> 6 = The 0th row is the visual right-hand side of the image, and the\n> 0th column is the visual top.\n> 8 = The 0th row is the visual left-hand side of the image, and the\n> 0th column is the visual bottom.\n> \n> 6 should be 90 degrees clockwise, while 8 should 270 degrees clockwise.\n> Fix this.\n\nhttps://lists.libcamera.org/pipermail/libcamera-devel/2020-September/012647.html\n\nThe important part is that \"Android defines the rotation as the\nclockwise angle by which the image needs to be rotated to appear in the\ncorrect orientation on the device screen\". Could you read the thread and\nlet me know if you still think the current implementation is incorrect ?\nDoes this patch fix an issue you've noticed, or does it stem from\nreading the code only ?\n\n> Sogned-off-by: Paul Elder \n\nSogned-off-by ?\n\n> ---\n> src/android/jpeg/exif.cpp | 4 ++--\n> 1 file changed, 2 insertions(+), 2 deletions(-)\n> \n> diff --git a/src/android/jpeg/exif.cpp b/src/android/jpeg/exif.cpp\n> index 33b3fa7f..b19cb4cd 100644\n> --- a/src/android/jpeg/exif.cpp\n> +++ b/src/android/jpeg/exif.cpp\n> @@ -263,13 +263,13 @@ void Exif::setOrientation(int orientation)\n> \t\tvalue = 1;\n> \t\tbreak;\n> \tcase 90:\n> -\t\tvalue = 8;\n> +\t\tvalue = 6;\n> \t\tbreak;\n> \tcase 180:\n> \t\tvalue = 3;\n> \t\tbreak;\n> \tcase 270:\n> -\t\tvalue = 6;\n> +\t\tvalue = 8;\n> \t\tbreak;\n> \t}\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 3A8A1C3383\n\tfor ;\n\tFri, 15 Jan 2021 17:28:14 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B719C680F7;\n\tFri, 15 Jan 2021 18:28:13 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id EBA18680EF\n\tfor ;\n\tFri, 15 Jan 2021 18:28:11 +0100 (CET)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 6C728527;\n\tFri, 15 Jan 2021 18:28:11 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"QzpUu64P\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1610731691;\n\tbh=dzlBJOIVaLxqSkz4qgff2J7AjC+4Wx03YXJo7zO5eE4=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=QzpUu64PQTxJOvBOK1dEd/2sxMGobFjXcYwCdwRMf40bkxwWC/KPsvSG5DD2rn1kl\n\tWQVv2FCHLQSwknWAm7+dwmwR17Obwc2y5WAniKlHLAnG+JM4yfjK5VOjOuqFDk6RW4\n\trB3GZADeJv5BA7BhtMDisZwXZj0BUjgwdoVvpwYs=","Date":"Fri, 15 Jan 2021 19:27:54 +0200","From":"Laurent Pinchart ","To":"Paul Elder ","Message-ID":"","References":"<20210114104035.302968-1-paul.elder@ideasonboard.com>\n\t<20210114104035.302968-4-paul.elder@ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210114104035.302968-4-paul.elder@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH 3/6] android: jpeg: exif: Fix\n\tsetOrientation EXIF values","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":14594,"web_url":"https://patchwork.libcamera.org/comment/14594/","msgid":"<20210119081036.GA1943@pyrite.rasen.tech>","date":"2021-01-19T08:10:36","subject":"Re: [libcamera-devel] [PATCH 3/6] android: jpeg: exif: Fix\n\tsetOrientation EXIF values","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"Hi Laurent,\n\nOn Fri, Jan 15, 2021 at 07:27:54PM +0200, Laurent Pinchart wrote:\n> Hi Paul,\n> \n> Thank you for the patch.\n> \n> On Thu, Jan 14, 2021 at 07:40:32PM +0900, Paul Elder wrote:\n> > The input to setOrientation is angle clockwise from 12 o'clock, while\n> > the EXIF output values were swapped for 90 and 270 degrees.\n> > \n> > From the EXIF spec:\n> > \n> > 6 = The 0th row is the visual right-hand side of the image, and the\n> > 0th column is the visual top.\n> > 8 = The 0th row is the visual left-hand side of the image, and the\n> > 0th column is the visual bottom.\n> > \n> > 6 should be 90 degrees clockwise, while 8 should 270 degrees clockwise.\n> > Fix this.\n> \n> https://lists.libcamera.org/pipermail/libcamera-devel/2020-September/012647.html\n> \n> The important part is that \"Android defines the rotation as the\n> clockwise angle by which the image needs to be rotated to appear in the\n> correct orientation on the device screen\". Could you read the thread and\n\nI see that both android jpeg orientation and sensor orientation share\nthis same definition.\n\nIn the tests, in verifyJpegExifExtraTags() (CameraTestUtils.java), the\nandroid result metadata JPEG orientation is compared with the EXIF\norientation tag:\n\nint exifOrientation = exif.getAttributeInt(ExifInterface.TAG_ORIENTATION, -1)\n...\nint requestedOrientation = result.get(CaptureResult.JPEG_ORIENTATION);\n...\ncollector.expectEquals(\"Exif orientaiton should match requested orientation\",\n\trequestedOrientation, getExifOrientationInDegree(exifOrientation,\n\tcollector));\n\nIn getExifOrientationInDegree() (CameraTestUtils.java), there's a nice\nswitch-case on exifOrientation:\n\ncase ExifInterface.ORIENTATION_NORMAL:\n\treturn 0;\ncase ExifInterface.ORIENTATION_ROTATE_90:\n\treturn 90;\ncase ExifInterface.ORIENTATION_ROTATE_180:\n\treturn 180;\ncase ExifInterface.ORIENTATION_ROTATE_270:\n\treturn 270;\n\nAnd the definitions of ExifInterface.ORIENTATION_ROTATE_{90,270}\naccording to [1]:\n\nORIENTATION_ROTATE_90\nConstant Value: 6 (0x00000006) \n\nORIENTATION_ROTATE_270\nConstant Value: 8 (0x00000008) \n\nWhich matches up with the test results, which prompted this patch.\n\n[1] https://developer.android.com/reference/android/media/ExifInterface\n\n> let me know if you still think the current implementation is incorrect ?\n\nSo according to the android properties documentation on jpeg and sensor\norientation, the current implementation should be correct, but CTS\ndisagrees.\n\n> Does this patch fix an issue you've noticed, or does it stem from\n> reading the code only ?\n> \n> > Sogned-off-by: Paul Elder \n> \n> Sogned-off-by ?\n\nWhoops :p\n\n> > ---\n> > src/android/jpeg/exif.cpp | 4 ++--\n> > 1 file changed, 2 insertions(+), 2 deletions(-)\n> > \n> > diff --git a/src/android/jpeg/exif.cpp b/src/android/jpeg/exif.cpp\n> > index 33b3fa7f..b19cb4cd 100644\n> > --- a/src/android/jpeg/exif.cpp\n> > +++ b/src/android/jpeg/exif.cpp\n> > @@ -263,13 +263,13 @@ void Exif::setOrientation(int orientation)\n> > \t\tvalue = 1;\n> > \t\tbreak;\n> > \tcase 90:\n> > -\t\tvalue = 8;\n> > +\t\tvalue = 6;\n> > \t\tbreak;\n> > \tcase 180:\n> > \t\tvalue = 3;\n> > \t\tbreak;\n> > \tcase 270:\n> > -\t\tvalue = 6;\n> > +\t\tvalue = 8;\n> > \t\tbreak;\n> > \t}\n> > \n\n\nThanks,\n\nPaul","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 9DA6DBD808\n\tfor ;\n\tTue, 19 Jan 2021 08:11:15 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 31BDE68139;\n\tTue, 19 Jan 2021 09:11:15 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3ABC060313\n\tfor ;\n\tTue, 19 Jan 2021 09:11:14 +0100 (CET)","from pyrite.rasen.tech (unknown\n\t[IPv6:2400:4051:61:600:2c71:1b79:d06d:5032])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 88B554FB;\n\tTue, 19 Jan 2021 09:11:12 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"rhE3U01G\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1611043873;\n\tbh=Y2G+ywxUOawyX7qIaPUb93bPNn+166ItIZztm2Dsjgo=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=rhE3U01GDSTCrKU8XVIZw/DfHI2kDIxpx5bFIpoGFLwwwgEHiCjms5BbZbpOJMpnv\n\tOMmxNMbI4anV/n1dx3vHFKvMBbmygFTZlvfEPDud/u22czSOiCpGbtUseF4JURfWKC\n\tf0uRk6yZUKFfpfX6oyGnY8DI5wCTpw7X6xbRVJnI=","Date":"Tue, 19 Jan 2021 17:10:36 +0900","From":"paul.elder@ideasonboard.com","To":"Laurent Pinchart ","Message-ID":"<20210119081036.GA1943@pyrite.rasen.tech>","References":"<20210114104035.302968-1-paul.elder@ideasonboard.com>\n\t<20210114104035.302968-4-paul.elder@ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH 3/6] android: jpeg: exif: Fix\n\tsetOrientation EXIF values","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":14595,"web_url":"https://patchwork.libcamera.org/comment/14595/","msgid":"","date":"2021-01-19T08:33:31","subject":"Re: [libcamera-devel] [PATCH 3/6] android: jpeg: exif: Fix\n\tsetOrientation EXIF values","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Paul,\n\nOn Tue, Jan 19, 2021 at 05:10:36PM +0900, paul.elder@ideasonboard.com wrote:\n> On Fri, Jan 15, 2021 at 07:27:54PM +0200, Laurent Pinchart wrote:\n> > On Thu, Jan 14, 2021 at 07:40:32PM +0900, Paul Elder wrote:\n> > > The input to setOrientation is angle clockwise from 12 o'clock, while\n> > > the EXIF output values were swapped for 90 and 270 degrees.\n> > > \n> > > From the EXIF spec:\n> > > \n> > > 6 = The 0th row is the visual right-hand side of the image, and the\n> > > 0th column is the visual top.\n> > > 8 = The 0th row is the visual left-hand side of the image, and the\n> > > 0th column is the visual bottom.\n> > > \n> > > 6 should be 90 degrees clockwise, while 8 should 270 degrees clockwise.\n> > > Fix this.\n> > \n> > https://lists.libcamera.org/pipermail/libcamera-devel/2020-September/012647.html\n> > \n> > The important part is that \"Android defines the rotation as the\n> > clockwise angle by which the image needs to be rotated to appear in the\n> > correct orientation on the device screen\". Could you read the thread and\n> \n> I see that both android jpeg orientation and sensor orientation share\n> this same definition.\n> \n> In the tests, in verifyJpegExifExtraTags() (CameraTestUtils.java), the\n> android result metadata JPEG orientation is compared with the EXIF\n> orientation tag:\n> \n> int exifOrientation = exif.getAttributeInt(ExifInterface.TAG_ORIENTATION, -1)\n> ...\n> int requestedOrientation = result.get(CaptureResult.JPEG_ORIENTATION);\n> ...\n> collector.expectEquals(\"Exif orientaiton should match requested orientation\",\n> \trequestedOrientation, getExifOrientationInDegree(exifOrientation,\n> \tcollector));\n> \n> In getExifOrientationInDegree() (CameraTestUtils.java), there's a nice\n> switch-case on exifOrientation:\n> \n> case ExifInterface.ORIENTATION_NORMAL:\n> \treturn 0;\n> case ExifInterface.ORIENTATION_ROTATE_90:\n> \treturn 90;\n> case ExifInterface.ORIENTATION_ROTATE_180:\n> \treturn 180;\n> case ExifInterface.ORIENTATION_ROTATE_270:\n> \treturn 270;\n> \n> And the definitions of ExifInterface.ORIENTATION_ROTATE_{90,270}\n> according to [1]:\n> \n> ORIENTATION_ROTATE_90\n> Constant Value: 6 (0x00000006) \n> \n> ORIENTATION_ROTATE_270\n> Constant Value: 8 (0x00000008) \n> \n> Which matches up with the test results, which prompted this patch.\n> \n> [1] https://developer.android.com/reference/android/media/ExifInterface\n> \n> > let me know if you still think the current implementation is incorrect ?\n> \n> So according to the android properties documentation on jpeg and sensor\n> orientation, the current implementation should be correct, but CTS\n> disagrees.\n\nAs discussed on IRC, this patch is correct, and the original code is\ncorrect too :-) This patch is needed because of another bug, which you\nfix by replacing the camera device orientation with the\nandroid.jpeg.orientation value taken from the request to set the Exif\norientation.\n\nWith an updated commit message to explain this,\n\nReviewed-by: Laurent Pinchart \n\n> > Does this patch fix an issue you've noticed, or does it stem from\n> > reading the code only ?\n> > \n> > > Sogned-off-by: Paul Elder \n> > \n> > Sogned-off-by ?\n> \n> Whoops :p\n> \n> > > ---\n> > > src/android/jpeg/exif.cpp | 4 ++--\n> > > 1 file changed, 2 insertions(+), 2 deletions(-)\n> > > \n> > > diff --git a/src/android/jpeg/exif.cpp b/src/android/jpeg/exif.cpp\n> > > index 33b3fa7f..b19cb4cd 100644\n> > > --- a/src/android/jpeg/exif.cpp\n> > > +++ b/src/android/jpeg/exif.cpp\n> > > @@ -263,13 +263,13 @@ void Exif::setOrientation(int orientation)\n> > > \t\tvalue = 1;\n> > > \t\tbreak;\n> > > \tcase 90:\n> > > -\t\tvalue = 8;\n> > > +\t\tvalue = 6;\n> > > \t\tbreak;\n> > > \tcase 180:\n> > > \t\tvalue = 3;\n> > > \t\tbreak;\n> > > \tcase 270:\n> > > -\t\tvalue = 6;\n> > > +\t\tvalue = 8;\n> > > \t\tbreak;\n> > > \t}\n> > >","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id B34A4C0F1C\n\tfor ;\n\tTue, 19 Jan 2021 08:33:50 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2B7EA68139;\n\tTue, 19 Jan 2021 09:33:50 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 8762160313\n\tfor ;\n\tTue, 19 Jan 2021 09:33:48 +0100 (CET)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id EF6E84FB;\n\tTue, 19 Jan 2021 09:33:47 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"gZYMqxF2\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1611045228;\n\tbh=H/zJO3IsYzLdcNLwZSh2oGoJRbYJIQJvuGFzIjFOIiI=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=gZYMqxF2PIvx1hRlHWD0bPdY8VvUtErlQaldEEOroE5LOheg4d9n3kJTb2MP3SEM2\n\tN/O1YFLT7rjw9tiCIESTXxfKc+AVGfo91dTKdqdhSxMP12ACgYrsRrT7kuhBlxCIXM\n\t+irI1fMwL5vup7RPArIEJ5x+EaRs9ooOOhXFMTtY=","Date":"Tue, 19 Jan 2021 10:33:31 +0200","From":"Laurent Pinchart ","To":"paul.elder@ideasonboard.com","Message-ID":"","References":"<20210114104035.302968-1-paul.elder@ideasonboard.com>\n\t<20210114104035.302968-4-paul.elder@ideasonboard.com>\n\t\n\t<20210119081036.GA1943@pyrite.rasen.tech>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210119081036.GA1943@pyrite.rasen.tech>","Subject":"Re: [libcamera-devel] [PATCH 3/6] android: jpeg: exif: Fix\n\tsetOrientation EXIF values","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]