[{"id":15983,"web_url":"https://patchwork.libcamera.org/comment/15983/","msgid":"","date":"2021-03-29T04:27:30","subject":"Re: [libcamera-devel] [PATCH v2 1/3] android: CameraDevice:\n\tValidate crop_rotate_scale_degrees in configuration","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Hiro,\n\nThank you for the patch.\n\nOn Mon, Mar 29, 2021 at 07:45:26AM +0900, Hirokazu Honda wrote:\n> Libcamera doesn't handle |crop_rotate_scale_degrees| in\n\ns/Libcamera/libcamera/ (the name is always written in lowercase)\n\n> camera3_stream at all. This adds the validation of the requested\n> |crop_rotate_scale_degrees| in configuration, but still not\n> handle the specified values.\n> \n> Signed-off-by: Hirokazu Honda \n> ---\n> src/android/camera_device.cpp | 39 +++++++++++++++++++++++++++++++++++\n> 1 file changed, 39 insertions(+)\n> \n> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> index ae693664..c5e55a18 100644\n> --- a/src/android/camera_device.cpp\n> +++ b/src/android/camera_device.cpp\n> @@ -256,6 +256,39 @@ void sortCamera3StreamConfigs(std::vector &unsortedConfigs,\n> \tunsortedConfigs = sortedConfigs;\n> }\n> \n> +/*\n> + * Returns where crop_rotate_scale_degrees in all camera3_stream in stream_list\n\ns/where/whether/ ?\n\n> + * are valid.\n\nI'd expand this a little bit to document that we're not validating the\nvalues against what libcamera support, but against what the Chrome OS\nAPI defines.\n\n * Check whether the crop_rotate_scale_degrees values for all streams in\n * the list are valid according to the Chrome OS camera HAL API.\n\n> + */\n> +bool validateCropRotate(const camera3_stream_configuration_t &stream_list)\n\ns/stream_list/streamList/\n\n> +{\n> +\tASSERT(stream_list.num_streams > 0);\n> +\n> +\tconst int cropRotateScaleDegrees =\n> +\t\tstream_list.streams[0]->crop_rotate_scale_degrees;\n> +\tfor (unsigned int i = 0; i < stream_list.num_streams; ++i) {\n> +\t\tconst camera3_stream_t &stream = *stream_list.streams[i];\n> +\t\tif (CAMERA3_STREAM_ROTATION_0 > stream.crop_rotate_scale_degrees ||\n> +\t\t CAMERA3_STREAM_ROTATION_270 < stream.crop_rotate_scale_degrees) {\n> +\t\t\tLOG(HAL, Error) << \"Invalid crop_rotate_scale_degrees: \"\n> +\t\t\t\t\t<< stream.crop_rotate_scale_degrees;\n> +\t\t\treturn false;\n> +\t\t}\n> +\t\tif (stream.crop_rotate_scale_degrees == CAMERA3_STREAM_ROTATION_180) {\n> +\t\t\tLOG(HAL, Error) << \"crop_rotate_scale_degrees should \"\n> +\t\t\t\t\t<< \"not be CAMERA3_STREAM_ROTATION_180\";\n\nA comment to explain why this is the case would be useful.\n\n> +\t\t\treturn false;\n> +\t\t}\n\nHow about simplifying this to\n\n\t\tswitch (stream.crop_rotate_scale_degrees) {\n\t\t/* 180° rotation is specified by Chrome OS as invalid. */\n\t\tcase CAMERA3_STREAM_ROTATION_180:\n\t\tdefault:\n\t\t\tLOG(HAL, Error) << \"Invalid crop_rotate_scale_degrees: \"\n\t\t\t\t\t<< stream.crop_rotate_scale_degrees;\n\t\t\treturn false;\n\n\t\tcase CAMERA3_STREAM_ROTATION_0:\n\t\tcase CAMERA3_STREAM_ROTATION_90:\n\t\tcase CAMERA3_STREAM_ROTATION_270:\n\t\t\tbreak;\n\t\t}\n\n> +\t\tif (cropRotateScaleDegrees != stream.crop_rotate_scale_degrees) {\n> +\t\t\tLOG(HAL, Error) << \"crop_rotate_scale_degrees in all \"\n> +\t\t\t\t\t<< \"streams are not identical\";\n> +\t\t\treturn false;\n> +\t\t}\n> +\t}\n> +\n> +\treturn true;\n> +}\n> +\n> } /* namespace */\n> \n> /*\n> @@ -1552,6 +1585,12 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)\n> \t\trunning_ = false;\n> \t}\n> \n> +\tif (stream_list->num_streams == 0)\n> +\t\treturn -EINVAL;\n> +\n> +\tif (!validateCropRotate(*stream_list))\n> +\t\treturn -EINVAL;\n\nThis should only be enabled when the 'android_platform' meson option is\nto to 'cros', as it's not available on Android. In the Android camera\nHAL v3.4, the first reserved field has been used for physical_camera_id,\nso there's a risk the value wouldn't be zero.\n\n> +\n> \t/*\n> \t * Generate an empty configuration, and construct a StreamConfiguration\n> \t * for each camera3_stream to add to it.","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 5DA04C32EF\n\tfor ;\n\tMon, 29 Mar 2021 04:28:17 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A6B546877D;\n\tMon, 29 Mar 2021 06:28:16 +0200 (CEST)","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 7A5F8602D2\n\tfor ;\n\tMon, 29 Mar 2021 06:28:15 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id C1F9331A;\n\tMon, 29 Mar 2021 06:28:14 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"Ma/Jyd1a\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1616992095;\n\tbh=BQSUFyMh4yS3ImEOOWRYUI25NVpT6IGjta7T7Ml3YUM=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Ma/Jyd1a1MUlWkFRITUqnn/oHuiOOJQUfkTb7f2VxKL3Cx3JxRL1bkVe3h0P4YL6Y\n\t9VItg5uY2FJMxdcwKfg0W3jztxfTIXWG9fvDha2tM4Jf4R7m2Mqkt21eTR/nRYMnhu\n\tiX1FtYn3Jujzc3TayrA+LSoJv33wBC6b3i4A/OFs=","Date":"Mon, 29 Mar 2021 07:27:30 +0300","From":"Laurent Pinchart ","To":"Hirokazu Honda ","Message-ID":"","References":"<20210328224528.55468-1-hiroh@chromium.org>\n\t<20210328224528.55468-2-hiroh@chromium.org>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210328224528.55468-2-hiroh@chromium.org>","Subject":"Re: [libcamera-devel] [PATCH v2 1/3] android: CameraDevice:\n\tValidate crop_rotate_scale_degrees in configuration","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=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16011,"web_url":"https://patchwork.libcamera.org/comment/16011/","msgid":"","date":"2021-03-29T09:37:15","subject":"Re: [libcamera-devel] [PATCH v2 1/3] android: CameraDevice:\n\tValidate crop_rotate_scale_degrees in configuration","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"Hi Laurent, thanks for reviewing.\n\nOn Mon, Mar 29, 2021 at 1:28 PM Laurent Pinchart\n wrote:\n>\n> Hi Hiro,\n>\n> Thank you for the patch.\n>\n> On Mon, Mar 29, 2021 at 07:45:26AM +0900, Hirokazu Honda wrote:\n> > Libcamera doesn't handle |crop_rotate_scale_degrees| in\n>\n> s/Libcamera/libcamera/ (the name is always written in lowercase)\n>\n> > camera3_stream at all. This adds the validation of the requested\n> > |crop_rotate_scale_degrees| in configuration, but still not\n> > handle the specified values.\n> >\n> > Signed-off-by: Hirokazu Honda \n> > ---\n> > src/android/camera_device.cpp | 39 +++++++++++++++++++++++++++++++++++\n> > 1 file changed, 39 insertions(+)\n> >\n> > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > index ae693664..c5e55a18 100644\n> > --- a/src/android/camera_device.cpp\n> > +++ b/src/android/camera_device.cpp\n> > @@ -256,6 +256,39 @@ void sortCamera3StreamConfigs(std::vector &unsortedConfigs,\n> > unsortedConfigs = sortedConfigs;\n> > }\n> >\n> > +/*\n> > + * Returns where crop_rotate_scale_degrees in all camera3_stream in stream_list\n>\n> s/where/whether/ ?\n>\n> > + * are valid.\n>\n> I'd expand this a little bit to document that we're not validating the\n> values against what libcamera support, but against what the Chrome OS\n> API defines.\n>\n> * Check whether the crop_rotate_scale_degrees values for all streams in\n> * the list are valid according to the Chrome OS camera HAL API.\n>\n> > + */\n> > +bool validateCropRotate(const camera3_stream_configuration_t &stream_list)\n>\n> s/stream_list/streamList/\n>\n> > +{\n> > + ASSERT(stream_list.num_streams > 0);\n> > +\n> > + const int cropRotateScaleDegrees =\n> > + stream_list.streams[0]->crop_rotate_scale_degrees;\n> > + for (unsigned int i = 0; i < stream_list.num_streams; ++i) {\n> > + const camera3_stream_t &stream = *stream_list.streams[i];\n> > + if (CAMERA3_STREAM_ROTATION_0 > stream.crop_rotate_scale_degrees ||\n> > + CAMERA3_STREAM_ROTATION_270 < stream.crop_rotate_scale_degrees) {\n> > + LOG(HAL, Error) << \"Invalid crop_rotate_scale_degrees: \"\n> > + << stream.crop_rotate_scale_degrees;\n> > + return false;\n> > + }\n> > + if (stream.crop_rotate_scale_degrees == CAMERA3_STREAM_ROTATION_180) {\n> > + LOG(HAL, Error) << \"crop_rotate_scale_degrees should \"\n> > + << \"not be CAMERA3_STREAM_ROTATION_180\";\n>\n> A comment to explain why this is the case would be useful.\n>\n> > + return false;\n> > + }\n>\n> How about simplifying this to\n>\n> switch (stream.crop_rotate_scale_degrees) {\n> /* 180° rotation is specified by Chrome OS as invalid. */\n> case CAMERA3_STREAM_ROTATION_180:\n> default:\n> LOG(HAL, Error) << \"Invalid crop_rotate_scale_degrees: \"\n> << stream.crop_rotate_scale_degrees;\n> return false;\n>\n> case CAMERA3_STREAM_ROTATION_0:\n> case CAMERA3_STREAM_ROTATION_90:\n> case CAMERA3_STREAM_ROTATION_270:\n> break;\n> }\n>\n> > + if (cropRotateScaleDegrees != stream.crop_rotate_scale_degrees) {\n> > + LOG(HAL, Error) << \"crop_rotate_scale_degrees in all \"\n> > + << \"streams are not identical\";\n> > + return false;\n> > + }\n> > + }\n> > +\n> > + return true;\n> > +}\n> > +\n> > } /* namespace */\n> >\n> > /*\n> > @@ -1552,6 +1585,12 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)\n> > running_ = false;\n> > }\n> >\n> > + if (stream_list->num_streams == 0)\n> > + return -EINVAL;\n> > +\n> > + if (!validateCropRotate(*stream_list))\n> > + return -EINVAL;\n>\n> This should only be enabled when the 'android_platform' meson option is\n> to to 'cros', as it's not available on Android. In the Android camera\n> HAL v3.4, the first reserved field has been used for physical_camera_id,\n> so there's a risk the value wouldn't be zero.\n>\n\nRight. If you don't mind, can I submit this patch as-is and quickly\nsubmit a patch of limiting this with android_platform='cros' later?\n\nRegards,\n-Hiro\n> > +\n> > /*\n> > * Generate an empty configuration, and construct a StreamConfiguration\n> > * for each camera3_stream to add to it.\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 6187FC32EA\n\tfor ;\n\tMon, 29 Mar 2021 09:37:28 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A1BF568783;\n\tMon, 29 Mar 2021 11:37:27 +0200 (CEST)","from mail-ej1-x635.google.com (mail-ej1-x635.google.com\n\t[IPv6:2a00:1450:4864:20::635])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 965306877E\n\tfor ;\n\tMon, 29 Mar 2021 11:37:26 +0200 (CEST)","by mail-ej1-x635.google.com with SMTP id u21so18338685ejo.13\n\tfor ;\n\tMon, 29 Mar 2021 02:37:26 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"ajI2F6Ic\"; 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:content-transfer-encoding;\n\tbh=I128ng29OzlaKV5SYBtzGoxX7/A2U95m982JRVtqn3E=;\n\tb=ajI2F6IcvrzS7SQkIEajMdrolbjyPRmAaC63anNPTLFr2PTbk10Qz6p1ZUTisK830N\n\twz3+na0jyEziUZd/uiTkLyOGr2CafzlN2B9BUzzHu6USvdDdw/q5BhG/cFO8FRyszNg6\n\ttaGol3cLF9S70alVvHJWptoKGgYCrnTl297r8=","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:content-transfer-encoding;\n\tbh=I128ng29OzlaKV5SYBtzGoxX7/A2U95m982JRVtqn3E=;\n\tb=O5yiiM7luDd49D63ysirq3xKISj5z+krdJ90OKP2R9XPAZ2EAW4RXvGKWwCsG+1rOk\n\tRgB5sAVKBjj0aINsylrfW5hVyL3x4Hb26u8lfsiMNBtWEe0TGDQV6NmOlQ3v3rpVYYKv\n\tqjBASiNkfvsbpZ2yDHRXDRTbKQVfyz2IToY4SRyXy0k80XXIrugNj8/dzRLaxBp3hXfJ\n\tOMMWUp+P62Z9zso2yuOuOndXw/3kcCtExg/Hq8GJrOuES9eqz0xHmi6U4s1qGSFTqT9d\n\ti8vkIFuSTdFn1Eqf36RCsCUoduep03ZUcbkcRqyWlmG4w4yshtj6oMNJ6RA1B0lgRh8l\n\tZh6A==","X-Gm-Message-State":"AOAM531tob5hc2nEbZKJNnPdP6fyMAUh6cqC/+0dDtJh5v4o3/88uR0T\n\tzkjeLNKyI3s24MlT0c0KULjG9i0jzqZvnoxGLdtYOQ==","X-Google-Smtp-Source":"ABdhPJyIGjSingDXNtvtZauk+CCrz3YhZqMPn/3uSexU46ErigPlrHvyGeEwDPad0vsr0GVBIdfM3+c08YRqKXZhg2w=","X-Received":"by 2002:a17:906:4801:: with SMTP id\n\tw1mr27703063ejq.475.1617010646153; \n\tMon, 29 Mar 2021 02:37:26 -0700 (PDT)","MIME-Version":"1.0","References":"<20210328224528.55468-1-hiroh@chromium.org>\n\t<20210328224528.55468-2-hiroh@chromium.org>\n\t","In-Reply-To":"","From":"Hirokazu Honda ","Date":"Mon, 29 Mar 2021 18:37:15 +0900","Message-ID":"","To":"Laurent Pinchart ","Subject":"Re: [libcamera-devel] [PATCH v2 1/3] android: CameraDevice:\n\tValidate crop_rotate_scale_degrees in configuration","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera devel ","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16048,"web_url":"https://patchwork.libcamera.org/comment/16048/","msgid":"","date":"2021-03-30T03:55:51","subject":"Re: [libcamera-devel] [PATCH v2 1/3] android: CameraDevice:\n\tValidate crop_rotate_scale_degrees in configuration","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Hiro,\n\nOn Mon, Mar 29, 2021 at 06:37:15PM +0900, Hirokazu Honda wrote:\n> On Mon, Mar 29, 2021 at 1:28 PM Laurent Pinchart wrote:\n> > On Mon, Mar 29, 2021 at 07:45:26AM +0900, Hirokazu Honda wrote:\n> > > Libcamera doesn't handle |crop_rotate_scale_degrees| in\n> >\n> > s/Libcamera/libcamera/ (the name is always written in lowercase)\n> >\n> > > camera3_stream at all. This adds the validation of the requested\n> > > |crop_rotate_scale_degrees| in configuration, but still not\n> > > handle the specified values.\n> > >\n> > > Signed-off-by: Hirokazu Honda \n> > > ---\n> > > src/android/camera_device.cpp | 39 +++++++++++++++++++++++++++++++++++\n> > > 1 file changed, 39 insertions(+)\n> > >\n> > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > > index ae693664..c5e55a18 100644\n> > > --- a/src/android/camera_device.cpp\n> > > +++ b/src/android/camera_device.cpp\n> > > @@ -256,6 +256,39 @@ void sortCamera3StreamConfigs(std::vector &unsortedConfigs,\n> > > unsortedConfigs = sortedConfigs;\n> > > }\n> > >\n> > > +/*\n> > > + * Returns where crop_rotate_scale_degrees in all camera3_stream in stream_list\n> >\n> > s/where/whether/ ?\n> >\n> > > + * are valid.\n> >\n> > I'd expand this a little bit to document that we're not validating the\n> > values against what libcamera support, but against what the Chrome OS\n> > API defines.\n> >\n> > * Check whether the crop_rotate_scale_degrees values for all streams in\n> > * the list are valid according to the Chrome OS camera HAL API.\n> >\n> > > + */\n> > > +bool validateCropRotate(const camera3_stream_configuration_t &stream_list)\n> >\n> > s/stream_list/streamList/\n> >\n> > > +{\n> > > + ASSERT(stream_list.num_streams > 0);\n> > > +\n> > > + const int cropRotateScaleDegrees =\n> > > + stream_list.streams[0]->crop_rotate_scale_degrees;\n> > > + for (unsigned int i = 0; i < stream_list.num_streams; ++i) {\n> > > + const camera3_stream_t &stream = *stream_list.streams[i];\n> > > + if (CAMERA3_STREAM_ROTATION_0 > stream.crop_rotate_scale_degrees ||\n> > > + CAMERA3_STREAM_ROTATION_270 < stream.crop_rotate_scale_degrees) {\n> > > + LOG(HAL, Error) << \"Invalid crop_rotate_scale_degrees: \"\n> > > + << stream.crop_rotate_scale_degrees;\n> > > + return false;\n> > > + }\n> > > + if (stream.crop_rotate_scale_degrees == CAMERA3_STREAM_ROTATION_180) {\n> > > + LOG(HAL, Error) << \"crop_rotate_scale_degrees should \"\n> > > + << \"not be CAMERA3_STREAM_ROTATION_180\";\n> >\n> > A comment to explain why this is the case would be useful.\n> >\n> > > + return false;\n> > > + }\n> >\n> > How about simplifying this to\n> >\n> > switch (stream.crop_rotate_scale_degrees) {\n> > /* 180° rotation is specified by Chrome OS as invalid. */\n> > case CAMERA3_STREAM_ROTATION_180:\n> > default:\n> > LOG(HAL, Error) << \"Invalid crop_rotate_scale_degrees: \"\n> > << stream.crop_rotate_scale_degrees;\n> > return false;\n> >\n> > case CAMERA3_STREAM_ROTATION_0:\n> > case CAMERA3_STREAM_ROTATION_90:\n> > case CAMERA3_STREAM_ROTATION_270:\n> > break;\n> > }\n> >\n> > > + if (cropRotateScaleDegrees != stream.crop_rotate_scale_degrees) {\n> > > + LOG(HAL, Error) << \"crop_rotate_scale_degrees in all \"\n> > > + << \"streams are not identical\";\n> > > + return false;\n> > > + }\n> > > + }\n> > > +\n> > > + return true;\n> > > +}\n> > > +\n> > > } /* namespace */\n> > >\n> > > /*\n> > > @@ -1552,6 +1585,12 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)\n> > > running_ = false;\n> > > }\n> > >\n> > > + if (stream_list->num_streams == 0)\n> > > + return -EINVAL;\n> > > +\n> > > + if (!validateCropRotate(*stream_list))\n> > > + return -EINVAL;\n> >\n> > This should only be enabled when the 'android_platform' meson option is\n> > to to 'cros', as it's not available on Android. In the Android camera\n> > HAL v3.4, the first reserved field has been used for physical_camera_id,\n> > so there's a risk the value wouldn't be zero.\n> \n> Right. If you don't mind, can I submit this patch as-is and quickly\n> submit a patch of limiting this with android_platform='cros' later?\n\nlibcamera is already used on Android by at least one project, and I'd\nlike to avoid breaking bisection for them. We introduce enough\nregression on the Android side already without noticing, it would be\nbetter to avoid at least the regressions that we are aware of :-) Would\nyou mind adding the android_platform option check already ?\n\n> > > +\n> > > /*\n> > > * Generate an empty configuration, and construct a StreamConfiguration\n> > > * for each camera3_stream to add to it.","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 F196AC32EA\n\tfor ;\n\tTue, 30 Mar 2021 03:56:37 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5D3A268782;\n\tTue, 30 Mar 2021 05:56:37 +0200 (CEST)","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 005C5602D7\n\tfor ;\n\tTue, 30 Mar 2021 05:56:35 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 588CF292;\n\tTue, 30 Mar 2021 05:56:35 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"f0xeRhZr\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1617076595;\n\tbh=9DEFhNW3LsAbPHg0y5YlpzcCTKheovrsVvYb91SHzUw=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=f0xeRhZrys7AFqyyXFX2cwhdJEAT6NkAYmHCognwFrwW+yl6l3KpHhJuDqsstFzRn\n\tOMeL9sRJCptcjyMm07uiqznMprQkzqynLHJXb0dykQgvAzTkmiQGtPS09HnR/JLK+b\n\t5VMM9SzXYLWS7rVbslB/8GEmMc9uaHugBdZ1v1BU=","Date":"Tue, 30 Mar 2021 06:55:51 +0300","From":"Laurent Pinchart ","To":"Hirokazu Honda ","Message-ID":"","References":"<20210328224528.55468-1-hiroh@chromium.org>\n\t<20210328224528.55468-2-hiroh@chromium.org>\n\t\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v2 1/3] android: CameraDevice:\n\tValidate crop_rotate_scale_degrees in configuration","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera devel ","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16050,"web_url":"https://patchwork.libcamera.org/comment/16050/","msgid":"","date":"2021-03-30T04:23:49","subject":"Re: [libcamera-devel] [PATCH v2 1/3] android: CameraDevice:\n\tValidate crop_rotate_scale_degrees in configuration","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"Hi Laurent,\n\nOn Tue, Mar 30, 2021 at 12:56 PM Laurent Pinchart\n wrote:\n>\n> Hi Hiro,\n>\n> On Mon, Mar 29, 2021 at 06:37:15PM +0900, Hirokazu Honda wrote:\n> > On Mon, Mar 29, 2021 at 1:28 PM Laurent Pinchart wrote:\n> > > On Mon, Mar 29, 2021 at 07:45:26AM +0900, Hirokazu Honda wrote:\n> > > > Libcamera doesn't handle |crop_rotate_scale_degrees| in\n> > >\n> > > s/Libcamera/libcamera/ (the name is always written in lowercase)\n> > >\n> > > > camera3_stream at all. This adds the validation of the requested\n> > > > |crop_rotate_scale_degrees| in configuration, but still not\n> > > > handle the specified values.\n> > > >\n> > > > Signed-off-by: Hirokazu Honda \n> > > > ---\n> > > > src/android/camera_device.cpp | 39 +++++++++++++++++++++++++++++++++++\n> > > > 1 file changed, 39 insertions(+)\n> > > >\n> > > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > > > index ae693664..c5e55a18 100644\n> > > > --- a/src/android/camera_device.cpp\n> > > > +++ b/src/android/camera_device.cpp\n> > > > @@ -256,6 +256,39 @@ void sortCamera3StreamConfigs(std::vector &unsortedConfigs,\n> > > > unsortedConfigs = sortedConfigs;\n> > > > }\n> > > >\n> > > > +/*\n> > > > + * Returns where crop_rotate_scale_degrees in all camera3_stream in stream_list\n> > >\n> > > s/where/whether/ ?\n> > >\n> > > > + * are valid.\n> > >\n> > > I'd expand this a little bit to document that we're not validating the\n> > > values against what libcamera support, but against what the Chrome OS\n> > > API defines.\n> > >\n> > > * Check whether the crop_rotate_scale_degrees values for all streams in\n> > > * the list are valid according to the Chrome OS camera HAL API.\n> > >\n> > > > + */\n> > > > +bool validateCropRotate(const camera3_stream_configuration_t &stream_list)\n> > >\n> > > s/stream_list/streamList/\n> > >\n> > > > +{\n> > > > + ASSERT(stream_list.num_streams > 0);\n> > > > +\n> > > > + const int cropRotateScaleDegrees =\n> > > > + stream_list.streams[0]->crop_rotate_scale_degrees;\n> > > > + for (unsigned int i = 0; i < stream_list.num_streams; ++i) {\n> > > > + const camera3_stream_t &stream = *stream_list.streams[i];\n> > > > + if (CAMERA3_STREAM_ROTATION_0 > stream.crop_rotate_scale_degrees ||\n> > > > + CAMERA3_STREAM_ROTATION_270 < stream.crop_rotate_scale_degrees) {\n> > > > + LOG(HAL, Error) << \"Invalid crop_rotate_scale_degrees: \"\n> > > > + << stream.crop_rotate_scale_degrees;\n> > > > + return false;\n> > > > + }\n> > > > + if (stream.crop_rotate_scale_degrees == CAMERA3_STREAM_ROTATION_180) {\n> > > > + LOG(HAL, Error) << \"crop_rotate_scale_degrees should \"\n> > > > + << \"not be CAMERA3_STREAM_ROTATION_180\";\n> > >\n> > > A comment to explain why this is the case would be useful.\n> > >\n> > > > + return false;\n> > > > + }\n> > >\n> > > How about simplifying this to\n> > >\n> > > switch (stream.crop_rotate_scale_degrees) {\n> > > /* 180° rotation is specified by Chrome OS as invalid. */\n> > > case CAMERA3_STREAM_ROTATION_180:\n> > > default:\n> > > LOG(HAL, Error) << \"Invalid crop_rotate_scale_degrees: \"\n> > > << stream.crop_rotate_scale_degrees;\n> > > return false;\n> > >\n> > > case CAMERA3_STREAM_ROTATION_0:\n> > > case CAMERA3_STREAM_ROTATION_90:\n> > > case CAMERA3_STREAM_ROTATION_270:\n> > > break;\n> > > }\n> > >\n> > > > + if (cropRotateScaleDegrees != stream.crop_rotate_scale_degrees) {\n> > > > + LOG(HAL, Error) << \"crop_rotate_scale_degrees in all \"\n> > > > + << \"streams are not identical\";\n> > > > + return false;\n> > > > + }\n> > > > + }\n> > > > +\n> > > > + return true;\n> > > > +}\n> > > > +\n> > > > } /* namespace */\n> > > >\n> > > > /*\n> > > > @@ -1552,6 +1585,12 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)\n> > > > running_ = false;\n> > > > }\n> > > >\n> > > > + if (stream_list->num_streams == 0)\n> > > > + return -EINVAL;\n> > > > +\n> > > > + if (!validateCropRotate(*stream_list))\n> > > > + return -EINVAL;\n> > >\n> > > This should only be enabled when the 'android_platform' meson option is\n> > > to to 'cros', as it's not available on Android. In the Android camera\n> > > HAL v3.4, the first reserved field has been used for physical_camera_id,\n> > > so there's a risk the value wouldn't be zero.\n> >\n> > Right. If you don't mind, can I submit this patch as-is and quickly\n> > submit a patch of limiting this with android_platform='cros' later?\n>\n> libcamera is already used on Android by at least one project, and I'd\n> like to avoid breaking bisection for them. We introduce enough\n> regression on the Android side already without noticing, it would be\n> better to avoid at least the regressions that we are aware of :-) Would\n> you mind adding the android_platform option check already ?\n>\n\nFair enough. I will do in the next patch series.\n\n> > > > +\n> > > > /*\n> > > > * Generate an empty configuration, and construct a StreamConfiguration\n> > > > * for each camera3_stream to add to it.\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 9C334C32EA\n\tfor ;\n\tTue, 30 Mar 2021 04:24:04 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id EC14968782;\n\tTue, 30 Mar 2021 06:24:03 +0200 (CEST)","from mail-ej1-x636.google.com (mail-ej1-x636.google.com\n\t[IPv6:2a00:1450:4864:20::636])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 410F1602D7\n\tfor ;\n\tTue, 30 Mar 2021 06:24:03 +0200 (CEST)","by mail-ej1-x636.google.com with SMTP id e14so22655332ejz.11\n\tfor ;\n\tMon, 29 Mar 2021 21:24:03 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"LOyZuH+K\"; 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:content-transfer-encoding;\n\tbh=86SSqkJXut3919Xpy3ZDau/rVUKBxbD9iVyAnTqjmPg=;\n\tb=LOyZuH+KA5NmM+fdgSxuVRbdLg7wxJrw1Qf3o5mGPNpydCAjWhEMGMmtmROwUpW2rO\n\t/+bABRWocExU6sQBwyDMPfFTcSoqmQeTyHib461MhNuNQboB0owmkG+m+XEoI5LG6Fk8\n\tndhpOtjjaK5b2eo+VjEF7kj6LkWjywbZiMea8=","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:content-transfer-encoding;\n\tbh=86SSqkJXut3919Xpy3ZDau/rVUKBxbD9iVyAnTqjmPg=;\n\tb=ZD4FCG4nbnrrj7AOa15zCnrP3urfBb+jWsj2Uaya7Q9ZqyCS1+ClcgFKooLxCwvP5d\n\tjl6touYhTT1hF+MCV4A3xy6adH7CowzYNpBYs30wfv5lrTEb1RuuAEYUmsyhlMultFMT\n\tzFryEZiFQz3g1qppUTmy/th4gYj1nnUbhWyYyDYZSvCa9rSKAiVhj65mR+qPkkD5/WZZ\n\t8Aigjts8bPYJ24iV4TGM6puE9nVS++U/SRZI8caMbv8s/f5QqbNmBu+hzSiIpu7Wa2Ny\n\tIpqXqkr/ahIinqReoAInYVT5HN5HzsBmQ/uFZK8cEetsQGQ4VOHg0wCGjT6VSVXFjtUk\n\ta50w==","X-Gm-Message-State":"AOAM5311wnFbJ/SPCDuwwygGQoqsJIIUrYTr8BOlMBRFysiNlw/fqo9N\n\tZ18PQj9w/3mHhGTKekyM0QOEoopNvtpaA6ZH8+gR6A==","X-Google-Smtp-Source":"ABdhPJw8+O6A1Gt6jw/41Hq1pnRTgYpDqXyyYr+tj4M7vCiBT6Asz8BziNVvfXNXh84BQO56w1+OYRuU6feZHki3PAo=","X-Received":"by 2002:a17:906:4747:: with SMTP id\n\tj7mr31037016ejs.221.1617078242890; \n\tMon, 29 Mar 2021 21:24:02 -0700 (PDT)","MIME-Version":"1.0","References":"<20210328224528.55468-1-hiroh@chromium.org>\n\t<20210328224528.55468-2-hiroh@chromium.org>\n\t\n\t\n\t","In-Reply-To":"","From":"Hirokazu Honda ","Date":"Tue, 30 Mar 2021 13:23:49 +0900","Message-ID":"","To":"Laurent Pinchart ","Subject":"Re: [libcamera-devel] [PATCH v2 1/3] android: CameraDevice:\n\tValidate crop_rotate_scale_degrees in configuration","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera devel ","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]