[{"id":14159,"web_url":"https://patchwork.libcamera.org/comment/14159/","msgid":"","date":"2020-12-09T07:26:02","subject":"Re: [libcamera-devel] [PATCH v2 4/4] android: camera_device:\n\tInitialize pixel array properties","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"Camera service on chromeos fails starting from this patch.\nLooks like our camera HAL implementation requires the entry of\nANDROID_SENSOR_INFO_ACTIVE_ARRAY_SIZE.\nShall we add else-statement to the if-statement?\nThat is,\n\n@@ -748,6 +754,13 @@ const camera_metadata_t *CameraDevice::getStaticMetadata()\n };\n staticMetadata_->addEntry(ANDROID_SENSOR_INFO_ACTIVE_ARRAY_SIZE,\n data.data(), data.size());\n+ } else {\n+ int32_t sensorSizes[] = {\n+ 0, 0, 2560, 1920,\n+ };\n+ staticMetadata_->addEntry(ANDROID_SENSOR_INFO_ACTIVE_ARRAY_SIZE,\n+ &sensorSizes, 4);\n+\n }\n\nWith this change, camera on chromeos can start.\nWhat do you think?\n\nBest Regards,\n-Hiro\n\nOn Wed, Dec 2, 2020 at 10:54 PM Jacopo Mondi wrote:\n>\n> Initialize pixel array properties in the Android camera HAL\n> inspecting the camera properties.\n>\n> If the camera does not provide any suitable property, not static\n> metadata is registered to the Android framework.\n>\n> Reviewed-by: Laurent Pinchart \n> Signed-off-by: Jacopo Mondi \n> ---\n> src/android/camera_device.cpp | 33 +++++++++++++++++++++++----------\n> 1 file changed, 23 insertions(+), 10 deletions(-)\n>\n> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> index 675af5705055..017a15cac284 100644\n> --- a/src/android/camera_device.cpp\n> +++ b/src/android/camera_device.cpp\n> @@ -593,6 +593,7 @@ const camera_metadata_t *CameraDevice::getStaticMetadata()\n> }\n>\n> const ControlInfoMap &controlsInfo = camera_->controls();\n> + const ControlList &properties = camera_->properties();\n>\n> /* Color correction static metadata. */\n> {\n> @@ -725,17 +726,29 @@ const camera_metadata_t *CameraDevice::getStaticMetadata()\n> staticMetadata_->addEntry(ANDROID_JPEG_MAX_SIZE, &maxJpegBufferSize_, 1);\n>\n> /* Sensor static metadata. */\n> - int32_t pixelArraySize[] = {\n> - 2592, 1944,\n> - };\n> - staticMetadata_->addEntry(ANDROID_SENSOR_INFO_PIXEL_ARRAY_SIZE,\n> - &pixelArraySize, 2);\n> + if (properties.contains(properties::PixelArraySize)) {\n> + const Size &size =\n> + properties.get(properties::PixelArraySize);\n> + std::vector data{\n> + static_cast(size.width),\n> + static_cast(size.height),\n> + };\n> + staticMetadata_->addEntry(ANDROID_SENSOR_INFO_PIXEL_ARRAY_SIZE,\n> + data.data(), data.size());\n> + }\n>\n> - int32_t sensorSizes[] = {\n> - 0, 0, 2560, 1920,\n> - };\n> - staticMetadata_->addEntry(ANDROID_SENSOR_INFO_ACTIVE_ARRAY_SIZE,\n> - &sensorSizes, 4);\n> + if (properties.contains(properties::PixelArrayActiveAreas)) {\n> + const Span &rects =\n> + properties.get>(properties::PixelArrayActiveAreas);\n> + std::vector data{\n> + static_cast(rects[0].x),\n> + static_cast(rects[0].y),\n> + static_cast(rects[0].width),\n> + static_cast(rects[0].height),\n> + };\n> + staticMetadata_->addEntry(ANDROID_SENSOR_INFO_ACTIVE_ARRAY_SIZE,\n> + data.data(), data.size());\n> + }\n>\n> int32_t sensitivityRange[] = {\n> 32, 2400,\n> --\n> 2.29.1\n>\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 825ADBDE1E\n\tfor ;\n\tWed, 9 Dec 2020 07:26:15 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D125E67E72;\n\tWed, 9 Dec 2020 08:26:14 +0100 (CET)","from mail-ej1-x644.google.com (mail-ej1-x644.google.com\n\t[IPv6:2a00:1450:4864:20::644])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 94484635F3\n\tfor ;\n\tWed, 9 Dec 2020 08:26:13 +0100 (CET)","by mail-ej1-x644.google.com with SMTP id qw4so609441ejb.12\n\tfor ;\n\tTue, 08 Dec 2020 23:26:13 -0800 (PST)"],"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=\"gBnBvE8l\"; 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=tkOhZ/tSboExtaEcA6+rQhVp1iBiy0qs9CKNslbLdwg=;\n\tb=gBnBvE8lYqr492cjOdNHytnSG7zZ6DkJ+LcLISx9hgCTZQq704DB5lehRyqBxYcAER\n\t2RWWC++Li5m+6I6gYwQXDHGfoKS7TtJYZJg2sBiLrOLEQjm07G7699cfIa3gChNjuetZ\n\t/G+0r1ti43Q2d8wxoRO3PMV+aiwmkq6hEhYk4=","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=tkOhZ/tSboExtaEcA6+rQhVp1iBiy0qs9CKNslbLdwg=;\n\tb=CeYazCbhikIO6mimlXP4L1cHmw35SDu/ZG8k23LDbH4YFBL6QiiJj7gv2QKiBSybLw\n\ttaxxx0KkgMr9h7hrN9WvRnWuRXOdqBEPCpr4ovU58tYfnS7y5BuDhUbicI0he6DpaXCY\n\tzr+ZtnKPoWA+lmIg3dthBQjxiPN8068/tsdZ2y+XJd2ZbRArmSlAdBXEK8Fxsf5akmY9\n\tLabTTgLxkcTlu5nIg8zpbQTCH1wUYcK+Vh+1CI87dms/tRMwlXDoFaMnaXfrqOr6dBXb\n\tX2FrqI41ljP2jOM1RTYmykeCfMwFrGPDXgJxXg+ytdOyUDoEGU+ap0otcNwV9HqH3IZO\n\tjrwA==","X-Gm-Message-State":"AOAM530IZuRYnGctKkR6v50KhiexdviPKMLedyYYeELmiLeZk+Vypt1H\n\tVd5VZpnHhB1vitGMrqRdgjv6tqjWke+UbpHcnH67mA==","X-Google-Smtp-Source":"ABdhPJxIUheiliZmKCZooixSgnp+qvMJa+ZDsMb/TxmKvqYSjpMBHO3Ze0UXbFiobOeakU4+Uqj8vAh5QF/y6UQYvRA=","X-Received":"by 2002:a17:906:af83:: with SMTP id\n\tmj3mr900283ejb.243.1607498773231; \n\tTue, 08 Dec 2020 23:26:13 -0800 (PST)","MIME-Version":"1.0","References":"<20201202135354.264212-1-jacopo@jmondi.org>\n\t<20201202135354.264212-5-jacopo@jmondi.org>","In-Reply-To":"<20201202135354.264212-5-jacopo@jmondi.org>","From":"Hirokazu Honda ","Date":"Wed, 9 Dec 2020 16:26:02 +0900","Message-ID":"","To":"Jacopo Mondi ","Subject":"Re: [libcamera-devel] [PATCH v2 4/4] android: camera_device:\n\tInitialize pixel array properties","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":14160,"web_url":"https://patchwork.libcamera.org/comment/14160/","msgid":"<20201209090416.el3b3gotwuowrrjk@uno.localdomain>","date":"2020-12-09T09:04:16","subject":"Re: [libcamera-devel] [PATCH v2 4/4] android: camera_device:\n\tInitialize pixel array properties","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Hiro,\n\nOn Wed, Dec 09, 2020 at 04:26:02PM +0900, Hirokazu Honda wrote:\n> Camera service on chromeos fails starting from this patch.\n\nI wonder why I haven't noticed. I probable have run cros_camera_test\nonly :/\n\n> Looks like our camera HAL implementation requires the entry of\n> ANDROID_SENSOR_INFO_ACTIVE_ARRAY_SIZE.\n> Shall we add else-statement to the if-statement?\n> That is,\n>\n> @@ -748,6 +754,13 @@ const camera_metadata_t *CameraDevice::getStaticMetadata()\n> };\n> staticMetadata_->addEntry(ANDROID_SENSOR_INFO_ACTIVE_ARRAY_SIZE,\n> data.data(), data.size());\n> + } else {\n> + int32_t sensorSizes[] = {\n> + 0, 0, 2560, 1920,\n> + };\n> + staticMetadata_->addEntry(ANDROID_SENSOR_INFO_ACTIVE_ARRAY_SIZE,\n> + &sensorSizes, 4);\n> +\n> }\n>\n> With this change, camera on chromeos can start.\n> What do you think?\n\nThat's the million dollar question. Should the HAL try to default to\nsome arbitrary values properties which are not provided by the sensor\ndriver (the ANDROID_SENSOR_INFO_ACTIVE_ARRAY_SIZE is basically\nretrieved from the V4L2_SEL_TGT_CROP_DEFAULT selection target).\n\nIf you ask me, the drivers should be fixed. It's a trivial operation\nand makes sure we don't report arbitrary values to the Android\nframework.\n\nThanks\n j\n\n>\n> Best Regards,\n> -Hiro\n>\n> On Wed, Dec 2, 2020 at 10:54 PM Jacopo Mondi wrote:\n> >\n> > Initialize pixel array properties in the Android camera HAL\n> > inspecting the camera properties.\n> >\n> > If the camera does not provide any suitable property, not static\n> > metadata is registered to the Android framework.\n> >\n> > Reviewed-by: Laurent Pinchart \n> > Signed-off-by: Jacopo Mondi \n> > ---\n> > src/android/camera_device.cpp | 33 +++++++++++++++++++++++----------\n> > 1 file changed, 23 insertions(+), 10 deletions(-)\n> >\n> > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > index 675af5705055..017a15cac284 100644\n> > --- a/src/android/camera_device.cpp\n> > +++ b/src/android/camera_device.cpp\n> > @@ -593,6 +593,7 @@ const camera_metadata_t *CameraDevice::getStaticMetadata()\n> > }\n> >\n> > const ControlInfoMap &controlsInfo = camera_->controls();\n> > + const ControlList &properties = camera_->properties();\n> >\n> > /* Color correction static metadata. */\n> > {\n> > @@ -725,17 +726,29 @@ const camera_metadata_t *CameraDevice::getStaticMetadata()\n> > staticMetadata_->addEntry(ANDROID_JPEG_MAX_SIZE, &maxJpegBufferSize_, 1);\n> >\n> > /* Sensor static metadata. */\n> > - int32_t pixelArraySize[] = {\n> > - 2592, 1944,\n> > - };\n> > - staticMetadata_->addEntry(ANDROID_SENSOR_INFO_PIXEL_ARRAY_SIZE,\n> > - &pixelArraySize, 2);\n> > + if (properties.contains(properties::PixelArraySize)) {\n> > + const Size &size =\n> > + properties.get(properties::PixelArraySize);\n> > + std::vector data{\n> > + static_cast(size.width),\n> > + static_cast(size.height),\n> > + };\n> > + staticMetadata_->addEntry(ANDROID_SENSOR_INFO_PIXEL_ARRAY_SIZE,\n> > + data.data(), data.size());\n> > + }\n> >\n> > - int32_t sensorSizes[] = {\n> > - 0, 0, 2560, 1920,\n> > - };\n> > - staticMetadata_->addEntry(ANDROID_SENSOR_INFO_ACTIVE_ARRAY_SIZE,\n> > - &sensorSizes, 4);\n> > + if (properties.contains(properties::PixelArrayActiveAreas)) {\n> > + const Span &rects =\n> > + properties.get>(properties::PixelArrayActiveAreas);\n> > + std::vector data{\n> > + static_cast(rects[0].x),\n> > + static_cast(rects[0].y),\n> > + static_cast(rects[0].width),\n> > + static_cast(rects[0].height),\n> > + };\n> > + staticMetadata_->addEntry(ANDROID_SENSOR_INFO_ACTIVE_ARRAY_SIZE,\n> > + data.data(), data.size());\n> > + }\n> >\n> > int32_t sensitivityRange[] = {\n> > 32, 2400,\n> > --\n> > 2.29.1\n> >\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 0374CBDE19\n\tfor ;\n\tWed, 9 Dec 2020 09:04:11 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7084167E4D;\n\tWed, 9 Dec 2020 10:04:10 +0100 (CET)","from relay11.mail.gandi.net (relay11.mail.gandi.net\n\t[217.70.178.231])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A8E19635C4\n\tfor ;\n\tWed, 9 Dec 2020 10:04:08 +0100 (CET)","from uno.localdomain (93-34-118-233.ip49.fastwebnet.it\n\t[93.34.118.233]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay11.mail.gandi.net (Postfix) with ESMTPSA id E08E1100005;\n\tWed, 9 Dec 2020 09:04:07 +0000 (UTC)"],"Date":"Wed, 9 Dec 2020 10:04:16 +0100","From":"Jacopo Mondi ","To":"Hirokazu Honda ","Message-ID":"<20201209090416.el3b3gotwuowrrjk@uno.localdomain>","References":"<20201202135354.264212-1-jacopo@jmondi.org>\n\t<20201202135354.264212-5-jacopo@jmondi.org>\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v2 4/4] android: camera_device:\n\tInitialize pixel array properties","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":14163,"web_url":"https://patchwork.libcamera.org/comment/14163/","msgid":"","date":"2020-12-09T10:10:56","subject":"Re: [libcamera-devel] [PATCH v2 4/4] android: camera_device:\n\tInitialize pixel array properties","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"On Wed, Dec 9, 2020 at 6:04 PM Jacopo Mondi wrote:\n>\n> Hi Hiro,\n>\n> On Wed, Dec 09, 2020 at 04:26:02PM +0900, Hirokazu Honda wrote:\n> > Camera service on chromeos fails starting from this patch.\n>\n> I wonder why I haven't noticed. I probable have run cros_camera_test\n> only :/\n>\n> > Looks like our camera HAL implementation requires the entry of\n> > ANDROID_SENSOR_INFO_ACTIVE_ARRAY_SIZE.\n> > Shall we add else-statement to the if-statement?\n> > That is,\n> >\n> > @@ -748,6 +754,13 @@ const camera_metadata_t *CameraDevice::getStaticMetadata()\n> > };\n> > staticMetadata_->addEntry(ANDROID_SENSOR_INFO_ACTIVE_ARRAY_SIZE,\n> > data.data(), data.size());\n> > + } else {\n> > + int32_t sensorSizes[] = {\n> > + 0, 0, 2560, 1920,\n> > + };\n> > + staticMetadata_->addEntry(ANDROID_SENSOR_INFO_ACTIVE_ARRAY_SIZE,\n> > + &sensorSizes, 4);\n> > +\n> > }\n> >\n> > With this change, camera on chromeos can start.\n> > What do you think?\n>\n> That's the million dollar question. Should the HAL try to default to\n> some arbitrary values properties which are not provided by the sensor\n> driver (the ANDROID_SENSOR_INFO_ACTIVE_ARRAY_SIZE is basically\n> retrieved from the V4L2_SEL_TGT_CROP_DEFAULT selection target).\n>\n> If you ask me, the drivers should be fixed. It's a trivial operation\n> and makes sure we don't report arbitrary values to the Android\n> framework.\n>\n\n+Hanlin Chen +Tomasz Figa\nTomasz, can you take a look the driver implementation on soraka-libcamera?\n\nRegards,\n-Hiro\n\n> Thanks\n> j\n>\n> >\n> > Best Regards,\n> > -Hiro\n> >\n> > On Wed, Dec 2, 2020 at 10:54 PM Jacopo Mondi wrote:\n> > >\n> > > Initialize pixel array properties in the Android camera HAL\n> > > inspecting the camera properties.\n> > >\n> > > If the camera does not provide any suitable property, not static\n> > > metadata is registered to the Android framework.\n> > >\n> > > Reviewed-by: Laurent Pinchart \n> > > Signed-off-by: Jacopo Mondi \n> > > ---\n> > > src/android/camera_device.cpp | 33 +++++++++++++++++++++++----------\n> > > 1 file changed, 23 insertions(+), 10 deletions(-)\n> > >\n> > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > > index 675af5705055..017a15cac284 100644\n> > > --- a/src/android/camera_device.cpp\n> > > +++ b/src/android/camera_device.cpp\n> > > @@ -593,6 +593,7 @@ const camera_metadata_t *CameraDevice::getStaticMetadata()\n> > > }\n> > >\n> > > const ControlInfoMap &controlsInfo = camera_->controls();\n> > > + const ControlList &properties = camera_->properties();\n> > >\n> > > /* Color correction static metadata. */\n> > > {\n> > > @@ -725,17 +726,29 @@ const camera_metadata_t *CameraDevice::getStaticMetadata()\n> > > staticMetadata_->addEntry(ANDROID_JPEG_MAX_SIZE, &maxJpegBufferSize_, 1);\n> > >\n> > > /* Sensor static metadata. */\n> > > - int32_t pixelArraySize[] = {\n> > > - 2592, 1944,\n> > > - };\n> > > - staticMetadata_->addEntry(ANDROID_SENSOR_INFO_PIXEL_ARRAY_SIZE,\n> > > - &pixelArraySize, 2);\n> > > + if (properties.contains(properties::PixelArraySize)) {\n> > > + const Size &size =\n> > > + properties.get(properties::PixelArraySize);\n> > > + std::vector data{\n> > > + static_cast(size.width),\n> > > + static_cast(size.height),\n> > > + };\n> > > + staticMetadata_->addEntry(ANDROID_SENSOR_INFO_PIXEL_ARRAY_SIZE,\n> > > + data.data(), data.size());\n> > > + }\n> > >\n> > > - int32_t sensorSizes[] = {\n> > > - 0, 0, 2560, 1920,\n> > > - };\n> > > - staticMetadata_->addEntry(ANDROID_SENSOR_INFO_ACTIVE_ARRAY_SIZE,\n> > > - &sensorSizes, 4);\n> > > + if (properties.contains(properties::PixelArrayActiveAreas)) {\n> > > + const Span &rects =\n> > > + properties.get>(properties::PixelArrayActiveAreas);\n> > > + std::vector data{\n> > > + static_cast(rects[0].x),\n> > > + static_cast(rects[0].y),\n> > > + static_cast(rects[0].width),\n> > > + static_cast(rects[0].height),\n> > > + };\n> > > + staticMetadata_->addEntry(ANDROID_SENSOR_INFO_ACTIVE_ARRAY_SIZE,\n> > > + data.data(), data.size());\n> > > + }\n> > >\n> > > int32_t sensitivityRange[] = {\n> > > 32, 2400,\n> > > --\n> > > 2.29.1\n> > >\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 8C43EBD808\n\tfor ;\n\tWed, 9 Dec 2020 10:11:10 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E8A1667E4D;\n\tWed, 9 Dec 2020 11:11:09 +0100 (CET)","from mail-ej1-x644.google.com (mail-ej1-x644.google.com\n\t[IPv6:2a00:1450:4864:20::644])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1C928635C4\n\tfor ;\n\tWed, 9 Dec 2020 11:11:08 +0100 (CET)","by mail-ej1-x644.google.com with SMTP id bo9so1248053ejb.13\n\tfor ;\n\tWed, 09 Dec 2020 02:11:08 -0800 (PST)"],"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=\"a+IelVrf\"; 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=yazPXMagKwKmRQ2RJQIWoSUT88Nu6sCRLrijQJc+Le0=;\n\tb=a+IelVrftoJN5QgWufx6cXXf+eKtXphbK1wjmGarILy4M2IPBleA52OGAm2Zjdoefh\n\tEgkFs/KlYrAXpa9CwR3LIy4cNdbBnkMwIUAFKN3nK7/aoHPMSO4EvHMEHrP3htfNEIbP\n\tIKtMY+CLiFexEH2OrD5FraScF14QwGAdlRHDg=","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=yazPXMagKwKmRQ2RJQIWoSUT88Nu6sCRLrijQJc+Le0=;\n\tb=TqXubSRF4ubI35HN6FS8T3jW57Qv124WbJVJBy45ZGOKQp76ngzZE1v9m1H09WIJpR\n\tMvHcXe//HK8y+TOLQX+sts5BqIWL9CL3FrxiVX+npQnKn13NkxUG/fGSOM9hJTSgooOu\n\tFOm+HomFi96D2fleU6l0FCSRXp/rXhvHOWG22ChsiZ6q1FglysMMbPTTeoUqWSIuzkbw\n\t9btEsim/uLK4ZhyXGDNw5knQWhrqiwXXlxuRdhiFWlMkkd8iSa17WFQOSGUkPtSlUGsL\n\thylMIAu8P3FZUVUz9hklX2kX94fmiGklcCWCmkmz7lMJxIff5G/+IgIytrDhwxSKhL+e\n\thwaA==","X-Gm-Message-State":"AOAM532nOulBEgdfc810+fMgLb9+vaPVz4yY56vEtni8ktyEDdnxLcwk\n\tE/72JV2nYjiNkacdY/85zrji1cKV25RcXdgKelbI2sOKj8t+pA==","X-Google-Smtp-Source":"ABdhPJzBgpZtXaEWfT8j/xMKgZzio379cnvrqwcZktfuc363rJakvWNSJFCFK8r+7fWpan4eQ7NZCbPNndKb1Bu6sKU=","X-Received":"by 2002:a17:907:9691:: with SMTP id\n\thd17mr1375309ejc.306.1607508667683; \n\tWed, 09 Dec 2020 02:11:07 -0800 (PST)","MIME-Version":"1.0","References":"<20201202135354.264212-1-jacopo@jmondi.org>\n\t<20201202135354.264212-5-jacopo@jmondi.org>\n\t\n\t<20201209090416.el3b3gotwuowrrjk@uno.localdomain>","In-Reply-To":"<20201209090416.el3b3gotwuowrrjk@uno.localdomain>","From":"Hirokazu Honda ","Date":"Wed, 9 Dec 2020 19:10:56 +0900","Message-ID":"","To":"Jacopo Mondi , Hanlin Chen , \n\tTomasz Figa ","Subject":"Re: [libcamera-devel] [PATCH v2 4/4] android: camera_device:\n\tInitialize pixel array properties","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":14177,"web_url":"https://patchwork.libcamera.org/comment/14177/","msgid":"<20201209161708.yuqzsxcikjlqkdjs@uno.localdomain>","date":"2020-12-09T16:17:08","subject":"Re: [libcamera-devel] [PATCH v2 4/4] android: camera_device:\n\tInitialize pixel array properties","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hello,\n\nOn Wed, Dec 09, 2020 at 07:10:56PM +0900, Hirokazu Honda wrote:\n> On Wed, Dec 9, 2020 at 6:04 PM Jacopo Mondi wrote:\n> >\n> > Hi Hiro,\n> >\n> > On Wed, Dec 09, 2020 at 04:26:02PM +0900, Hirokazu Honda wrote:\n> > > Camera service on chromeos fails starting from this patch.\n> >\n> > I wonder why I haven't noticed. I probable have run cros_camera_test\n> > only :/\n> >\n> > > Looks like our camera HAL implementation requires the entry of\n> > > ANDROID_SENSOR_INFO_ACTIVE_ARRAY_SIZE.\n> > > Shall we add else-statement to the if-statement?\n> > > That is,\n> > >\n> > > @@ -748,6 +754,13 @@ const camera_metadata_t *CameraDevice::getStaticMetadata()\n> > > };\n> > > staticMetadata_->addEntry(ANDROID_SENSOR_INFO_ACTIVE_ARRAY_SIZE,\n> > > data.data(), data.size());\n> > > + } else {\n> > > + int32_t sensorSizes[] = {\n> > > + 0, 0, 2560, 1920,\n> > > + };\n> > > + staticMetadata_->addEntry(ANDROID_SENSOR_INFO_ACTIVE_ARRAY_SIZE,\n> > > + &sensorSizes, 4);\n> > > +\n> > > }\n> > >\n> > > With this change, camera on chromeos can start.\n> > > What do you think?\n> >\n> > That's the million dollar question. Should the HAL try to default to\n> > some arbitrary values properties which are not provided by the sensor\n> > driver (the ANDROID_SENSOR_INFO_ACTIVE_ARRAY_SIZE is basically\n> > retrieved from the V4L2_SEL_TGT_CROP_DEFAULT selection target).\n> >\n> > If you ask me, the drivers should be fixed. It's a trivial operation\n> > and makes sure we don't report arbitrary values to the Android\n> > framework.\n> >\n>\n> +Hanlin Chen +Tomasz Figa\n> Tomasz, can you take a look the driver implementation on soraka-libcamera?\n\nThe patches are trivial. They're available here\nhttps://jmondi.org/cgit/linux/log/?h=soraka/g_selection\n(based on the most recent media-master)\n\nI've not been able to test them (Tomasz has details) otherwise I would\nhave sent them upstream.\n\nWhat is the procedure to have them integrated on your side ?\n\nThanks\n j\n\n\n>\n> Regards,\n> -Hiro\n>\n> > Thanks\n> > j\n> >\n> > >\n> > > Best Regards,\n> > > -Hiro\n> > >\n> > > On Wed, Dec 2, 2020 at 10:54 PM Jacopo Mondi wrote:\n> > > >\n> > > > Initialize pixel array properties in the Android camera HAL\n> > > > inspecting the camera properties.\n> > > >\n> > > > If the camera does not provide any suitable property, not static\n> > > > metadata is registered to the Android framework.\n> > > >\n> > > > Reviewed-by: Laurent Pinchart \n> > > > Signed-off-by: Jacopo Mondi \n> > > > ---\n> > > > src/android/camera_device.cpp | 33 +++++++++++++++++++++++----------\n> > > > 1 file changed, 23 insertions(+), 10 deletions(-)\n> > > >\n> > > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > > > index 675af5705055..017a15cac284 100644\n> > > > --- a/src/android/camera_device.cpp\n> > > > +++ b/src/android/camera_device.cpp\n> > > > @@ -593,6 +593,7 @@ const camera_metadata_t *CameraDevice::getStaticMetadata()\n> > > > }\n> > > >\n> > > > const ControlInfoMap &controlsInfo = camera_->controls();\n> > > > + const ControlList &properties = camera_->properties();\n> > > >\n> > > > /* Color correction static metadata. */\n> > > > {\n> > > > @@ -725,17 +726,29 @@ const camera_metadata_t *CameraDevice::getStaticMetadata()\n> > > > staticMetadata_->addEntry(ANDROID_JPEG_MAX_SIZE, &maxJpegBufferSize_, 1);\n> > > >\n> > > > /* Sensor static metadata. */\n> > > > - int32_t pixelArraySize[] = {\n> > > > - 2592, 1944,\n> > > > - };\n> > > > - staticMetadata_->addEntry(ANDROID_SENSOR_INFO_PIXEL_ARRAY_SIZE,\n> > > > - &pixelArraySize, 2);\n> > > > + if (properties.contains(properties::PixelArraySize)) {\n> > > > + const Size &size =\n> > > > + properties.get(properties::PixelArraySize);\n> > > > + std::vector data{\n> > > > + static_cast(size.width),\n> > > > + static_cast(size.height),\n> > > > + };\n> > > > + staticMetadata_->addEntry(ANDROID_SENSOR_INFO_PIXEL_ARRAY_SIZE,\n> > > > + data.data(), data.size());\n> > > > + }\n> > > >\n> > > > - int32_t sensorSizes[] = {\n> > > > - 0, 0, 2560, 1920,\n> > > > - };\n> > > > - staticMetadata_->addEntry(ANDROID_SENSOR_INFO_ACTIVE_ARRAY_SIZE,\n> > > > - &sensorSizes, 4);\n> > > > + if (properties.contains(properties::PixelArrayActiveAreas)) {\n> > > > + const Span &rects =\n> > > > + properties.get>(properties::PixelArrayActiveAreas);\n> > > > + std::vector data{\n> > > > + static_cast(rects[0].x),\n> > > > + static_cast(rects[0].y),\n> > > > + static_cast(rects[0].width),\n> > > > + static_cast(rects[0].height),\n> > > > + };\n> > > > + staticMetadata_->addEntry(ANDROID_SENSOR_INFO_ACTIVE_ARRAY_SIZE,\n> > > > + data.data(), data.size());\n> > > > + }\n> > > >\n> > > > int32_t sensitivityRange[] = {\n> > > > 32, 2400,\n> > > > --\n> > > > 2.29.1\n> > > >\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 160BFBD80A\n\tfor ;\n\tWed, 9 Dec 2020 16:17:02 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A1BB1635F3;\n\tWed, 9 Dec 2020 17:17:01 +0100 (CET)","from relay7-d.mail.gandi.net (relay7-d.mail.gandi.net\n\t[217.70.183.200])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5A877600F9\n\tfor ;\n\tWed, 9 Dec 2020 17:17:00 +0100 (CET)","from uno.localdomain (93-34-118-233.ip49.fastwebnet.it\n\t[93.34.118.233]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay7-d.mail.gandi.net (Postfix) with ESMTPSA id 708AA2000C;\n\tWed, 9 Dec 2020 16:16:59 +0000 (UTC)"],"X-Originating-IP":"93.34.118.233","Date":"Wed, 9 Dec 2020 17:17:08 +0100","From":"Jacopo Mondi ","To":"Hirokazu Honda ","Message-ID":"<20201209161708.yuqzsxcikjlqkdjs@uno.localdomain>","References":"<20201202135354.264212-1-jacopo@jmondi.org>\n\t<20201202135354.264212-5-jacopo@jmondi.org>\n\t\n\t<20201209090416.el3b3gotwuowrrjk@uno.localdomain>\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v2 4/4] android: camera_device:\n\tInitialize pixel array properties","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":"Hanlin Chen ,\n\tlibcamera-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":14178,"web_url":"https://patchwork.libcamera.org/comment/14178/","msgid":"","date":"2020-12-10T04:10:19","subject":"Re: [libcamera-devel] [PATCH v2 4/4] android: camera_device:\n\tInitialize pixel array properties","submitter":{"id":9,"url":"https://patchwork.libcamera.org/api/people/9/","name":"Tomasz Figa","email":"tfiga@chromium.org"},"content":"Hi Jacopo,\n\nOn Thu, Dec 10, 2020 at 1:17 AM Jacopo Mondi wrote:\n>\n> Hello,\n>\n> On Wed, Dec 09, 2020 at 07:10:56PM +0900, Hirokazu Honda wrote:\n> > On Wed, Dec 9, 2020 at 6:04 PM Jacopo Mondi wrote:\n> > >\n> > > Hi Hiro,\n> > >\n> > > On Wed, Dec 09, 2020 at 04:26:02PM +0900, Hirokazu Honda wrote:\n> > > > Camera service on chromeos fails starting from this patch.\n> > >\n> > > I wonder why I haven't noticed. I probable have run cros_camera_test\n> > > only :/\n> > >\n> > > > Looks like our camera HAL implementation requires the entry of\n> > > > ANDROID_SENSOR_INFO_ACTIVE_ARRAY_SIZE.\n> > > > Shall we add else-statement to the if-statement?\n> > > > That is,\n> > > >\n> > > > @@ -748,6 +754,13 @@ const camera_metadata_t *CameraDevice::getStaticMetadata()\n> > > > };\n> > > > staticMetadata_->addEntry(ANDROID_SENSOR_INFO_ACTIVE_ARRAY_SIZE,\n> > > > data.data(), data.size());\n> > > > + } else {\n> > > > + int32_t sensorSizes[] = {\n> > > > + 0, 0, 2560, 1920,\n> > > > + };\n> > > > + staticMetadata_->addEntry(ANDROID_SENSOR_INFO_ACTIVE_ARRAY_SIZE,\n> > > > + &sensorSizes, 4);\n> > > > +\n> > > > }\n> > > >\n> > > > With this change, camera on chromeos can start.\n> > > > What do you think?\n> > >\n> > > That's the million dollar question. Should the HAL try to default to\n> > > some arbitrary values properties which are not provided by the sensor\n> > > driver (the ANDROID_SENSOR_INFO_ACTIVE_ARRAY_SIZE is basically\n> > > retrieved from the V4L2_SEL_TGT_CROP_DEFAULT selection target).\n> > >\n> > > If you ask me, the drivers should be fixed. It's a trivial operation\n> > > and makes sure we don't report arbitrary values to the Android\n> > > framework.\n\nDoesn't the active array size in V4L2 correspond to the resolution as\nset by S_FMT? Right now we don't have any provisions to support\noptically black pixels or other inactive pixels and it was always\nassumed that all the pixels in the mode are active.\n\n> > >\n> >\n> > +Hanlin Chen +Tomasz Figa\n> > Tomasz, can you take a look the driver implementation on soraka-libcamera?\n>\n> The patches are trivial. They're available here\n> https://jmondi.org/cgit/linux/log/?h=soraka/g_selection\n> (based on the most recent media-master)\n>\n> I've not been able to test them (Tomasz has details) otherwise I would\n> have sent them upstream.\n>\n> What is the procedure to have them integrated on your side ?\n\nThe patch needs to be submitted upstream and ideally merged to the\nmaintainer tree, so that we can cherry pick directly from upstream. In\njustified cases we can also pick a work in progress patch from the\nmailing list, but we try to avoid it as much as possible.\n\nThere is some more information on our kernel development process in\nhttps://chromium.googlesource.com/chromiumos/docs/+/master/kernel_development.md#upstream_backport_fromlist_and-you\n.\n\n>\n> Thanks\n> j\n>\n>\n> >\n> > Regards,\n> > -Hiro\n> >\n> > > Thanks\n> > > j\n> > >\n> > > >\n> > > > Best Regards,\n> > > > -Hiro\n> > > >\n> > > > On Wed, Dec 2, 2020 at 10:54 PM Jacopo Mondi wrote:\n> > > > >\n> > > > > Initialize pixel array properties in the Android camera HAL\n> > > > > inspecting the camera properties.\n> > > > >\n> > > > > If the camera does not provide any suitable property, not static\n> > > > > metadata is registered to the Android framework.\n> > > > >\n> > > > > Reviewed-by: Laurent Pinchart \n> > > > > Signed-off-by: Jacopo Mondi \n> > > > > ---\n> > > > > src/android/camera_device.cpp | 33 +++++++++++++++++++++++----------\n> > > > > 1 file changed, 23 insertions(+), 10 deletions(-)\n> > > > >\n> > > > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > > > > index 675af5705055..017a15cac284 100644\n> > > > > --- a/src/android/camera_device.cpp\n> > > > > +++ b/src/android/camera_device.cpp\n> > > > > @@ -593,6 +593,7 @@ const camera_metadata_t *CameraDevice::getStaticMetadata()\n> > > > > }\n> > > > >\n> > > > > const ControlInfoMap &controlsInfo = camera_->controls();\n> > > > > + const ControlList &properties = camera_->properties();\n> > > > >\n> > > > > /* Color correction static metadata. */\n> > > > > {\n> > > > > @@ -725,17 +726,29 @@ const camera_metadata_t *CameraDevice::getStaticMetadata()\n> > > > > staticMetadata_->addEntry(ANDROID_JPEG_MAX_SIZE, &maxJpegBufferSize_, 1);\n> > > > >\n> > > > > /* Sensor static metadata. */\n> > > > > - int32_t pixelArraySize[] = {\n> > > > > - 2592, 1944,\n> > > > > - };\n> > > > > - staticMetadata_->addEntry(ANDROID_SENSOR_INFO_PIXEL_ARRAY_SIZE,\n> > > > > - &pixelArraySize, 2);\n> > > > > + if (properties.contains(properties::PixelArraySize)) {\n> > > > > + const Size &size =\n> > > > > + properties.get(properties::PixelArraySize);\n> > > > > + std::vector data{\n> > > > > + static_cast(size.width),\n> > > > > + static_cast(size.height),\n> > > > > + };\n> > > > > + staticMetadata_->addEntry(ANDROID_SENSOR_INFO_PIXEL_ARRAY_SIZE,\n> > > > > + data.data(), data.size());\n> > > > > + }\n> > > > >\n> > > > > - int32_t sensorSizes[] = {\n> > > > > - 0, 0, 2560, 1920,\n> > > > > - };\n> > > > > - staticMetadata_->addEntry(ANDROID_SENSOR_INFO_ACTIVE_ARRAY_SIZE,\n> > > > > - &sensorSizes, 4);\n> > > > > + if (properties.contains(properties::PixelArrayActiveAreas)) {\n> > > > > + const Span &rects =\n> > > > > + properties.get>(properties::PixelArrayActiveAreas);\n> > > > > + std::vector data{\n> > > > > + static_cast(rects[0].x),\n> > > > > + static_cast(rects[0].y),\n> > > > > + static_cast(rects[0].width),\n> > > > > + static_cast(rects[0].height),\n> > > > > + };\n> > > > > + staticMetadata_->addEntry(ANDROID_SENSOR_INFO_ACTIVE_ARRAY_SIZE,\n> > > > > + data.data(), data.size());\n> > > > > + }\n> > > > >\n> > > > > int32_t sensitivityRange[] = {\n> > > > > 32, 2400,\n> > > > > --\n> > > > > 2.29.1\n> > > > >\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 5F6FDBD80A\n\tfor ;\n\tThu, 10 Dec 2020 04:10:35 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D80C167F6C;\n\tThu, 10 Dec 2020 05:10:34 +0100 (CET)","from mail-ed1-x541.google.com (mail-ed1-x541.google.com\n\t[IPv6:2a00:1450:4864:20::541])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 403CA60322\n\tfor ;\n\tThu, 10 Dec 2020 05:10:33 +0100 (CET)","by mail-ed1-x541.google.com with SMTP id v22so4015893edt.9\n\tfor ;\n\tWed, 09 Dec 2020 20:10:33 -0800 (PST)","from mail-wm1-f53.google.com (mail-wm1-f53.google.com.\n\t[209.85.128.53]) by smtp.gmail.com with ESMTPSA id\n\tt19sm3264672eje.86.2020.12.09.20.10.31\n\tfor \n\t(version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128);\n\tWed, 09 Dec 2020 20:10:31 -0800 (PST)","by mail-wm1-f53.google.com with SMTP id a3so3871022wmb.5\n\tfor ;\n\tWed, 09 Dec 2020 20:10:31 -0800 (PST)"],"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=\"RX9GBQbh\"; 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=dQJkkBzUzuoAPVGp9W99zKs5LOCa1o9dRRWryNEGQsY=;\n\tb=RX9GBQbhBatB4ZD77Ab6VsOBCyRYMZIXEowDgUyVO/fCf7Iqaht6RGil8KM89/xZbJ\n\tzBN6B4uF4pdoNZcEuOC2M/jykHt9AREijM6OTejRZNhL/eX7cHAyKc3Ubu8qRGEiPEga\n\tpW7dqjPkNe7be5lbxMkSn5MrYABKnuHwAIVkM=","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=dQJkkBzUzuoAPVGp9W99zKs5LOCa1o9dRRWryNEGQsY=;\n\tb=Op7BNSQ6MKBwWo2UiLWdU6+UWZebcAWG5tZ2FwKYb9cyt4JMHAS6fy11VgyovZnblz\n\to/QjAaF1lh1pFJ6EN6QkW04PhvFrBYDdgzKHz3eKTsq9f2m4n69ffmx/KvHDcd/zCW+q\n\t+lzda2tCzmO6shG4bFB2evAqpLsjOebLzzahbW5yHOHetkTFad7DnpAQEjmPIZk1/8Sh\n\tMxHuDP5hi+vnEjERUxaSfROVNqDO5EGSrRphY5Jo0tlRTiyRB19Qvbon982++TfCK1Gw\n\tqCCHX3Ar9+fWp4lgL6smLvf9yCc7cahLx29Mf+pwI7qtQ5EVkUxfIR9Kuj3hK4vfC0YP\n\toL7w==","X-Gm-Message-State":"AOAM532el+SgfAZn+wQlt2Vb6r5jsQKpnsWMUUIkOl5Y3hiQ3lfghSYi\n\twFFi16nn2TCgx15n4Q9oojLHfKhV4Y83cQ==","X-Google-Smtp-Source":"ABdhPJxqiPW8gZOQ7FMKHnirA3OzrB+sI1CrbFvwipXTZed7Cq4B2aFGpDZ4p2a6RvXHu9TgPSypyw==","X-Received":["by 2002:a05:6402:139a:: with SMTP id\n\tb26mr4931393edv.47.1607573432174; \n\tWed, 09 Dec 2020 20:10:32 -0800 (PST)","by 2002:a1c:63d4:: with SMTP id\n\tx203mr5456865wmb.28.1607573430814; \n\tWed, 09 Dec 2020 20:10:30 -0800 (PST)"],"MIME-Version":"1.0","References":"<20201202135354.264212-1-jacopo@jmondi.org>\n\t<20201202135354.264212-5-jacopo@jmondi.org>\n\t\n\t<20201209090416.el3b3gotwuowrrjk@uno.localdomain>\n\t\n\t<20201209161708.yuqzsxcikjlqkdjs@uno.localdomain>","In-Reply-To":"<20201209161708.yuqzsxcikjlqkdjs@uno.localdomain>","From":"Tomasz Figa ","Date":"Thu, 10 Dec 2020 13:10:19 +0900","X-Gmail-Original-Message-ID":"","Message-ID":"","To":"Jacopo Mondi ","Subject":"Re: [libcamera-devel] [PATCH v2 4/4] android: camera_device:\n\tInitialize pixel array properties","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":"Hanlin Chen ,\n\tlibcamera devel ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":14191,"web_url":"https://patchwork.libcamera.org/comment/14191/","msgid":"<20201210102959.hk5nagqwzup2irxu@uno.localdomain>","date":"2020-12-10T10:29:59","subject":"Re: [libcamera-devel] [PATCH v2 4/4] android: camera_device:\n\tInitialize pixel array properties","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Tomasz,\n\nOn Thu, Dec 10, 2020 at 01:10:19PM +0900, Tomasz Figa wrote:\n> Hi Jacopo,\n>\n> On Thu, Dec 10, 2020 at 1:17 AM Jacopo Mondi wrote:\n> >\n> > Hello,\n> >\n> > On Wed, Dec 09, 2020 at 07:10:56PM +0900, Hirokazu Honda wrote:\n> > > On Wed, Dec 9, 2020 at 6:04 PM Jacopo Mondi wrote:\n> > > >\n> > > > Hi Hiro,\n> > > >\n> > > > On Wed, Dec 09, 2020 at 04:26:02PM +0900, Hirokazu Honda wrote:\n> > > > > Camera service on chromeos fails starting from this patch.\n> > > >\n> > > > I wonder why I haven't noticed. I probable have run cros_camera_test\n> > > > only :/\n> > > >\n> > > > > Looks like our camera HAL implementation requires the entry of\n> > > > > ANDROID_SENSOR_INFO_ACTIVE_ARRAY_SIZE.\n> > > > > Shall we add else-statement to the if-statement?\n> > > > > That is,\n> > > > >\n> > > > > @@ -748,6 +754,13 @@ const camera_metadata_t *CameraDevice::getStaticMetadata()\n> > > > > };\n> > > > > staticMetadata_->addEntry(ANDROID_SENSOR_INFO_ACTIVE_ARRAY_SIZE,\n> > > > > data.data(), data.size());\n> > > > > + } else {\n> > > > > + int32_t sensorSizes[] = {\n> > > > > + 0, 0, 2560, 1920,\n> > > > > + };\n> > > > > + staticMetadata_->addEntry(ANDROID_SENSOR_INFO_ACTIVE_ARRAY_SIZE,\n> > > > > + &sensorSizes, 4);\n> > > > > +\n> > > > > }\n> > > > >\n> > > > > With this change, camera on chromeos can start.\n> > > > > What do you think?\n> > > >\n> > > > That's the million dollar question. Should the HAL try to default to\n> > > > some arbitrary values properties which are not provided by the sensor\n> > > > driver (the ANDROID_SENSOR_INFO_ACTIVE_ARRAY_SIZE is basically\n> > > > retrieved from the V4L2_SEL_TGT_CROP_DEFAULT selection target).\n> > > >\n> > > > If you ask me, the drivers should be fixed. It's a trivial operation\n> > > > and makes sure we don't report arbitrary values to the Android\n> > > > framework.\n>\n> Doesn't the active array size in V4L2 correspond to the resolution as\n> set by S_FMT? Right now we don't have any provisions to support\n\nNot necessarly. The sensor driver might expose modes might be smaller\nthan the active array size, or modes that overlaps to support\ndifferent frame resolutions (ie 4:3 = [x, y]; 16:9 = [x1, y1] and\n(x > x1; y1 > x).\n\nIt's very much up to sensor driver and the active array size is a\nsensor property that should not depend on the current implementation.\n\nThe discussion about how V4L2 selection targets should be used/updated\nto more clearly expose these information is quest I've attempted to\nstart a few months ago then dropped the ball as it seems there's no\nclear consensus on the direction.\n\nRight now (I think) it's safer to rely on drivers reporting these\ninformation through the existing selection targets with the\nassociation:\n\nNATIVE = physical pixel matrix size\nBOUNDS = readable area of the physical pixel array size (black and\n valid pixels)\nDEFAULT = valid and readable area of the physical pixel matrix\n\n> optically black pixels or other inactive pixels and it was always\n> assumed that all the pixels in the mode are active.\n>\n> > > >\n> > >\n> > > +Hanlin Chen +Tomasz Figa\n> > > Tomasz, can you take a look the driver implementation on soraka-libcamera?\n> >\n> > The patches are trivial. They're available here\n> > https://jmondi.org/cgit/linux/log/?h=soraka/g_selection\n> > (based on the most recent media-master)\n> >\n> > I've not been able to test them (Tomasz has details) otherwise I would\n> > have sent them upstream.\n> >\n> > What is the procedure to have them integrated on your side ?\n>\n> The patch needs to be submitted upstream and ideally merged to the\n> maintainer tree, so that we can cherry pick directly from upstream. In\n> justified cases we can also pick a work in progress patch from the\n> mailing list, but we try to avoid it as much as possible.\n>\n> There is some more information on our kernel development process in\n> https://chromium.googlesource.com/chromiumos/docs/+/master/kernel_development.md#upstream_backport_fromlist_and-you\n> .\n>\n> >\n> > Thanks\n> > j\n> >\n> >\n> > >\n> > > Regards,\n> > > -Hiro\n> > >\n> > > > Thanks\n> > > > j\n> > > >\n> > > > >\n> > > > > Best Regards,\n> > > > > -Hiro\n> > > > >\n> > > > > On Wed, Dec 2, 2020 at 10:54 PM Jacopo Mondi wrote:\n> > > > > >\n> > > > > > Initialize pixel array properties in the Android camera HAL\n> > > > > > inspecting the camera properties.\n> > > > > >\n> > > > > > If the camera does not provide any suitable property, not static\n> > > > > > metadata is registered to the Android framework.\n> > > > > >\n> > > > > > Reviewed-by: Laurent Pinchart \n> > > > > > Signed-off-by: Jacopo Mondi \n> > > > > > ---\n> > > > > > src/android/camera_device.cpp | 33 +++++++++++++++++++++++----------\n> > > > > > 1 file changed, 23 insertions(+), 10 deletions(-)\n> > > > > >\n> > > > > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > > > > > index 675af5705055..017a15cac284 100644\n> > > > > > --- a/src/android/camera_device.cpp\n> > > > > > +++ b/src/android/camera_device.cpp\n> > > > > > @@ -593,6 +593,7 @@ const camera_metadata_t *CameraDevice::getStaticMetadata()\n> > > > > > }\n> > > > > >\n> > > > > > const ControlInfoMap &controlsInfo = camera_->controls();\n> > > > > > + const ControlList &properties = camera_->properties();\n> > > > > >\n> > > > > > /* Color correction static metadata. */\n> > > > > > {\n> > > > > > @@ -725,17 +726,29 @@ const camera_metadata_t *CameraDevice::getStaticMetadata()\n> > > > > > staticMetadata_->addEntry(ANDROID_JPEG_MAX_SIZE, &maxJpegBufferSize_, 1);\n> > > > > >\n> > > > > > /* Sensor static metadata. */\n> > > > > > - int32_t pixelArraySize[] = {\n> > > > > > - 2592, 1944,\n> > > > > > - };\n> > > > > > - staticMetadata_->addEntry(ANDROID_SENSOR_INFO_PIXEL_ARRAY_SIZE,\n> > > > > > - &pixelArraySize, 2);\n> > > > > > + if (properties.contains(properties::PixelArraySize)) {\n> > > > > > + const Size &size =\n> > > > > > + properties.get(properties::PixelArraySize);\n> > > > > > + std::vector data{\n> > > > > > + static_cast(size.width),\n> > > > > > + static_cast(size.height),\n> > > > > > + };\n> > > > > > + staticMetadata_->addEntry(ANDROID_SENSOR_INFO_PIXEL_ARRAY_SIZE,\n> > > > > > + data.data(), data.size());\n> > > > > > + }\n> > > > > >\n> > > > > > - int32_t sensorSizes[] = {\n> > > > > > - 0, 0, 2560, 1920,\n> > > > > > - };\n> > > > > > - staticMetadata_->addEntry(ANDROID_SENSOR_INFO_ACTIVE_ARRAY_SIZE,\n> > > > > > - &sensorSizes, 4);\n> > > > > > + if (properties.contains(properties::PixelArrayActiveAreas)) {\n> > > > > > + const Span &rects =\n> > > > > > + properties.get>(properties::PixelArrayActiveAreas);\n> > > > > > + std::vector data{\n> > > > > > + static_cast(rects[0].x),\n> > > > > > + static_cast(rects[0].y),\n> > > > > > + static_cast(rects[0].width),\n> > > > > > + static_cast(rects[0].height),\n> > > > > > + };\n> > > > > > + staticMetadata_->addEntry(ANDROID_SENSOR_INFO_ACTIVE_ARRAY_SIZE,\n> > > > > > + data.data(), data.size());\n> > > > > > + }\n> > > > > >\n> > > > > > int32_t sensitivityRange[] = {\n> > > > > > 32, 2400,\n> > > > > > --\n> > > > > > 2.29.1\n> > > > > >\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 04FEFBD80A\n\tfor ;\n\tThu, 10 Dec 2020 10:29:54 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 660C267F72;\n\tThu, 10 Dec 2020 11:29:53 +0100 (CET)","from relay1-d.mail.gandi.net (relay1-d.mail.gandi.net\n\t[217.70.183.193])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id F04B860323\n\tfor ;\n\tThu, 10 Dec 2020 11:29:51 +0100 (CET)","from uno.localdomain (93-34-118-233.ip49.fastwebnet.it\n\t[93.34.118.233]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay1-d.mail.gandi.net (Postfix) with ESMTPSA id B395124000D;\n\tThu, 10 Dec 2020 10:29:50 +0000 (UTC)"],"X-Originating-IP":"93.34.118.233","Date":"Thu, 10 Dec 2020 11:29:59 +0100","From":"Jacopo Mondi ","To":"Tomasz Figa ","Message-ID":"<20201210102959.hk5nagqwzup2irxu@uno.localdomain>","References":"<20201202135354.264212-1-jacopo@jmondi.org>\n\t<20201202135354.264212-5-jacopo@jmondi.org>\n\t\n\t<20201209090416.el3b3gotwuowrrjk@uno.localdomain>\n\t\n\t<20201209161708.yuqzsxcikjlqkdjs@uno.localdomain>\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v2 4/4] android: camera_device:\n\tInitialize pixel array properties","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":"Hanlin Chen ,\n\tlibcamera devel ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":14193,"web_url":"https://patchwork.libcamera.org/comment/14193/","msgid":"","date":"2020-12-10T10:35:51","subject":"Re: [libcamera-devel] [PATCH v2 4/4] android: camera_device:\n\tInitialize pixel array properties","submitter":{"id":9,"url":"https://patchwork.libcamera.org/api/people/9/","name":"Tomasz Figa","email":"tfiga@chromium.org"},"content":"On Thu, Dec 10, 2020 at 7:29 PM Jacopo Mondi wrote:\n>\n> Hi Tomasz,\n>\n> On Thu, Dec 10, 2020 at 01:10:19PM +0900, Tomasz Figa wrote:\n> > Hi Jacopo,\n> >\n> > On Thu, Dec 10, 2020 at 1:17 AM Jacopo Mondi wrote:\n> > >\n> > > Hello,\n> > >\n> > > On Wed, Dec 09, 2020 at 07:10:56PM +0900, Hirokazu Honda wrote:\n> > > > On Wed, Dec 9, 2020 at 6:04 PM Jacopo Mondi wrote:\n> > > > >\n> > > > > Hi Hiro,\n> > > > >\n> > > > > On Wed, Dec 09, 2020 at 04:26:02PM +0900, Hirokazu Honda wrote:\n> > > > > > Camera service on chromeos fails starting from this patch.\n> > > > >\n> > > > > I wonder why I haven't noticed. I probable have run cros_camera_test\n> > > > > only :/\n> > > > >\n> > > > > > Looks like our camera HAL implementation requires the entry of\n> > > > > > ANDROID_SENSOR_INFO_ACTIVE_ARRAY_SIZE.\n> > > > > > Shall we add else-statement to the if-statement?\n> > > > > > That is,\n> > > > > >\n> > > > > > @@ -748,6 +754,13 @@ const camera_metadata_t *CameraDevice::getStaticMetadata()\n> > > > > > };\n> > > > > > staticMetadata_->addEntry(ANDROID_SENSOR_INFO_ACTIVE_ARRAY_SIZE,\n> > > > > > data.data(), data.size());\n> > > > > > + } else {\n> > > > > > + int32_t sensorSizes[] = {\n> > > > > > + 0, 0, 2560, 1920,\n> > > > > > + };\n> > > > > > + staticMetadata_->addEntry(ANDROID_SENSOR_INFO_ACTIVE_ARRAY_SIZE,\n> > > > > > + &sensorSizes, 4);\n> > > > > > +\n> > > > > > }\n> > > > > >\n> > > > > > With this change, camera on chromeos can start.\n> > > > > > What do you think?\n> > > > >\n> > > > > That's the million dollar question. Should the HAL try to default to\n> > > > > some arbitrary values properties which are not provided by the sensor\n> > > > > driver (the ANDROID_SENSOR_INFO_ACTIVE_ARRAY_SIZE is basically\n> > > > > retrieved from the V4L2_SEL_TGT_CROP_DEFAULT selection target).\n> > > > >\n> > > > > If you ask me, the drivers should be fixed. It's a trivial operation\n> > > > > and makes sure we don't report arbitrary values to the Android\n> > > > > framework.\n> >\n> > Doesn't the active array size in V4L2 correspond to the resolution as\n> > set by S_FMT? Right now we don't have any provisions to support\n>\n> Not necessarly. The sensor driver might expose modes might be smaller\n> than the active array size, or modes that overlaps to support\n> different frame resolutions (ie 4:3 = [x, y]; 16:9 = [x1, y1] and\n> (x > x1; y1 > x).\n\nGood point.\n\n>\n> It's very much up to sensor driver and the active array size is a\n> sensor property that should not depend on the current implementation.\n>\n> The discussion about how V4L2 selection targets should be used/updated\n> to more clearly expose these information is quest I've attempted to\n> start a few months ago then dropped the ball as it seems there's no\n> clear consensus on the direction.\n>\n> Right now (I think) it's safer to rely on drivers reporting these\n> information through the existing selection targets with the\n> association:\n>\n> NATIVE = physical pixel matrix size\n> BOUNDS = readable area of the physical pixel array size (black and\n> valid pixels)\n> DEFAULT = valid and readable area of the physical pixel matrix\n\nMakes sense. We should try to get this definition merged upstream.\n\n>\n> > optically black pixels or other inactive pixels and it was always\n> > assumed that all the pixels in the mode are active.\n> >\n> > > > >\n> > > >\n> > > > +Hanlin Chen +Tomasz Figa\n> > > > Tomasz, can you take a look the driver implementation on soraka-libcamera?\n> > >\n> > > The patches are trivial. They're available here\n> > > https://jmondi.org/cgit/linux/log/?h=soraka/g_selection\n> > > (based on the most recent media-master)\n> > >\n> > > I've not been able to test them (Tomasz has details) otherwise I would\n> > > have sent them upstream.\n> > >\n> > > What is the procedure to have them integrated on your side ?\n> >\n> > The patch needs to be submitted upstream and ideally merged to the\n> > maintainer tree, so that we can cherry pick directly from upstream. In\n> > justified cases we can also pick a work in progress patch from the\n> > mailing list, but we try to avoid it as much as possible.\n> >\n> > There is some more information on our kernel development process in\n> > https://chromium.googlesource.com/chromiumos/docs/+/master/kernel_development.md#upstream_backport_fromlist_and-you\n> > .\n> >\n> > >\n> > > Thanks\n> > > j\n> > >\n> > >\n> > > >\n> > > > Regards,\n> > > > -Hiro\n> > > >\n> > > > > Thanks\n> > > > > j\n> > > > >\n> > > > > >\n> > > > > > Best Regards,\n> > > > > > -Hiro\n> > > > > >\n> > > > > > On Wed, Dec 2, 2020 at 10:54 PM Jacopo Mondi wrote:\n> > > > > > >\n> > > > > > > Initialize pixel array properties in the Android camera HAL\n> > > > > > > inspecting the camera properties.\n> > > > > > >\n> > > > > > > If the camera does not provide any suitable property, not static\n> > > > > > > metadata is registered to the Android framework.\n> > > > > > >\n> > > > > > > Reviewed-by: Laurent Pinchart \n> > > > > > > Signed-off-by: Jacopo Mondi \n> > > > > > > ---\n> > > > > > > src/android/camera_device.cpp | 33 +++++++++++++++++++++++----------\n> > > > > > > 1 file changed, 23 insertions(+), 10 deletions(-)\n> > > > > > >\n> > > > > > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > > > > > > index 675af5705055..017a15cac284 100644\n> > > > > > > --- a/src/android/camera_device.cpp\n> > > > > > > +++ b/src/android/camera_device.cpp\n> > > > > > > @@ -593,6 +593,7 @@ const camera_metadata_t *CameraDevice::getStaticMetadata()\n> > > > > > > }\n> > > > > > >\n> > > > > > > const ControlInfoMap &controlsInfo = camera_->controls();\n> > > > > > > + const ControlList &properties = camera_->properties();\n> > > > > > >\n> > > > > > > /* Color correction static metadata. */\n> > > > > > > {\n> > > > > > > @@ -725,17 +726,29 @@ const camera_metadata_t *CameraDevice::getStaticMetadata()\n> > > > > > > staticMetadata_->addEntry(ANDROID_JPEG_MAX_SIZE, &maxJpegBufferSize_, 1);\n> > > > > > >\n> > > > > > > /* Sensor static metadata. */\n> > > > > > > - int32_t pixelArraySize[] = {\n> > > > > > > - 2592, 1944,\n> > > > > > > - };\n> > > > > > > - staticMetadata_->addEntry(ANDROID_SENSOR_INFO_PIXEL_ARRAY_SIZE,\n> > > > > > > - &pixelArraySize, 2);\n> > > > > > > + if (properties.contains(properties::PixelArraySize)) {\n> > > > > > > + const Size &size =\n> > > > > > > + properties.get(properties::PixelArraySize);\n> > > > > > > + std::vector data{\n> > > > > > > + static_cast(size.width),\n> > > > > > > + static_cast(size.height),\n> > > > > > > + };\n> > > > > > > + staticMetadata_->addEntry(ANDROID_SENSOR_INFO_PIXEL_ARRAY_SIZE,\n> > > > > > > + data.data(), data.size());\n> > > > > > > + }\n> > > > > > >\n> > > > > > > - int32_t sensorSizes[] = {\n> > > > > > > - 0, 0, 2560, 1920,\n> > > > > > > - };\n> > > > > > > - staticMetadata_->addEntry(ANDROID_SENSOR_INFO_ACTIVE_ARRAY_SIZE,\n> > > > > > > - &sensorSizes, 4);\n> > > > > > > + if (properties.contains(properties::PixelArrayActiveAreas)) {\n> > > > > > > + const Span &rects =\n> > > > > > > + properties.get>(properties::PixelArrayActiveAreas);\n> > > > > > > + std::vector data{\n> > > > > > > + static_cast(rects[0].x),\n> > > > > > > + static_cast(rects[0].y),\n> > > > > > > + static_cast(rects[0].width),\n> > > > > > > + static_cast(rects[0].height),\n> > > > > > > + };\n> > > > > > > + staticMetadata_->addEntry(ANDROID_SENSOR_INFO_ACTIVE_ARRAY_SIZE,\n> > > > > > > + data.data(), data.size());\n> > > > > > > + }\n> > > > > > >\n> > > > > > > int32_t sensitivityRange[] = {\n> > > > > > > 32, 2400,\n> > > > > > > --\n> > > > > > > 2.29.1\n> > > > > > >\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 7265ABD808\n\tfor ;\n\tThu, 10 Dec 2020 10:36:07 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id ED78267F76;\n\tThu, 10 Dec 2020 11:36:06 +0100 (CET)","from mail-ej1-x641.google.com (mail-ej1-x641.google.com\n\t[IPv6:2a00:1450:4864:20::641])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 40A3F67F2D\n\tfor ;\n\tThu, 10 Dec 2020 11:36:05 +0100 (CET)","by mail-ej1-x641.google.com with SMTP id n26so6616150eju.6\n\tfor ;\n\tThu, 10 Dec 2020 02:36:05 -0800 (PST)","from mail-wm1-f42.google.com (mail-wm1-f42.google.com.\n\t[209.85.128.42]) by smtp.gmail.com with ESMTPSA id\n\tb14sm4678818edm.68.2020.12.10.02.36.03\n\tfor \n\t(version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128);\n\tThu, 10 Dec 2020 02:36:03 -0800 (PST)","by mail-wm1-f42.google.com with SMTP id g25so2507203wmh.1\n\tfor ;\n\tThu, 10 Dec 2020 02:36:03 -0800 (PST)"],"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=\"d3iBHZNs\"; 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=JnQelaWSGUUUAB0d0Vt8phrMPeM3V9M3S0wezk8ii7s=;\n\tb=d3iBHZNslW5czjVkVeegy2/kaa1bvzKAthxSO0vAcQT2iKQCYDJuuXvHDdDbnizFp7\n\tKgLUJ0YXO7O0Aa+/3/eJFk30m7MouFeKhO8sbnbr7QH6BkWASSjGaI7fKveVvxb5ttqc\n\tdG8CR2cA94J6chnpbTO3aaSaC+FCCp2zvzp3Y=","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=JnQelaWSGUUUAB0d0Vt8phrMPeM3V9M3S0wezk8ii7s=;\n\tb=rJ2g4U62mGYSRXJlRWqfGjmI2mbjjjemOBAzmo3AlJp2e5LUyGha1dsgnWzrG05Naj\n\tE7VnrNaNbxOPf1o9sPck8N/NQH5vgtpWWnjHReJZw7DgHpDsCQrMDn2o34GYYI6cM7Y6\n\tmlT3YNz6GmnRLPc+CLaGaqUvH/9udp98hLHQdoLckASJWs59KlMO9JNM0uEmGZSxKfiR\n\tecabtbBN+MGbrHLrLFdiCHPWrFQbQAS6LW/R+w/Z7+MB8C0RFK3YbXyyCcpSw42EgkY4\n\tt9o/Xl1vvJFdt2ELKDp77Vf5+VRocDrr6B3XfyL/XQg4uIJck1AHSqocuo+O5wFSUJgB\n\tss5g==","X-Gm-Message-State":"AOAM531v1HXjned/4XG0clpi/zDoqC6AWUC/3lCeH6+o/gCuW1ljTeIV\n\tno1x76CngQB6/y189WHmrBW00yCWk2ydAGMP","X-Google-Smtp-Source":"ABdhPJyd/qtvnuqVw5i4L+wWsM5+9qtdC9I+IMKc3VCNBI/uy8PTYt1hbwTDipgzYOgrBQGv/DI4fA==","X-Received":["by 2002:a17:906:3b4d:: with SMTP id\n\th13mr5679982ejf.289.1607596564585; \n\tThu, 10 Dec 2020 02:36:04 -0800 (PST)","by 2002:a1c:5605:: with SMTP id k5mr7411557wmb.99.1607596563260; \n\tThu, 10 Dec 2020 02:36:03 -0800 (PST)"],"MIME-Version":"1.0","References":"<20201202135354.264212-1-jacopo@jmondi.org>\n\t<20201202135354.264212-5-jacopo@jmondi.org>\n\t\n\t<20201209090416.el3b3gotwuowrrjk@uno.localdomain>\n\t\n\t<20201209161708.yuqzsxcikjlqkdjs@uno.localdomain>\n\t\n\t<20201210102959.hk5nagqwzup2irxu@uno.localdomain>","In-Reply-To":"<20201210102959.hk5nagqwzup2irxu@uno.localdomain>","From":"Tomasz Figa ","Date":"Thu, 10 Dec 2020 19:35:51 +0900","X-Gmail-Original-Message-ID":"","Message-ID":"","To":"Jacopo Mondi ","Subject":"Re: [libcamera-devel] [PATCH v2 4/4] android: camera_device:\n\tInitialize pixel array properties","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":"Hanlin Chen ,\n\tlibcamera devel ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":14201,"web_url":"https://patchwork.libcamera.org/comment/14201/","msgid":"","date":"2020-12-10T16:28:29","subject":"Re: [libcamera-devel] [PATCH v2 4/4] android: camera_device:\n\tInitialize pixel array properties","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Wed, Dec 09, 2020 at 05:17:08PM +0100, Jacopo Mondi wrote:\n> On Wed, Dec 09, 2020 at 07:10:56PM +0900, Hirokazu Honda wrote:\n> > On Wed, Dec 9, 2020 at 6:04 PM Jacopo Mondi wrote:\n> > > On Wed, Dec 09, 2020 at 04:26:02PM +0900, Hirokazu Honda wrote:\n> > > > Camera service on chromeos fails starting from this patch.\n> > >\n> > > I wonder why I haven't noticed. I probable have run cros_camera_test\n> > > only :/\n> > >\n> > > > Looks like our camera HAL implementation requires the entry of\n> > > > ANDROID_SENSOR_INFO_ACTIVE_ARRAY_SIZE.\n> > > > Shall we add else-statement to the if-statement?\n> > > > That is,\n> > > >\n> > > > @@ -748,6 +754,13 @@ const camera_metadata_t *CameraDevice::getStaticMetadata()\n> > > > };\n> > > > staticMetadata_->addEntry(ANDROID_SENSOR_INFO_ACTIVE_ARRAY_SIZE,\n> > > > data.data(), data.size());\n> > > > + } else {\n> > > > + int32_t sensorSizes[] = {\n> > > > + 0, 0, 2560, 1920,\n> > > > + };\n> > > > + staticMetadata_->addEntry(ANDROID_SENSOR_INFO_ACTIVE_ARRAY_SIZE,\n> > > > + &sensorSizes, 4);\n> > > > +\n> > > > }\n> > > >\n> > > > With this change, camera on chromeos can start.\n> > > > What do you think?\n> > >\n> > > That's the million dollar question. Should the HAL try to default to\n> > > some arbitrary values properties which are not provided by the sensor\n> > > driver (the ANDROID_SENSOR_INFO_ACTIVE_ARRAY_SIZE is basically\n> > > retrieved from the V4L2_SEL_TGT_CROP_DEFAULT selection target).\n> > >\n> > > If you ask me, the drivers should be fixed. It's a trivial operation\n> > > and makes sure we don't report arbitrary values to the Android\n> > > framework.\n> > >\n> >\n> > +Hanlin Chen +Tomasz Figa\n> > Tomasz, can you take a look the driver implementation on soraka-libcamera?\n> \n> The patches are trivial. They're available here\n> https://jmondi.org/cgit/linux/log/?h=soraka/g_selection\n> (based on the most recent media-master)\n> \n> I've not been able to test them (Tomasz has details) otherwise I would\n> have sent them upstream.\n\nTo avoid blocking this series, how about merging it with the above\nworkaround (with a \\todo comment), which we'll remove as soon as the\nsensor patches can get integrated ?\n\n> What is the procedure to have them integrated on your side ?\n>\n> > > > On Wed, Dec 2, 2020 at 10:54 PM Jacopo Mondi wrote:\n> > > > >\n> > > > > Initialize pixel array properties in the Android camera HAL\n> > > > > inspecting the camera properties.\n> > > > >\n> > > > > If the camera does not provide any suitable property, not static\n> > > > > metadata is registered to the Android framework.\n> > > > >\n> > > > > Reviewed-by: Laurent Pinchart \n> > > > > Signed-off-by: Jacopo Mondi \n> > > > > ---\n> > > > > src/android/camera_device.cpp | 33 +++++++++++++++++++++++----------\n> > > > > 1 file changed, 23 insertions(+), 10 deletions(-)\n> > > > >\n> > > > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > > > > index 675af5705055..017a15cac284 100644\n> > > > > --- a/src/android/camera_device.cpp\n> > > > > +++ b/src/android/camera_device.cpp\n> > > > > @@ -593,6 +593,7 @@ const camera_metadata_t *CameraDevice::getStaticMetadata()\n> > > > > }\n> > > > >\n> > > > > const ControlInfoMap &controlsInfo = camera_->controls();\n> > > > > + const ControlList &properties = camera_->properties();\n> > > > >\n> > > > > /* Color correction static metadata. */\n> > > > > {\n> > > > > @@ -725,17 +726,29 @@ const camera_metadata_t *CameraDevice::getStaticMetadata()\n> > > > > staticMetadata_->addEntry(ANDROID_JPEG_MAX_SIZE, &maxJpegBufferSize_, 1);\n> > > > >\n> > > > > /* Sensor static metadata. */\n> > > > > - int32_t pixelArraySize[] = {\n> > > > > - 2592, 1944,\n> > > > > - };\n> > > > > - staticMetadata_->addEntry(ANDROID_SENSOR_INFO_PIXEL_ARRAY_SIZE,\n> > > > > - &pixelArraySize, 2);\n> > > > > + if (properties.contains(properties::PixelArraySize)) {\n> > > > > + const Size &size =\n> > > > > + properties.get(properties::PixelArraySize);\n> > > > > + std::vector data{\n> > > > > + static_cast(size.width),\n> > > > > + static_cast(size.height),\n> > > > > + };\n> > > > > + staticMetadata_->addEntry(ANDROID_SENSOR_INFO_PIXEL_ARRAY_SIZE,\n> > > > > + data.data(), data.size());\n> > > > > + }\n> > > > >\n> > > > > - int32_t sensorSizes[] = {\n> > > > > - 0, 0, 2560, 1920,\n> > > > > - };\n> > > > > - staticMetadata_->addEntry(ANDROID_SENSOR_INFO_ACTIVE_ARRAY_SIZE,\n> > > > > - &sensorSizes, 4);\n> > > > > + if (properties.contains(properties::PixelArrayActiveAreas)) {\n> > > > > + const Span &rects =\n> > > > > + properties.get>(properties::PixelArrayActiveAreas);\n> > > > > + std::vector data{\n> > > > > + static_cast(rects[0].x),\n> > > > > + static_cast(rects[0].y),\n> > > > > + static_cast(rects[0].width),\n> > > > > + static_cast(rects[0].height),\n> > > > > + };\n> > > > > + staticMetadata_->addEntry(ANDROID_SENSOR_INFO_ACTIVE_ARRAY_SIZE,\n> > > > > + data.data(), data.size());\n> > > > > + }\n> > > > >\n> > > > > int32_t sensitivityRange[] = {\n> > > > > 32, 2400,","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 E121BBD808\n\tfor ;\n\tThu, 10 Dec 2020 16:28:36 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 775C067F75;\n\tThu, 10 Dec 2020 17:28:36 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 844B567F6E\n\tfor ;\n\tThu, 10 Dec 2020 17:28:35 +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 E6FC825E;\n\tThu, 10 Dec 2020 17:28:34 +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=\"IlPMBgXL\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1607617715;\n\tbh=57B7lMVh2TbhMlfk3nG+3y+WkUjihkoECqnO1KUeCpI=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=IlPMBgXLoAsgzEZG7drBBh5mz1mEk394nXDMMziJG+5/d7RmtJQF7oZaDs2deHLyu\n\tWq/cJWW/IWeBmGa2e5I2XgByi7tZJipiRXjczaMzvGN4925NxORuusfK1Goqe8Xzte\n\trhn0EetWqMaRMOSsf3rv+OKrrqRE3gOU8xR0Gv1I=","Date":"Thu, 10 Dec 2020 18:28:29 +0200","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"","References":"<20201202135354.264212-1-jacopo@jmondi.org>\n\t<20201202135354.264212-5-jacopo@jmondi.org>\n\t\n\t<20201209090416.el3b3gotwuowrrjk@uno.localdomain>\n\t\n\t<20201209161708.yuqzsxcikjlqkdjs@uno.localdomain>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20201209161708.yuqzsxcikjlqkdjs@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH v2 4/4] android: camera_device:\n\tInitialize pixel array properties","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":"Hanlin Chen ,\n\tlibcamera-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":14202,"web_url":"https://patchwork.libcamera.org/comment/14202/","msgid":"<20201210163928.3bylrctquumam24m@uno.localdomain>","date":"2020-12-10T16:39:28","subject":"Re: [libcamera-devel] [PATCH v2 4/4] android: camera_device:\n\tInitialize pixel array properties","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent,\n\nOn Thu, Dec 10, 2020 at 06:28:29PM +0200, Laurent Pinchart wrote:\n> Hi Jacopo,\n>\n> On Wed, Dec 09, 2020 at 05:17:08PM +0100, Jacopo Mondi wrote:\n> > On Wed, Dec 09, 2020 at 07:10:56PM +0900, Hirokazu Honda wrote:\n> > > On Wed, Dec 9, 2020 at 6:04 PM Jacopo Mondi wrote:\n> > > > On Wed, Dec 09, 2020 at 04:26:02PM +0900, Hirokazu Honda wrote:\n> > > > > Camera service on chromeos fails starting from this patch.\n> > > >\n> > > > I wonder why I haven't noticed. I probable have run cros_camera_test\n> > > > only :/\n> > > >\n> > > > > Looks like our camera HAL implementation requires the entry of\n> > > > > ANDROID_SENSOR_INFO_ACTIVE_ARRAY_SIZE.\n> > > > > Shall we add else-statement to the if-statement?\n> > > > > That is,\n> > > > >\n> > > > > @@ -748,6 +754,13 @@ const camera_metadata_t *CameraDevice::getStaticMetadata()\n> > > > > };\n> > > > > staticMetadata_->addEntry(ANDROID_SENSOR_INFO_ACTIVE_ARRAY_SIZE,\n> > > > > data.data(), data.size());\n> > > > > + } else {\n> > > > > + int32_t sensorSizes[] = {\n> > > > > + 0, 0, 2560, 1920,\n> > > > > + };\n> > > > > + staticMetadata_->addEntry(ANDROID_SENSOR_INFO_ACTIVE_ARRAY_SIZE,\n> > > > > + &sensorSizes, 4);\n> > > > > +\n> > > > > }\n> > > > >\n> > > > > With this change, camera on chromeos can start.\n> > > > > What do you think?\n> > > >\n> > > > That's the million dollar question. Should the HAL try to default to\n> > > > some arbitrary values properties which are not provided by the sensor\n> > > > driver (the ANDROID_SENSOR_INFO_ACTIVE_ARRAY_SIZE is basically\n> > > > retrieved from the V4L2_SEL_TGT_CROP_DEFAULT selection target).\n> > > >\n> > > > If you ask me, the drivers should be fixed. It's a trivial operation\n> > > > and makes sure we don't report arbitrary values to the Android\n> > > > framework.\n> > > >\n> > >\n> > > +Hanlin Chen +Tomasz Figa\n> > > Tomasz, can you take a look the driver implementation on soraka-libcamera?\n> >\n> > The patches are trivial. They're available here\n> > https://jmondi.org/cgit/linux/log/?h=soraka/g_selection\n> > (based on the most recent media-master)\n> >\n> > I've not been able to test them (Tomasz has details) otherwise I would\n> > have sent them upstream.\n>\n> To avoid blocking this series, how about merging it with the above\n> workaround (with a \\todo comment), which we'll remove as soon as the\n> sensor patches can get integrated ?\n\nThe series is merged. It's libcamera master that's now broken on\nsoraka. Should we temporary address this by defaulting\nANDROID_SENSOR_INFO_ACTIVE_ARRAY_SIZE (and ANDROID_SENSOR_INFO_PIXEL_ARRAY_SIZE\nwhich the framework requires as well) as Hiro proposed ?\n\nI have a patch I'm carrying in my local tree, I can easily send it\nout.\n\n>\n> > What is the procedure to have them integrated on your side ?\n> >\n> > > > > On Wed, Dec 2, 2020 at 10:54 PM Jacopo Mondi wrote:\n> > > > > >\n> > > > > > Initialize pixel array properties in the Android camera HAL\n> > > > > > inspecting the camera properties.\n> > > > > >\n> > > > > > If the camera does not provide any suitable property, not static\n> > > > > > metadata is registered to the Android framework.\n> > > > > >\n> > > > > > Reviewed-by: Laurent Pinchart \n> > > > > > Signed-off-by: Jacopo Mondi \n> > > > > > ---\n> > > > > > src/android/camera_device.cpp | 33 +++++++++++++++++++++++----------\n> > > > > > 1 file changed, 23 insertions(+), 10 deletions(-)\n> > > > > >\n> > > > > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > > > > > index 675af5705055..017a15cac284 100644\n> > > > > > --- a/src/android/camera_device.cpp\n> > > > > > +++ b/src/android/camera_device.cpp\n> > > > > > @@ -593,6 +593,7 @@ const camera_metadata_t *CameraDevice::getStaticMetadata()\n> > > > > > }\n> > > > > >\n> > > > > > const ControlInfoMap &controlsInfo = camera_->controls();\n> > > > > > + const ControlList &properties = camera_->properties();\n> > > > > >\n> > > > > > /* Color correction static metadata. */\n> > > > > > {\n> > > > > > @@ -725,17 +726,29 @@ const camera_metadata_t *CameraDevice::getStaticMetadata()\n> > > > > > staticMetadata_->addEntry(ANDROID_JPEG_MAX_SIZE, &maxJpegBufferSize_, 1);\n> > > > > >\n> > > > > > /* Sensor static metadata. */\n> > > > > > - int32_t pixelArraySize[] = {\n> > > > > > - 2592, 1944,\n> > > > > > - };\n> > > > > > - staticMetadata_->addEntry(ANDROID_SENSOR_INFO_PIXEL_ARRAY_SIZE,\n> > > > > > - &pixelArraySize, 2);\n> > > > > > + if (properties.contains(properties::PixelArraySize)) {\n> > > > > > + const Size &size =\n> > > > > > + properties.get(properties::PixelArraySize);\n> > > > > > + std::vector data{\n> > > > > > + static_cast(size.width),\n> > > > > > + static_cast(size.height),\n> > > > > > + };\n> > > > > > + staticMetadata_->addEntry(ANDROID_SENSOR_INFO_PIXEL_ARRAY_SIZE,\n> > > > > > + data.data(), data.size());\n> > > > > > + }\n> > > > > >\n> > > > > > - int32_t sensorSizes[] = {\n> > > > > > - 0, 0, 2560, 1920,\n> > > > > > - };\n> > > > > > - staticMetadata_->addEntry(ANDROID_SENSOR_INFO_ACTIVE_ARRAY_SIZE,\n> > > > > > - &sensorSizes, 4);\n> > > > > > + if (properties.contains(properties::PixelArrayActiveAreas)) {\n> > > > > > + const Span &rects =\n> > > > > > + properties.get>(properties::PixelArrayActiveAreas);\n> > > > > > + std::vector data{\n> > > > > > + static_cast(rects[0].x),\n> > > > > > + static_cast(rects[0].y),\n> > > > > > + static_cast(rects[0].width),\n> > > > > > + static_cast(rects[0].height),\n> > > > > > + };\n> > > > > > + staticMetadata_->addEntry(ANDROID_SENSOR_INFO_ACTIVE_ARRAY_SIZE,\n> > > > > > + data.data(), data.size());\n> > > > > > + }\n> > > > > >\n> > > > > > int32_t sensitivityRange[] = {\n> > > > > > 32, 2400,\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 607C3BD808\n\tfor ;\n\tThu, 10 Dec 2020 16:39:22 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 05D7C67F78;\n\tThu, 10 Dec 2020 17:39:22 +0100 (CET)","from relay3-d.mail.gandi.net (relay3-d.mail.gandi.net\n\t[217.70.183.195])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 0FF2067F6E\n\tfor ;\n\tThu, 10 Dec 2020 17:39:21 +0100 (CET)","from uno.localdomain (93-34-118-233.ip49.fastwebnet.it\n\t[93.34.118.233]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay3-d.mail.gandi.net (Postfix) with ESMTPSA id 46F8860008;\n\tThu, 10 Dec 2020 16:39:19 +0000 (UTC)"],"X-Originating-IP":"93.34.118.233","Date":"Thu, 10 Dec 2020 17:39:28 +0100","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Message-ID":"<20201210163928.3bylrctquumam24m@uno.localdomain>","References":"<20201202135354.264212-1-jacopo@jmondi.org>\n\t<20201202135354.264212-5-jacopo@jmondi.org>\n\t\n\t<20201209090416.el3b3gotwuowrrjk@uno.localdomain>\n\t\n\t<20201209161708.yuqzsxcikjlqkdjs@uno.localdomain>\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v2 4/4] android: camera_device:\n\tInitialize pixel array properties","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":"Hanlin Chen ,\n\tlibcamera-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":14204,"web_url":"https://patchwork.libcamera.org/comment/14204/","msgid":"","date":"2020-12-10T17:44:41","subject":"Re: [libcamera-devel] [PATCH v2 4/4] android: camera_device:\n\tInitialize pixel array properties","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Thu, Dec 10, 2020 at 05:39:28PM +0100, Jacopo Mondi wrote:\n> On Thu, Dec 10, 2020 at 06:28:29PM +0200, Laurent Pinchart wrote:\n> > On Wed, Dec 09, 2020 at 05:17:08PM +0100, Jacopo Mondi wrote:\n> > > On Wed, Dec 09, 2020 at 07:10:56PM +0900, Hirokazu Honda wrote:\n> > > > On Wed, Dec 9, 2020 at 6:04 PM Jacopo Mondi wrote:\n> > > > > On Wed, Dec 09, 2020 at 04:26:02PM +0900, Hirokazu Honda wrote:\n> > > > > > Camera service on chromeos fails starting from this patch.\n> > > > >\n> > > > > I wonder why I haven't noticed. I probable have run cros_camera_test\n> > > > > only :/\n> > > > >\n> > > > > > Looks like our camera HAL implementation requires the entry of\n> > > > > > ANDROID_SENSOR_INFO_ACTIVE_ARRAY_SIZE.\n> > > > > > Shall we add else-statement to the if-statement?\n> > > > > > That is,\n> > > > > >\n> > > > > > @@ -748,6 +754,13 @@ const camera_metadata_t *CameraDevice::getStaticMetadata()\n> > > > > > };\n> > > > > > staticMetadata_->addEntry(ANDROID_SENSOR_INFO_ACTIVE_ARRAY_SIZE,\n> > > > > > data.data(), data.size());\n> > > > > > + } else {\n> > > > > > + int32_t sensorSizes[] = {\n> > > > > > + 0, 0, 2560, 1920,\n> > > > > > + };\n> > > > > > + staticMetadata_->addEntry(ANDROID_SENSOR_INFO_ACTIVE_ARRAY_SIZE,\n> > > > > > + &sensorSizes, 4);\n> > > > > > +\n> > > > > > }\n> > > > > >\n> > > > > > With this change, camera on chromeos can start.\n> > > > > > What do you think?\n> > > > >\n> > > > > That's the million dollar question. Should the HAL try to default to\n> > > > > some arbitrary values properties which are not provided by the sensor\n> > > > > driver (the ANDROID_SENSOR_INFO_ACTIVE_ARRAY_SIZE is basically\n> > > > > retrieved from the V4L2_SEL_TGT_CROP_DEFAULT selection target).\n> > > > >\n> > > > > If you ask me, the drivers should be fixed. It's a trivial operation\n> > > > > and makes sure we don't report arbitrary values to the Android\n> > > > > framework.\n> > > > >\n> > > >\n> > > > +Hanlin Chen +Tomasz Figa\n> > > > Tomasz, can you take a look the driver implementation on soraka-libcamera?\n> > >\n> > > The patches are trivial. They're available here\n> > > https://jmondi.org/cgit/linux/log/?h=soraka/g_selection\n> > > (based on the most recent media-master)\n> > >\n> > > I've not been able to test them (Tomasz has details) otherwise I would\n> > > have sent them upstream.\n> >\n> > To avoid blocking this series, how about merging it with the above\n> > workaround (with a \\todo comment), which we'll remove as soon as the\n> > sensor patches can get integrated ?\n> \n> The series is merged. It's libcamera master that's now broken on\n> soraka. Should we temporary address this by defaulting\n> ANDROID_SENSOR_INFO_ACTIVE_ARRAY_SIZE (and ANDROID_SENSOR_INFO_PIXEL_ARRAY_SIZE\n> which the framework requires as well) as Hiro proposed ?\n> \n> I have a patch I'm carrying in my local tree, I can easily send it\n> out.\n\nOops, sorry, I should have looked at the latest code. Yes, I think a\ntemporary fix would be good until the patches get merged in mainline.\nBackporting to the Soraka kernel should then hopefully be fast.\n\n> > > What is the procedure to have them integrated on your side ?\n> > >\n> > > > > > On Wed, Dec 2, 2020 at 10:54 PM Jacopo Mondi wrote:\n> > > > > > >\n> > > > > > > Initialize pixel array properties in the Android camera HAL\n> > > > > > > inspecting the camera properties.\n> > > > > > >\n> > > > > > > If the camera does not provide any suitable property, not static\n> > > > > > > metadata is registered to the Android framework.\n> > > > > > >\n> > > > > > > Reviewed-by: Laurent Pinchart \n> > > > > > > Signed-off-by: Jacopo Mondi \n> > > > > > > ---\n> > > > > > > src/android/camera_device.cpp | 33 +++++++++++++++++++++++----------\n> > > > > > > 1 file changed, 23 insertions(+), 10 deletions(-)\n> > > > > > >\n> > > > > > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > > > > > > index 675af5705055..017a15cac284 100644\n> > > > > > > --- a/src/android/camera_device.cpp\n> > > > > > > +++ b/src/android/camera_device.cpp\n> > > > > > > @@ -593,6 +593,7 @@ const camera_metadata_t *CameraDevice::getStaticMetadata()\n> > > > > > > }\n> > > > > > >\n> > > > > > > const ControlInfoMap &controlsInfo = camera_->controls();\n> > > > > > > + const ControlList &properties = camera_->properties();\n> > > > > > >\n> > > > > > > /* Color correction static metadata. */\n> > > > > > > {\n> > > > > > > @@ -725,17 +726,29 @@ const camera_metadata_t *CameraDevice::getStaticMetadata()\n> > > > > > > staticMetadata_->addEntry(ANDROID_JPEG_MAX_SIZE, &maxJpegBufferSize_, 1);\n> > > > > > >\n> > > > > > > /* Sensor static metadata. */\n> > > > > > > - int32_t pixelArraySize[] = {\n> > > > > > > - 2592, 1944,\n> > > > > > > - };\n> > > > > > > - staticMetadata_->addEntry(ANDROID_SENSOR_INFO_PIXEL_ARRAY_SIZE,\n> > > > > > > - &pixelArraySize, 2);\n> > > > > > > + if (properties.contains(properties::PixelArraySize)) {\n> > > > > > > + const Size &size =\n> > > > > > > + properties.get(properties::PixelArraySize);\n> > > > > > > + std::vector data{\n> > > > > > > + static_cast(size.width),\n> > > > > > > + static_cast(size.height),\n> > > > > > > + };\n> > > > > > > + staticMetadata_->addEntry(ANDROID_SENSOR_INFO_PIXEL_ARRAY_SIZE,\n> > > > > > > + data.data(), data.size());\n> > > > > > > + }\n> > > > > > >\n> > > > > > > - int32_t sensorSizes[] = {\n> > > > > > > - 0, 0, 2560, 1920,\n> > > > > > > - };\n> > > > > > > - staticMetadata_->addEntry(ANDROID_SENSOR_INFO_ACTIVE_ARRAY_SIZE,\n> > > > > > > - &sensorSizes, 4);\n> > > > > > > + if (properties.contains(properties::PixelArrayActiveAreas)) {\n> > > > > > > + const Span &rects =\n> > > > > > > + properties.get>(properties::PixelArrayActiveAreas);\n> > > > > > > + std::vector data{\n> > > > > > > + static_cast(rects[0].x),\n> > > > > > > + static_cast(rects[0].y),\n> > > > > > > + static_cast(rects[0].width),\n> > > > > > > + static_cast(rects[0].height),\n> > > > > > > + };\n> > > > > > > + staticMetadata_->addEntry(ANDROID_SENSOR_INFO_ACTIVE_ARRAY_SIZE,\n> > > > > > > + data.data(), data.size());\n> > > > > > > + }\n> > > > > > >\n> > > > > > > int32_t sensitivityRange[] = {\n> > > > > > > 32, 2400,","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 73B00BD80A\n\tfor ;\n\tThu, 10 Dec 2020 17:44:48 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 45AF667F79;\n\tThu, 10 Dec 2020 18:44:48 +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 DBCEB67E4D\n\tfor ;\n\tThu, 10 Dec 2020 18:44:46 +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 6639C25E;\n\tThu, 10 Dec 2020 18:44:46 +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=\"UwFog4n8\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1607622286;\n\tbh=05NMQM1ro0U5hK4+wcYgw2x2tNjyUQCG1gNRrZzA3uE=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=UwFog4n8f/2oyCAvO9ixu9vvMKeAamcS4D5upJ44k3318hijKxRk4DeniyH7InJ4n\n\tJ8HFN6Flxq55at/+iCBz0r6WU2o5//ZQnj/eXrrv1Yrc1iTiogSDIzEQRDCnciu8nE\n\torqv7cd7jRw4+VcWa/2Xw5rdp/MursEEwW8ilfXM=","Date":"Thu, 10 Dec 2020 19:44:41 +0200","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"","References":"<20201202135354.264212-1-jacopo@jmondi.org>\n\t<20201202135354.264212-5-jacopo@jmondi.org>\n\t\n\t<20201209090416.el3b3gotwuowrrjk@uno.localdomain>\n\t\n\t<20201209161708.yuqzsxcikjlqkdjs@uno.localdomain>\n\t\n\t<20201210163928.3bylrctquumam24m@uno.localdomain>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20201210163928.3bylrctquumam24m@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH v2 4/4] android: camera_device:\n\tInitialize pixel array properties","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":"Hanlin Chen ,\n\tlibcamera-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":14222,"web_url":"https://patchwork.libcamera.org/comment/14222/","msgid":"<20201211142225.fgpb7ydxgxuk7vru@uno.localdomain>","date":"2020-12-11T14:22:25","subject":"Re: [libcamera-devel] [PATCH v2 4/4] android: camera_device:\n\tInitialize pixel array properties","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hello,\n\nOn Thu, Dec 10, 2020 at 05:39:28PM +0100, Jacopo Mondi wrote:\n> Hi Laurent,\n>\n> On Thu, Dec 10, 2020 at 06:28:29PM +0200, Laurent Pinchart wrote:\n> > Hi Jacopo,\n> >\n> > On Wed, Dec 09, 2020 at 05:17:08PM +0100, Jacopo Mondi wrote:\n> > > On Wed, Dec 09, 2020 at 07:10:56PM +0900, Hirokazu Honda wrote:\n> > > > On Wed, Dec 9, 2020 at 6:04 PM Jacopo Mondi wrote:\n> > > > > On Wed, Dec 09, 2020 at 04:26:02PM +0900, Hirokazu Honda wrote:\n> > > > > > Camera service on chromeos fails starting from this patch.\n> > > > >\n> > > > > I wonder why I haven't noticed. I probable have run cros_camera_test\n> > > > > only :/\n> > > > >\n> > > > > > Looks like our camera HAL implementation requires the entry of\n> > > > > > ANDROID_SENSOR_INFO_ACTIVE_ARRAY_SIZE.\n> > > > > > Shall we add else-statement to the if-statement?\n> > > > > > That is,\n> > > > > >\n> > > > > > @@ -748,6 +754,13 @@ const camera_metadata_t *CameraDevice::getStaticMetadata()\n> > > > > > };\n> > > > > > staticMetadata_->addEntry(ANDROID_SENSOR_INFO_ACTIVE_ARRAY_SIZE,\n> > > > > > data.data(), data.size());\n> > > > > > + } else {\n> > > > > > + int32_t sensorSizes[] = {\n> > > > > > + 0, 0, 2560, 1920,\n> > > > > > + };\n> > > > > > + staticMetadata_->addEntry(ANDROID_SENSOR_INFO_ACTIVE_ARRAY_SIZE,\n> > > > > > + &sensorSizes, 4);\n> > > > > > +\n> > > > > > }\n> > > > > >\n> > > > > > With this change, camera on chromeos can start.\n> > > > > > What do you think?\n> > > > >\n> > > > > That's the million dollar question. Should the HAL try to default to\n> > > > > some arbitrary values properties which are not provided by the sensor\n> > > > > driver (the ANDROID_SENSOR_INFO_ACTIVE_ARRAY_SIZE is basically\n> > > > > retrieved from the V4L2_SEL_TGT_CROP_DEFAULT selection target).\n> > > > >\n> > > > > If you ask me, the drivers should be fixed. It's a trivial operation\n> > > > > and makes sure we don't report arbitrary values to the Android\n> > > > > framework.\n> > > > >\n> > > >\n> > > > +Hanlin Chen +Tomasz Figa\n> > > > Tomasz, can you take a look the driver implementation on soraka-libcamera?\n> > >\n> > > The patches are trivial. They're available here\n> > > https://jmondi.org/cgit/linux/log/?h=soraka/g_selection\n> > > (based on the most recent media-master)\n> > >\n> > > I've not been able to test them (Tomasz has details) otherwise I would\n> > > have sent them upstream.\n> >\n> > To avoid blocking this series, how about merging it with the above\n> > workaround (with a \\todo comment), which we'll remove as soon as the\n> > sensor patches can get integrated ?\n>\n> The series is merged. It's libcamera master that's now broken on\n> soraka. Should we temporary address this by defaulting\n> ANDROID_SENSOR_INFO_ACTIVE_ARRAY_SIZE (and ANDROID_SENSOR_INFO_PIXEL_ARRAY_SIZE\n> which the framework requires as well) as Hiro proposed ?\n>\n> I have a patch I'm carrying in my local tree, I can easily send it\n> out.\n\nFinally tested on Soraka, the patches have been sent to linux-media\nhttps://patchwork.linuxtv.org/project/linux-media/list/?series=4127\n\nShould I merge the temporary workaround to give time to upstream to\nreview and cros to integrate them in their downstream ?\n\nThanks\n j\n\n>\n> >\n> > > What is the procedure to have them integrated on your side ?\n> > >\n> > > > > > On Wed, Dec 2, 2020 at 10:54 PM Jacopo Mondi wrote:\n> > > > > > >\n> > > > > > > Initialize pixel array properties in the Android camera HAL\n> > > > > > > inspecting the camera properties.\n> > > > > > >\n> > > > > > > If the camera does not provide any suitable property, not static\n> > > > > > > metadata is registered to the Android framework.\n> > > > > > >\n> > > > > > > Reviewed-by: Laurent Pinchart \n> > > > > > > Signed-off-by: Jacopo Mondi \n> > > > > > > ---\n> > > > > > > src/android/camera_device.cpp | 33 +++++++++++++++++++++++----------\n> > > > > > > 1 file changed, 23 insertions(+), 10 deletions(-)\n> > > > > > >\n> > > > > > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > > > > > > index 675af5705055..017a15cac284 100644\n> > > > > > > --- a/src/android/camera_device.cpp\n> > > > > > > +++ b/src/android/camera_device.cpp\n> > > > > > > @@ -593,6 +593,7 @@ const camera_metadata_t *CameraDevice::getStaticMetadata()\n> > > > > > > }\n> > > > > > >\n> > > > > > > const ControlInfoMap &controlsInfo = camera_->controls();\n> > > > > > > + const ControlList &properties = camera_->properties();\n> > > > > > >\n> > > > > > > /* Color correction static metadata. */\n> > > > > > > {\n> > > > > > > @@ -725,17 +726,29 @@ const camera_metadata_t *CameraDevice::getStaticMetadata()\n> > > > > > > staticMetadata_->addEntry(ANDROID_JPEG_MAX_SIZE, &maxJpegBufferSize_, 1);\n> > > > > > >\n> > > > > > > /* Sensor static metadata. */\n> > > > > > > - int32_t pixelArraySize[] = {\n> > > > > > > - 2592, 1944,\n> > > > > > > - };\n> > > > > > > - staticMetadata_->addEntry(ANDROID_SENSOR_INFO_PIXEL_ARRAY_SIZE,\n> > > > > > > - &pixelArraySize, 2);\n> > > > > > > + if (properties.contains(properties::PixelArraySize)) {\n> > > > > > > + const Size &size =\n> > > > > > > + properties.get(properties::PixelArraySize);\n> > > > > > > + std::vector data{\n> > > > > > > + static_cast(size.width),\n> > > > > > > + static_cast(size.height),\n> > > > > > > + };\n> > > > > > > + staticMetadata_->addEntry(ANDROID_SENSOR_INFO_PIXEL_ARRAY_SIZE,\n> > > > > > > + data.data(), data.size());\n> > > > > > > + }\n> > > > > > >\n> > > > > > > - int32_t sensorSizes[] = {\n> > > > > > > - 0, 0, 2560, 1920,\n> > > > > > > - };\n> > > > > > > - staticMetadata_->addEntry(ANDROID_SENSOR_INFO_ACTIVE_ARRAY_SIZE,\n> > > > > > > - &sensorSizes, 4);\n> > > > > > > + if (properties.contains(properties::PixelArrayActiveAreas)) {\n> > > > > > > + const Span &rects =\n> > > > > > > + properties.get>(properties::PixelArrayActiveAreas);\n> > > > > > > + std::vector data{\n> > > > > > > + static_cast(rects[0].x),\n> > > > > > > + static_cast(rects[0].y),\n> > > > > > > + static_cast(rects[0].width),\n> > > > > > > + static_cast(rects[0].height),\n> > > > > > > + };\n> > > > > > > + staticMetadata_->addEntry(ANDROID_SENSOR_INFO_ACTIVE_ARRAY_SIZE,\n> > > > > > > + data.data(), data.size());\n> > > > > > > + }\n> > > > > > >\n> > > > > > > int32_t sensitivityRange[] = {\n> > > > > > > 32, 2400,\n> >\n> > --\n> > Regards,\n> >\n> > Laurent Pinchart\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 02C57BD808\n\tfor ;\n\tFri, 11 Dec 2020 14:22:18 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5240467FA0;\n\tFri, 11 Dec 2020 15:22:17 +0100 (CET)","from relay7-d.mail.gandi.net (relay7-d.mail.gandi.net\n\t[217.70.183.200])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 7409B67F06\n\tfor ;\n\tFri, 11 Dec 2020 15:22:16 +0100 (CET)","from uno.localdomain (2-224-242-101.ip172.fastwebnet.it\n\t[2.224.242.101]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay7-d.mail.gandi.net (Postfix) with ESMTPSA id A2C932000D;\n\tFri, 11 Dec 2020 14:22:15 +0000 (UTC)"],"X-Originating-IP":"2.224.242.101","Date":"Fri, 11 Dec 2020 15:22:25 +0100","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Message-ID":"<20201211142225.fgpb7ydxgxuk7vru@uno.localdomain>","References":"<20201202135354.264212-1-jacopo@jmondi.org>\n\t<20201202135354.264212-5-jacopo@jmondi.org>\n\t\n\t<20201209090416.el3b3gotwuowrrjk@uno.localdomain>\n\t\n\t<20201209161708.yuqzsxcikjlqkdjs@uno.localdomain>\n\t\n\t<20201210163928.3bylrctquumam24m@uno.localdomain>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20201210163928.3bylrctquumam24m@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH v2 4/4] android: camera_device:\n\tInitialize pixel array properties","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":"Hanlin Chen ,\n\tlibcamera-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":14224,"web_url":"https://patchwork.libcamera.org/comment/14224/","msgid":"","date":"2020-12-11T16:37:33","subject":"Re: [libcamera-devel] [PATCH v2 4/4] android: camera_device:\n\tInitialize pixel array properties","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Fri, Dec 11, 2020 at 03:22:25PM +0100, Jacopo Mondi wrote:\n> On Thu, Dec 10, 2020 at 05:39:28PM +0100, Jacopo Mondi wrote:\n> > On Thu, Dec 10, 2020 at 06:28:29PM +0200, Laurent Pinchart wrote:\n> > > On Wed, Dec 09, 2020 at 05:17:08PM +0100, Jacopo Mondi wrote:\n> > > > On Wed, Dec 09, 2020 at 07:10:56PM +0900, Hirokazu Honda wrote:\n> > > > > On Wed, Dec 9, 2020 at 6:04 PM Jacopo Mondi wrote:\n> > > > > > On Wed, Dec 09, 2020 at 04:26:02PM +0900, Hirokazu Honda wrote:\n> > > > > > > Camera service on chromeos fails starting from this patch.\n> > > > > >\n> > > > > > I wonder why I haven't noticed. I probable have run cros_camera_test\n> > > > > > only :/\n> > > > > >\n> > > > > > > Looks like our camera HAL implementation requires the entry of\n> > > > > > > ANDROID_SENSOR_INFO_ACTIVE_ARRAY_SIZE.\n> > > > > > > Shall we add else-statement to the if-statement?\n> > > > > > > That is,\n> > > > > > >\n> > > > > > > @@ -748,6 +754,13 @@ const camera_metadata_t *CameraDevice::getStaticMetadata()\n> > > > > > > };\n> > > > > > > staticMetadata_->addEntry(ANDROID_SENSOR_INFO_ACTIVE_ARRAY_SIZE,\n> > > > > > > data.data(), data.size());\n> > > > > > > + } else {\n> > > > > > > + int32_t sensorSizes[] = {\n> > > > > > > + 0, 0, 2560, 1920,\n> > > > > > > + };\n> > > > > > > + staticMetadata_->addEntry(ANDROID_SENSOR_INFO_ACTIVE_ARRAY_SIZE,\n> > > > > > > + &sensorSizes, 4);\n> > > > > > > +\n> > > > > > > }\n> > > > > > >\n> > > > > > > With this change, camera on chromeos can start.\n> > > > > > > What do you think?\n> > > > > >\n> > > > > > That's the million dollar question. Should the HAL try to default to\n> > > > > > some arbitrary values properties which are not provided by the sensor\n> > > > > > driver (the ANDROID_SENSOR_INFO_ACTIVE_ARRAY_SIZE is basically\n> > > > > > retrieved from the V4L2_SEL_TGT_CROP_DEFAULT selection target).\n> > > > > >\n> > > > > > If you ask me, the drivers should be fixed. It's a trivial operation\n> > > > > > and makes sure we don't report arbitrary values to the Android\n> > > > > > framework.\n> > > > > >\n> > > > >\n> > > > > +Hanlin Chen +Tomasz Figa\n> > > > > Tomasz, can you take a look the driver implementation on soraka-libcamera?\n> > > >\n> > > > The patches are trivial. They're available here\n> > > > https://jmondi.org/cgit/linux/log/?h=soraka/g_selection\n> > > > (based on the most recent media-master)\n> > > >\n> > > > I've not been able to test them (Tomasz has details) otherwise I would\n> > > > have sent them upstream.\n> > >\n> > > To avoid blocking this series, how about merging it with the above\n> > > workaround (with a \\todo comment), which we'll remove as soon as the\n> > > sensor patches can get integrated ?\n> >\n> > The series is merged. It's libcamera master that's now broken on\n> > soraka. Should we temporary address this by defaulting\n> > ANDROID_SENSOR_INFO_ACTIVE_ARRAY_SIZE (and ANDROID_SENSOR_INFO_PIXEL_ARRAY_SIZE\n> > which the framework requires as well) as Hiro proposed ?\n> >\n> > I have a patch I'm carrying in my local tree, I can easily send it\n> > out.\n> \n> Finally tested on Soraka, the patches have been sent to linux-media\n> https://patchwork.linuxtv.org/project/linux-media/list/?series=4127\n\nThanks a million for your hard work there, I know what was involved in\ngetting a recent enough kernel up and running on the device :-)\n\n> Should I merge the temporary workaround to give time to upstream to\n> review and cros to integrate them in their downstream ?\n\nPlease do.\n\n> > > > What is the procedure to have them integrated on your side ?\n> > > >\n> > > > > > > On Wed, Dec 2, 2020 at 10:54 PM Jacopo Mondi wrote:\n> > > > > > > >\n> > > > > > > > Initialize pixel array properties in the Android camera HAL\n> > > > > > > > inspecting the camera properties.\n> > > > > > > >\n> > > > > > > > If the camera does not provide any suitable property, not static\n> > > > > > > > metadata is registered to the Android framework.\n> > > > > > > >\n> > > > > > > > Reviewed-by: Laurent Pinchart \n> > > > > > > > Signed-off-by: Jacopo Mondi \n> > > > > > > > ---\n> > > > > > > > src/android/camera_device.cpp | 33 +++++++++++++++++++++++----------\n> > > > > > > > 1 file changed, 23 insertions(+), 10 deletions(-)\n> > > > > > > >\n> > > > > > > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > > > > > > > index 675af5705055..017a15cac284 100644\n> > > > > > > > --- a/src/android/camera_device.cpp\n> > > > > > > > +++ b/src/android/camera_device.cpp\n> > > > > > > > @@ -593,6 +593,7 @@ const camera_metadata_t *CameraDevice::getStaticMetadata()\n> > > > > > > > }\n> > > > > > > >\n> > > > > > > > const ControlInfoMap &controlsInfo = camera_->controls();\n> > > > > > > > + const ControlList &properties = camera_->properties();\n> > > > > > > >\n> > > > > > > > /* Color correction static metadata. */\n> > > > > > > > {\n> > > > > > > > @@ -725,17 +726,29 @@ const camera_metadata_t *CameraDevice::getStaticMetadata()\n> > > > > > > > staticMetadata_->addEntry(ANDROID_JPEG_MAX_SIZE, &maxJpegBufferSize_, 1);\n> > > > > > > >\n> > > > > > > > /* Sensor static metadata. */\n> > > > > > > > - int32_t pixelArraySize[] = {\n> > > > > > > > - 2592, 1944,\n> > > > > > > > - };\n> > > > > > > > - staticMetadata_->addEntry(ANDROID_SENSOR_INFO_PIXEL_ARRAY_SIZE,\n> > > > > > > > - &pixelArraySize, 2);\n> > > > > > > > + if (properties.contains(properties::PixelArraySize)) {\n> > > > > > > > + const Size &size =\n> > > > > > > > + properties.get(properties::PixelArraySize);\n> > > > > > > > + std::vector data{\n> > > > > > > > + static_cast(size.width),\n> > > > > > > > + static_cast(size.height),\n> > > > > > > > + };\n> > > > > > > > + staticMetadata_->addEntry(ANDROID_SENSOR_INFO_PIXEL_ARRAY_SIZE,\n> > > > > > > > + data.data(), data.size());\n> > > > > > > > + }\n> > > > > > > >\n> > > > > > > > - int32_t sensorSizes[] = {\n> > > > > > > > - 0, 0, 2560, 1920,\n> > > > > > > > - };\n> > > > > > > > - staticMetadata_->addEntry(ANDROID_SENSOR_INFO_ACTIVE_ARRAY_SIZE,\n> > > > > > > > - &sensorSizes, 4);\n> > > > > > > > + if (properties.contains(properties::PixelArrayActiveAreas)) {\n> > > > > > > > + const Span &rects =\n> > > > > > > > + properties.get>(properties::PixelArrayActiveAreas);\n> > > > > > > > + std::vector data{\n> > > > > > > > + static_cast(rects[0].x),\n> > > > > > > > + static_cast(rects[0].y),\n> > > > > > > > + static_cast(rects[0].width),\n> > > > > > > > + static_cast(rects[0].height),\n> > > > > > > > + };\n> > > > > > > > + staticMetadata_->addEntry(ANDROID_SENSOR_INFO_ACTIVE_ARRAY_SIZE,\n> > > > > > > > + data.data(), data.size());\n> > > > > > > > + }\n> > > > > > > >\n> > > > > > > > int32_t sensitivityRange[] = {\n> > > > > > > > 32, 2400,","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 4E5CABD80A\n\tfor ;\n\tFri, 11 Dec 2020 16:37:41 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C191D67FA0;\n\tFri, 11 Dec 2020 17:37:40 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5DF4667F06\n\tfor ;\n\tFri, 11 Dec 2020 17:37:39 +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 C6E6599;\n\tFri, 11 Dec 2020 17:37:38 +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=\"SIgAQnKC\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1607704659;\n\tbh=7nJ3OOEeF1OgP6R+/CI982IESrG7EpAJO8BfdieND74=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=SIgAQnKCoI34AN+N6VOyth3AhSNZ/Po/o0wM4O/ey+gKKdmWfja5lsTQLii2aKFhy\n\tiUlCCCIXi7+U2SJnQMOfnE3OCU0K7uQofNFNestrN3yHOlZVgeZxWbzH0Ga/bdS+TZ\n\thDXqHMu5Np39kmJjFpDX5EWRytymsOxmOHRXIkCY=","Date":"Fri, 11 Dec 2020 18:37:33 +0200","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"","References":"<20201202135354.264212-1-jacopo@jmondi.org>\n\t<20201202135354.264212-5-jacopo@jmondi.org>\n\t\n\t<20201209090416.el3b3gotwuowrrjk@uno.localdomain>\n\t\n\t<20201209161708.yuqzsxcikjlqkdjs@uno.localdomain>\n\t\n\t<20201210163928.3bylrctquumam24m@uno.localdomain>\n\t<20201211142225.fgpb7ydxgxuk7vru@uno.localdomain>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20201211142225.fgpb7ydxgxuk7vru@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH v2 4/4] android: camera_device:\n\tInitialize pixel array properties","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":"Hanlin Chen ,\n\tlibcamera-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\" "}}]