[{"id":25562,"web_url":"https://patchwork.libcamera.org/comment/25562/","msgid":"","date":"2022-10-24T23:59:07","subject":"Re: [libcamera-devel] [PATCH 11/11] Adds useful debug print\n\tstatements.","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Nicholas,\n\nThank you for the patch.\n\nOn Mon, Oct 24, 2022 at 12:55:43AM -0500, Nicholas Roth via libcamera-devel wrote:\n> From: Nicholas Roth \n> \n> ---\n> src/android/camera_capabilities.cpp | 12 +++++++++---\n> src/android/camera_hal_manager.cpp | 3 ++-\n> src/libcamera/base/log.cpp | 6 +++++-\n> src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 2 ++\n> src/libcamera/v4l2_subdevice.cpp | 3 ++-\n> 5 files changed, 20 insertions(+), 6 deletions(-)\n> \n> diff --git a/src/android/camera_capabilities.cpp b/src/android/camera_capabilities.cpp\n> index 64bd8dde..ef0d10d0 100644\n> --- a/src/android/camera_capabilities.cpp\n> +++ b/src/android/camera_capabilities.cpp\n> @@ -374,14 +374,20 @@ void CameraCapabilities::computeHwLevel(\n> \tcamera_metadata_enum_android_info_supported_hardware_level\n> \t\thwLevel = ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_FULL;\n> \n> -\tif (!caps.count(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_MANUAL_SENSOR))\n> +\tif (!caps.count(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_MANUAL_SENSOR)) {\n> +\t\tLOG(HAL, Info) << noFull << \"missing manual sensor\";\n> \t\thwLevel = ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_LIMITED;\n> +\t}\n> \n> -\tif (!caps.count(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_MANUAL_POST_PROCESSING))\n> +\tif (!caps.count(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_MANUAL_POST_PROCESSING)) {\n> +\t\tLOG(HAL, Info) << noFull << \"missing manual post processing\";\n> \t\thwLevel = ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_LIMITED;\n> +\t}\n> \n> -\tif (!caps.count(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_BURST_CAPTURE))\n> +\tif (!caps.count(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_BURST_CAPTURE)) {\n> +\t\tLOG(HAL, Info) << noFull << \"missing burst capture\";\n> \t\thwLevel = ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_LIMITED;\n> +\t}\n\nPaul, Jacopo, could you maybe review this part ?\n\n> \n> \tfound = staticMetadata_->getEntry(ANDROID_SYNC_MAX_LATENCY, &entry);\n> \tif (!found || *entry.data.i32 != 0) {\n> diff --git a/src/android/camera_hal_manager.cpp b/src/android/camera_hal_manager.cpp\n> index 7512cc4e..7fac4e3f 100644\n> --- a/src/android/camera_hal_manager.cpp\n> +++ b/src/android/camera_hal_manager.cpp\n> @@ -140,7 +140,8 @@ void CameraHalManager::cameraAdded(std::shared_ptr cam)\n> \t */\n> \tif (!isCameraExternal && !halConfig_.exists()) {\n> \t\tLOG(HAL, Error)\n> -\t\t\t<< \"HAL configuration file is mandatory for internal cameras\";\n> +\t\t\t<< \"HAL configuration file is mandatory for internal cameras.\"\n> +\t\t\t<< \" Camera NOT loaded: \\\"\" << cam->id() << \"\\\"\";\n> \t\treturn;\n> \t}\n> \n> diff --git a/src/libcamera/base/log.cpp b/src/libcamera/base/log.cpp\n> index 55fbd7b0..b8c2c99f 100644\n> --- a/src/libcamera/base/log.cpp\n> +++ b/src/libcamera/base/log.cpp\n> @@ -625,8 +625,12 @@ void Logger::parseLogFile()\n> void Logger::parseLogLevels()\n> {\n> \tconst char *debug = utils::secure_getenv(\"LIBCAMERA_LOG_LEVELS\");\n> -\tif (!debug)\n> +\tif (!debug) {\n> +\t\tsyslog(LOG_INFO, \"Could not find LIBCAMERA_LOG_LEVELS in env\");\n> \t\treturn;\n> +\t} else {\n> +\t\tsyslog(LOG_INFO, \"LIBCAMERA_LOG_LEVELS is %s\", debug);\n> +\t}\n\nI don't think we should log a message to syslog every time libcamera is\nstarted to report the LIBCAMERA_LOG_LEVELS value. Is this a debugging\nleftover ?\n\n> \n> \tfor (const char *pair = debug; *debug != '\\0'; pair = debug) {\n> \t\tconst char *comma = strchrnul(debug, ',');\n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp\n> index 2d38f0fb..a2038704 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp\n> @@ -33,10 +33,12 @@ bool RkISP1Path::init(MediaDevice *media)\n> \tstd::string resizer = std::string(\"rkisp1_resizer_\") + name_ + \"path\";\n> \tstd::string video = std::string(\"rkisp1_\") + name_ + \"path\";\n> \n> +\tLOG(RkISP1, Debug) << \"Creating \" << resizer;\n> \tresizer_ = V4L2Subdevice::fromEntityName(media, resizer);\n> \tif (resizer_->open() < 0)\n> \t\treturn false;\n> \n> +\tLOG(RkISP1, Debug) << \"Creating \" << video;\n\nHere too, is this something that you added to debug specific issues you\nwere facing, or do you expect this to be useful for users ? If the\nlatter, could you explain why ?\n\n> \tvideo_ = V4L2VideoDevice::fromEntityName(media, video);\n> \tif (video_->open() < 0)\n> \t\treturn false;\n> diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp\n> index 15e8206a..8f86387b 100644\n> --- a/src/libcamera/v4l2_subdevice.cpp\n> +++ b/src/libcamera/v4l2_subdevice.cpp\n> @@ -392,7 +392,8 @@ int V4L2Subdevice::getSelection(unsigned int pad, unsigned int target,\n> \tif (ret < 0) {\n> \t\tLOG(V4L2, Error)\n> \t\t\t<< \"Unable to get rectangle \" << target << \" on pad \"\n> -\t\t\t<< pad << \": \" << strerror(-ret);\n> +\t\t\t<< pad << \": \" << strerror(-ret) << \".\"\n> +\t\t\t<< \"device path: \" << devicePath() << \" device node: \" << deviceNode();\n\nIf the device path and device node are useful in error messages, they\nshould be printed in all of them, not just this one. This should then be\ndone through the logPrefix() function. The code currently uses the\nentity name as a log prefix, is that not enough ? The device path, in\nparticular, seems too verbose to me.\n\n> \t\treturn ret;\n> \t}\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 02647BDB16\n\tfor ;\n\tMon, 24 Oct 2022 23:59:34 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4518462F29;\n\tTue, 25 Oct 2022 01:59:34 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 0C83962EAE\n\tfor ;\n\tTue, 25 Oct 2022 01:59:33 +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 668C52F5;\n\tTue, 25 Oct 2022 01:59:32 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1666655974;\n\tbh=5GDhEqhSvG248l+pESVRCiQo5Ksfhfcha6EpL2yR3T4=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=mIzmUAUJQUywsgbre7Jbx9zSeWQMVd+Y4BINMTS9uUKODCJGgnWSia/94CPaH4Dt8\n\tHvqaBvNdfdt069tapBTE5zO6S01NL1VW+5yCUusotoC0LQPcFXQmjTgPCnk6vhsLsx\n\tv68Wy9ZPN6gNTqt81Wppk/3GTamPDjz+4IZJlk2F7l7a+rYDy4dcOAPHIPjIleyQCL\n\tCXkWyz94JkKAaexPIEbovyDuZvblOnkC5oZIgUls5tedUV7n8BNE37ADqgti9MIPNj\n\t6H8aP2Kp+UaoSqOX71f0erAZu1olUEHWyx8z7VRYYWilvuNo72RiKlQhqZxAuMB5wJ\n\tW2aENJCJxjQ6A==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1666655972;\n\tbh=5GDhEqhSvG248l+pESVRCiQo5Ksfhfcha6EpL2yR3T4=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Qfs9ZAcsZj+Fa0J/TJSJK2b2WYYf1eHcPtZhwfaw47RGBeojiJoAm+J80L/Wol4AS\n\toq5sSv9KiYzWCEI7Xs59Eb436ed/H/x+pN3wrymgPsyOQk0GYwBKKdPjMIblXmqX+X\n\tmnc7Ja8HZ2ODdYKpyddaDSfmVFc+YXgmoXh92cpU="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"Qfs9ZAcs\"; dkim-atps=neutral","Date":"Tue, 25 Oct 2022 02:59:07 +0300","To":"Nicholas Roth via libcamera-devel ","Message-ID":"","References":"<20221024055543.116040-1-nicholas@rothemail.net>\n\t<20221024055543.116040-12-nicholas@rothemail.net>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20221024055543.116040-12-nicholas@rothemail.net>","Subject":"Re: [libcamera-devel] [PATCH 11/11] Adds useful debug print\n\tstatements.","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","From":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"nicholas@rothemail.net","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":25574,"web_url":"https://patchwork.libcamera.org/comment/25574/","msgid":"<20221025104937.mebbabewfcekawco@uno.localdomain>","date":"2022-10-25T10:49:37","subject":"Re: [libcamera-devel] [PATCH 11/11] Adds useful debug print\n\tstatements.","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hello,\n\nOn Tue, Oct 25, 2022 at 02:59:07AM +0300, Laurent Pinchart via libcamera-devel wrote:\n> Hi Nicholas,\n>\n> Thank you for the patch.\n>\n> On Mon, Oct 24, 2022 at 12:55:43AM -0500, Nicholas Roth via libcamera-devel wrote:\n> > From: Nicholas Roth \n> >\n> > ---\n> > src/android/camera_capabilities.cpp | 12 +++++++++---\n> > src/android/camera_hal_manager.cpp | 3 ++-\n> > src/libcamera/base/log.cpp | 6 +++++-\n> > src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 2 ++\n> > src/libcamera/v4l2_subdevice.cpp | 3 ++-\n> > 5 files changed, 20 insertions(+), 6 deletions(-)\n> >\n> > diff --git a/src/android/camera_capabilities.cpp b/src/android/camera_capabilities.cpp\n> > index 64bd8dde..ef0d10d0 100644\n> > --- a/src/android/camera_capabilities.cpp\n> > +++ b/src/android/camera_capabilities.cpp\n> > @@ -374,14 +374,20 @@ void CameraCapabilities::computeHwLevel(\n> > \tcamera_metadata_enum_android_info_supported_hardware_level\n> > \t\thwLevel = ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_FULL;\n> >\n> > -\tif (!caps.count(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_MANUAL_SENSOR))\n> > +\tif (!caps.count(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_MANUAL_SENSOR)) {\n> > +\t\tLOG(HAL, Info) << noFull << \"missing manual sensor\";\n> > \t\thwLevel = ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_LIMITED;\n> > +\t}\n> >\n> > -\tif (!caps.count(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_MANUAL_POST_PROCESSING))\n> > +\tif (!caps.count(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_MANUAL_POST_PROCESSING)) {\n> > +\t\tLOG(HAL, Info) << noFull << \"missing manual post processing\";\n> > \t\thwLevel = ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_LIMITED;\n> > +\t}\n> >\n> > -\tif (!caps.count(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_BURST_CAPTURE))\n> > +\tif (!caps.count(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_BURST_CAPTURE)) {\n> > +\t\tLOG(HAL, Info) << noFull << \"missing burst capture\";\n> > \t\thwLevel = ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_LIMITED;\n> > +\t}\n>\n> Paul, Jacopo, could you maybe review this part ?\n>\n\nConsidering that we have below\n\n\tfound = staticMetadata_->getEntry(ANDROID_SYNC_MAX_LATENCY, &entry);\n\tif (!found || *entry.data.i32 != 0) {\n\t\tLOG(HAL, Info) << noFull << \"missing or invalid max sync latency\";\n\t\thwLevel = ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_LIMITED;\n\t}\n\nI guess it doesn't hurt.\n\nHowever I guess there was no debug printout here because 'caps' gets\npopulated in 'computeCapabilities()' where each 'validate$Capability()'\ncall already prints out what it has enabled\n\nIn example:\n\nbool CameraCapabilities::validateManualSensorCapability()\n{\n\tconst char *noMode = \"Manual sensor capability unavailable: \";\n\n\tif (!staticMetadata_->entryContains(ANDROID_CONTROL_AE_AVAILABLE_MODES,\n\t\t\t\t\t\t ANDROID_CONTROL_AE_MODE_OFF)) {\n\t\tLOG(HAL, Info) << noMode << \"missing AE mode off\";\n\t\treturn false;\n\t}\n\n\tif (!staticMetadata_->entryContains(ANDROID_CONTROL_AE_LOCK_AVAILABLE,\n\t\t\t\t\t\t ANDROID_CONTROL_AE_LOCK_AVAILABLE_TRUE)) {\n\t\tLOG(HAL, Info) << noMode << \"missing AE lock\";\n\t\treturn false;\n\t}\n\n ..\n\n}\n\nAll in all, I don't mind, but I can live without this, unless Nicholas\nhas found it particularly relevant for reasons I am missing.\n\n\n> >\n> > \tfound = staticMetadata_->getEntry(ANDROID_SYNC_MAX_LATENCY, &entry);\n> > \tif (!found || *entry.data.i32 != 0) {\n> > diff --git a/src/android/camera_hal_manager.cpp b/src/android/camera_hal_manager.cpp\n> > index 7512cc4e..7fac4e3f 100644\n> > --- a/src/android/camera_hal_manager.cpp\n> > +++ b/src/android/camera_hal_manager.cpp\n> > @@ -140,7 +140,8 @@ void CameraHalManager::cameraAdded(std::shared_ptr cam)\n> > \t */\n> > \tif (!isCameraExternal && !halConfig_.exists()) {\n> > \t\tLOG(HAL, Error)\n> > -\t\t\t<< \"HAL configuration file is mandatory for internal cameras\";\n> > +\t\t\t<< \"HAL configuration file is mandatory for internal cameras.\"\n> > +\t\t\t<< \" Camera NOT loaded: \\\"\" << cam->id() << \"\\\"\";\n> > \t\treturn;\n> > \t}\n> >\n> > diff --git a/src/libcamera/base/log.cpp b/src/libcamera/base/log.cpp\n> > index 55fbd7b0..b8c2c99f 100644\n> > --- a/src/libcamera/base/log.cpp\n> > +++ b/src/libcamera/base/log.cpp\n> > @@ -625,8 +625,12 @@ void Logger::parseLogFile()\n> > void Logger::parseLogLevels()\n> > {\n> > \tconst char *debug = utils::secure_getenv(\"LIBCAMERA_LOG_LEVELS\");\n> > -\tif (!debug)\n> > +\tif (!debug) {\n> > +\t\tsyslog(LOG_INFO, \"Could not find LIBCAMERA_LOG_LEVELS in env\");\n> > \t\treturn;\n> > +\t} else {\n> > +\t\tsyslog(LOG_INFO, \"LIBCAMERA_LOG_LEVELS is %s\", debug);\n> > +\t}\n>\n> I don't think we should log a message to syslog every time libcamera is\n> started to report the LIBCAMERA_LOG_LEVELS value. Is this a debugging\n> leftover ?\n>\n> >\n> > \tfor (const char *pair = debug; *debug != '\\0'; pair = debug) {\n> > \t\tconst char *comma = strchrnul(debug, ',');\n> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp\n> > index 2d38f0fb..a2038704 100644\n> > --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp\n> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp\n> > @@ -33,10 +33,12 @@ bool RkISP1Path::init(MediaDevice *media)\n> > \tstd::string resizer = std::string(\"rkisp1_resizer_\") + name_ + \"path\";\n> > \tstd::string video = std::string(\"rkisp1_\") + name_ + \"path\";\n> >\n> > +\tLOG(RkISP1, Debug) << \"Creating \" << resizer;\n> > \tresizer_ = V4L2Subdevice::fromEntityName(media, resizer);\n> > \tif (resizer_->open() < 0)\n> > \t\treturn false;\n> >\n> > +\tLOG(RkISP1, Debug) << \"Creating \" << video;\n>\n> Here too, is this something that you added to debug specific issues you\n> were facing, or do you expect this to be useful for users ? If the\n> latter, could you explain why ?\n>\n> > \tvideo_ = V4L2VideoDevice::fromEntityName(media, video);\n> > \tif (video_->open() < 0)\n> > \t\treturn false;\n> > diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp\n> > index 15e8206a..8f86387b 100644\n> > --- a/src/libcamera/v4l2_subdevice.cpp\n> > +++ b/src/libcamera/v4l2_subdevice.cpp\n> > @@ -392,7 +392,8 @@ int V4L2Subdevice::getSelection(unsigned int pad, unsigned int target,\n> > \tif (ret < 0) {\n> > \t\tLOG(V4L2, Error)\n> > \t\t\t<< \"Unable to get rectangle \" << target << \" on pad \"\n> > -\t\t\t<< pad << \": \" << strerror(-ret);\n> > +\t\t\t<< pad << \": \" << strerror(-ret) << \".\"\n> > +\t\t\t<< \"device path: \" << devicePath() << \" device node: \" << deviceNode();\n>\n> If the device path and device node are useful in error messages, they\n> should be printed in all of them, not just this one. This should then be\n> done through the logPrefix() function. The code currently uses the\n> entity name as a log prefix, is that not enough ? The device path, in\n> particular, seems too verbose to me.\n>\n> > \t\treturn ret;\n> > \t}\n> >\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 92B11BDB16\n\tfor ;\n\tTue, 25 Oct 2022 10:49:41 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id F1E9A62F38;\n\tTue, 25 Oct 2022 12:49:40 +0200 (CEST)","from relay7-d.mail.gandi.net (relay7-d.mail.gandi.net\n\t[IPv6:2001:4b98:dc4:8::227])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id BBCE261F4C\n\tfor ;\n\tTue, 25 Oct 2022 12:49:39 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby mail.gandi.net (Postfix) with ESMTPSA id F0EC32000A;\n\tTue, 25 Oct 2022 10:49:38 +0000 (UTC)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1666694981;\n\tbh=/GiBKb6swSuc22+khvxlJ+fyvgbKFbYblVQ7x7soBYM=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=WqP1w9D1aw03UdF078jiyUn0bcmS1QBtkcjSvqhAewe9tOtBdq2DhwmVpWWfeuSsz\n\t8iohi/cl1JPmiEnbGwBmtFnGbxutp7mWpDQGOD4dCZsvT8ERVqLeFoe2i+gWBmZUIP\n\t2BqJEXAGmWD5i5ciWkV9g22FXagx7y00C+G77SFN1Kqq8dFLlZ/2MC/m8St9OaoqSl\n\tOD/b8EmgOCD7rgrbSY2st+W6aHFtVkkthi/+SoYKygMz/DFrs9rHkV07LOI3eqbOvl\n\tfnlikPJqxXQql3yYFDkS/iNA4M71uR0nUUtjsPUuK0M98geizm7wtRHS3joD0VbR8w\n\tc0af+Y/9Flupw==","Date":"Tue, 25 Oct 2022 12:49:37 +0200","To":"Laurent Pinchart ","Message-ID":"<20221025104937.mebbabewfcekawco@uno.localdomain>","References":"<20221024055543.116040-1-nicholas@rothemail.net>\n\t<20221024055543.116040-12-nicholas@rothemail.net>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH 11/11] Adds useful debug print\n\tstatements.","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","From":"Jacopo Mondi via libcamera-devel ","Reply-To":"Jacopo Mondi ","Cc":"nicholas@rothemail.net, Nicholas Roth via libcamera-devel\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":25575,"web_url":"https://patchwork.libcamera.org/comment/25575/","msgid":"<166669639522.2677993.8312005651762272005@Monstersaurus>","date":"2022-10-25T11:13:15","subject":"Re: [libcamera-devel] [PATCH 11/11] Adds useful debug print\n\tstatements.","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Laurent,\n\nQuoting Laurent Pinchart via libcamera-devel (2022-10-25 00:59:07)\n> Hi Nicholas,\n> \n> Thank you for the patch.\n> \n> On Mon, Oct 24, 2022 at 12:55:43AM -0500, Nicholas Roth via libcamera-devel wrote:\n> > From: Nicholas Roth \n> > \n> > ---\n> > src/android/camera_capabilities.cpp | 12 +++++++++---\n> > src/android/camera_hal_manager.cpp | 3 ++-\n> > src/libcamera/base/log.cpp | 6 +++++-\n> > src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 2 ++\n> > src/libcamera/v4l2_subdevice.cpp | 3 ++-\n> > 5 files changed, 20 insertions(+), 6 deletions(-)\n> > \n> > diff --git a/src/android/camera_capabilities.cpp b/src/android/camera_capabilities.cpp\n> > index 64bd8dde..ef0d10d0 100644\n\n\n\n> > diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp\n> > index 15e8206a..8f86387b 100644\n> > --- a/src/libcamera/v4l2_subdevice.cpp\n> > +++ b/src/libcamera/v4l2_subdevice.cpp\n> > @@ -392,7 +392,8 @@ int V4L2Subdevice::getSelection(unsigned int pad, unsigned int target,\n> > if (ret < 0) {\n> > LOG(V4L2, Error)\n> > << \"Unable to get rectangle \" << target << \" on pad \"\n> > - << pad << \": \" << strerror(-ret);\n> > + << pad << \": \" << strerror(-ret) << \".\"\n> > + << \"device path: \" << devicePath() << \" device node: \" << deviceNode();\n> \n> If the device path and device node are useful in error messages, they\n> should be printed in all of them, not just this one. This should then be\n> done through the logPrefix() function. The code currently uses the\n> entity name as a log prefix, is that not enough ? The device path, in\n> particular, seems too verbose to me.\n\nThis makes me remember https://patchwork.libcamera.org/patch/12207/, and\nwish it was something that could get more traction. It can help to see\nwhat device nodes are being worked on when the logs are reported.\n\nYour earlier comment of:\n> I don't have a very strong opinion here, but I wonder if we could print\n> the video node and entity name at open time, and only the entity name in\n> other places ?\n\nstill stands, and is likely reasonable. I'll see if I can post a v2 with\nthat.\n\n--\nKieran\n\n\n> > return ret;\n> > }\n> > \n> \n> -- \n> Regards,\n> \n> Laurent Pinchart","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 404F6BD16B\n\tfor ;\n\tTue, 25 Oct 2022 11:13:20 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6D19262F39;\n\tTue, 25 Oct 2022 13:13:19 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 692F761F4C\n\tfor ;\n\tTue, 25 Oct 2022 13:13:18 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id E558B3D7;\n\tTue, 25 Oct 2022 13:13:17 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1666696399;\n\tbh=lpVkbk06oEq/gTAedvan51lCRkwj2Q5ab1BdQ5ky6kw=;\n\th=In-Reply-To:References:To:Date:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=iwaL9TFYA8xSIWN5Trmfsrgzn4p+RrqUbPyoOKqYmwcOijwfNJ5npb+oVwbkQChjA\n\tY7lkPuHDDd+EHXOADu21EpWlMx3a8dyNrMq/tnnDeuzZNj78guKk55HvfXRvf+cgs7\n\tXtLXOMvekeQtzRB0jRdxDFvNEmC5ODv4z+lACFB1QoHWFc+HzjJKBTUa8MVXmXav20\n\tGyHi+QFxhZBPD2O2k5KH35EHgVAdFZ9zGrpR2j39Rh5wDIQChvxKCyMQzU30397T79\n\tjsgDnDwYxomgsj1fOqsXTO0SUj8/PAKVVlcgTjCss8SnHqorFS9SPFewWYLV2A1Jwf\n\tl54ODAzFdHz5Q==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1666696398;\n\tbh=lpVkbk06oEq/gTAedvan51lCRkwj2Q5ab1BdQ5ky6kw=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=eoF1dIlsF5VqHGUOeGeiFuLUBDFqvJa4intyXwteXKK4FSJTGEnundpdNU+L/Eqc7\n\t0AQIAn7qmXuROiL4IYFPGI2Fpm1a/C3PeXAWDdNLS59VGziAo6z09EWWEHMn4B6+yQ\n\t3KfTk8Uqrj/1toqsZLErVGqH6gSvz6H7AJqn8bQQ="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"eoF1dIls\"; dkim-atps=neutral","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"","References":"<20221024055543.116040-1-nicholas@rothemail.net>\n\t<20221024055543.116040-12-nicholas@rothemail.net>\n\t","To":"Laurent Pinchart ,\n\tNicholas Roth via libcamera-devel ","Date":"Tue, 25 Oct 2022 12:13:15 +0100","Message-ID":"<166669639522.2677993.8312005651762272005@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH 11/11] Adds useful debug print\n\tstatements.","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","From":"Kieran Bingham via libcamera-devel\n\t","Reply-To":"Kieran Bingham ","Cc":"nicholas@rothemail.net","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":25585,"web_url":"https://patchwork.libcamera.org/comment/25585/","msgid":"","date":"2022-10-26T03:09:39","subject":"Re: [libcamera-devel] [PATCH 11/11] Adds useful debug print\n\tstatements.","submitter":{"id":137,"url":"https://patchwork.libcamera.org/api/people/137/","name":"Nicholas Roth","email":"nicholas@rothemail.net"},"content":"> All in all, I don't mind, but I can live without this, unless Nicholas\n> has found it particularly relevant for reasons I am missing.\n\nThis calls attention to the fact that the sensor is missing an important\npiece of functionality required for a FULL instead of LIMITED camera to be\nreported to Android.\n\nOn Tue, Oct 25, 2022 at 5:49 AM Jacopo Mondi wrote:\n\n> Hello,\n>\n> On Tue, Oct 25, 2022 at 02:59:07AM +0300, Laurent Pinchart via\n> libcamera-devel wrote:\n> > Hi Nicholas,\n> >\n> > Thank you for the patch.\n> >\n> > On Mon, Oct 24, 2022 at 12:55:43AM -0500, Nicholas Roth via\n> libcamera-devel wrote:\n> > > From: Nicholas Roth \n> > >\n> > > ---\n> > > src/android/camera_capabilities.cpp | 12 +++++++++---\n> > > src/android/camera_hal_manager.cpp | 3 ++-\n> > > src/libcamera/base/log.cpp | 6 +++++-\n> > > src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 2 ++\n> > > src/libcamera/v4l2_subdevice.cpp | 3 ++-\n> > > 5 files changed, 20 insertions(+), 6 deletions(-)\n> > >\n> > > diff --git a/src/android/camera_capabilities.cpp\n> b/src/android/camera_capabilities.cpp\n> > > index 64bd8dde..ef0d10d0 100644\n> > > --- a/src/android/camera_capabilities.cpp\n> > > +++ b/src/android/camera_capabilities.cpp\n> > > @@ -374,14 +374,20 @@ void CameraCapabilities::computeHwLevel(\n> > > camera_metadata_enum_android_info_supported_hardware_level\n> > > hwLevel = ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_FULL;\n> > >\n> > > - if\n> (!caps.count(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_MANUAL_SENSOR))\n> > > + if\n> (!caps.count(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_MANUAL_SENSOR)) {\n> > > + LOG(HAL, Info) << noFull << \"missing manual sensor\";\n> > > hwLevel = ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_LIMITED;\n> > > + }\n> > >\n> > > - if\n> (!caps.count(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_MANUAL_POST_PROCESSING))\n> > > + if\n> (!caps.count(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_MANUAL_POST_PROCESSING))\n> {\n> > > + LOG(HAL, Info) << noFull << \"missing manual post\n> processing\";\n> > > hwLevel = ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_LIMITED;\n> > > + }\n> > >\n> > > - if\n> (!caps.count(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_BURST_CAPTURE))\n> > > + if\n> (!caps.count(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_BURST_CAPTURE)) {\n> > > + LOG(HAL, Info) << noFull << \"missing burst capture\";\n> > > hwLevel = ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_LIMITED;\n> > > + }\n> >\n> > Paul, Jacopo, could you maybe review this part ?\n> >\n>\n> Considering that we have below\n>\n> found = staticMetadata_->getEntry(ANDROID_SYNC_MAX_LATENCY,\n> &entry);\n> if (!found || *entry.data.i32 != 0) {\n> LOG(HAL, Info) << noFull << \"missing or invalid max sync\n> latency\";\n> hwLevel = ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_LIMITED;\n> }\n>\n> I guess it doesn't hurt.\n>\n> However I guess there was no debug printout here because 'caps' gets\n> populated in 'computeCapabilities()' where each 'validate$Capability()'\n> call already prints out what it has enabled\n>\n> In example:\n>\n> bool CameraCapabilities::validateManualSensorCapability()\n> {\n> const char *noMode = \"Manual sensor capability unavailable: \";\n>\n> if\n> (!staticMetadata_->entryContains(ANDROID_CONTROL_AE_AVAILABLE_MODES,\n>\n> ANDROID_CONTROL_AE_MODE_OFF)) {\n> LOG(HAL, Info) << noMode << \"missing AE mode off\";\n> return false;\n> }\n>\n> if\n> (!staticMetadata_->entryContains(ANDROID_CONTROL_AE_LOCK_AVAILABLE,\n>\n> ANDROID_CONTROL_AE_LOCK_AVAILABLE_TRUE)) {\n> LOG(HAL, Info) << noMode << \"missing AE lock\";\n> return false;\n> }\n>\n> ..\n>\n> }\n>\n> All in all, I don't mind, but I can live without this, unless Nicholas\n> has found it particularly relevant for reasons I am missing.\n>\n>\n> > >\n> > > found = staticMetadata_->getEntry(ANDROID_SYNC_MAX_LATENCY,\n> &entry);\n> > > if (!found || *entry.data.i32 != 0) {\n> > > diff --git a/src/android/camera_hal_manager.cpp\n> b/src/android/camera_hal_manager.cpp\n> > > index 7512cc4e..7fac4e3f 100644\n> > > --- a/src/android/camera_hal_manager.cpp\n> > > +++ b/src/android/camera_hal_manager.cpp\n> > > @@ -140,7 +140,8 @@ void\n> CameraHalManager::cameraAdded(std::shared_ptr cam)\n> > > */\n> > > if (!isCameraExternal && !halConfig_.exists()) {\n> > > LOG(HAL, Error)\n> > > - << \"HAL configuration file is mandatory for\n> internal cameras\";\n> > > + << \"HAL configuration file is mandatory for\n> internal cameras.\"\n> > > + << \" Camera NOT loaded: \\\"\" << cam->id() << \"\\\"\";\n> > > return;\n> > > }\n> > >\n> > > diff --git a/src/libcamera/base/log.cpp b/src/libcamera/base/log.cpp\n> > > index 55fbd7b0..b8c2c99f 100644\n> > > --- a/src/libcamera/base/log.cpp\n> > > +++ b/src/libcamera/base/log.cpp\n> > > @@ -625,8 +625,12 @@ void Logger::parseLogFile()\n> > > void Logger::parseLogLevels()\n> > > {\n> > > const char *debug = utils::secure_getenv(\"LIBCAMERA_LOG_LEVELS\");\n> > > - if (!debug)\n> > > + if (!debug) {\n> > > + syslog(LOG_INFO, \"Could not find LIBCAMERA_LOG_LEVELS in\n> env\");\n> > > return;\n> > > + } else {\n> > > + syslog(LOG_INFO, \"LIBCAMERA_LOG_LEVELS is %s\", debug);\n> > > + }\n> >\n> > I don't think we should log a message to syslog every time libcamera is\n> > started to report the LIBCAMERA_LOG_LEVELS value. Is this a debugging\n> > leftover ?\n> >\n> > >\n> > > for (const char *pair = debug; *debug != '\\0'; pair = debug) {\n> > > const char *comma = strchrnul(debug, ',');\n> > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp\n> b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp\n> > > index 2d38f0fb..a2038704 100644\n> > > --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp\n> > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp\n> > > @@ -33,10 +33,12 @@ bool RkISP1Path::init(MediaDevice *media)\n> > > std::string resizer = std::string(\"rkisp1_resizer_\") + name_ +\n> \"path\";\n> > > std::string video = std::string(\"rkisp1_\") + name_ + \"path\";\n> > >\n> > > + LOG(RkISP1, Debug) << \"Creating \" << resizer;\n> > > resizer_ = V4L2Subdevice::fromEntityName(media, resizer);\n> > > if (resizer_->open() < 0)\n> > > return false;\n> > >\n> > > + LOG(RkISP1, Debug) << \"Creating \" << video;\n> >\n> > Here too, is this something that you added to debug specific issues you\n> > were facing, or do you expect this to be useful for users ? If the\n> > latter, could you explain why ?\n> >\n> > > video_ = V4L2VideoDevice::fromEntityName(media, video);\n> > > if (video_->open() < 0)\n> > > return false;\n> > > diff --git a/src/libcamera/v4l2_subdevice.cpp\n> b/src/libcamera/v4l2_subdevice.cpp\n> > > index 15e8206a..8f86387b 100644\n> > > --- a/src/libcamera/v4l2_subdevice.cpp\n> > > +++ b/src/libcamera/v4l2_subdevice.cpp\n> > > @@ -392,7 +392,8 @@ int V4L2Subdevice::getSelection(unsigned int pad,\n> unsigned int target,\n> > > if (ret < 0) {\n> > > LOG(V4L2, Error)\n> > > << \"Unable to get rectangle \" << target << \" on\n> pad \"\n> > > - << pad << \": \" << strerror(-ret);\n> > > + << pad << \": \" << strerror(-ret) << \".\"\n> > > + << \"device path: \" << devicePath() << \" device\n> node: \" << deviceNode();\n> >\n> > If the device path and device node are useful in error messages, they\n> > should be printed in all of them, not just this one. This should then be\n> > done through the logPrefix() function. The code currently uses the\n> > entity name as a log prefix, is that not enough ? The device path, in\n> > particular, seems too verbose to me.\n> >\n> > > return ret;\n> > > }\n> > >\n> >\n> > --\n> > Regards,\n> >\n> > Laurent Pinchart\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 5D355BD16B\n\tfor ;\n\tWed, 26 Oct 2022 03:09:54 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9AD7262F48;\n\tWed, 26 Oct 2022 05:09:53 +0200 (CEST)","from mail-io1-xd34.google.com (mail-io1-xd34.google.com\n\t[IPv6:2607:f8b0:4864:20::d34])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 0B1DC603CF\n\tfor ;\n\tWed, 26 Oct 2022 05:09:52 +0200 (CEST)","by mail-io1-xd34.google.com with SMTP id l127so12265336iof.12\n\tfor ;\n\tTue, 25 Oct 2022 20:09:51 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1666753793;\n\tbh=p8EgbL4U22RD9cS2cJx29zpZhAr6LTim4rq2uYJxkdM=;\n\th=References:In-Reply-To:Date:To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=nJ61UB4rKkoEICywQ3jn3Bk/G89YVhae/hRcf5XbKNKvncYsJMKnQCwROZ/CbTvMg\n\tLPQfGbisb4rKBpaho5dJ/zCnWylfUwYSwR0eHh3md7pIC4srmiPcSYUbYFExIzoq2M\n\tLNq7OgxzCYHu2Q4ZmCK2079VZ8maWI5HXcMxxaeLmBCUbUCPWDtPT2T/UIvJ4TpJUF\n\tNwgr1uVGPFNXWeVPC1b40E+iWLvY7k+Xd+RZ2KYZ2mM+MZRrHu9XIUNS99RhOmc4YU\n\tLd+2eJVQmgnUJudOvj2MkmByC5uxOvDfrR+QjJyIA9rxtYN7emSu6tny2rGFZpIsWq\n\tJYI0b4Bdq2yHQ==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=rothemail-net.20210112.gappssmtp.com; s=20210112;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:from:to:cc:subject:date:message-id:reply-to;\n\tbh=8dkVF1Vh7jpd8PL9JR6fL9BiJ/ANAJyBhT2O5z5SkGU=;\n\tb=oHJoRTrrdPaDSrJbCqJEnX0j0Uee0g08NA9rfGvS3NseVbGpZJDGZ2jsJzosguRLeX\n\tGOQ5VP9/n61y4e6ExzoIf9UA6E5jOP6gNxM6DDTz63YPmWoBkO4rxO2FGGW8aF3ceqic\n\tG59YjrtDezi9GeH1IakpdTHqLptG9+aVHavQSfrcqlZN7CIJpzCk8dZi/8aC9CJK2+jw\n\tpx8Hgkxh4k/FfC2ruQs8tqhFw1cVmOhUXXA21GTkz/Fcmlmctr73gtGtpy5viG9x5d4F\n\tkFsTBp4raOeGtwa/1v21aF4yb0z0WZ65/wD1dTnqOuXTNR7mYfnV+HhxGP8fpt+UtkUX\n\tisgA=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected)\n\theader.d=rothemail-net.20210112.gappssmtp.com\n\theader.i=@rothemail-net.20210112.gappssmtp.com header.b=\"oHJoRTrr\"; \n\tdkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:x-gm-message-state:from:to:cc:subject:date:message-id\n\t:reply-to;\n\tbh=8dkVF1Vh7jpd8PL9JR6fL9BiJ/ANAJyBhT2O5z5SkGU=;\n\tb=IlAmd6iyj8otOlQ/lSj6jM23+hWwbkW9nxUyuew+oi1xBrsP2KYyOzh8kLh8EkQ5Dj\n\tTpnIpxvP+1qTA8mNGw+0onlT50nPc+vJufrAzJr0aCnQuYQ8u8K19AYY+Y6Aq+zNrThq\n\tReD1XgceC+wX6wVIjfmxJKPH77AX7ewhNCFovElMafyvPY53JaCP0JhYmVqA+7CcGAlj\n\tmAYtXyaa3vJ75ANuPU+lAQAKlELPBIBg/BT17VE0xtLmLO7gMs+jN8Uuxcu+WE3XRE2M\n\tDsp1Es91wtqTyajccQPtVN+mPuYyLKBzf9rbRnpXpXZU/bj7w6DgWvYfD3OJ+ijgTaLa\n\tPDiA==","X-Gm-Message-State":"ACrzQf2+hUMjI7rGH56kBUmsfQqXBbfAaHzlqGGnM7TVldC9b1/UMggb\n\tDZhPgF1aU0Y+69mKvWH0qJlDu7Il6Mt9rgkiaNx1MeCwmX9XGafj","X-Google-Smtp-Source":"AMsMyM6YwL9BnNuyxQdRF4uGoHOHYQuU08Qtw6mnJwaECB/BrfYDwGLIB2SzkI2DWsLGa0KavnkXwWGO/zoxn8TL5ps=","X-Received":"by 2002:a5e:c012:0:b0:6c0:caff:3add with SMTP id\n\tu18-20020a5ec012000000b006c0caff3addmr2063042iol.126.1666753790617;\n\tTue, 25 Oct 2022 20:09:50 -0700 (PDT)","MIME-Version":"1.0","References":"<20221024055543.116040-1-nicholas@rothemail.net>\n\t<20221024055543.116040-12-nicholas@rothemail.net>\n\t\n\t<20221025104937.mebbabewfcekawco@uno.localdomain>","In-Reply-To":"<20221025104937.mebbabewfcekawco@uno.localdomain>","Date":"Tue, 25 Oct 2022 22:09:39 -0500","Message-ID":"","To":"Jacopo Mondi ","Content-Type":"multipart/alternative; boundary=\"0000000000009e4f5c05ebe75ba1\"","Subject":"Re: [libcamera-devel] [PATCH 11/11] Adds useful debug print\n\tstatements.","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","From":"Nicholas Roth via libcamera-devel ","Reply-To":"Nicholas Roth ","Cc":"Nicholas Roth via libcamera-devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":25586,"web_url":"https://patchwork.libcamera.org/comment/25586/","msgid":"","date":"2022-10-26T03:10:51","subject":"Re: [libcamera-devel] [PATCH 11/11] Adds useful debug print\n\tstatements.","submitter":{"id":137,"url":"https://patchwork.libcamera.org/api/people/137/","name":"Nicholas Roth","email":"nicholas@rothemail.net"},"content":"Replied too fast! Specifically, this calls attention to _why_ that may be\nthe case. I had to hunt this down and debug in the code when I found that\nmy camera was marked as LIMITED. It will be helpful for other users to see\nthis in their logs right above the warning marking their cameras as LIMITED\nso that they know why.\n\n\nOn Tue, Oct 25, 2022 at 5:49 AM Jacopo Mondi wrote:\n\n> Hello,\n>\n> On Tue, Oct 25, 2022 at 02:59:07AM +0300, Laurent Pinchart via\n> libcamera-devel wrote:\n> > Hi Nicholas,\n> >\n> > Thank you for the patch.\n> >\n> > On Mon, Oct 24, 2022 at 12:55:43AM -0500, Nicholas Roth via\n> libcamera-devel wrote:\n> > > From: Nicholas Roth \n> > >\n> > > ---\n> > > src/android/camera_capabilities.cpp | 12 +++++++++---\n> > > src/android/camera_hal_manager.cpp | 3 ++-\n> > > src/libcamera/base/log.cpp | 6 +++++-\n> > > src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 2 ++\n> > > src/libcamera/v4l2_subdevice.cpp | 3 ++-\n> > > 5 files changed, 20 insertions(+), 6 deletions(-)\n> > >\n> > > diff --git a/src/android/camera_capabilities.cpp\n> b/src/android/camera_capabilities.cpp\n> > > index 64bd8dde..ef0d10d0 100644\n> > > --- a/src/android/camera_capabilities.cpp\n> > > +++ b/src/android/camera_capabilities.cpp\n> > > @@ -374,14 +374,20 @@ void CameraCapabilities::computeHwLevel(\n> > > camera_metadata_enum_android_info_supported_hardware_level\n> > > hwLevel = ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_FULL;\n> > >\n> > > - if\n> (!caps.count(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_MANUAL_SENSOR))\n> > > + if\n> (!caps.count(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_MANUAL_SENSOR)) {\n> > > + LOG(HAL, Info) << noFull << \"missing manual sensor\";\n> > > hwLevel = ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_LIMITED;\n> > > + }\n> > >\n> > > - if\n> (!caps.count(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_MANUAL_POST_PROCESSING))\n> > > + if\n> (!caps.count(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_MANUAL_POST_PROCESSING))\n> {\n> > > + LOG(HAL, Info) << noFull << \"missing manual post\n> processing\";\n> > > hwLevel = ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_LIMITED;\n> > > + }\n> > >\n> > > - if\n> (!caps.count(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_BURST_CAPTURE))\n> > > + if\n> (!caps.count(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_BURST_CAPTURE)) {\n> > > + LOG(HAL, Info) << noFull << \"missing burst capture\";\n> > > hwLevel = ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_LIMITED;\n> > > + }\n> >\n> > Paul, Jacopo, could you maybe review this part ?\n> >\n>\n> Considering that we have below\n>\n> found = staticMetadata_->getEntry(ANDROID_SYNC_MAX_LATENCY,\n> &entry);\n> if (!found || *entry.data.i32 != 0) {\n> LOG(HAL, Info) << noFull << \"missing or invalid max sync\n> latency\";\n> hwLevel = ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_LIMITED;\n> }\n>\n> I guess it doesn't hurt.\n>\n> However I guess there was no debug printout here because 'caps' gets\n> populated in 'computeCapabilities()' where each 'validate$Capability()'\n> call already prints out what it has enabled\n>\n> In example:\n>\n> bool CameraCapabilities::validateManualSensorCapability()\n> {\n> const char *noMode = \"Manual sensor capability unavailable: \";\n>\n> if\n> (!staticMetadata_->entryContains(ANDROID_CONTROL_AE_AVAILABLE_MODES,\n>\n> ANDROID_CONTROL_AE_MODE_OFF)) {\n> LOG(HAL, Info) << noMode << \"missing AE mode off\";\n> return false;\n> }\n>\n> if\n> (!staticMetadata_->entryContains(ANDROID_CONTROL_AE_LOCK_AVAILABLE,\n>\n> ANDROID_CONTROL_AE_LOCK_AVAILABLE_TRUE)) {\n> LOG(HAL, Info) << noMode << \"missing AE lock\";\n> return false;\n> }\n>\n> ..\n>\n> }\n>\n> All in all, I don't mind, but I can live without this, unless Nicholas\n> has found it particularly relevant for reasons I am missing.\n>\n>\n> > >\n> > > found = staticMetadata_->getEntry(ANDROID_SYNC_MAX_LATENCY,\n> &entry);\n> > > if (!found || *entry.data.i32 != 0) {\n> > > diff --git a/src/android/camera_hal_manager.cpp\n> b/src/android/camera_hal_manager.cpp\n> > > index 7512cc4e..7fac4e3f 100644\n> > > --- a/src/android/camera_hal_manager.cpp\n> > > +++ b/src/android/camera_hal_manager.cpp\n> > > @@ -140,7 +140,8 @@ void\n> CameraHalManager::cameraAdded(std::shared_ptr cam)\n> > > */\n> > > if (!isCameraExternal && !halConfig_.exists()) {\n> > > LOG(HAL, Error)\n> > > - << \"HAL configuration file is mandatory for\n> internal cameras\";\n> > > + << \"HAL configuration file is mandatory for\n> internal cameras.\"\n> > > + << \" Camera NOT loaded: \\\"\" << cam->id() << \"\\\"\";\n> > > return;\n> > > }\n> > >\n> > > diff --git a/src/libcamera/base/log.cpp b/src/libcamera/base/log.cpp\n> > > index 55fbd7b0..b8c2c99f 100644\n> > > --- a/src/libcamera/base/log.cpp\n> > > +++ b/src/libcamera/base/log.cpp\n> > > @@ -625,8 +625,12 @@ void Logger::parseLogFile()\n> > > void Logger::parseLogLevels()\n> > > {\n> > > const char *debug = utils::secure_getenv(\"LIBCAMERA_LOG_LEVELS\");\n> > > - if (!debug)\n> > > + if (!debug) {\n> > > + syslog(LOG_INFO, \"Could not find LIBCAMERA_LOG_LEVELS in\n> env\");\n> > > return;\n> > > + } else {\n> > > + syslog(LOG_INFO, \"LIBCAMERA_LOG_LEVELS is %s\", debug);\n> > > + }\n> >\n> > I don't think we should log a message to syslog every time libcamera is\n> > started to report the LIBCAMERA_LOG_LEVELS value. Is this a debugging\n> > leftover ?\n> >\n> > >\n> > > for (const char *pair = debug; *debug != '\\0'; pair = debug) {\n> > > const char *comma = strchrnul(debug, ',');\n> > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp\n> b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp\n> > > index 2d38f0fb..a2038704 100644\n> > > --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp\n> > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp\n> > > @@ -33,10 +33,12 @@ bool RkISP1Path::init(MediaDevice *media)\n> > > std::string resizer = std::string(\"rkisp1_resizer_\") + name_ +\n> \"path\";\n> > > std::string video = std::string(\"rkisp1_\") + name_ + \"path\";\n> > >\n> > > + LOG(RkISP1, Debug) << \"Creating \" << resizer;\n> > > resizer_ = V4L2Subdevice::fromEntityName(media, resizer);\n> > > if (resizer_->open() < 0)\n> > > return false;\n> > >\n> > > + LOG(RkISP1, Debug) << \"Creating \" << video;\n> >\n> > Here too, is this something that you added to debug specific issues you\n> > were facing, or do you expect this to be useful for users ? If the\n> > latter, could you explain why ?\n> >\n> > > video_ = V4L2VideoDevice::fromEntityName(media, video);\n> > > if (video_->open() < 0)\n> > > return false;\n> > > diff --git a/src/libcamera/v4l2_subdevice.cpp\n> b/src/libcamera/v4l2_subdevice.cpp\n> > > index 15e8206a..8f86387b 100644\n> > > --- a/src/libcamera/v4l2_subdevice.cpp\n> > > +++ b/src/libcamera/v4l2_subdevice.cpp\n> > > @@ -392,7 +392,8 @@ int V4L2Subdevice::getSelection(unsigned int pad,\n> unsigned int target,\n> > > if (ret < 0) {\n> > > LOG(V4L2, Error)\n> > > << \"Unable to get rectangle \" << target << \" on\n> pad \"\n> > > - << pad << \": \" << strerror(-ret);\n> > > + << pad << \": \" << strerror(-ret) << \".\"\n> > > + << \"device path: \" << devicePath() << \" device\n> node: \" << deviceNode();\n> >\n> > If the device path and device node are useful in error messages, they\n> > should be printed in all of them, not just this one. This should then be\n> > done through the logPrefix() function. The code currently uses the\n> > entity name as a log prefix, is that not enough ? The device path, in\n> > particular, seems too verbose to me.\n> >\n> > > return ret;\n> > > }\n> > >\n> >\n> > --\n> > Regards,\n> >\n> > Laurent Pinchart\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 24D9FBDB16\n\tfor ;\n\tWed, 26 Oct 2022 03:11:05 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id CE47962F4A;\n\tWed, 26 Oct 2022 05:11:04 +0200 (CEST)","from mail-io1-xd2b.google.com (mail-io1-xd2b.google.com\n\t[IPv6:2607:f8b0:4864:20::d2b])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2ED61603CF\n\tfor ;\n\tWed, 26 Oct 2022 05:11:03 +0200 (CEST)","by mail-io1-xd2b.google.com with SMTP id s27so2767526ioa.10\n\tfor ;\n\tTue, 25 Oct 2022 20:11:03 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1666753864;\n\tbh=2MN7u4tLs5zedS57SK24kwIfOUdA9GcqShxfvkO9Zl0=;\n\th=References:In-Reply-To:Date:To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=3LdaZH2L2IQYHi9g3zPikoUaLfRUJKjSZId5UYp0Y8wedA2uDbCzVzbB+2zO2PFHu\n\tXvZcn0vqlEEfH3+54Q4H72X7hL0nA5QOId1YjpSgJUowJaojHiNGC4HCuwksdXgNF8\n\tfFGUw9toHGmNs28mrF9ZyILYNfRtH8WLoQk4014AWCllhYL+hbbULHG+XXNIOKSgre\n\tUnYqvnHzsJU6Mrkd2M8zTOy52DauFtQYSjPIPbiNh8HpOFZeKYif41QP9IUzXE2M7d\n\tnKsB1OpRSmhwibqc8P44hiAhInjBXFuLeeQjlexmXcED+1GB2dMkjNInaWwqX1v4fS\n\tDzDW6zdHSAmWQ==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=rothemail-net.20210112.gappssmtp.com; s=20210112;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:from:to:cc:subject:date:message-id:reply-to;\n\tbh=2rV41BINpC2uiNG1l5ut9ip15b8vZ5/3X0oqBqDsa3A=;\n\tb=dMFL37l1cp1ZLECygyMzDPv3Aephl4zwScCd4gH+dp1+FAmKd56d7KoNy2wsoNlPIZ\n\tqx9zWq/Gma8JXPJJm4M6oIbqjaXry87dvfEyIZyDJ7j03uCaUhZ8yBTXeHJIXqjCup8A\n\txY1TvsnGT8wZsN3odQNOP1RTAhoHtQgErkb9LIe6IzzO5OhH6SjI7yuj/kij86fldfsu\n\tVlaOVoPszn3aaNwHPVgOhyp/DdQS1P6GKSuoAvtx3/+Pv949YmJmf9KftGXR33cBscN7\n\tYFmvT74mBEDXZNb8F00oaytfXsGHkRGTEr1jM4KIx4mjOpCYh4B7XsNq6MmNzo2vfe9C\n\tLvwA=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected)\n\theader.d=rothemail-net.20210112.gappssmtp.com\n\theader.i=@rothemail-net.20210112.gappssmtp.com header.b=\"dMFL37l1\"; \n\tdkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:x-gm-message-state:from:to:cc:subject:date:message-id\n\t:reply-to;\n\tbh=2rV41BINpC2uiNG1l5ut9ip15b8vZ5/3X0oqBqDsa3A=;\n\tb=ScjTAXDAiiUAauN+jcCbVlUiir014NdkoeErrcSExhZE2AuycfctiCi7ZF51jFZcze\n\tEucS8zYTesVcYEpd/WfYbM5WmV8WyuPPMFT/HrKF8vM/rvrMHKsfy5SAlCkK4VMtIcB3\n\t55aEWUgW0Sdy+M7lW2+8LP6b/r8ZGLrOW/L+k3K2wlK7FXKLbgNlpFAnOYtxUHrQRKg5\n\t2Afv/XBZC2lEd8yBnNQBpZdAZJ/oDEgXDWIl1LhxBYJFBvixLhMYcDIBQWx3PQI/a3+a\n\tMZxRj8WwES0ob+QGCbMECwrPZsYTLMzlTtTVNUa5OuAHahoyEWMXwJUqFAa2cvaTnVwQ\n\tgDeg==","X-Gm-Message-State":"ACrzQf0dUoJ+z9k/Tbn72H7omPTpRmm98GyqvTs2NT+I1Xsrs7TnhBQt\n\tR9Yn7hDb6KF+ynhDhSSSTdRpt8Z2bRNHSGy/w/OTv+mrajp7Qg==","X-Google-Smtp-Source":"AMsMyM7ORbRcV2ayf5keneT0ehszfOXOC1kXiobm72JH2DjKN3inNSJQhR6MnKiHJpTqe5A8vRTUzl5Rnl1wAbHHE6I=","X-Received":"by 2002:a05:6602:154f:b0:6bc:f701:cb37 with SMTP id\n\th15-20020a056602154f00b006bcf701cb37mr25739850iow.136.1666753861938;\n\tTue, 25 Oct 2022 20:11:01 -0700 (PDT)","MIME-Version":"1.0","References":"<20221024055543.116040-1-nicholas@rothemail.net>\n\t<20221024055543.116040-12-nicholas@rothemail.net>\n\t\n\t<20221025104937.mebbabewfcekawco@uno.localdomain>","In-Reply-To":"<20221025104937.mebbabewfcekawco@uno.localdomain>","Date":"Tue, 25 Oct 2022 22:10:51 -0500","Message-ID":"","To":"Jacopo Mondi ","Content-Type":"multipart/alternative; boundary=\"000000000000de87ea05ebe75fb7\"","Subject":"Re: [libcamera-devel] [PATCH 11/11] Adds useful debug print\n\tstatements.","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","From":"Nicholas Roth via libcamera-devel ","Reply-To":"Nicholas Roth ","Cc":"Nicholas Roth via libcamera-devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]