[libcamera-devel] [PATCH v6 5/9] android: Add infrastructure for determining capabilities and hardware level
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Sun Jul 25 02:55:55 CEST 2021
Hi Paul,
Thank you for the patch.
On Fri, Jul 23, 2021 at 07:15:32PM +0900, paul.elder at ideasonboard.com wrote:
> On Thu, Jul 22, 2021 at 04:02:59PM +0200, Jacopo Mondi wrote:
> > On Tue, Jul 20, 2021 at 07:13:03PM +0900, Paul Elder wrote:
> > > Add the infrastructure for checking and reporting capabilities. Use
> > > these capabilities to determine the hardware level as well.
> > >
> > > Bug: https://bugs.libcamera.org/show_bug.cgi?id=55
> > > Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
> > >
> > > ---
> > > I'm thinking that we should hardcode the validate* functions to return
> > > false for now until we add all the required capabilities. They cannot
> > > return true anyway until that happens.
> >
> > As you said, they will return false anyhow, what would we gain in
> > hardocding it ?
>
> Not all of the checks are implemented yet, so if the ones that are here
> pass then it'll return true.
I don't think it's necessary, but I don't mind. I'd document what checks
are missing with a comment just before the return at the end of the
function though.
> > > Changes in v6:
> > > - make all args const
> > > - convert the caps list from set to vector
> > >
> > > Changes in v5:
> > > - change the whole thing to turn on capabilities after they are all
> > > confirmed, instead of disabling them as conditions are not met
> > >
> > > Changes in v4:
> > > - rebase on camera capabilities refactoring
> > > - switch to std::set from std::map
> > > - make hwlevel similar to capabilities
> > >
> > > Changes in v3:
> > > - fix some compiler errors
> > > - go ahead and initialize the capabilities to true, update the commit
> > > message accordingly
> > >
> > > Changes in v2:
> > > - add a flag for FULL, since there are a few requirements that are not
> > > obtained from capabilities alone
> > > - add burst capture capability, since that is required for FULL as well
> > > ---
> > > src/android/camera_capabilities.cpp | 174 +++++++++++++++++++++++++---
> > > src/android/camera_capabilities.h | 12 ++
> > > 2 files changed, 171 insertions(+), 15 deletions(-)
> > >
> > > diff --git a/src/android/camera_capabilities.cpp b/src/android/camera_capabilities.cpp
> > > index 9e2714f1..c97a17e6 100644
> > > --- a/src/android/camera_capabilities.cpp
> > > +++ b/src/android/camera_capabilities.cpp
> > > @@ -7,8 +7,10 @@
> > >
> > > #include "camera_capabilities.h"
> > >
> > > +#include <algorithm>
> > > #include <array>
> > > #include <cmath>
> > > +#include <map>
> > >
> > > #include <hardware/camera3.h>
> > >
> > > @@ -114,8 +116,153 @@ const std::map<int, const Camera3Format> camera3FormatsMap = {
> > > },
> > > };
> > >
> > > +const std::map<camera_metadata_enum_android_info_supported_hardware_level, std::string>
> > > +hwLevelStrings = {
> > > + { ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_LIMITED, "LIMITED" },
> > > + { ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_FULL, "FULL" },
> > > + { ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_LEGACY, "LEGACY" },
> > > + { ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_3, "LEVEL_3" },
> > > + { ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_EXTERNAL, "EXTERNAL" },
> > > +};
> > > +
> > > } /* namespace */
> > >
> > > +bool CameraCapabilities::validateManualSensorCapability(const CameraMetadata *staticMetadata)
> > > +{
> > > + camera_metadata_ro_entry_t entry;
> > > + bool found;
> > > +
> > > + const char *noMode = "Manual sensor capability unavailable: ";
You can repeat this below, and the compiler should de-duplicate strings
automatically. The code would be a bit more readable I think.
> > > +
> > > + if (!(found = staticMetadata->entryContains<uint8_t>(ANDROID_CONTROL_AE_AVAILABLE_MODES,
> > > + ANDROID_CONTROL_AE_MODE_OFF))) {
> > > + LOG(HAL, Info)
> > > + << noMode << "missing AE mode off: "
> > > + << (found ? "not supported" : "not found");
> >
> > Probably a leftover, if found==true you won't get here, so you can
> > remove the ternary operator.
>
> Oh yeah you're right.
>
> > > + return false;
> > > + }
> > > +
> > > + found = staticMetadata->getEntry(ANDROID_CONTROL_AE_LOCK_AVAILABLE,
> > > + &entry);
> > > + if (!found || !(*entry.data.u8 == ANDROID_CONTROL_AE_LOCK_AVAILABLE_TRUE)) {
> >
> > I'm sorry if I repeat the same question as in v5, but can't this just
> > be
> > staticMetadata->entryContains(ANDROID_CONTROL_AE_LOCK_AVAILABLE,
> > ANDROID_CONTROL_AE_LOCK_AVAILABLE_TRUE);
> >
> > I re-read your answer, but I don't get why this won't work. It would
> > translate in:
> >
> > template<typename T> bool entryContains(uint32_t tag, T value) const
> > {
> > camera_metadata_ro_entry_t entry;
> > if (!getEntry(tag, &entry))
> > return false;
> >
> > return entryContainsOne<T>(entry, value);
> > }
> >
> > template<>
> > bool CameraMetadata::entryContainsOne<uint8_t>(const camera_metadata_ro_entry_t &entry,
> > uint8_t value) const
> > {
> > for (unsigned int i = 0; i < entry.count; i++) {
> > if (entry.data.u8[i] == value)
> > return true;
> > }
> >
> > return false;
> > }
> >
> >
> > The first part does what you do with 'found'
> > The specialization checks for entry.data.u8[0] to be
> > LOCK_AVAILABLE_TRUE.
> >
> > Why is it different than open-coding it ?
>
> Oh, I see what you mean. I guess it should work then.
>
> > > + LOG(HAL, Info) << noMode << "missing AE lock";
> >
> > Here the ternary operation might make sense
> >
> > > + return false;
> > > + }
> > > +
> > > + return true;
> > > +}
> > > +
> > > +bool CameraCapabilities::validateManualPostProcessingCapability(const CameraMetadata *staticMetadata)
> > > +{
> > > + camera_metadata_ro_entry_t entry;
> > > + bool found;
> > > +
> > > + const char *noMode = "Manual post processing capability unavailable: ";
> > > +
> > > + if (!staticMetadata->entryContains<uint8_t>(ANDROID_CONTROL_AWB_AVAILABLE_MODES,
> > > + ANDROID_CONTROL_AWB_MODE_OFF)) {
> > > + LOG(HAL, Info) << noMode << "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) << noMode << "missing AWB lock";
> > > + return false;
> > > + }
> > > +
> > > + return true;
> > > +}
> > > +
> > > +bool CameraCapabilities::validateBurstCaptureCapability(const CameraMetadata *staticMetadata)
> > > +{
> > > + camera_metadata_ro_entry_t entry;
> > > + bool found;
> > > +
> > > + const char *noMode = "Burst capture capability unavailable: ";
> > > +
> > > + found = staticMetadata->getEntry(ANDROID_CONTROL_AE_LOCK_AVAILABLE,
> > > + &entry);
> > > + if (!found || !(*entry.data.u8 == ANDROID_CONTROL_AE_LOCK_AVAILABLE_TRUE)) {
> > > + LOG(HAL, Info) << noMode << "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) << noMode << "missing AWB lock";
> > > + return false;
> > > + }
> > > +
> > > + found = staticMetadata->getEntry(ANDROID_SYNC_MAX_LATENCY, &entry);
> > > + if (!found || *entry.data.i32 < 0 || 4 < *entry.data.i32) {
Anything wrong with *entry.data.i32 > 4 ?
> > > + LOG(HAL, Info)
> > > + << noMode << "max sync latency is "
> > > + << (found ? std::to_string(*entry.data.i32) : "not present");
> > > + return false;
> > > + }
> > > +
> > > + return true;
> > > +}
> > > +
> > > +std::vector<camera_metadata_enum_android_request_available_capabilities>
> > > +CameraCapabilities::computeCapabilities(const CameraMetadata *staticMetadata)
I wonder, would it make sense for this function to return a set ? It
would make the find operations easier, and nicer to write:
if (std::find(caps.begin(), caps.end(),
ANDROID_REQUEST_AVAILABLE_CAPABILITIES_MANUAL_SENSOR) == caps.end()) {
hwLevel = ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_LIMITED;
}
could become
if (!caps.count(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_MANUAL_SENSOR))
hwLevel = ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_LIMITED;
(I really wish the contains() function was part of C++17, not C++20
:-().
It would create a problem when calling CameraMetadata::addEntry()
though, but that's a single call, so we could convert to a vector there.
Performance-wise none of this is critical, I'd favour readability if I
had to pick a single criteria.
> > > +{
> > > + std::vector<camera_metadata_enum_android_request_available_capabilities>
> > > + capabilities;
> > > +
> > > + capabilities.push_back(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_BACKWARD_COMPATIBLE);
> > > +
> > > + if (validateManualSensorCapability(staticMetadata))
> > > + capabilities.push_back(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_MANUAL_SENSOR);
> > > +
> > > + if (validateManualPostProcessingCapability(staticMetadata))
> > > + capabilities.push_back(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_MANUAL_POST_PROCESSING);
> > > +
> > > + if (validateBurstCaptureCapability(staticMetadata))
> > > + capabilities.push_back(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_BURST_CAPTURE);
> > > +
> > > + if (rawStreamAvailable_)
> > > + capabilities.push_back(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_RAW);
> > > +
> > > + return capabilities;
> > > +}
> > > +
> > > +camera_metadata_enum_android_info_supported_hardware_level
> > > +CameraCapabilities::computeHwLevel(const CameraMetadata *staticMetadata,
> > > + const std::vector<camera_metadata_enum_android_request_available_capabilities> &caps)
> > > +{
> > > + camera_metadata_ro_entry_t entry;
> > > + bool found;
> > > + camera_metadata_enum_android_info_supported_hardware_level
> > > + hwLevel = ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_FULL;
> > > +
> > > + if (std::find(caps.begin(), caps.end(),
> > > + ANDROID_REQUEST_AVAILABLE_CAPABILITIES_MANUAL_SENSOR) == caps.end()) {
> > > + hwLevel = ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_LIMITED;
> > > + }
> > > +
> > > + if (std::find(caps.begin(), caps.end(),
> > > + ANDROID_REQUEST_AVAILABLE_CAPABILITIES_MANUAL_POST_PROCESSING) == caps.end()) {
> > > + hwLevel = ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_LIMITED;
> > > + }
> > > +
> > > + if (std::find(caps.begin(), caps.end(),
> > > + ANDROID_REQUEST_AVAILABLE_CAPABILITIES_BURST_CAPTURE) == caps.end()) {
> > > + 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;
> > > +
> > > + hwLevel_ = hwLevel;
> > > +
> > > + return hwLevel;
I'd make this a void function, and use hwLevel_ in
CameraCapabilities::initializeStaticMetadata().
> > > +}
> > > +
> > > int CameraCapabilities::initialize(std::shared_ptr<libcamera::Camera> camera,
> > > int orientation, int facing)
> > > {
> > > @@ -840,11 +987,6 @@ int CameraCapabilities::initializeStaticMetadata()
> > > uint8_t croppingType = ANDROID_SCALER_CROPPING_TYPE_CENTER_ONLY;
> > > staticMetadata_->addEntry(ANDROID_SCALER_CROPPING_TYPE, croppingType);
> > >
> > > - /* Info static metadata. */
> > > - uint8_t supportedHWLevel = ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_LIMITED;
> > > - staticMetadata_->addEntry(ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL,
> > > - supportedHWLevel);
> > > -
> > > /* Request static metadata. */
> > > int32_t partialResultCount = 1;
> > > staticMetadata_->addEntry(ANDROID_REQUEST_PARTIAL_RESULT_COUNT,
> > > @@ -865,21 +1007,23 @@ int CameraCapabilities::initializeStaticMetadata()
> > > staticMetadata_->addEntry(ANDROID_REQUEST_MAX_NUM_INPUT_STREAMS,
> > > maxNumInputStreams);
> > >
> > > - std::vector<uint8_t> availableCapabilities = {
> > > - ANDROID_REQUEST_AVAILABLE_CAPABILITIES_BACKWARD_COMPATIBLE,
> > > - };
> > > -
> > > - /* Report if camera supports RAW. */
> > > - if (rawStreamAvailable_)
> > > - availableCapabilities.push_back(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_RAW);
> > > -
> > > /* Number of { RAW, YUV, JPEG } supported output streams */
> > > int32_t numOutStreams[] = { rawStreamAvailable_, 2, 1 };
> > > staticMetadata_->addEntry(ANDROID_REQUEST_MAX_NUM_OUTPUT_STREAMS,
> > > numOutStreams);
> > >
> > > - staticMetadata_->addEntry(ANDROID_REQUEST_AVAILABLE_CAPABILITIES,
> > > - availableCapabilities);
> > > + /* Check capabilities */
> > > + std::vector<camera_metadata_enum_android_request_available_capabilities>
> > > + capabilities = computeCapabilities(staticMetadata_.get());
> > > + staticMetadata_->addEntry(ANDROID_REQUEST_AVAILABLE_CAPABILITIES, capabilities);
> > > +
> > > + 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: "
> > > + << hwLevelStrings.find((camera_metadata_enum_android_info_supported_hardware_level)hwLevel)->second;
Can't you skip the cast, as hwLevel is already a
camera_metadata_enum_android_info_supported_hardware_level ? (What a
horrible type name...)
> > >
> > > std::vector<int32_t> availableCharacteristicsKeys = {
> > > ANDROID_COLOR_CORRECTION_AVAILABLE_ABERRATION_MODES,
> > > diff --git a/src/android/camera_capabilities.h b/src/android/camera_capabilities.h
> > > index 42a976d3..6ef81714 100644
> > > --- a/src/android/camera_capabilities.h
> > > +++ b/src/android/camera_capabilities.h
> > > @@ -42,6 +42,17 @@ private:
> > > int androidFormat;
> > > };
> > >
> > > + bool validateManualSensorCapability(const CameraMetadata *staticMetadata);
> > > + bool validateManualPostProcessingCapability(const CameraMetadata *staticMetadata);
> > > + bool validateBurstCaptureCapability(const CameraMetadata *staticMetadata);
> > > +
> > > + std::vector<camera_metadata_enum_android_request_available_capabilities>
> > > + computeCapabilities(const CameraMetadata *staticMetadata);
> > > +
> > > + camera_metadata_enum_android_info_supported_hardware_level
> > > + computeHwLevel(const CameraMetadata *staticMetadata,
> > > + const std::vector<camera_metadata_enum_android_request_available_capabilities> &caps);
As all these are non-static member functions, do you need to pass the
staticMetadata argument ? It seems you could use staticMetadata_
internally.
> > > +
> > > std::vector<libcamera::Size>
> > > initializeYUVResolutions(const libcamera::PixelFormat &pixelFormat,
> > > const std::vector<libcamera::Size> &resolutions);
> > > @@ -56,6 +67,7 @@ private:
> > > int facing_;
> > > int orientation_;
> > > bool rawStreamAvailable_;
> > > + camera_metadata_enum_android_info_supported_hardware_level hwLevel_;
> > >
> > > std::vector<Camera3StreamConfiguration> streamConfigurations_;
> > > std::map<int, libcamera::PixelFormat> formatsMap_;
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list