<div dir="ltr">> 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.<div><br></div><div>This calls attention to the fact that the sensor is missing an important piece of functionality required for a FULL instead of LIMITED camera to be reported to Android.</div></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>