[{"id":15823,"web_url":"https://patchwork.libcamera.org/comment/15823/","msgid":"<6b8b395f-2f2e-4c72-bc2e-5430d1cbb98d@ideasonboard.com>","date":"2021-03-23T09:04:37","subject":"Re: [libcamera-devel] [PATCH] android: exif: Contain IMAGE_WIDTH\n\tand IMAGE_LENGTH data","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi Hiro,\n\nOn 3/23/21 1:32 PM, Hirokazu Honda wrote:\n> ChromeOS camera test checks if exif data has the IMAGE_WIDTH and\n> IMAGE_LENGTH and they are the same as the requested jpeg size.\n> This adds the resolution data to exif.\n>\n> Signed-off-by: Hirokazu Honda \n>\n> ---\n> src/android/jpeg/exif.cpp | 2 ++\n> 1 file changed, 2 insertions(+)\n>\n> diff --git a/src/android/jpeg/exif.cpp b/src/android/jpeg/exif.cpp\n> index 922086cd..29c7be0f 100644\n> --- a/src/android/jpeg/exif.cpp\n> +++ b/src/android/jpeg/exif.cpp\n> @@ -286,6 +286,8 @@ void Exif::setModel(const std::string &model)\n>\n> void Exif::setSize(const Size &size)\n> {\n> +\tsetLong(EXIF_IFD_0, EXIF_TAG_IMAGE_LENGTH, size.height);\n> +\tsetLong(EXIF_IFD_0, EXIF_TAG_IMAGE_WIDTH, size.width);\nI am reading the EXIF spec and for ImageLength and ImageWidth, it states:\n\n```\nImageWidth\nThe number of columns of image data, equal to the number of pixels per \nrow. In JPEG compressed\ndata, this tag shall not be used because a JPEG marker is used instead \nof it.\n```\n\nSame for ImageLength.\n\nWe compress the image using JPEG post-processor right? Hence, I think \nthese tags shouldn't be applicable (as per spec's point of view).\n\n> \tsetLong(EXIF_IFD_EXIF, EXIF_TAG_PIXEL_Y_DIMENSION, size.height);\n> \tsetLong(EXIF_IFD_EXIF, EXIF_TAG_PIXEL_X_DIMENSION, size.width);\n> }\n> --\n> 2.31.0.rc2.261.g7f71774620-goog\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel\n--\nRegards,\nUmang Jain","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 A6D45C32E4\n\tfor ;\n\tTue, 23 Mar 2021 09:04:45 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id EE49D68D65;\n\tTue, 23 Mar 2021 10:04:44 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5649A602DB\n\tfor ;\n\tTue, 23 Mar 2021 10:04:43 +0100 (CET)","from localhost.localdomain (unknown [103.238.109.27])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id A6724885\n\tfor ;\n\tTue, 23 Mar 2021 10:04:42 +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=\"W20+5PjE\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1616490282;\n\tbh=UWCRH0daQxPbmRY7y3yIMxuXSMu9ST8ZVUgZyO9y4zA=;\n\th=Subject:To:References:From:Date:In-Reply-To:From;\n\tb=W20+5PjEBcLTjt+SW47QMJWy5UZnyAilTw9BB3ssZOtmQUkifss2kAp9HcjAnsq/5\n\tNc0s1RBuUZrZhu7qSN02LspYczbDfEzpYfRoeNxT9yFCZ1J1TjBqOiF/RgZk2TEC83\n\tgjLqBG8wIsePJSwEuyXhfuLmnC3JaSMgyj9nVAT8=","To":"libcamera-devel@lists.libcamera.org","References":"<20210323080231.3389137-1-hiroh@chromium.org>","From":"Umang Jain ","Message-ID":"<6b8b395f-2f2e-4c72-bc2e-5430d1cbb98d@ideasonboard.com>","Date":"Tue, 23 Mar 2021 14:34:37 +0530","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101\n\tThunderbird/68.11.0","MIME-Version":"1.0","In-Reply-To":"<20210323080231.3389137-1-hiroh@chromium.org>","Content-Language":"en-US","Subject":"Re: [libcamera-devel] [PATCH] android: exif: Contain IMAGE_WIDTH\n\tand IMAGE_LENGTH data","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","Content-Transfer-Encoding":"7bit","Content-Type":"text/plain; charset=\"us-ascii\"; Format=\"flowed\"","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":15884,"web_url":"https://patchwork.libcamera.org/comment/15884/","msgid":"","date":"2021-03-25T06:18:21","subject":"Re: [libcamera-devel] [PATCH] android: exif: Contain IMAGE_WIDTH\n\tand IMAGE_LENGTH data","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"Hi Umang, thanks for reviewing,\n\nOn Tue, Mar 23, 2021 at 6:04 PM Umang Jain wrote:\n>\n> Hi Hiro,\n>\n> On 3/23/21 1:32 PM, Hirokazu Honda wrote:\n> > ChromeOS camera test checks if exif data has the IMAGE_WIDTH and\n> > IMAGE_LENGTH and they are the same as the requested jpeg size.\n> > This adds the resolution data to exif.\n> >\n> > Signed-off-by: Hirokazu Honda \n> >\n> > ---\n> > src/android/jpeg/exif.cpp | 2 ++\n> > 1 file changed, 2 insertions(+)\n> >\n> > diff --git a/src/android/jpeg/exif.cpp b/src/android/jpeg/exif.cpp\n> > index 922086cd..29c7be0f 100644\n> > --- a/src/android/jpeg/exif.cpp\n> > +++ b/src/android/jpeg/exif.cpp\n> > @@ -286,6 +286,8 @@ void Exif::setModel(const std::string &model)\n> >\n> > void Exif::setSize(const Size &size)\n> > {\n> > + setLong(EXIF_IFD_0, EXIF_TAG_IMAGE_LENGTH, size.height);\n> > + setLong(EXIF_IFD_0, EXIF_TAG_IMAGE_WIDTH, size.width);\n> I am reading the EXIF spec and for ImageLength and ImageWidth, it states:\n>\n> ```\n> ImageWidth\n> The number of columns of image data, equal to the number of pixels per\n> row. In JPEG compressed\n> data, this tag shall not be used because a JPEG marker is used instead\n> of it.\n> ```\n>\n> Same for ImageLength.\n>\n> We compress the image using JPEG post-processor right? Hence, I think\n> these tags shouldn't be applicable (as per spec's point of view).\n>\n\nI think you're right.\nOur test code [1] seems to come from Android CTS test code [2].\nI am asking Android camera people if the check is correct.\nI will update here their response.\n\n[1] https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/platform2/camera/camera3_test/camera3_exif_validator.cc;l=337;drc=82fcae1960c0283214607107037c747c0e545617\n[2] https://cs.android.com/android/platform/superproject/+/master:cts/tests/camera/utils/src/android/hardware/camera2/cts/CameraTestUtils.java;l=2669;drc=556fa09af46eeb2e8abe418d0f049c154bc4181a\n\nRegards,\n-Hiro\n\n> > setLong(EXIF_IFD_EXIF, EXIF_TAG_PIXEL_Y_DIMENSION, size.height);\n> > setLong(EXIF_IFD_EXIF, EXIF_TAG_PIXEL_X_DIMENSION, size.width);\n> > }\n> > --\n> > 2.31.0.rc2.261.g7f71774620-goog\n> > _______________________________________________\n> > libcamera-devel mailing list\n> > libcamera-devel@lists.libcamera.org\n> > https://lists.libcamera.org/listinfo/libcamera-devel\n> --\n> Regards,\n> Umang Jain\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","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 C4959BDC66\n\tfor ;\n\tThu, 25 Mar 2021 06:18:32 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0706568D62;\n\tThu, 25 Mar 2021 07:18:32 +0100 (CET)","from mail-ed1-x535.google.com (mail-ed1-x535.google.com\n\t[IPv6:2a00:1450:4864:20::535])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 00DF96051A\n\tfor ;\n\tThu, 25 Mar 2021 07:18:30 +0100 (CET)","by mail-ed1-x535.google.com with SMTP id x21so1109047eds.4\n\tfor ;\n\tWed, 24 Mar 2021 23:18:30 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"PtBPpVat\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org;\n\ts=google; \n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=066Fkh3gTtW9SecyxbAPJzsX8lgXcqWNFFi6zsG6Kqc=;\n\tb=PtBPpVatXLQT6mWg+F/sXtPKSHU4Q6H6bRfVWhSemiowvaTJ+bZKokRCtJlzUvUhms\n\t+PxATOvLH3m3bXkdJye0CAFkXiOkqiBbhNqwn4uApmqtiybBQWxj1XXkx6ju30IG7OZG\n\t6klThMdb0blYjQRMZz30qLPOpRmdZLf+4can0=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=066Fkh3gTtW9SecyxbAPJzsX8lgXcqWNFFi6zsG6Kqc=;\n\tb=nPjSIX0gBzC0wmC343YLSnIohpOEEsj6XiF2eHxrH86P2ghoJoPdNaTgZ3ZLSE1DAH\n\tCfaSLqBS88cajQXtO052vpxi+EyaGFvASnWoxICiNxqQxjA5G2nOlVyX6vjIIPevQf5w\n\tucbZBDflBoC+7Z8n+qeQLCWwuvgVVFUQnebK5mPi992kE4EEeP/IMA1GIHLuIRf7zxE6\n\to0Z+Cpl4DkAPERdMOS4LBiw0SiAkrVBkWsspg3TgsewnsEEvx88Eip3KkYRqKfo9RiAV\n\tY9f6Q7dUiTkCzzxMv41kJEdRoZ44SDMKb7Xa15k/YZ0sXGHgPGA0yi9gkGKDFGHVcUyv\n\tPncA==","X-Gm-Message-State":"AOAM532VV3mVDpoMSrdD4AtqbjHhP12qUBSw5ogSFeC+WyEDxjj6a6aj\n\tIV2A7G4AXFIiiwkeXHexxIfjLofmVEKIUfEmqP3jm0iWy8CGSQ==","X-Google-Smtp-Source":"ABdhPJybnvI6WNIIdlD2ICJIAuixrmxd5xGz2+vvBhEshmKyv8SbhmR91wl7Ok/CjPzwUiqHy0FIc69DacEKs2v9h0M=","X-Received":"by 2002:aa7:d4cb:: with SMTP id\n\tt11mr7168509edr.202.1616653110566; \n\tWed, 24 Mar 2021 23:18:30 -0700 (PDT)","MIME-Version":"1.0","References":"<20210323080231.3389137-1-hiroh@chromium.org>\n\t<6b8b395f-2f2e-4c72-bc2e-5430d1cbb98d@ideasonboard.com>","In-Reply-To":"<6b8b395f-2f2e-4c72-bc2e-5430d1cbb98d@ideasonboard.com>","From":"Hirokazu Honda ","Date":"Thu, 25 Mar 2021 15:18:21 +0900","Message-ID":"","To":"Umang Jain ","Subject":"Re: [libcamera-devel] [PATCH] android: exif: Contain IMAGE_WIDTH\n\tand IMAGE_LENGTH data","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 ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16083,"web_url":"https://patchwork.libcamera.org/comment/16083/","msgid":"","date":"2021-04-01T15:27:05","subject":"Re: [libcamera-devel] [PATCH] android: exif: Contain IMAGE_WIDTH\n\tand IMAGE_LENGTH data","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"Hi,\n\nOn Thu, Mar 25, 2021 at 3:18 PM Hirokazu Honda wrote:\n>\n> Hi Umang, thanks for reviewing,\n>\n> On Tue, Mar 23, 2021 at 6:04 PM Umang Jain wrote:\n> >\n> > Hi Hiro,\n> >\n> > On 3/23/21 1:32 PM, Hirokazu Honda wrote:\n> > > ChromeOS camera test checks if exif data has the IMAGE_WIDTH and\n> > > IMAGE_LENGTH and they are the same as the requested jpeg size.\n> > > This adds the resolution data to exif.\n> > >\n> > > Signed-off-by: Hirokazu Honda \n> > >\n> > > ---\n> > > src/android/jpeg/exif.cpp | 2 ++\n> > > 1 file changed, 2 insertions(+)\n> > >\n> > > diff --git a/src/android/jpeg/exif.cpp b/src/android/jpeg/exif.cpp\n> > > index 922086cd..29c7be0f 100644\n> > > --- a/src/android/jpeg/exif.cpp\n> > > +++ b/src/android/jpeg/exif.cpp\n> > > @@ -286,6 +286,8 @@ void Exif::setModel(const std::string &model)\n> > >\n> > > void Exif::setSize(const Size &size)\n> > > {\n> > > + setLong(EXIF_IFD_0, EXIF_TAG_IMAGE_LENGTH, size.height);\n> > > + setLong(EXIF_IFD_0, EXIF_TAG_IMAGE_WIDTH, size.width);\n> > I am reading the EXIF spec and for ImageLength and ImageWidth, it states:\n> >\n> > ```\n> > ImageWidth\n> > The number of columns of image data, equal to the number of pixels per\n> > row. In JPEG compressed\n> > data, this tag shall not be used because a JPEG marker is used instead\n> > of it.\n> > ```\n> >\n> > Same for ImageLength.\n> >\n> > We compress the image using JPEG post-processor right? Hence, I think\n> > these tags shouldn't be applicable (as per spec's point of view).\n> >\n>\n> I think you're right.\n> Our test code [1] seems to come from Android CTS test code [2].\n> I am asking Android camera people if the check is correct.\n> I will update here their response.\n>\n> [1] https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/platform2/camera/camera3_test/camera3_exif_validator.cc;l=337;drc=82fcae1960c0283214607107037c747c0e545617\n> [2] https://cs.android.com/android/platform/superproject/+/master:cts/tests/camera/utils/src/android/hardware/camera2/cts/CameraTestUtils.java;l=2669;drc=556fa09af46eeb2e8abe418d0f049c154bc4181a\n>\n\nI found, if there is no exif tags of IMAGE_WIDTH and IMAGE_LENGTH,\nAndroid sets them to ones of JPEG markers. [1]\nIt is because some CTS tests that checks the exif tags passed albeit\nwe don't set the tags.\nI reached out Android developers about the tests and the code.\nThey agree the EXIF spec states these tags are not needed.\nHowever, they said it was pretty common that these tags exist in pract\nand their CTS tries to make sure common tags are included in EXIF.\nSo they are reluctant to change it and apps may be expecting them to\nexif on JPEGS from Android devices now as the code exists since 2016.\nFWIW, CDD doesn't include required EXIF tags unfortunately.\n\nThere are two solutions for us.\n1.) Don't add these tags in libcamera\nThis is correct in terms of EXIF spec.\nEven if we don't set the tags, they are set by Android to ones of JPEG markers.\nThe problem is that chromeos camera test fails due to the wrong check\n[2]. We remove the checks on this solution.\n\n2.) Add these tags in libcamera\nAndroid and chromeos camera tests will pass.\nAlthough this is wrong in terms of EXIF spec, it might be practical to\ndo this because some Android HAL clients (wrongly) expect the\nbehavior.\n\nHow do you think?\n\n[1] https://cs.android.com/android/platform/superproject/+/master:frameworks/base/media/java/android/media/ExifInterface.java;l=2907\n[2] https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/platform2/camera/camera3_test/camera3_exif_validator.cc;l=337;drc=82fcae1960c0283214607107037c747c0e545617\n\nBest Regards,\n-Hiro\n> Regards,\n> -Hiro\n>\n> > > setLong(EXIF_IFD_EXIF, EXIF_TAG_PIXEL_Y_DIMENSION, size.height);\n> > > setLong(EXIF_IFD_EXIF, EXIF_TAG_PIXEL_X_DIMENSION, size.width);\n> > > }\n> > > --\n> > > 2.31.0.rc2.261.g7f71774620-goog\n> > > _______________________________________________\n> > > libcamera-devel mailing list\n> > > libcamera-devel@lists.libcamera.org\n> > > https://lists.libcamera.org/listinfo/libcamera-devel\n> > --\n> > Regards,\n> > Umang Jain\n> > _______________________________________________\n> > libcamera-devel mailing list\n> > libcamera-devel@lists.libcamera.org\n> > https://lists.libcamera.org/listinfo/libcamera-devel","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 2710FC0DA3\n\tfor ;\n\tThu, 1 Apr 2021 15:27:18 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6819768783;\n\tThu, 1 Apr 2021 17:27:17 +0200 (CEST)","from mail-ej1-x635.google.com (mail-ej1-x635.google.com\n\t[IPv6:2a00:1450:4864:20::635])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id BBE456877C\n\tfor ;\n\tThu, 1 Apr 2021 17:27:15 +0200 (CEST)","by mail-ej1-x635.google.com with SMTP id l4so3444940ejc.10\n\tfor ;\n\tThu, 01 Apr 2021 08:27:15 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"CnVXma3j\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org;\n\ts=google; \n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=JA1sOjZ9f9zXRpzNigXlRJOb6G7ZQcuDBahlkn5ZnvM=;\n\tb=CnVXma3j0bLOv/Fv6+gmupoScwppmIf98oNNAj9ITJ/Cxumk36VRojn+MbJ/ORariO\n\tYDqI7eTQRUzorWwPfh5FcmboxdiQfRUBH8zSUeP/WmGfz3MYEc2XznFJsWlucN1WvyNJ\n\tcxddJxfZfllaGfZSZLJ1Mpl7bivUh9HSevOyA=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=JA1sOjZ9f9zXRpzNigXlRJOb6G7ZQcuDBahlkn5ZnvM=;\n\tb=ds+WrVfo/6Vmlcmc7YOjkwXFUjXD7484I6XOmLxJlRt8NdYpkymPtw+bei2GRU+Liv\n\tpugzcEehjz7nsXBPAw54nnvbJRSEYi6VbTEbKEXDhjZ0JDhyfxzVC3g+wfdTfQXQaecc\n\tGQEkq1Xwg2RO1Vf2lGXtXgPFXTL92XQZ+Q9KoaFZZIZVCbeLeYH1An2hNaHsrM4fmgJi\n\tVNntUtZYE7xhuMgxACx7qQdAtIUsr3iBTD2WIE7bVTtw1PKYzHFsZ8GfaCf39/C6Bae+\n\tQL4NjA6uBtJoYHALItUat3Gv0rP0lnqQppdTo/ct2DoJsLkd+0Pwn45dwtvWuTd2JOGq\n\tiZqg==","X-Gm-Message-State":"AOAM530xnmDQXdEJTnHzsyIknDezGLDANHpUhk7wcjkWLsr2hkHsF0qs\n\t87xj4bL31ESgpWKfDBMWF++dksj2Ikl56+WbkbX2H6MAq6KPFA==","X-Google-Smtp-Source":"ABdhPJxzSLoZxIqSfiUJFvx5VK2OEcz7QJA9MEIJnu4y0wGTIX7VUg2s8RfjjgQw0RibLFSuip0v3/yRl/3gh9XA2pA=","X-Received":"by 2002:a17:907:1b06:: with SMTP id\n\tmp6mr9845873ejc.292.1617290835278; \n\tThu, 01 Apr 2021 08:27:15 -0700 (PDT)","MIME-Version":"1.0","References":"<20210323080231.3389137-1-hiroh@chromium.org>\n\t<6b8b395f-2f2e-4c72-bc2e-5430d1cbb98d@ideasonboard.com>\n\t","In-Reply-To":"","From":"Hirokazu Honda ","Date":"Fri, 2 Apr 2021 00:27:05 +0900","Message-ID":"","To":"Umang Jain ","Subject":"Re: [libcamera-devel] [PATCH] android: exif: Contain IMAGE_WIDTH\n\tand IMAGE_LENGTH data","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 ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16103,"web_url":"https://patchwork.libcamera.org/comment/16103/","msgid":"","date":"2021-04-03T01:47:35","subject":"Re: [libcamera-devel] [PATCH] android: exif: Contain IMAGE_WIDTH\n\tand IMAGE_LENGTH data","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Hiro,\n\nOn Fri, Apr 02, 2021 at 12:27:05AM +0900, Hirokazu Honda wrote:\n> On Thu, Mar 25, 2021 at 3:18 PM Hirokazu Honda wrote:\n> > On Tue, Mar 23, 2021 at 6:04 PM Umang Jain wrote:\n> > > On 3/23/21 1:32 PM, Hirokazu Honda wrote:\n> > > > ChromeOS camera test checks if exif data has the IMAGE_WIDTH and\n> > > > IMAGE_LENGTH and they are the same as the requested jpeg size.\n> > > > This adds the resolution data to exif.\n> > > >\n> > > > Signed-off-by: Hirokazu Honda \n> > > >\n> > > > ---\n> > > > src/android/jpeg/exif.cpp | 2 ++\n> > > > 1 file changed, 2 insertions(+)\n> > > >\n> > > > diff --git a/src/android/jpeg/exif.cpp b/src/android/jpeg/exif.cpp\n> > > > index 922086cd..29c7be0f 100644\n> > > > --- a/src/android/jpeg/exif.cpp\n> > > > +++ b/src/android/jpeg/exif.cpp\n> > > > @@ -286,6 +286,8 @@ void Exif::setModel(const std::string &model)\n> > > >\n> > > > void Exif::setSize(const Size &size)\n> > > > {\n> > > > + setLong(EXIF_IFD_0, EXIF_TAG_IMAGE_LENGTH, size.height);\n> > > > + setLong(EXIF_IFD_0, EXIF_TAG_IMAGE_WIDTH, size.width);\n> > > I am reading the EXIF spec and for ImageLength and ImageWidth, it states:\n> > >\n> > > ```\n> > > ImageWidth\n> > > The number of columns of image data, equal to the number of pixels per\n> > > row. In JPEG compressed\n> > > data, this tag shall not be used because a JPEG marker is used instead\n> > > of it.\n> > > ```\n> > >\n> > > Same for ImageLength.\n> > >\n> > > We compress the image using JPEG post-processor right? Hence, I think\n> > > these tags shouldn't be applicable (as per spec's point of view).\n> >\n> > I think you're right.\n> > Our test code [1] seems to come from Android CTS test code [2].\n> > I am asking Android camera people if the check is correct.\n> > I will update here their response.\n> >\n> > [1] https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/platform2/camera/camera3_test/camera3_exif_validator.cc;l=337;drc=82fcae1960c0283214607107037c747c0e545617\n> > [2] https://cs.android.com/android/platform/superproject/+/master:cts/tests/camera/utils/src/android/hardware/camera2/cts/CameraTestUtils.java;l=2669;drc=556fa09af46eeb2e8abe418d0f049c154bc4181a\n> \n> I found, if there is no exif tags of IMAGE_WIDTH and IMAGE_LENGTH,\n> Android sets them to ones of JPEG markers. [1]\n> It is because some CTS tests that checks the exif tags passed albeit\n> we don't set the tags.\n> I reached out Android developers about the tests and the code.\n> They agree the EXIF spec states these tags are not needed.\n> However, they said it was pretty common that these tags exist in pract\n\nNo wonder they're pretty common, if Android adds them erroneously :-)\n\n> and their CTS tries to make sure common tags are included in EXIF.\n> So they are reluctant to change it and apps may be expecting them to\n> exif on JPEGS from Android devices now as the code exists since 2016.\n> FWIW, CDD doesn't include required EXIF tags unfortunately.\n> \n> There are two solutions for us.\n> 1.) Don't add these tags in libcamera\n> This is correct in terms of EXIF spec.\n> Even if we don't set the tags, they are set by Android to ones of JPEG markers.\n> The problem is that chromeos camera test fails due to the wrong check\n> [2]. We remove the checks on this solution.\n> \n> 2.) Add these tags in libcamera\n> Android and chromeos camera tests will pass.\n> Although this is wrong in terms of EXIF spec, it might be practical to\n> do this because some Android HAL clients (wrongly) expect the\n> behavior.\n> \n> How do you think?\n\nI really dislike when we're forced to do the wrong thing because someone\nmade a mistake and won't fix it :-(\n\nOur HAL implementation is meant to support both Chrome OS and Android.\nOn Android, if the camera service adds the Exif tags, it doesn't matter\nif we include them or not, applications will always see the same\n(incorrect) behaviour with the tags included. I'd say that relying on\nthe Android camera service to do so is likely best: the incorrect\nbehaviour is an Android requirement due to bug-to-bug backward\ncompatibility, so letting the Android camera service handle it would\nmake sense. They could then decide to change the behaviour later (which\nis unlikely though), without needing a modification in libcamera.\n\nOn Chrome OS, if there's no worry about possibly issues with\napplications, I think it would be better to not include the tags, to be\ncompliant with the Exif specification, and dropping the corresponding\ncheck in [2]. If there's a risk of breakage with existing applications,\nhowever, that can be reconsidered.\n\nOne additional question is how widespread applications relying on those\ntags are. If they're mostly related to Android, the above should hold\ntrue. If non-Android applications have started including and/or\nconsuming those tags in JPEG images, then it's a different story as it\nwould mean that the whole world isn't compliant with the specification,\nand it wouldn't make sense to be pedantic.\n\nWhat do you think ?\n\n> [1] https://cs.android.com/android/platform/superproject/+/master:frameworks/base/media/java/android/media/ExifInterface.java;l=2907\n> [2] https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/platform2/camera/camera3_test/camera3_exif_validator.cc;l=337;drc=82fcae1960c0283214607107037c747c0e545617\n> \n> > > > setLong(EXIF_IFD_EXIF, EXIF_TAG_PIXEL_Y_DIMENSION, size.height);\n> > > > setLong(EXIF_IFD_EXIF, EXIF_TAG_PIXEL_X_DIMENSION, size.width);\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 61BADBD695\n\tfor ;\n\tSat, 3 Apr 2021 01:48:23 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9B73A602D7;\n\tSat, 3 Apr 2021 03:48:22 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 8291F602CF\n\tfor ;\n\tSat, 3 Apr 2021 03:48:20 +0200 (CEST)","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 F18613D7;\n\tSat, 3 Apr 2021 03:48:19 +0200 (CEST)"],"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=\"XdeM3Hw4\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1617414500;\n\tbh=MYRdKCr5ptokmLaSl1rWM6AvfvR7y3bc7Jme+2jbe9g=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=XdeM3Hw4EP3N29PF8I6cEUtV/zG78T+2+zKSbJbSTKDdn5H+CXf8BRYhXLNCA6npy\n\tPKHykdkyafQnNUOA7DzGTz9ZEDaz/GUVOkIDLpCVYcUgbc4mMI7B1nc13drfHrgVqB\n\t647dwKrGDuArfnx2p8NfRMaQkCzCQsGn+ySpOptQ=","Date":"Sat, 3 Apr 2021 04:47:35 +0300","From":"Laurent Pinchart ","To":"Hirokazu Honda ","Message-ID":"","References":"<20210323080231.3389137-1-hiroh@chromium.org>\n\t<6b8b395f-2f2e-4c72-bc2e-5430d1cbb98d@ideasonboard.com>\n\t\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH] android: exif: Contain IMAGE_WIDTH\n\tand IMAGE_LENGTH data","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 ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16111,"web_url":"https://patchwork.libcamera.org/comment/16111/","msgid":"","date":"2021-04-03T11:17:40","subject":"Re: [libcamera-devel] [PATCH] android: exif: Contain IMAGE_WIDTH\n\tand IMAGE_LENGTH data","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"HI Laurent,\nthanks for commenting.\n\nOn Sat, Apr 3, 2021 at 10:48 AM Laurent Pinchart\n wrote:\n>\n> Hi Hiro,\n>\n> On Fri, Apr 02, 2021 at 12:27:05AM +0900, Hirokazu Honda wrote:\n> > On Thu, Mar 25, 2021 at 3:18 PM Hirokazu Honda wrote:\n> > > On Tue, Mar 23, 2021 at 6:04 PM Umang Jain wrote:\n> > > > On 3/23/21 1:32 PM, Hirokazu Honda wrote:\n> > > > > ChromeOS camera test checks if exif data has the IMAGE_WIDTH and\n> > > > > IMAGE_LENGTH and they are the same as the requested jpeg size.\n> > > > > This adds the resolution data to exif.\n> > > > >\n> > > > > Signed-off-by: Hirokazu Honda \n> > > > >\n> > > > > ---\n> > > > > src/android/jpeg/exif.cpp | 2 ++\n> > > > > 1 file changed, 2 insertions(+)\n> > > > >\n> > > > > diff --git a/src/android/jpeg/exif.cpp b/src/android/jpeg/exif.cpp\n> > > > > index 922086cd..29c7be0f 100644\n> > > > > --- a/src/android/jpeg/exif.cpp\n> > > > > +++ b/src/android/jpeg/exif.cpp\n> > > > > @@ -286,6 +286,8 @@ void Exif::setModel(const std::string &model)\n> > > > >\n> > > > > void Exif::setSize(const Size &size)\n> > > > > {\n> > > > > + setLong(EXIF_IFD_0, EXIF_TAG_IMAGE_LENGTH, size.height);\n> > > > > + setLong(EXIF_IFD_0, EXIF_TAG_IMAGE_WIDTH, size.width);\n> > > > I am reading the EXIF spec and for ImageLength and ImageWidth, it states:\n> > > >\n> > > > ```\n> > > > ImageWidth\n> > > > The number of columns of image data, equal to the number of pixels per\n> > > > row. In JPEG compressed\n> > > > data, this tag shall not be used because a JPEG marker is used instead\n> > > > of it.\n> > > > ```\n> > > >\n> > > > Same for ImageLength.\n> > > >\n> > > > We compress the image using JPEG post-processor right? Hence, I think\n> > > > these tags shouldn't be applicable (as per spec's point of view).\n> > >\n> > > I think you're right.\n> > > Our test code [1] seems to come from Android CTS test code [2].\n> > > I am asking Android camera people if the check is correct.\n> > > I will update here their response.\n> > >\n> > > [1] https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/platform2/camera/camera3_test/camera3_exif_validator.cc;l=337;drc=82fcae1960c0283214607107037c747c0e545617\n> > > [2] https://cs.android.com/android/platform/superproject/+/master:cts/tests/camera/utils/src/android/hardware/camera2/cts/CameraTestUtils.java;l=2669;drc=556fa09af46eeb2e8abe418d0f049c154bc4181a\n> >\n> > I found, if there is no exif tags of IMAGE_WIDTH and IMAGE_LENGTH,\n> > Android sets them to ones of JPEG markers. [1]\n> > It is because some CTS tests that checks the exif tags passed albeit\n> > we don't set the tags.\n> > I reached out Android developers about the tests and the code.\n> > They agree the EXIF spec states these tags are not needed.\n> > However, they said it was pretty common that these tags exist in pract\n>\n> No wonder they're pretty common, if Android adds them erroneously :-)\n>\n> > and their CTS tries to make sure common tags are included in EXIF.\n> > So they are reluctant to change it and apps may be expecting them to\n> > exif on JPEGS from Android devices now as the code exists since 2016.\n> > FWIW, CDD doesn't include required EXIF tags unfortunately.\n> >\n> > There are two solutions for us.\n> > 1.) Don't add these tags in libcamera\n> > This is correct in terms of EXIF spec.\n> > Even if we don't set the tags, they are set by Android to ones of JPEG markers.\n> > The problem is that chromeos camera test fails due to the wrong check\n> > [2]. We remove the checks on this solution.\n> >\n> > 2.) Add these tags in libcamera\n> > Android and chromeos camera tests will pass.\n> > Although this is wrong in terms of EXIF spec, it might be practical to\n> > do this because some Android HAL clients (wrongly) expect the\n> > behavior.\n> >\n> > How do you think?\n>\n> I really dislike when we're forced to do the wrong thing because someone\n> made a mistake and won't fix it :-(\n>\n> Our HAL implementation is meant to support both Chrome OS and Android.\n> On Android, if the camera service adds the Exif tags, it doesn't matter\n> if we include them or not, applications will always see the same\n> (incorrect) behaviour with the tags included. I'd say that relying on\n> the Android camera service to do so is likely best: the incorrect\n> behaviour is an Android requirement due to bug-to-bug backward\n> compatibility, so letting the Android camera service handle it would\n> make sense. They could then decide to change the behaviour later (which\n> is unlikely though), without needing a modification in libcamera.\n>\n> On Chrome OS, if there's no worry about possibly issues with\n> applications, I think it would be better to not include the tags, to be\n> compliant with the Exif specification, and dropping the corresponding\n> check in [2]. If there's a risk of breakage with existing applications,\n> however, that can be reconsidered.\n>\n\nI understand your opinion. I think there is no issue if HAL doesn't\nproduce a JPEG image with the tags.\nI am going to remove the check in the chromeos camera test.\n\n> One additional question is how widespread applications relying on those\n> tags are. If they're mostly related to Android, the above should hold\n> true. If non-Android applications have started including and/or\n> consuming those tags in JPEG images, then it's a different story as it\n> would mean that the whole world isn't compliant with the specification,\n> and it wouldn't make sense to be pedantic.\n>\n> What do you think ?\n>\n\nI have no idea to be honest.\nIf we get an issue with some apps, let's be re-back to this patch.\n-Hiro\n\n> > [1] https://cs.android.com/android/platform/superproject/+/master:frameworks/base/media/java/android/media/ExifInterface.java;l=2907\n> > [2] https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/platform2/camera/camera3_test/camera3_exif_validator.cc;l=337;drc=82fcae1960c0283214607107037c747c0e545617\n> >\n> > > > > setLong(EXIF_IFD_EXIF, EXIF_TAG_PIXEL_Y_DIMENSION, size.height);\n> > > > > setLong(EXIF_IFD_EXIF, EXIF_TAG_PIXEL_X_DIMENSION, size.width);\n> > > > > }\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","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 28211BD695\n\tfor ;\n\tSat, 3 Apr 2021 11:17:54 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 97AE9602E6;\n\tSat, 3 Apr 2021 13:17:53 +0200 (CEST)","from mail-yb1-xb36.google.com (mail-yb1-xb36.google.com\n\t[IPv6:2607:f8b0:4864:20::b36])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 8E53E602CF\n\tfor ;\n\tSat, 3 Apr 2021 13:17:52 +0200 (CEST)","by mail-yb1-xb36.google.com with SMTP id j2so7557466ybj.8\n\tfor ;\n\tSat, 03 Apr 2021 04:17:52 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"lu2I6f1f\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org;\n\ts=google; \n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=6jpd+vEGGBUy0FGNjuBVGdl1AmHA2RcFjVXJSmtwVSU=;\n\tb=lu2I6f1fyhHO3psTQzQ2S7qAYC+Ix79A07vXnaU2eWDSysv2A/Zy8lNtnONdvYfDME\n\tk3FJN+IzxL25Sb4tjvrCDR9AZbv+0A6MtddScLdJWdl8BZw+zMytSTnqLtA0LF7C9WDX\n\tOqbgrCJXKDQ8hsL0XUIOaco7d3scuKO6iZ77o=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=6jpd+vEGGBUy0FGNjuBVGdl1AmHA2RcFjVXJSmtwVSU=;\n\tb=G6WlfHNahz2rvaPqoZ4TP4r3ApZuC0YqlDwN8sCQFzsBYnk/JyYnSSCs4M6SDuZgM2\n\tRgUYWSbtE8/DYh7l+BvQNf1udzK5WFIQVyf+suV2koSwTo3ndSdf2KH3AE4M+SFWvYn/\n\tP2VXfuxv1y4rv62R1tXWftBW6bbnz7rQgEFY75EJ+j/5zEad0H0Z/984u2pSc/FB/3Vq\n\tuImG2m/7Wl6jNCBe08Kznjm8YwD5NED3ksAgnzP4oRDDJ0tKJu6p/nkGGOBpFXH/t5NS\n\tZdlPmNu7BU2fr+fUjxO3uuOhSjkWOBPQ2czCV1jd0RbpME+dx64/CdY6nD90DNuyG4YX\n\tv9lw==","X-Gm-Message-State":"AOAM531v2YoImXHxMFa+yK3i3VoCDcCeyNFuvajRQlkqwkT94ZSHwVuv\n\tCXItL97hwcN4fpvJZkJtqyTu+Aw3/an7hyztcsWQiHs7r3+0TA==","X-Google-Smtp-Source":"ABdhPJzqlC6FHCq10FudeqEYThCUYhfRM10ngmTLt5kYhcYl4kaHEV1mo8tKZqnSSK8IPPtkQuIZOaMxbkDSLgXmBy0=","X-Received":"by 2002:a25:a345:: with SMTP id\n\td63mr10735494ybi.169.1617448671506; \n\tSat, 03 Apr 2021 04:17:51 -0700 (PDT)","MIME-Version":"1.0","References":"<20210323080231.3389137-1-hiroh@chromium.org>\n\t<6b8b395f-2f2e-4c72-bc2e-5430d1cbb98d@ideasonboard.com>\n\t\n\t\n\t","In-Reply-To":"","From":"Hirokazu Honda ","Date":"Sat, 3 Apr 2021 20:17:40 +0900","Message-ID":"","To":"Laurent Pinchart ","Subject":"Re: [libcamera-devel] [PATCH] android: exif: Contain IMAGE_WIDTH\n\tand IMAGE_LENGTH data","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 ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]