[libcamera-devel] [PATCH v6 5/9] android: Add infrastructure for determining capabilities and hardware level
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Sat Jul 31 04:18:08 CEST 2021
Hi Paul,
On Fri, Jul 30, 2021 at 06:35:18PM +0900, paul.elder at ideasonboard.com wrote:
> On Sun, Jul 25, 2021 at 03:55:55AM +0300, Laurent Pinchart wrote:
> > 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.
>
> I think this way is more readable. Otherwise we have the exact same
> string copy and pasted multiple times in here, making lines longer, and
> if we change it then it's more changes.
>
> > > > > +
> > > > > + 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 ?
>
> I think it's better to line up the conditions in order of small ->
> large, since it's the same order that we read things. This way it's
> clear that the condition is )0,4(.
Do you say, in English, "if the value is smaller than zero or larger
than four", or "if the value is smaller than zero or four is smaller
than the value" ? :-) "4 < *entry.data.i32" requires me to pause and
really focus on the code to read it.
> > > > > + 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
>
> That's what I had first but Jacopo said "would it make sense for this
> function to return a vector?" :D
>
> > 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;
>
> This is why I thought set would be better.
>
> > (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.
>
> And this is why Jacopo said that vector would be better.
Pros and cons. I find the ability to write caps.count() worth the need
to convert to a vector, but that's just me. You can pick one :-)
> > 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().
>
> Ah okay.
>
> > > > > +}
> > > > > +
> > > > > 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...)
>
> Oh maybe this was leftover from when I had int or something.
>
> > > > >
> > > > > 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.
>
> I guess I could.
>
> > > > > +
> > > > > 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