<div dir="ltr"><div><div dir="ltr" class="gmail_signature" data-smartmail="gmail_signature"><div dir="ltr"><div dir="ltr"><div style="font-size:12.8px"><div style="color:rgb(136,136,136);font-size:12.8px"><div style="font-size:small"><font face="arial, helvetica, sans-serif" color="#000000">Replied too fast! Specifically, this calls attention to _why_ that may be the case. I had to hunt this down and debug in the code when I found that my camera was marked as LIMITED. It will be helpful for other users to see this in their logs right above the warning marking their cameras as LIMITED so that they know why.</font></div></div></div></div></div></div></div><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, Oct 25, 2022 at 5:49 AM Jacopo Mondi <<a href="mailto:jacopo@jmondi.org">jacopo@jmondi.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hello,<br>
<br>
On Tue, Oct 25, 2022 at 02:59:07AM +0300, Laurent Pinchart via libcamera-devel wrote:<br>
> Hi Nicholas,<br>
><br>
> Thank you for the patch.<br>
><br>
> On Mon, Oct 24, 2022 at 12:55:43AM -0500, Nicholas Roth via libcamera-devel wrote:<br>
> > From: Nicholas Roth <<a href="mailto:nicholas@rothemail.net" target="_blank">nicholas@rothemail.net</a>><br>
> ><br>
> > ---<br>
> > src/android/camera_capabilities.cpp | 12 +++++++++---<br>
> > src/android/camera_hal_manager.cpp | 3 ++-<br>
> > src/libcamera/base/log.cpp | 6 +++++-<br>
> > src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 2 ++<br>
> > src/libcamera/v4l2_subdevice.cpp | 3 ++-<br>
> > 5 files changed, 20 insertions(+), 6 deletions(-)<br>
> ><br>
> > diff --git a/src/android/camera_capabilities.cpp b/src/android/camera_capabilities.cpp<br>
> > index 64bd8dde..ef0d10d0 100644<br>
> > --- a/src/android/camera_capabilities.cpp<br>
> > +++ b/src/android/camera_capabilities.cpp<br>
> > @@ -374,14 +374,20 @@ void CameraCapabilities::computeHwLevel(<br>
> > camera_metadata_enum_android_info_supported_hardware_level<br>
> > hwLevel = ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_FULL;<br>
> ><br>
> > - if (!caps.count(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_MANUAL_SENSOR))<br>
> > + if (!caps.count(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_MANUAL_SENSOR)) {<br>
> > + LOG(HAL, Info) << noFull << "missing manual sensor";<br>
> > hwLevel = ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_LIMITED;<br>
> > + }<br>
> ><br>
> > - if (!caps.count(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_MANUAL_POST_PROCESSING))<br>
> > + if (!caps.count(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_MANUAL_POST_PROCESSING)) {<br>
> > + LOG(HAL, Info) << noFull << "missing manual post processing";<br>
> > hwLevel = ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_LIMITED;<br>
> > + }<br>
> ><br>
> > - if (!caps.count(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_BURST_CAPTURE))<br>
> > + if (!caps.count(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_BURST_CAPTURE)) {<br>
> > + LOG(HAL, Info) << noFull << "missing burst capture";<br>
> > hwLevel = ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_LIMITED;<br>
> > + }<br>
><br>
> Paul, Jacopo, could you maybe review this part ?<br>
><br>
<br>
Considering that we have below<br>
<br>
found = staticMetadata_->getEntry(ANDROID_SYNC_MAX_LATENCY, &entry);<br>
if (!found || *entry.data.i32 != 0) {<br>
LOG(HAL, Info) << noFull << "missing or invalid max sync latency";<br>
hwLevel = ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_LIMITED;<br>
}<br>
<br>
I guess it doesn't hurt.<br>
<br>
However I guess there was no debug printout here because 'caps' gets<br>
populated in 'computeCapabilities()' where each 'validate$Capability()'<br>
call already prints out what it has enabled<br>
<br>
In example:<br>
<br>
bool CameraCapabilities::validateManualSensorCapability()<br>
{<br>
const char *noMode = "Manual sensor capability unavailable: ";<br>
<br>
if (!staticMetadata_->entryContains<uint8_t>(ANDROID_CONTROL_AE_AVAILABLE_MODES,<br>
ANDROID_CONTROL_AE_MODE_OFF)) {<br>
LOG(HAL, Info) << noMode << "missing AE mode off";<br>
return false;<br>
}<br>
<br>
if (!staticMetadata_->entryContains<uint8_t>(ANDROID_CONTROL_AE_LOCK_AVAILABLE,<br>
ANDROID_CONTROL_AE_LOCK_AVAILABLE_TRUE)) {<br>
LOG(HAL, Info) << noMode << "missing AE lock";<br>
return false;<br>
}<br>
<br>
..<br>
<br>
}<br>
<br>
All in all, I don't mind, but I can live without this, unless Nicholas<br>
has found it particularly relevant for reasons I am missing.<br>
<br>
<br>
> ><br>
> > found = staticMetadata_->getEntry(ANDROID_SYNC_MAX_LATENCY, &entry);<br>
> > if (!found || *entry.data.i32 != 0) {<br>
> > diff --git a/src/android/camera_hal_manager.cpp b/src/android/camera_hal_manager.cpp<br>
> > index 7512cc4e..7fac4e3f 100644<br>
> > --- a/src/android/camera_hal_manager.cpp<br>
> > +++ b/src/android/camera_hal_manager.cpp<br>
> > @@ -140,7 +140,8 @@ void CameraHalManager::cameraAdded(std::shared_ptr<Camera> cam)<br>
> > */<br>
> > if (!isCameraExternal && !halConfig_.exists()) {<br>
> > LOG(HAL, Error)<br>
> > - << "HAL configuration file is mandatory for internal cameras";<br>
> > + << "HAL configuration file is mandatory for internal cameras."<br>
> > + << " Camera NOT loaded: \"" << cam->id() << "\"";<br>
> > return;<br>
> > }<br>
> ><br>
> > diff --git a/src/libcamera/base/log.cpp b/src/libcamera/base/log.cpp<br>
> > index 55fbd7b0..b8c2c99f 100644<br>
> > --- a/src/libcamera/base/log.cpp<br>
> > +++ b/src/libcamera/base/log.cpp<br>
> > @@ -625,8 +625,12 @@ void Logger::parseLogFile()<br>
> > void Logger::parseLogLevels()<br>
> > {<br>
> > const char *debug = utils::secure_getenv("LIBCAMERA_LOG_LEVELS");<br>
> > - if (!debug)<br>
> > + if (!debug) {<br>
> > + syslog(LOG_INFO, "Could not find LIBCAMERA_LOG_LEVELS in env");<br>
> > return;<br>
> > + } else {<br>
> > + syslog(LOG_INFO, "LIBCAMERA_LOG_LEVELS is %s", debug);<br>
> > + }<br>
><br>
> I don't think we should log a message to syslog every time libcamera is<br>
> started to report the LIBCAMERA_LOG_LEVELS value. Is this a debugging<br>
> leftover ?<br>
><br>
> ><br>
> > for (const char *pair = debug; *debug != '\0'; pair = debug) {<br>
> > const char *comma = strchrnul(debug, ',');<br>
> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp<br>
> > index 2d38f0fb..a2038704 100644<br>
> > --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp<br>
> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp<br>
> > @@ -33,10 +33,12 @@ bool RkISP1Path::init(MediaDevice *media)<br>
> > std::string resizer = std::string("rkisp1_resizer_") + name_ + "path";<br>
> > std::string video = std::string("rkisp1_") + name_ + "path";<br>
> ><br>
> > + LOG(RkISP1, Debug) << "Creating " << resizer;<br>
> > resizer_ = V4L2Subdevice::fromEntityName(media, resizer);<br>
> > if (resizer_->open() < 0)<br>
> > return false;<br>
> ><br>
> > + LOG(RkISP1, Debug) << "Creating " << video;<br>
><br>
> Here too, is this something that you added to debug specific issues you<br>
> were facing, or do you expect this to be useful for users ? If the<br>
> latter, could you explain why ?<br>
><br>
> > video_ = V4L2VideoDevice::fromEntityName(media, video);<br>
> > if (video_->open() < 0)<br>
> > return false;<br>
> > diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp<br>
> > index 15e8206a..8f86387b 100644<br>
> > --- a/src/libcamera/v4l2_subdevice.cpp<br>
> > +++ b/src/libcamera/v4l2_subdevice.cpp<br>
> > @@ -392,7 +392,8 @@ int V4L2Subdevice::getSelection(unsigned int pad, unsigned int target,<br>
> > if (ret < 0) {<br>
> > LOG(V4L2, Error)<br>
> > << "Unable to get rectangle " << target << " on pad "<br>
> > - << pad << ": " << strerror(-ret);<br>
> > + << pad << ": " << strerror(-ret) << "."<br>
> > + << "device path: " << devicePath() << " device node: " << deviceNode();<br>
><br>
> If the device path and device node are useful in error messages, they<br>
> should be printed in all of them, not just this one. This should then be<br>
> done through the logPrefix() function. The code currently uses the<br>
> entity name as a log prefix, is that not enough ? The device path, in<br>
> particular, seems too verbose to me.<br>
><br>
> > return ret;<br>
> > }<br>
> ><br>
><br>
> --<br>
> Regards,<br>
><br>
> Laurent Pinchart<br>
</blockquote></div>