| Message ID | 20210827120757.110615-1-jacopo@jmondi.org |
|---|---|
| Headers | show |
| Series |
|
| Related | show |
Hi Jacopo, On Fri, Aug 27, 2021 at 02:07:46PM +0200, Jacopo Mondi wrote: > Now that building the list of supported stream configuration requires > applying a configuration to the Camera, re-initialize the camera > controls by applying a configuration generated for the Viewfinder stream > role before building the list of static metadata. > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> Looks good, but I have a question. > --- > src/android/camera_capabilities.cpp | 18 +++++++++++++++--- > 1 file changed, 15 insertions(+), 3 deletions(-) > > diff --git a/src/android/camera_capabilities.cpp b/src/android/camera_capabilities.cpp > index fdda90379ce2..723a4fd5a880 100644 > --- a/src/android/camera_capabilities.cpp > +++ b/src/android/camera_capabilities.cpp > @@ -394,11 +394,14 @@ int CameraCapabilities::initialize(std::shared_ptr<libcamera::Camera> camera, > } > > ret = initializeStreamConfigurations(); > - camera_->release(); > - if (ret) > + if (ret) { > + camera_->release(); > return ret; > + } > > - return initializeStaticMetadata(); > + ret = initializeStaticMetadata(); > + camera_->release(); > + return ret; > } > > std::vector<Size> > @@ -682,6 +685,15 @@ int CameraCapabilities::initializeStaticMetadata() > return -EINVAL; > } > > + std::unique_ptr<CameraConfiguration> cameraConfig = > + camera_->generateConfiguration({ StreamRole::Viewfinder }); From what I see, initializeStreamConfigurations() generates a configuration for the StillCapture role. Is it fine that this uses a different role? (Or was the other one changed to Viewfinder where I wasn't paying attention?) Paul > + int ret = camera_->configure(cameraConfig.get()); > + if (ret) { > + LOG(HAL, Error) << "Failed to initialize the camera state"; > + staticMetadata_.reset(); > + return ret; > + } > + > const ControlInfoMap &controlsInfo = camera_->controls(); > const ControlList &properties = camera_->properties(); > > -- > 2.32.0 >
Hi Jacopo, On Fri, Aug 27, 2021 at 02:07:47PM +0200, Jacopo Mondi wrote: > As we now collect the per-stream frame durations at > initializeStreamConfigurations() times, the Camera is now guaranteed to > support the controls::FrameDurationLimits control. > > Remove the check for its presence when populating the > ANDROID_CONTROL_AE_AVAILABLE_TARGET_FPS_RANGES static metadata. > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > --- > src/android/camera_capabilities.cpp | 83 ++++++++++++++--------------- > 1 file changed, 41 insertions(+), 42 deletions(-) > > diff --git a/src/android/camera_capabilities.cpp b/src/android/camera_capabilities.cpp > index 723a4fd5a880..c45d74189aaa 100644 > --- a/src/android/camera_capabilities.cpp > +++ b/src/android/camera_capabilities.cpp > @@ -858,55 +858,54 @@ int CameraCapabilities::initializeStaticMetadata() > staticMetadata_->addEntry(ANDROID_CONTROL_AE_AVAILABLE_MODES, > aeAvailableModes); > > + /* Initialize the AE frame duration limits. */ > int64_t minFrameDurationNsec = -1; > int64_t maxFrameDurationNsec = -1; > const auto frameDurationsInfo = controlsInfo.find(&controls::FrameDurationLimits); > - if (frameDurationsInfo != controlsInfo.end()) { > - minFrameDurationNsec = frameDurationsInfo->second.min().get<int64_t>() * 1000; > - maxFrameDurationNsec = frameDurationsInfo->second.max().get<int64_t>() * 1000; > + minFrameDurationNsec = frameDurationsInfo->second.min().get<int64_t>() * 1000; > + maxFrameDurationNsec = frameDurationsInfo->second.max().get<int64_t>() * 1000; Why not just initialize these directly? Otherwise, looks good. Reviewed-by: Paul Elder <paul.elder@ideasonboard.com> > > - /* > - * Adjust the minimum frame duration to comply with Android > - * requirements. The camera service mandates all preview/record > - * streams to have a minimum frame duration < 33,366 milliseconds > - * (see MAX_PREVIEW_RECORD_DURATION_NS in the camera service > - * implementation). > - * > - * If we're close enough (+ 500 useconds) to that value, round > - * the minimum frame duration of the camera to an accepted > - * value. > - */ > - static constexpr int64_t MAX_PREVIEW_RECORD_DURATION_NS = 1e9 / 29.97; > - if (minFrameDurationNsec > MAX_PREVIEW_RECORD_DURATION_NS && > - minFrameDurationNsec < MAX_PREVIEW_RECORD_DURATION_NS + 500000) > - minFrameDurationNsec = MAX_PREVIEW_RECORD_DURATION_NS - 1000; > + /* > + * Adjust the minimum frame duration to comply with Android > + * requirements. The camera service mandates all preview/record > + * streams to have a minimum frame duration < 33,366 milliseconds > + * (see MAX_PREVIEW_RECORD_DURATION_NS in the camera service > + * implementation). > + * > + * If we're close enough (+ 500 useconds) to that value, round > + * the minimum frame duration of the camera to an accepted > + * value. > + */ > + static constexpr int64_t MAX_PREVIEW_RECORD_DURATION_NS = 1e9 / 29.97; > + if (minFrameDurationNsec > MAX_PREVIEW_RECORD_DURATION_NS && > + minFrameDurationNsec < MAX_PREVIEW_RECORD_DURATION_NS + 500000) > + minFrameDurationNsec = MAX_PREVIEW_RECORD_DURATION_NS - 1000; > > - /* > - * The AE routine frame rate limits are computed using the frame > - * duration limits, as libcamera clips the AE routine to the > - * frame durations. > - */ > - int32_t maxFps = std::round(1e9 / minFrameDurationNsec); > - int32_t minFps = std::round(1e9 / maxFrameDurationNsec); > - minFps = std::max(1, minFps); > + /* > + * The AE routine frame rate limits are computed using the frame > + * duration limits, as libcamera clips the AE routine to the > + * frame durations. > + */ > + int32_t maxFps = std::round(1e9 / minFrameDurationNsec); > + int32_t minFps = std::round(1e9 / maxFrameDurationNsec); > + minFps = std::max(1, minFps); > > - /* > - * Force rounding errors so that we have the proper frame > - * durations for when we reuse these variables later > - */ > - minFrameDurationNsec = 1e9 / maxFps; > - maxFrameDurationNsec = 1e9 / minFps; > + /* > + * Force rounding errors so that we have the proper frame > + * durations for when we reuse these variables later > + */ > + minFrameDurationNsec = 1e9 / maxFps; > + maxFrameDurationNsec = 1e9 / minFps; > > - /* > - * Register to the camera service {min, max} and {max, max} > - * intervals as requested by the metadata documentation. > - */ > - int32_t availableAeFpsTarget[] = { > - minFps, maxFps, maxFps, maxFps > - }; > - staticMetadata_->addEntry(ANDROID_CONTROL_AE_AVAILABLE_TARGET_FPS_RANGES, > - availableAeFpsTarget); > - } > + /* > + * Register to the camera service {min, max} and {max, max} > + * intervals as requested by the metadata documentation. > + */ > + int32_t availableAeFpsTarget[] = { > + minFps, maxFps, maxFps, maxFps > + }; > + staticMetadata_->addEntry(ANDROID_CONTROL_AE_AVAILABLE_TARGET_FPS_RANGES, > + availableAeFpsTarget); > > std::vector<int32_t> aeCompensationRange = { > 0, 0, > -- > 2.32.0 >
Hi Jacopo, On Fri, Aug 27, 2021 at 02:07:50PM +0200, Jacopo Mondi wrote: > While building the list of supported stream configurations also collet s/collet/collect > the absolute max frame durations to be used to populate the sensor > maximum frame duration. > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> Looks good. Reviewed-by: Paul Elder <paul.elder@ideasonboard.com> > --- > src/android/camera_capabilities.cpp | 9 ++++++--- > src/android/camera_capabilities.h | 1 + > 2 files changed, 7 insertions(+), 3 deletions(-) > > diff --git a/src/android/camera_capabilities.cpp b/src/android/camera_capabilities.cpp > index 23a9a3fbec50..388552963c47 100644 > --- a/src/android/camera_capabilities.cpp > +++ b/src/android/camera_capabilities.cpp > @@ -385,6 +385,7 @@ int CameraCapabilities::initialize(std::shared_ptr<libcamera::Camera> camera, > orientation_ = orientation; > facing_ = facing; > rawStreamAvailable_ = false; > + maxFrameDuration_ = 0; > > /* Acquire the camera and initialize available stream configurations. */ > int ret = camera_->acquire(); > @@ -659,6 +660,9 @@ int CameraCapabilities::initializeStreamConfigurations() > }); > maxJpegSize = std::max(maxJpegSize, res); > } > + > + maxFrameDuration_ = std::max(maxFrameDuration_, > + maxFrameDuration); > } > > /* > @@ -1132,9 +1136,8 @@ int CameraCapabilities::initializeStaticMetadata() > staticMetadata_->addEntry(ANDROID_SENSOR_INFO_TIMESTAMP_SOURCE, > timestampSource); > > - if (maxFrameDurationNsec > 0) > - staticMetadata_->addEntry(ANDROID_SENSOR_INFO_MAX_FRAME_DURATION, > - maxFrameDurationNsec); > + staticMetadata_->addEntry(ANDROID_SENSOR_INFO_MAX_FRAME_DURATION, > + maxFrameDuration_); > > /* Statistics static metadata. */ > uint8_t faceDetectMode = ANDROID_STATISTICS_FACE_DETECT_MODE_OFF; > diff --git a/src/android/camera_capabilities.h b/src/android/camera_capabilities.h > index 6e55ddab445e..2cf97ae80095 100644 > --- a/src/android/camera_capabilities.h > +++ b/src/android/camera_capabilities.h > @@ -71,6 +71,7 @@ private: > int facing_; > int orientation_; > bool rawStreamAvailable_; > + int64_t maxFrameDuration_; > camera_metadata_enum_android_info_supported_hardware_level hwLevel_; > std::set<camera_metadata_enum_android_request_available_capabilities> capabilities_; > > -- > 2.32.0 >
Hi Paul, On Tue, Aug 31, 2021 at 11:21:02AM +0900, Paul Elder wrote: > Hi Jacopo, > > On Fri, Aug 27, 2021 at 02:07:47PM +0200, Jacopo Mondi wrote: > > As we now collect the per-stream frame durations at > > initializeStreamConfigurations() times, the Camera is now guaranteed to > > support the controls::FrameDurationLimits control. > > > > Remove the check for its presence when populating the > > ANDROID_CONTROL_AE_AVAILABLE_TARGET_FPS_RANGES static metadata. > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > --- > > src/android/camera_capabilities.cpp | 83 ++++++++++++++--------------- > > 1 file changed, 41 insertions(+), 42 deletions(-) > > > > diff --git a/src/android/camera_capabilities.cpp b/src/android/camera_capabilities.cpp > > index 723a4fd5a880..c45d74189aaa 100644 > > --- a/src/android/camera_capabilities.cpp > > +++ b/src/android/camera_capabilities.cpp > > @@ -858,55 +858,54 @@ int CameraCapabilities::initializeStaticMetadata() > > staticMetadata_->addEntry(ANDROID_CONTROL_AE_AVAILABLE_MODES, > > aeAvailableModes); > > > > + /* Initialize the AE frame duration limits. */ > > int64_t minFrameDurationNsec = -1; > > int64_t maxFrameDurationNsec = -1; > > const auto frameDurationsInfo = controlsInfo.find(&controls::FrameDurationLimits); > > - if (frameDurationsInfo != controlsInfo.end()) { > > - minFrameDurationNsec = frameDurationsInfo->second.min().get<int64_t>() * 1000; > > - maxFrameDurationNsec = frameDurationsInfo->second.max().get<int64_t>() * 1000; > > + minFrameDurationNsec = frameDurationsInfo->second.min().get<int64_t>() * 1000; > > + maxFrameDurationNsec = frameDurationsInfo->second.max().get<int64_t>() * 1000; > > Why not just initialize these directly? You're right, I'll do so! Thanks j > > > Otherwise, looks good. > > Reviewed-by: Paul Elder <paul.elder@ideasonboard.com> > > > > > - /* > > - * Adjust the minimum frame duration to comply with Android > > - * requirements. The camera service mandates all preview/record > > - * streams to have a minimum frame duration < 33,366 milliseconds > > - * (see MAX_PREVIEW_RECORD_DURATION_NS in the camera service > > - * implementation). > > - * > > - * If we're close enough (+ 500 useconds) to that value, round > > - * the minimum frame duration of the camera to an accepted > > - * value. > > - */ > > - static constexpr int64_t MAX_PREVIEW_RECORD_DURATION_NS = 1e9 / 29.97; > > - if (minFrameDurationNsec > MAX_PREVIEW_RECORD_DURATION_NS && > > - minFrameDurationNsec < MAX_PREVIEW_RECORD_DURATION_NS + 500000) > > - minFrameDurationNsec = MAX_PREVIEW_RECORD_DURATION_NS - 1000; > > + /* > > + * Adjust the minimum frame duration to comply with Android > > + * requirements. The camera service mandates all preview/record > > + * streams to have a minimum frame duration < 33,366 milliseconds > > + * (see MAX_PREVIEW_RECORD_DURATION_NS in the camera service > > + * implementation). > > + * > > + * If we're close enough (+ 500 useconds) to that value, round > > + * the minimum frame duration of the camera to an accepted > > + * value. > > + */ > > + static constexpr int64_t MAX_PREVIEW_RECORD_DURATION_NS = 1e9 / 29.97; > > + if (minFrameDurationNsec > MAX_PREVIEW_RECORD_DURATION_NS && > > + minFrameDurationNsec < MAX_PREVIEW_RECORD_DURATION_NS + 500000) > > + minFrameDurationNsec = MAX_PREVIEW_RECORD_DURATION_NS - 1000; > > > > - /* > > - * The AE routine frame rate limits are computed using the frame > > - * duration limits, as libcamera clips the AE routine to the > > - * frame durations. > > - */ > > - int32_t maxFps = std::round(1e9 / minFrameDurationNsec); > > - int32_t minFps = std::round(1e9 / maxFrameDurationNsec); > > - minFps = std::max(1, minFps); > > + /* > > + * The AE routine frame rate limits are computed using the frame > > + * duration limits, as libcamera clips the AE routine to the > > + * frame durations. > > + */ > > + int32_t maxFps = std::round(1e9 / minFrameDurationNsec); > > + int32_t minFps = std::round(1e9 / maxFrameDurationNsec); > > + minFps = std::max(1, minFps); > > > > - /* > > - * Force rounding errors so that we have the proper frame > > - * durations for when we reuse these variables later > > - */ > > - minFrameDurationNsec = 1e9 / maxFps; > > - maxFrameDurationNsec = 1e9 / minFps; > > + /* > > + * Force rounding errors so that we have the proper frame > > + * durations for when we reuse these variables later > > + */ > > + minFrameDurationNsec = 1e9 / maxFps; > > + maxFrameDurationNsec = 1e9 / minFps; > > > > - /* > > - * Register to the camera service {min, max} and {max, max} > > - * intervals as requested by the metadata documentation. > > - */ > > - int32_t availableAeFpsTarget[] = { > > - minFps, maxFps, maxFps, maxFps > > - }; > > - staticMetadata_->addEntry(ANDROID_CONTROL_AE_AVAILABLE_TARGET_FPS_RANGES, > > - availableAeFpsTarget); > > - } > > + /* > > + * Register to the camera service {min, max} and {max, max} > > + * intervals as requested by the metadata documentation. > > + */ > > + int32_t availableAeFpsTarget[] = { > > + minFps, maxFps, maxFps, maxFps > > + }; > > + staticMetadata_->addEntry(ANDROID_CONTROL_AE_AVAILABLE_TARGET_FPS_RANGES, > > + availableAeFpsTarget); > > > > std::vector<int32_t> aeCompensationRange = { > > 0, 0, > > -- > > 2.32.0 > >
Hi Paul, On Tue, Aug 31, 2021 at 11:17:07AM +0900, Paul Elder wrote: > Hi Jacopo, > > On Fri, Aug 27, 2021 at 02:07:46PM +0200, Jacopo Mondi wrote: > > Now that building the list of supported stream configuration requires > > applying a configuration to the Camera, re-initialize the camera > > controls by applying a configuration generated for the Viewfinder stream > > role before building the list of static metadata. > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > Looks good, but I have a question. > > > --- > > src/android/camera_capabilities.cpp | 18 +++++++++++++++--- > > 1 file changed, 15 insertions(+), 3 deletions(-) > > > > diff --git a/src/android/camera_capabilities.cpp b/src/android/camera_capabilities.cpp > > index fdda90379ce2..723a4fd5a880 100644 > > --- a/src/android/camera_capabilities.cpp > > +++ b/src/android/camera_capabilities.cpp > > @@ -394,11 +394,14 @@ int CameraCapabilities::initialize(std::shared_ptr<libcamera::Camera> camera, > > } > > > > ret = initializeStreamConfigurations(); > > - camera_->release(); > > - if (ret) > > + if (ret) { > > + camera_->release(); > > return ret; > > + } > > > > - return initializeStaticMetadata(); > > + ret = initializeStaticMetadata(); > > + camera_->release(); > > + return ret; > > } > > > > std::vector<Size> > > @@ -682,6 +685,15 @@ int CameraCapabilities::initializeStaticMetadata() > > return -EINVAL; > > } > > > > + std::unique_ptr<CameraConfiguration> cameraConfig = > > + camera_->generateConfiguration({ StreamRole::Viewfinder }); > > From what I see, initializeStreamConfigurations() generates a > configuration for the StillCapture role. Is it fine that this uses a It does so only to retrieve the maximum resolution. A todo note reminds us that we should get that from a camera property * Get the maximum output resolutions * \todo Get this from the camera properties once defined */ std::unique_ptr<CameraConfiguration> cameraConfig = camera_->generateConfiguration({ StillCapture }); > different role? (Or was the other one changed to Viewfinder where I > wasn't paying attention?) Well, the choice of the Viewfinder role is pretty arbitrary, but we have to define a 'mode' the camera should be set to when initializing the static metadata. We don't have the luxury of assuming much from the underlying camera, and we have to construct the static metadata from the camera properties (static) and the camera controls (change depending on the configuration). Before this series the Camera::controls where static as well, so the Camera didn't get configured at all during the static metadata initialization, and we relied on the PH-initialized ControlInfoMap. As we now need to apply a Configuration to the Camera to collect per-configuration control limits, we need to re-initialize them to a known default before inspecting them. I considered Viewfinder as a good default, but I'm open to other proposals. > > > Paul > > > + int ret = camera_->configure(cameraConfig.get()); > > + if (ret) { > > + LOG(HAL, Error) << "Failed to initialize the camera state"; > > + staticMetadata_.reset(); > > + return ret; > > + } > > + > > const ControlInfoMap &controlsInfo = camera_->controls(); > > const ControlList &properties = camera_->properties(); > > > > -- > > 2.32.0 > >
Hi Jacopo, On Fri, Aug 27, 2021 at 02:07:53PM +0200, Jacopo Mondi wrote: > The ANDROID_SCALER_AVAILABLE_STREAM_CONFIGURATIONS and > ANDROID_SCALER_AVAILABLE_MIN_FRAME_DURATIONS static metadata are > populated by looping on the streamConfigurations_ vector. > > Unify them in a single loop to avoid repeating it. > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> Reviewed-by: Paul Elder <paul.elder@ideasonboard.com> > --- > src/android/camera_capabilities.cpp | 23 ++++++++++------------- > 1 file changed, 10 insertions(+), 13 deletions(-) > > diff --git a/src/android/camera_capabilities.cpp b/src/android/camera_capabilities.cpp > index 484242d0ad81..8bde06e824ef 100644 > --- a/src/android/camera_capabilities.cpp > +++ b/src/android/camera_capabilities.cpp > @@ -1255,7 +1255,9 @@ int CameraCapabilities::initializeStaticMetadata() > maxZoom); > > std::vector<uint32_t> availableStreamConfigurations; > + std::vector<int64_t> minFrameDurations; > availableStreamConfigurations.reserve(streamConfigurations_.size() * 4); > + minFrameDurations.reserve(streamConfigurations_.size() * 4); > for (const auto &entry : streamConfigurations_) { > /* > * Filter out YUV streams not capable of running at 30 FPS. > @@ -1272,12 +1274,19 @@ int CameraCapabilities::initializeStaticMetadata() > if (entry.androidFormat != HAL_PIXEL_FORMAT_BLOB && fps < 30) > continue; > > + /* Stream configuration map. */ > availableStreamConfigurations.push_back(entry.androidFormat); > availableStreamConfigurations.push_back(entry.resolution.width); > availableStreamConfigurations.push_back(entry.resolution.height); > availableStreamConfigurations.push_back( > ANDROID_SCALER_AVAILABLE_STREAM_CONFIGURATIONS_OUTPUT); > > + /* Per-stream durations. */ > + minFrameDurations.push_back(entry.androidFormat); > + minFrameDurations.push_back(entry.resolution.width); > + minFrameDurations.push_back(entry.resolution.height); > + minFrameDurations.push_back(entry.minFrameDurationNsec); > + > LOG(HAL, Debug) > << "Output Stream:" << utils::hex(entry.androidFormat) > << "(" << entry.resolution.width << "x" > @@ -1285,22 +1294,10 @@ int CameraCapabilities::initializeStaticMetadata() > << entry.minFrameDurationNsec << "]" > << "@" << fps; > } > + > staticMetadata_->addEntry(ANDROID_SCALER_AVAILABLE_STREAM_CONFIGURATIONS, > availableStreamConfigurations); > > - std::vector<int64_t> minFrameDurations; > - minFrameDurations.reserve(streamConfigurations_.size() * 4); > - for (const auto &entry : streamConfigurations_) { > - unsigned int fps = static_cast<unsigned int> > - (floor(1e9 / entry.minFrameDurationNsec + 0.05f)); > - if (entry.androidFormat != HAL_PIXEL_FORMAT_BLOB && fps < 30) > - continue; > - > - minFrameDurations.push_back(entry.androidFormat); > - minFrameDurations.push_back(entry.resolution.width); > - minFrameDurations.push_back(entry.resolution.height); > - minFrameDurations.push_back(entry.minFrameDurationNsec); > - } > staticMetadata_->addEntry(ANDROID_SCALER_AVAILABLE_MIN_FRAME_DURATIONS, > minFrameDurations); > > -- > 2.32.0 >
Hi Jacopo, On 8/31/21 3:27 PM, Jacopo Mondi wrote: > Hi Paul, > > On Tue, Aug 31, 2021 at 11:17:07AM +0900, Paul Elder wrote: >> Hi Jacopo, >> >> On Fri, Aug 27, 2021 at 02:07:46PM +0200, Jacopo Mondi wrote: >>> Now that building the list of supported stream configuration requires >>> applying a configuration to the Camera, re-initialize the camera >>> controls by applying a configuration generated for the Viewfinder stream >>> role before building the list of static metadata. >>> >>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> >> Looks good, but I have a question. >> >>> --- >>> src/android/camera_capabilities.cpp | 18 +++++++++++++++--- >>> 1 file changed, 15 insertions(+), 3 deletions(-) >>> >>> diff --git a/src/android/camera_capabilities.cpp b/src/android/camera_capabilities.cpp >>> index fdda90379ce2..723a4fd5a880 100644 >>> --- a/src/android/camera_capabilities.cpp >>> +++ b/src/android/camera_capabilities.cpp >>> @@ -394,11 +394,14 @@ int CameraCapabilities::initialize(std::shared_ptr<libcamera::Camera> camera, >>> } >>> >>> ret = initializeStreamConfigurations(); >>> - camera_->release(); >>> - if (ret) >>> + if (ret) { >>> + camera_->release(); >>> return ret; >>> + } >>> >>> - return initializeStaticMetadata(); >>> + ret = initializeStaticMetadata(); >>> + camera_->release(); >>> + return ret; >>> } >>> >>> std::vector<Size> >>> @@ -682,6 +685,15 @@ int CameraCapabilities::initializeStaticMetadata() >>> return -EINVAL; >>> } >>> >>> + std::unique_ptr<CameraConfiguration> cameraConfig = >>> + camera_->generateConfiguration({ StreamRole::Viewfinder }); >> From what I see, initializeStreamConfigurations() generates a >> configuration for the StillCapture role. Is it fine that this uses a > It does so only to retrieve the maximum resolution. > A todo note reminds us that we should get that from a camera property > > * Get the maximum output resolutions > * \todo Get this from the camera properties once defined > */ > std::unique_ptr<CameraConfiguration> cameraConfig = > camera_->generateConfiguration({ StillCapture }); > >> different role? (Or was the other one changed to Viewfinder where I >> wasn't paying attention?) > Well, the choice of the Viewfinder role is pretty arbitrary, but we > have to define a 'mode' the camera should be set to when initializing > the static metadata. We don't have the luxury of assuming much from > the underlying camera, and we have to construct the static metadata > from the camera properties (static) and the camera controls (change > depending on the configuration). > > Before this series the Camera::controls where static as well, so the > Camera didn't get configured at all during the static metadata > initialization, and we relied on the PH-initialized ControlInfoMap. > > As we now need to apply a Configuration to the Camera to collect > per-configuration control limits, we need to re-initialize them to a > known default before inspecting them. > > I considered Viewfinder as a good default, but I'm open to other > proposals. I agree with the reasoning but also feels to me that there should be comment placed in the file to address this. Up to you. Reviewed-by: Umang Jain <umang.jain@ideasonboard.com> > >> >> Paul >> >>> + int ret = camera_->configure(cameraConfig.get()); >>> + if (ret) { >>> + LOG(HAL, Error) << "Failed to initialize the camera state"; >>> + staticMetadata_.reset(); >>> + return ret; >>> + } >>> + >>> const ControlInfoMap &controlsInfo = camera_->controls(); >>> const ControlList &properties = camera_->properties(); >>> >>> -- >>> 2.32.0 >>>
Hi Jacopo Thanks for the patch. On 8/27/21 5:37 PM, Jacopo Mondi wrote: > As we now collect the per-stream frame durations at > initializeStreamConfigurations() times, the Camera is now guaranteed to > support the controls::FrameDurationLimits control. > > Remove the check for its presence when populating the > ANDROID_CONTROL_AE_AVAILABLE_TARGET_FPS_RANGES static metadata. > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> Reviewed-by: Umang Jain <umang.jain@ideasonboard.com> > --- > src/android/camera_capabilities.cpp | 83 ++++++++++++++--------------- > 1 file changed, 41 insertions(+), 42 deletions(-) > > diff --git a/src/android/camera_capabilities.cpp b/src/android/camera_capabilities.cpp > index 723a4fd5a880..c45d74189aaa 100644 > --- a/src/android/camera_capabilities.cpp > +++ b/src/android/camera_capabilities.cpp > @@ -858,55 +858,54 @@ int CameraCapabilities::initializeStaticMetadata() > staticMetadata_->addEntry(ANDROID_CONTROL_AE_AVAILABLE_MODES, > aeAvailableModes); > > + /* Initialize the AE frame duration limits. */ > int64_t minFrameDurationNsec = -1; > int64_t maxFrameDurationNsec = -1; > const auto frameDurationsInfo = controlsInfo.find(&controls::FrameDurationLimits); > - if (frameDurationsInfo != controlsInfo.end()) { > - minFrameDurationNsec = frameDurationsInfo->second.min().get<int64_t>() * 1000; > - maxFrameDurationNsec = frameDurationsInfo->second.max().get<int64_t>() * 1000; > + minFrameDurationNsec = frameDurationsInfo->second.min().get<int64_t>() * 1000; > + maxFrameDurationNsec = frameDurationsInfo->second.max().get<int64_t>() * 1000; > > - /* > - * Adjust the minimum frame duration to comply with Android > - * requirements. The camera service mandates all preview/record > - * streams to have a minimum frame duration < 33,366 milliseconds > - * (see MAX_PREVIEW_RECORD_DURATION_NS in the camera service > - * implementation). > - * > - * If we're close enough (+ 500 useconds) to that value, round > - * the minimum frame duration of the camera to an accepted > - * value. > - */ > - static constexpr int64_t MAX_PREVIEW_RECORD_DURATION_NS = 1e9 / 29.97; > - if (minFrameDurationNsec > MAX_PREVIEW_RECORD_DURATION_NS && > - minFrameDurationNsec < MAX_PREVIEW_RECORD_DURATION_NS + 500000) > - minFrameDurationNsec = MAX_PREVIEW_RECORD_DURATION_NS - 1000; > + /* > + * Adjust the minimum frame duration to comply with Android > + * requirements. The camera service mandates all preview/record > + * streams to have a minimum frame duration < 33,366 milliseconds > + * (see MAX_PREVIEW_RECORD_DURATION_NS in the camera service > + * implementation). > + * > + * If we're close enough (+ 500 useconds) to that value, round > + * the minimum frame duration of the camera to an accepted > + * value. > + */ > + static constexpr int64_t MAX_PREVIEW_RECORD_DURATION_NS = 1e9 / 29.97; > + if (minFrameDurationNsec > MAX_PREVIEW_RECORD_DURATION_NS && > + minFrameDurationNsec < MAX_PREVIEW_RECORD_DURATION_NS + 500000) > + minFrameDurationNsec = MAX_PREVIEW_RECORD_DURATION_NS - 1000; > > - /* > - * The AE routine frame rate limits are computed using the frame > - * duration limits, as libcamera clips the AE routine to the > - * frame durations. > - */ > - int32_t maxFps = std::round(1e9 / minFrameDurationNsec); > - int32_t minFps = std::round(1e9 / maxFrameDurationNsec); > - minFps = std::max(1, minFps); > + /* > + * The AE routine frame rate limits are computed using the frame > + * duration limits, as libcamera clips the AE routine to the > + * frame durations. > + */ > + int32_t maxFps = std::round(1e9 / minFrameDurationNsec); > + int32_t minFps = std::round(1e9 / maxFrameDurationNsec); > + minFps = std::max(1, minFps); > > - /* > - * Force rounding errors so that we have the proper frame > - * durations for when we reuse these variables later > - */ > - minFrameDurationNsec = 1e9 / maxFps; > - maxFrameDurationNsec = 1e9 / minFps; > + /* > + * Force rounding errors so that we have the proper frame > + * durations for when we reuse these variables later > + */ > + minFrameDurationNsec = 1e9 / maxFps; > + maxFrameDurationNsec = 1e9 / minFps; > > - /* > - * Register to the camera service {min, max} and {max, max} > - * intervals as requested by the metadata documentation. > - */ > - int32_t availableAeFpsTarget[] = { > - minFps, maxFps, maxFps, maxFps > - }; > - staticMetadata_->addEntry(ANDROID_CONTROL_AE_AVAILABLE_TARGET_FPS_RANGES, > - availableAeFpsTarget); > - } > + /* > + * Register to the camera service {min, max} and {max, max} > + * intervals as requested by the metadata documentation. > + */ > + int32_t availableAeFpsTarget[] = { > + minFps, maxFps, maxFps, maxFps > + }; > + staticMetadata_->addEntry(ANDROID_CONTROL_AE_AVAILABLE_TARGET_FPS_RANGES, > + availableAeFpsTarget); > > std::vector<int32_t> aeCompensationRange = { > 0, 0,
Hi Jacopo, Thank you for the patch On 8/27/21 5:37 PM, Jacopo Mondi wrote: > While building the list of supported stream configurations also collet > the absolute max frame durations to be used to populate the sensor > maximum frame duration. > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> Reviewed-by: Umang Jain <umang.jain@ideasonboard.com> > --- > src/android/camera_capabilities.cpp | 9 ++++++--- > src/android/camera_capabilities.h | 1 + > 2 files changed, 7 insertions(+), 3 deletions(-) > > diff --git a/src/android/camera_capabilities.cpp b/src/android/camera_capabilities.cpp > index 23a9a3fbec50..388552963c47 100644 > --- a/src/android/camera_capabilities.cpp > +++ b/src/android/camera_capabilities.cpp > @@ -385,6 +385,7 @@ int CameraCapabilities::initialize(std::shared_ptr<libcamera::Camera> camera, > orientation_ = orientation; > facing_ = facing; > rawStreamAvailable_ = false; > + maxFrameDuration_ = 0; > > /* Acquire the camera and initialize available stream configurations. */ > int ret = camera_->acquire(); > @@ -659,6 +660,9 @@ int CameraCapabilities::initializeStreamConfigurations() > }); > maxJpegSize = std::max(maxJpegSize, res); > } > + > + maxFrameDuration_ = std::max(maxFrameDuration_, > + maxFrameDuration); > } > > /* > @@ -1132,9 +1136,8 @@ int CameraCapabilities::initializeStaticMetadata() > staticMetadata_->addEntry(ANDROID_SENSOR_INFO_TIMESTAMP_SOURCE, > timestampSource); > > - if (maxFrameDurationNsec > 0) > - staticMetadata_->addEntry(ANDROID_SENSOR_INFO_MAX_FRAME_DURATION, > - maxFrameDurationNsec); > + staticMetadata_->addEntry(ANDROID_SENSOR_INFO_MAX_FRAME_DURATION, > + maxFrameDuration_); > > /* Statistics static metadata. */ > uint8_t faceDetectMode = ANDROID_STATISTICS_FACE_DETECT_MODE_OFF; > diff --git a/src/android/camera_capabilities.h b/src/android/camera_capabilities.h > index 6e55ddab445e..2cf97ae80095 100644 > --- a/src/android/camera_capabilities.h > +++ b/src/android/camera_capabilities.h > @@ -71,6 +71,7 @@ private: > int facing_; > int orientation_; > bool rawStreamAvailable_; > + int64_t maxFrameDuration_; > camera_metadata_enum_android_info_supported_hardware_level hwLevel_; > std::set<camera_metadata_enum_android_request_available_capabilities> capabilities_; >
Hi Jacopo, On Tue, Aug 31, 2021 at 11:57:50AM +0200, Jacopo Mondi wrote: > Hi Paul, > > On Tue, Aug 31, 2021 at 11:17:07AM +0900, Paul Elder wrote: > > Hi Jacopo, > > > > On Fri, Aug 27, 2021 at 02:07:46PM +0200, Jacopo Mondi wrote: > > > Now that building the list of supported stream configuration requires > > > applying a configuration to the Camera, re-initialize the camera > > > controls by applying a configuration generated for the Viewfinder stream > > > role before building the list of static metadata. > > > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > > > Looks good, but I have a question. > > > > > --- > > > src/android/camera_capabilities.cpp | 18 +++++++++++++++--- > > > 1 file changed, 15 insertions(+), 3 deletions(-) > > > > > > diff --git a/src/android/camera_capabilities.cpp b/src/android/camera_capabilities.cpp > > > index fdda90379ce2..723a4fd5a880 100644 > > > --- a/src/android/camera_capabilities.cpp > > > +++ b/src/android/camera_capabilities.cpp > > > @@ -394,11 +394,14 @@ int CameraCapabilities::initialize(std::shared_ptr<libcamera::Camera> camera, > > > } > > > > > > ret = initializeStreamConfigurations(); > > > - camera_->release(); > > > - if (ret) > > > + if (ret) { > > > + camera_->release(); > > > return ret; > > > + } > > > > > > - return initializeStaticMetadata(); > > > + ret = initializeStaticMetadata(); > > > + camera_->release(); > > > + return ret; > > > } > > > > > > std::vector<Size> > > > @@ -682,6 +685,15 @@ int CameraCapabilities::initializeStaticMetadata() > > > return -EINVAL; > > > } > > > > > > + std::unique_ptr<CameraConfiguration> cameraConfig = > > > + camera_->generateConfiguration({ StreamRole::Viewfinder }); > > > > From what I see, initializeStreamConfigurations() generates a > > configuration for the StillCapture role. Is it fine that this uses a > > It does so only to retrieve the maximum resolution. > A todo note reminds us that we should get that from a camera property > > * Get the maximum output resolutions > * \todo Get this from the camera properties once defined > */ > std::unique_ptr<CameraConfiguration> cameraConfig = > camera_->generateConfiguration({ StillCapture }); > > > different role? (Or was the other one changed to Viewfinder where I > > wasn't paying attention?) > > Well, the choice of the Viewfinder role is pretty arbitrary, but we > have to define a 'mode' the camera should be set to when initializing > the static metadata. We don't have the luxury of assuming much from > the underlying camera, and we have to construct the static metadata > from the camera properties (static) and the camera controls (change > depending on the configuration). > > Before this series the Camera::controls where static as well, so the > Camera didn't get configured at all during the static metadata > initialization, and we relied on the PH-initialized ControlInfoMap. > > As we now need to apply a Configuration to the Camera to collect > per-configuration control limits, we need to re-initialize them to a > known default before inspecting them. > > I considered Viewfinder as a good default, but I'm open to other > proposals. Ah, okay, I see. Reviewed-by: Paul Elder <paul.elder@ideasonboard.com> > > > > > > + int ret = camera_->configure(cameraConfig.get()); > > > + if (ret) { > > > + LOG(HAL, Error) << "Failed to initialize the camera state"; > > > + staticMetadata_.reset(); > > > + return ret; > > > + } > > > + > > > const ControlInfoMap &controlsInfo = camera_->controls(); > > > const ControlList &properties = camera_->properties(); > > > > > > -- > > > 2.32.0 > > >
Hi Jacopo, On 8/27/21 5:37 PM, Jacopo Mondi wrote: > The ANDROID_SCALER_AVAILABLE_STREAM_CONFIGURATIONS and > ANDROID_SCALER_AVAILABLE_MIN_FRAME_DURATIONS static metadata are > populated by looping on the streamConfigurations_ vector. > > Unify them in a single loop to avoid repeating it. > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> Reviewed-by: Umang Jain <umang.jain@ideasonboard.com> > --- > src/android/camera_capabilities.cpp | 23 ++++++++++------------- > 1 file changed, 10 insertions(+), 13 deletions(-) > > diff --git a/src/android/camera_capabilities.cpp b/src/android/camera_capabilities.cpp > index 484242d0ad81..8bde06e824ef 100644 > --- a/src/android/camera_capabilities.cpp > +++ b/src/android/camera_capabilities.cpp > @@ -1255,7 +1255,9 @@ int CameraCapabilities::initializeStaticMetadata() > maxZoom); > > std::vector<uint32_t> availableStreamConfigurations; > + std::vector<int64_t> minFrameDurations; > availableStreamConfigurations.reserve(streamConfigurations_.size() * 4); > + minFrameDurations.reserve(streamConfigurations_.size() * 4); > for (const auto &entry : streamConfigurations_) { > /* > * Filter out YUV streams not capable of running at 30 FPS. > @@ -1272,12 +1274,19 @@ int CameraCapabilities::initializeStaticMetadata() > if (entry.androidFormat != HAL_PIXEL_FORMAT_BLOB && fps < 30) > continue; > > + /* Stream configuration map. */ > availableStreamConfigurations.push_back(entry.androidFormat); > availableStreamConfigurations.push_back(entry.resolution.width); > availableStreamConfigurations.push_back(entry.resolution.height); > availableStreamConfigurations.push_back( > ANDROID_SCALER_AVAILABLE_STREAM_CONFIGURATIONS_OUTPUT); > > + /* Per-stream durations. */ > + minFrameDurations.push_back(entry.androidFormat); > + minFrameDurations.push_back(entry.resolution.width); > + minFrameDurations.push_back(entry.resolution.height); > + minFrameDurations.push_back(entry.minFrameDurationNsec); > + > LOG(HAL, Debug) > << "Output Stream:" << utils::hex(entry.androidFormat) > << "(" << entry.resolution.width << "x" > @@ -1285,22 +1294,10 @@ int CameraCapabilities::initializeStaticMetadata() > << entry.minFrameDurationNsec << "]" > << "@" << fps; > } > + > staticMetadata_->addEntry(ANDROID_SCALER_AVAILABLE_STREAM_CONFIGURATIONS, > availableStreamConfigurations); > > - std::vector<int64_t> minFrameDurations; > - minFrameDurations.reserve(streamConfigurations_.size() * 4); > - for (const auto &entry : streamConfigurations_) { > - unsigned int fps = static_cast<unsigned int> > - (floor(1e9 / entry.minFrameDurationNsec + 0.05f)); > - if (entry.androidFormat != HAL_PIXEL_FORMAT_BLOB && fps < 30) > - continue; > - > - minFrameDurations.push_back(entry.androidFormat); > - minFrameDurations.push_back(entry.resolution.width); > - minFrameDurations.push_back(entry.resolution.height); > - minFrameDurations.push_back(entry.minFrameDurationNsec); > - } > staticMetadata_->addEntry(ANDROID_SCALER_AVAILABLE_MIN_FRAME_DURATIONS, > minFrameDurations); >
As the patch subject suggests, this series is composed of two main parts, which concur in correctly handling the frame durations reported to the Android HAL. The series is based on Umang's https://patchwork.libcamera.org/project/libcamera/list/?series=2328 which I would really like to see integrated soon and has been tested with https://patchwork.libcamera.org/project/libcamera/list/?series=2406 which fixes a few CTS tests for devices that do not support manual sensor mode. The series starts by allowing the IPU3 pipeline handler to select the optimal sensor size to feed the ImgU with. Umang's series is a requirement for this patch, otherwise the ImgU configuration fails. Patches 2 and 3 update the Camera's ControlInfoMap in response to a Camera::configure() call, by updating the controls limits in the ph and the IPA. From patch 4 on the Android HAL parts begins by collecting per-stream frame durations. As the frame durations are updated as part of the Camera's ControlInfoMap limits update it is necessary to configure the camera during the HAL startup. A few cleanup patches follows, including the correct handling of STALL durations Patch 10 introduces a filtering criteria for YUV streams: only streams that can produce 30 FPS can be registered for preview. I was not able to find this clearly specified in documentation but the requirement has been confirmed by the cros camera team and in the Intel HAL implementation. After two more cleanup patches the way ANDROID_CONTROL_AE_AVAILABLE_TARGET_FPS_RANGES gets populated is changed to comply with the documentation. Note that this does not align with the Intel HAL implementation which registers 8 values instead of 4. The last part of the series is there mostly for comments. The idea is that streams capable of running at more than 30 FPS should be limited to such frame rate, as it has proven to be the optimal one for the platform, and including more frame rates (in example ov5670 is capable of 60FPS at smaller resolutions) would require a different handling of ANDROID_CONTROL_AE_AVAILABLE_TARGET_FPS_RANGES which I've not been able to fully clarify yet. As a confirmation, the Intel HAL limits all its streams to 30 FPS as well for IQ optimization reasons. The series contains one patch to limit the reported duration to 30 FPS and please CTS without actually limiting the frame rate. The patch is then reverted and the same mechanism is implemented in the IPA, this time actually setting the right vblank to maintain the 30 FPS frame rate (confirmed inspecting timestamps reported by 'cam'). With both solutions applied I got several 0 failures CTS runs. With the HAL-only solution the flaky recording tests failed once, with the IPA solution I got not failures in 3 CTS runs. Thanks j Jacopo Mondi (16): libcamera: ipu3: Use the optimal sensor size libcamera: ipu3: Split controls init/update ipa: ipu3: Update camera controls in configure() android: capabilities: Collect per-stream frame durations android: capabilities: Initialize camera state when building properties android: capabilties: Assume controls::FrameDurationLimits is supported android: capabilities: Use per-configuration durations android: capabilties: Correctly populate STALL durations android: capabilities: Collect absolute max frame durations android: Filter preview streams on FPS android: capabilities: Print output stream list android: Populate streams and duration in the same loop android: capabilties: Fix ANDROID_CONTROL_AE_AVAILABLE_TARGET_FPS_RANGES android: capabilities: Cap frame rate to 30 FPS Revert "android: capabilities: Cap frame rate to 30 FPS" ipa: ipu3: Cap frame duration to 30 FPS include/libcamera/ipa/ipu3.mojom | 3 +- src/android/camera_capabilities.cpp | 199 +++++++++++++++++---------- src/android/camera_capabilities.h | 3 + src/ipa/ipu3/ipu3.cpp | 119 +++++++++++----- src/libcamera/pipeline/ipu3/ipu3.cpp | 75 ++++++---- 5 files changed, 267 insertions(+), 132 deletions(-) -- 2.32.0