[libcamera-devel] [RFC PATCH] wip: android: capabilities: Capability detection by population
paul.elder at ideasonboard.com
paul.elder at ideasonboard.com
Thu Jul 15 10:46:41 CEST 2021
Hi Jacopo,
On Thu, Jul 15, 2021 at 10:44:01AM +0200, Jacopo Mondi wrote:
> Hi Paul,
> I'm so sorry for getting that late to this one
>
> On Sun, Jul 11, 2021 at 11:50:44PM +0300, Laurent Pinchart wrote:
> > Hi Paul,
> >
> > Thank you for the patch.
> >
> > On Fri, Jul 09, 2021 at 08:34:46PM +0900, paul.elder at ideasonboard.com wrote:
> > > On Fri, Jul 09, 2021 at 08:28:14PM +0900, Paul Elder wrote:
> > > > Implement capability and hardware level detection based on the static
> > > > metadata that has been set, instead of disabling them as requirements
> > > > are not met. This results in cleaner code where the static metadata is
> > > > set.
> > > >
> > > > Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
> > > >
> > > > ---
> > > > This currently does not work as the ControlInfo constructor for Spans is
> > > > broken. This is more of an RFC for Jacopo to check that this is the
> > > > direction that he wants.
>
> Thanks, this goes exactly in the direction I asked for
Phew, that's good. I continue in this direction then.
>
> > > >
> > > > Clearly this is not meant to be merged, as the actual capability
> > > > requirements are not fully specified yet. Plus they belong in separate
> > > > patches anyway.
> > > >
> > > > This patch does not apply either, as it depends on many unreleased
> > > > patches.
> >
> > That's quite a long disclaimers list :-)
> >
> > > > ---
> > > > src/android/camera_capabilities.cpp | 219 ++++++++++++++++++++--------
> > > > 1 file changed, 157 insertions(+), 62 deletions(-)
> > > >
> > > > diff --git a/src/android/camera_capabilities.cpp b/src/android/camera_capabilities.cpp
> > > > index 83c7f0d0..ceb5cfe8 100644
> > > > --- a/src/android/camera_capabilities.cpp
> > > > +++ b/src/android/camera_capabilities.cpp
> > > > @@ -262,6 +262,149 @@ std::vector<T> setMetadata(CameraMetadata *metadata, uint32_t tag,
> > > > return ret;
> > > > }
> > > >
> > > > +
> > > > +template<typename T>
> > > > +bool metadataContains(camera_metadata_ro_entry_t &entry, T value);
> > > > +
> > > > +template<>
> > > > +bool metadataContains<uint8_t>(camera_metadata_ro_entry_t &entry, uint8_t value)
> > > > +{
> > > > + for (unsigned int i = 0; i < entry.count; i++)
> > > > + if (entry.data.u8[i] == value)
> > > > + return true;
> > > > +
> > > > + return false;
> > > > +}
> >
> > This function could also be moved to the CameraMetadata class (named
> > entryContains() or something similar), and include the getEntry() call.
> >
> > > > +
> > > > +bool validateManualSensorCapability(CameraMetadata *staticMetadata)
> > > > +{
> > > > + camera_metadata_ro_entry_t entry;
> > > > + bool found;
> > > > +
> > > > + found = staticMetadata->getEntry(ANDROID_CONTROL_AE_AVAILABLE_MODES,
> > > > + &entry);
> > > > + if (!found || !metadataContains<uint8_t>(entry, ANDROID_CONTROL_AE_MODE_OFF)) {
> > > > + LOG(HAL, Info)
> >
> > Should this be downgraded to Debug ? An overall Info message that prints
> > the hardware level is good, but maybe detailed messages would be too
> > verbose. Or, as we only print a message on the first missing metadata
> > entry, would something along the lines of
> >
> > "Manual sensor capability unvailable: missing AE mode OFF"
> >
> > be good to include as an Info message ?
> >
> > > > + << "Missing AE mode off: "
> > > > + << (found ? "not supported" : "not found");
> > > > + return false;
> > > > + }
> > > > +
> > > > + found = staticMetadata->getEntry(ANDROID_CONTROL_AE_LOCK_AVAILABLE,
> > > > + &entry);
> > > > + if (!found || !(*entry.data.u8 == ANDROID_CONTROL_AE_LOCK_AVAILABLE_TRUE)) {
> > > > + LOG(HAL, Info) << "Missing AE lock";
> > > > + return false;
> > > > + }
> > > > +
> > > > + found = staticMetadata->getEntry(ANDROID_EDGE_AVAILABLE_EDGE_MODES,
> > > > + &entry);
> >
> > If you think it could help making code more readable, the CameraMetadata
> > class could get a hasEntry() function.
> >
> > > > + if (!found) {
> > > > + LOG(HAL, Info) << "Missing edge modes";
> > > > + return false;
> > > > + }
> > > > +
> > > > + return true;
> > > > +}
> > > > +
> > > > +bool validateManualPostProcessingCapability(CameraMetadata *staticMetadata)
> > > > +{
> > > > + camera_metadata_ro_entry_t entry;
> > > > + bool found;
> > > > +
> > > > + found = staticMetadata->getEntry(ANDROID_CONTROL_AWB_AVAILABLE_MODES,
> > > > + &entry);
> > > > + if (!found || !metadataContains<uint8_t>(entry, ANDROID_CONTROL_AWB_MODE_OFF)) {
> > > > + LOG(HAL, Info) << "Missing AWB mode off";
> > > > + return false;
> > > > + }
> > > > +
> > > > + found = staticMetadata->getEntry(ANDROID_CONTROL_AWB_LOCK_AVAILABLE,
> > > > + &entry);
> > > > + if (!found || !(*entry.data.u8 == ANDROID_CONTROL_AWB_LOCK_AVAILABLE_TRUE)) {
> > > > + LOG(HAL, Info) << "Missing AWB lock";
> > > > + return false;
> > > > + }
> > > > +
> > > > + return true;
> > > > +}
> > > > +
> > > > +bool validateBurstCaptureCapability(CameraMetadata *staticMetadata)
> > > > +{
> > > > + camera_metadata_ro_entry_t entry;
> > > > + bool found;
> > > > +
> > > > + found = staticMetadata->getEntry(ANDROID_CONTROL_AE_LOCK_AVAILABLE,
> > > > + &entry);
> > > > + if (!found || !(*entry.data.u8 == ANDROID_CONTROL_AE_LOCK_AVAILABLE_TRUE)) {
> > > > + LOG(HAL, Info) << "Missing AE lock";
> > > > + return false;
> > > > + }
> > > > +
> > > > + found = staticMetadata->getEntry(ANDROID_CONTROL_AWB_LOCK_AVAILABLE,
> > > > + &entry);
> > > > + if (!found || !(*entry.data.u8 == ANDROID_CONTROL_AWB_LOCK_AVAILABLE_TRUE)) {
> > > > + LOG(HAL, Info) << "Missing AWB lock";
> > > > + return false;
> > > > + }
> > > > +
> > > > + found = staticMetadata->getEntry(ANDROID_SYNC_MAX_LATENCY, &entry);
> > > > + if (!found || *entry.data.i32 < 0 || 4 < *entry.data.i32) {
> > > > + LOG(HAL, Info)
> > > > + << "Max sync latency is "
> > > > + << (found ? std::to_string(*entry.data.i32) : "not present");
> > > > + return false;
> > > > + }
> > > > +
> > > > + return true;
> > > > +}
> > > > +
> > > > +std::set<camera_metadata_enum_android_request_available_capabilities>
> > > > +computeCapabilities(CameraMetadata *staticMetadata,
> > > > + std::set<camera_metadata_enum_android_request_available_capabilities> &existingCaps)
> > > > +{
> > > > + std::set<camera_metadata_enum_android_request_available_capabilities>
> > > > + capabilities = existingCaps;
> > > > +
> > > > + capabilities.insert(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_BACKWARD_COMPATIBLE);
> > > > +
> > > > + if (validateManualSensorCapability(staticMetadata))
> > > > + capabilities.insert(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_MANUAL_SENSOR);
> > > > +
> > > > + if (validateManualPostProcessingCapability(staticMetadata))
> > > > + capabilities.insert(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_MANUAL_POST_PROCESSING);
> > > > +
> > > > + if (validateBurstCaptureCapability(staticMetadata))
> > > > + capabilities.insert(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_BURST_CAPTURE);
> > > > +
> > > > + return capabilities;
> > > > +}
> > > > +
> > > > +camera_metadata_enum_android_info_supported_hardware_level
> >
> > It's impressive how long those names can be.
> >
> > It could make sense to store the hardware level in the
> > CameraCapabilities class, as I expect we'll need to tune the behaviour
> > of the HAL based on it.
> >
> > > > +computeHwLevel(CameraMetadata *staticMetadata,
> > > > + std::set<camera_metadata_enum_android_request_available_capabilities> capabilities)
> > > > +{
> > > > + camera_metadata_ro_entry_t entry;
> > > > + bool found;
> > > > + camera_metadata_enum_android_info_supported_hardware_level
> > > > + hwLevel = ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_FULL;
> > > > +
> > > > + if (!capabilities.count(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_MANUAL_SENSOR))
> > > > + hwLevel = ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_LIMITED;
> > > > +
> > > > + if (!capabilities.count(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_MANUAL_POST_PROCESSING))
> > > > + hwLevel = ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_LIMITED;
> > > > +
> > > > + if (!capabilities.count(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_BURST_CAPTURE))
> > > > + hwLevel = ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_LIMITED;
> > > > +
> > > > + found = staticMetadata->getEntry(ANDROID_SYNC_MAX_LATENCY, &entry);
> > > > + if (!found || *entry.data.i32 != 0)
> > > > + hwLevel = ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_LIMITED;
> > > > +
> > > > + return hwLevel;
> > > > +}
> > > > +
> > > > } /* namespace */
> > > >
> > > > int CameraCapabilities::initialize(std::shared_ptr<libcamera::Camera> camera,
> > > > @@ -655,17 +798,7 @@ int CameraCapabilities::initializeStaticMetadata()
> > > > };
> > > >
> > > > std::set<camera_metadata_enum_android_request_available_capabilities>
> > > > - capabilities = {
> > > > - ANDROID_REQUEST_AVAILABLE_CAPABILITIES_BACKWARD_COMPATIBLE,
> > > > - ANDROID_REQUEST_AVAILABLE_CAPABILITIES_MANUAL_SENSOR,
> > > > - ANDROID_REQUEST_AVAILABLE_CAPABILITIES_MANUAL_POST_PROCESSING,
> > > > - ANDROID_REQUEST_AVAILABLE_CAPABILITIES_BURST_CAPTURE,
> > > > - };
> > > > -
> > > > - std::set<camera_metadata_enum_android_info_supported_hardware_level>
> > > > - hwLevels = {
> > > > - ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_FULL,
> > > > - };
> > > > + capabilities = {};
> > > >
> > > > /* Color correction static metadata. */
> > > > {
> > > > @@ -692,19 +825,12 @@ int CameraCapabilities::initializeStaticMetadata()
> > > > staticMetadata_->addEntry(ANDROID_CONTROL_AE_AVAILABLE_ANTIBANDING_MODES,
> > > > aeAvailableAntiBandingModes);
> > > >
> > > > - std::vector<uint8_t> aeModes = setMetadata<uint8_t, bool>(
> > > > + setMetadata<uint8_t, bool>(
> > > > staticMetadata_.get(),
> > > > ANDROID_CONTROL_AE_AVAILABLE_MODES,
> > > > controlsInfo, &controls::AeEnable,
> > > > { ANDROID_CONTROL_AE_MODE_ON });
> > > >
> > > > - if (std::find(aeModes.begin(), aeModes.end(),
> > > > - ANDROID_CONTROL_AE_MODE_OFF) == aeModes.end()) {
> > > > - LOG(HAL, Info) << "AE cannot be turned off";
> > > > - hwLevels.erase(ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_FULL);
> > > > - capabilities.erase(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_MANUAL_SENSOR);
> > > > - }
> > > > -
> > > > int64_t minFrameDurationNsec = -1;
> > > > int64_t maxFrameDurationNsec = -1;
> > > > const auto frameDurationsInfo = controlsInfo.find(&controls::FrameDurationLimits);
> > > > @@ -791,17 +917,11 @@ int CameraCapabilities::initializeStaticMetadata()
> > > > staticMetadata_->addEntry(ANDROID_CONTROL_AVAILABLE_VIDEO_STABILIZATION_MODES,
> > > > availableStabilizationModes);
> > > >
> > > > - std::vector<uint8_t> awbModes = setMetadata<uint8_t, int32_t>(
> > > > + setMetadata<uint8_t, int32_t>(
> > > > staticMetadata_.get(),
> > > > ANDROID_CONTROL_AWB_AVAILABLE_MODES,
> > > > controlsInfo, &controls::AwbMode,
> > > > { ANDROID_CONTROL_AWB_MODE_AUTO });
> > > > - if (std::find(awbModes.begin(), awbModes.end(),
> > > > - ANDROID_CONTROL_AWB_MODE_OFF) == awbModes.end()) {
> > > > - LOG(HAL, Info) << "AWB cannot be turned off";
> > > > - hwLevels.erase(ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_FULL);
> > > > - capabilities.erase(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_MANUAL_POST_PROCESSING);
> > > > - }
> > > >
> > > > std::vector<int32_t> availableMaxRegions = {
> > > > 0, 0, 0,
> > > > @@ -817,29 +937,19 @@ int CameraCapabilities::initializeStaticMetadata()
> > > > staticMetadata_->addEntry(ANDROID_CONTROL_SCENE_MODE_OVERRIDES,
> > > > sceneModesOverride);
> > > >
> > > > - uint8_t aeLockAvailable = setMetadata<uint8_t, bool>(
> > > > + setMetadata<uint8_t, bool>(
> > > > staticMetadata_.get(),
> > > > ANDROID_CONTROL_AE_LOCK_AVAILABLE,
> > > > controlsInfo, &controls::AeLock,
> > > > ControlRange::Max,
> > > > ANDROID_CONTROL_AE_LOCK_AVAILABLE_FALSE);
> > > > - if (aeLockAvailable != ANDROID_CONTROL_AE_LOCK_AVAILABLE_TRUE) {
> > > > - LOG(HAL, Info) << "AE lock is unavailable";
> > > > - capabilities.erase(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_BURST_CAPTURE);
> > > > - capabilities.erase(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_MANUAL_SENSOR);
> > > > - }
> > > >
> > > > - uint8_t awbLockAvailable = setMetadata<uint8_t, bool>(
> > > > + setMetadata<uint8_t, bool>(
> > > > staticMetadata_.get(),
> > > > ANDROID_CONTROL_AWB_LOCK_AVAILABLE,
> > > > controlsInfo, &controls::AwbLock,
> > > > ControlRange::Max,
> > > > ANDROID_CONTROL_AWB_LOCK_AVAILABLE_FALSE);
> > > > - if (awbLockAvailable != ANDROID_CONTROL_AWB_LOCK_AVAILABLE_TRUE) {
> > > > - LOG(HAL, Info) << "AWB lock is unavailable";
> > > > - capabilities.erase(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_BURST_CAPTURE);
> > > > - capabilities.erase(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_MANUAL_POST_PROCESSING);
> > > > - }
> > > >
> > > > char availableControlModes = ANDROID_CONTROL_MODE_AUTO;
> > > > staticMetadata_->addEntry(ANDROID_CONTROL_AVAILABLE_MODES,
> > > > @@ -859,10 +969,6 @@ int CameraCapabilities::initializeStaticMetadata()
> > > > availableCharacteristicsKeys_.insert(ANDROID_EDGE_AVAILABLE_EDGE_MODES);
> > > > availableRequestKeys_.insert(ANDROID_EDGE_MODE);
> > > > availableResultKeys_.insert(ANDROID_EDGE_MODE);
> > > > - } else {
> > > > - LOG(HAL, Info) << "Edge mode unavailable";
> > > > - hwLevels.erase(ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_FULL);
> > > > - capabilities.erase(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_MANUAL_SENSOR);
> > > > }
> > > >
> > > > /* JPEG static metadata. */
> > > > @@ -1046,17 +1152,11 @@ int CameraCapabilities::initializeStaticMetadata()
> > > > }
> > > >
> > > > /* Sync static metadata. */
> > > > - int32_t maxLatency = setMetadata<int32_t, int32_t>(
> > > > + setMetadata<int32_t, int32_t>(
> > > > staticMetadata_.get(), ANDROID_SYNC_MAX_LATENCY,
> > > > controlsInfo, &controls::draft::MaxLatency,
> > > > ControlRange::Def,
> > > > ANDROID_SYNC_MAX_LATENCY_UNKNOWN);
> > > > - LOG(HAL, Info) << "Max sync latency is " << maxLatency;
> > > > - /* CTS allows a sync latency of up to 4 for burst capture capability */
> > > > - if (maxLatency < 0 || 4 < maxLatency)
> > > > - capabilities.erase(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_BURST_CAPTURE);
> > > > - if (maxLatency != 0)
> > > > - hwLevels.erase(ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_FULL);
> > > >
> > > > /* Flash static metadata. */
> > > > char flashAvailable = ANDROID_FLASH_INFO_AVAILABLE_FALSE;
> > > > @@ -1220,19 +1320,14 @@ int CameraCapabilities::initializeStaticMetadata()
> > > > numOutStreams);
> > > >
> > > > /* Check capabilities */
> > > > - std::vector<uint8_t> availableCapabilities(capabilities.begin(),
> > > > - capabilities.end());
> > > > - staticMetadata_->addEntry(ANDROID_REQUEST_AVAILABLE_CAPABILITIES,
> > > > - availableCapabilities);
> > > > -
> > > > - uint8_t hwLevel = ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_LIMITED;
> > > > - if (capabilities.count(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_MANUAL_SENSOR) &&
> > > > - capabilities.count(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_MANUAL_POST_PROCESSING) &&
> > > > - capabilities.count(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_BURST_CAPTURE) &&
> > > > - hwLevels.count(ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_FULL))
> > > > - hwLevel = ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_FULL;
> > > > - staticMetadata_->addEntry(ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL,
> > > > - hwLevel);
> > > > + capabilities = computeCapabilities(staticMetadata_.get(), capabilities);
>
> So far I share all comments Laurent made, there's space for
> simplifying the implementation, but the direction is good!
>
> > >
> > > Just to clarify, this is here because above this we have:
> > >
> > > /* Report if camera supports RAW. */
> > > bool rawStreamAvailable = false;
> > > std::unique_ptr<CameraConfiguration> cameraConfig =
> > > camera_->generateConfiguration({ StreamRole::Raw });
> > > if (cameraConfig && !cameraConfig->empty()) {
> > > const PixelFormatInfo &info =
> > > PixelFormatInfo::info(cameraConfig->at(0).pixelFormat);
> > > /* Only advertise RAW support if RAW16 is possible. */
> > > if (info.colourEncoding == PixelFormatInfo::ColourEncodingRAW &&
> > > info.bitsPerPixel == 16) {
> > > rawStreamAvailable = true;
> > > capabilities.insert(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_RAW);
> > > }
> > > }
> > >
> > > There's no way to capture this information in the static metadata
> > > otherwise, and I don't think it's a good idea to feed arbitrary
> > > variables into the capabilities gatherer, since at that point we might
> > > as well keep the method that I had earlier (which has the benefit of not
> > > having to walk the static metadata again after setting it, but hey, a
> > > multiple of linear time is still linear time :D).
> > >
> > > So yeah, we allow static metadata setters to directly set the
> > > capabilities if it alone is sufficient and necessary to determine some
> > > capability.
> >
> > This looks fine to me. I'd have a slight preference for calling
> > computeCapabilities() first, and then extending it with
> > ANDROID_REQUEST_AVAILABLE_CAPABILITIES_RAW (and possibly other
> > capabilities), but if that causes issue, I'm OK with the current order.
> >
>
> I have a patch in my backlog that moves RAW capabilities detection to
> initializetreamConfigurations() as the check for RAW support was
> duplicated. I recorded the RAW support information in a class
> variable, and initializeStaticMetadata() inspects that to populate the
> capabilities.
>
> Patch has not been sent as it was part of the frameduration work, but
> I can send it out with a few related cleanups. In the meantime
> https://paste.debian.net/1204421/
>
> Do you think it would be ok for the capabilities checker to inspect a
> class variable ?
Oh yeah I think that would be better.
Thanks,
Paul
>
> > > > + std::vector<camera_metadata_enum_android_request_available_capabilities>
> > > > + capsVec = std::vector<camera_metadata_enum_android_request_available_capabilities>(capabilities.begin(), capabilities.end());
> > > > + staticMetadata_->addEntry(ANDROID_REQUEST_AVAILABLE_CAPABILITIES, capsVec);
> > > > +
> > > > + camera_metadata_enum_android_info_supported_hardware_level hwLevel =
> > > > + computeHwLevel(staticMetadata_.get(), capabilities);
> > > > + staticMetadata_->addEntry(ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL, hwLevel);
> > > >
> > > > LOG(HAL, Info)
> > > > << "Hardware level: "
> >
> > --
> > Regards,
> >
> > Laurent Pinchart
More information about the libcamera-devel
mailing list