| Message ID | 20210524094123.1187354-1-paul.elder@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Paul, Thank you for the patch. On Mon, May 24, 2021 at 06:41:21PM +0900, Paul Elder wrote: > We need a separate control to report the nominal frame duration, but > it's also useful to report the min/max frame duration values that will > be used. Split the FrameDurations control into FrameDuration and > FrameDurationLimits respectively to support both of these. I'm not 100% sure yet it will be useful to report the adjusted FrameDurationLimits in metadata, but a separate control for the nominal frame duration is certainly less confusing. > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > include/libcamera/ipa/raspberrypi.h | 2 +- > src/android/camera_device.cpp | 2 +- > src/ipa/raspberrypi/raspberrypi.cpp | 4 ++-- > src/libcamera/control_ids.yaml | 8 +++++++- > src/libcamera/pipeline/ipu3/ipu3.cpp | 6 +++--- > 5 files changed, 14 insertions(+), 8 deletions(-) > > diff --git a/include/libcamera/ipa/raspberrypi.h b/include/libcamera/ipa/raspberrypi.h > index d10c1733..a8790000 100644 > --- a/include/libcamera/ipa/raspberrypi.h > +++ b/include/libcamera/ipa/raspberrypi.h > @@ -44,7 +44,7 @@ static const ControlInfoMap Controls = { > { &controls::Sharpness, ControlInfo(0.0f, 16.0f, 1.0f) }, > { &controls::ColourCorrectionMatrix, ControlInfo(-16.0f, 16.0f) }, > { &controls::ScalerCrop, ControlInfo(Rectangle{}, Rectangle(65535, 65535, 65535, 65535), Rectangle{}) }, > - { &controls::FrameDurations, ControlInfo(INT64_C(1000), INT64_C(1000000000)) }, > + { &controls::FrameDurationLimits, ControlInfo(INT64_C(1000), INT64_C(1000000000)) }, > { &controls::draft::NoiseReductionMode, ControlInfo(controls::draft::NoiseReductionModeValues) }, > }; > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > index b32e8be5..0eea2b95 100644 > --- a/src/android/camera_device.cpp > +++ b/src/android/camera_device.cpp > @@ -824,7 +824,7 @@ const camera_metadata_t *CameraDevice::getStaticMetadata() > > int64_t minFrameDurationNsec = -1; > int64_t maxFrameDurationNsec = -1; > - const auto frameDurationsInfo = controlsInfo.find(&controls::FrameDurations); > + 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; > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp > index 52d91db2..79a327e2 100644 > --- a/src/ipa/raspberrypi/raspberrypi.cpp > +++ b/src/ipa/raspberrypi/raspberrypi.cpp > @@ -860,7 +860,7 @@ void IPARPi::queueRequest(const ControlList &controls) > break; > } > > - case controls::FRAME_DURATIONS: { > + case controls::FRAME_DURATION_LIMITS: { > auto frameDurations = ctrl.second.get<Span<const int64_t>>(); > applyFrameDurations(frameDurations[0], frameDurations[1]); > break; > @@ -1075,7 +1075,7 @@ void IPARPi::applyFrameDurations(double minFrameDuration, double maxFrameDuratio > maxFrameDuration_ = std::max(maxFrameDuration_, minFrameDuration_); > > /* Return the validated limits via metadata. */ > - libcameraMetadata_.set(controls::FrameDurations, > + libcameraMetadata_.set(controls::FrameDurationLimits, > { static_cast<int64_t>(minFrameDuration_), > static_cast<int64_t>(maxFrameDuration_) }); > > diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml > index 88d81ac4..338fbdc9 100644 > --- a/src/libcamera/control_ids.yaml > +++ b/src/libcamera/control_ids.yaml > @@ -323,7 +323,13 @@ controls: > step to respect the received gain factor and shall report > their total value in the request metadata. > > - - FrameDurations: > + - FrameDuration: > + type: int64_t > + description: | > + The nominal frame duration from start of frame exposure to start of > + next exposure, expressed in microseconds. > + > + - FrameDurationLimits: > type: int64_t > description: | > The minimum and maximum (in that order) frame duration, > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > index 8ae47c6d..6e17753f 100644 > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > @@ -980,9 +980,9 @@ int PipelineHandlerIPU3::initControls(IPU3CameraData *data) > frameDurations[i] = frameSize / (sensorInfo.pixelRate / 1000000U); > } > > - controls[&controls::FrameDurations] = ControlInfo(frameDurations[0], > - frameDurations[1], > - frameDurations[2]); > + controls[&controls::FrameDuration] = ControlInfo(frameDurations[0], > + frameDurations[1], > + frameDurations[2]); > > /* > * Compute the scaler crop limits.
Hi Paul, Laurent, On Mon, May 24, 2021 at 03:13:15PM +0300, Laurent Pinchart wrote: > Hi Paul, > > Thank you for the patch. > > On Mon, May 24, 2021 at 06:41:21PM +0900, Paul Elder wrote: > > We need a separate control to report the nominal frame duration, but > > it's also useful to report the min/max frame duration values that will > > be used. Split the FrameDurations control into FrameDuration and > > FrameDurationLimits respectively to support both of these. > > I'm not 100% sure yet it will be useful to report the adjusted > FrameDurationLimits in metadata, but a separate control for the nominal > frame duration is certainly less confusing. > > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > --- [snip] > > diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml > > index 88d81ac4..338fbdc9 100644 > > --- a/src/libcamera/control_ids.yaml > > +++ b/src/libcamera/control_ids.yaml > > @@ -323,7 +323,13 @@ controls: > > step to respect the received gain factor and shall report > > their total value in the request metadata. > > > > - - FrameDurations: > > + - FrameDuration: > > + type: int64_t > > + description: | > > + The nominal frame duration from start of frame exposure to start of > > + next exposure, expressed in microseconds. Isn't this a bit poor ? Should we at least specify this is meant to be returned in metadata only ? > > + > > + - FrameDurationLimits: > > type: int64_t > > description: | > > The minimum and maximum (in that order) frame duration, > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > > index 8ae47c6d..6e17753f 100644 > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > > @@ -980,9 +980,9 @@ int PipelineHandlerIPU3::initControls(IPU3CameraData *data) > > frameDurations[i] = frameSize / (sensorInfo.pixelRate / 1000000U); > > } > > > > - controls[&controls::FrameDurations] = ControlInfo(frameDurations[0], > > - frameDurations[1], > > - frameDurations[2]); > > + controls[&controls::FrameDuration] = ControlInfo(frameDurations[0], > > + frameDurations[1], > > + frameDurations[2]); > > > > /* > > * Compute the scaler crop limits. > > -- > Regards, > > Laurent Pinchart
Hi Paul, Thank you for your work. On Mon, 24 May 2021 at 10:41, Paul Elder <paul.elder@ideasonboard.com> wrote: > We need a separate control to report the nominal frame duration, but > it's also useful to report the min/max frame duration values that will > be used. Split the FrameDurations control into FrameDuration and > FrameDurationLimits respectively to support both of these. > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> > --- > include/libcamera/ipa/raspberrypi.h | 2 +- > src/android/camera_device.cpp | 2 +- > src/ipa/raspberrypi/raspberrypi.cpp | 4 ++-- > src/libcamera/control_ids.yaml | 8 +++++++- > src/libcamera/pipeline/ipu3/ipu3.cpp | 6 +++--- > 5 files changed, 14 insertions(+), 8 deletions(-) > > diff --git a/include/libcamera/ipa/raspberrypi.h > b/include/libcamera/ipa/raspberrypi.h > index d10c1733..a8790000 100644 > --- a/include/libcamera/ipa/raspberrypi.h > +++ b/include/libcamera/ipa/raspberrypi.h > @@ -44,7 +44,7 @@ static const ControlInfoMap Controls = { > { &controls::Sharpness, ControlInfo(0.0f, 16.0f, 1.0f) }, > { &controls::ColourCorrectionMatrix, ControlInfo(-16.0f, 16.0f) }, > { &controls::ScalerCrop, ControlInfo(Rectangle{}, Rectangle(65535, > 65535, 65535, 65535), Rectangle{}) }, > - { &controls::FrameDurations, ControlInfo(INT64_C(1000), > INT64_C(1000000000)) }, > + { &controls::FrameDurationLimits, ControlInfo(INT64_C(1000), > INT64_C(1000000000)) }, > { &controls::draft::NoiseReductionMode, > ControlInfo(controls::draft::NoiseReductionModeValues) }, > }; > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > index b32e8be5..0eea2b95 100644 > --- a/src/android/camera_device.cpp > +++ b/src/android/camera_device.cpp > @@ -824,7 +824,7 @@ const camera_metadata_t > *CameraDevice::getStaticMetadata() > > int64_t minFrameDurationNsec = -1; > int64_t maxFrameDurationNsec = -1; > - const auto frameDurationsInfo = > controlsInfo.find(&controls::FrameDurations); > + 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; > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp > b/src/ipa/raspberrypi/raspberrypi.cpp > index 52d91db2..79a327e2 100644 > --- a/src/ipa/raspberrypi/raspberrypi.cpp > +++ b/src/ipa/raspberrypi/raspberrypi.cpp > @@ -860,7 +860,7 @@ void IPARPi::queueRequest(const ControlList &controls) > break; > } > > - case controls::FRAME_DURATIONS: { > + case controls::FRAME_DURATION_LIMITS: { > auto frameDurations = ctrl.second.get<Span<const > int64_t>>(); > applyFrameDurations(frameDurations[0], > frameDurations[1]); > break; > @@ -1075,7 +1075,7 @@ void IPARPi::applyFrameDurations(double > minFrameDuration, double maxFrameDuratio > maxFrameDuration_ = std::max(maxFrameDuration_, minFrameDuration_); > > /* Return the validated limits via metadata. */ > - libcameraMetadata_.set(controls::FrameDurations, > + libcameraMetadata_.set(controls::FrameDurationLimits, > { static_cast<int64_t>(minFrameDuration_), > static_cast<int64_t>(maxFrameDuration_) > }); > > diff --git a/src/libcamera/control_ids.yaml > b/src/libcamera/control_ids.yaml > index 88d81ac4..338fbdc9 100644 > --- a/src/libcamera/control_ids.yaml > +++ b/src/libcamera/control_ids.yaml > @@ -323,7 +323,13 @@ controls: > step to respect the received gain factor and shall report > their total value in the request metadata. > > - - FrameDurations: > + - FrameDuration: > + type: int64_t > + description: | > + The nominal frame duration from start of frame exposure to start > of > + next exposure, expressed in microseconds. > Just a little nit-pick, is nominal the right word here? If this control were to return the frame duration between frame N and frame N-1 for every frame, would it be better to replace nominal with instantaneous, as the control value would be an accurate representation of the inter-frame duration for that particular frame. > + > + - FrameDurationLimits: > type: int64_t > description: | > The minimum and maximum (in that order) frame duration, > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp > b/src/libcamera/pipeline/ipu3/ipu3.cpp > index 8ae47c6d..6e17753f 100644 > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > @@ -980,9 +980,9 @@ int PipelineHandlerIPU3::initControls(IPU3CameraData > *data) > frameDurations[i] = frameSize / (sensorInfo.pixelRate / > 1000000U); > } > > - controls[&controls::FrameDurations] = > ControlInfo(frameDurations[0], > - > frameDurations[1], > - > frameDurations[2]); > + controls[&controls::FrameDuration] = ControlInfo(frameDurations[0], > + frameDurations[1], > + > frameDurations[2]); > > /* > * Compute the scaler crop limits. > -- > 2.27.0 > >
diff --git a/include/libcamera/ipa/raspberrypi.h b/include/libcamera/ipa/raspberrypi.h index d10c1733..a8790000 100644 --- a/include/libcamera/ipa/raspberrypi.h +++ b/include/libcamera/ipa/raspberrypi.h @@ -44,7 +44,7 @@ static const ControlInfoMap Controls = { { &controls::Sharpness, ControlInfo(0.0f, 16.0f, 1.0f) }, { &controls::ColourCorrectionMatrix, ControlInfo(-16.0f, 16.0f) }, { &controls::ScalerCrop, ControlInfo(Rectangle{}, Rectangle(65535, 65535, 65535, 65535), Rectangle{}) }, - { &controls::FrameDurations, ControlInfo(INT64_C(1000), INT64_C(1000000000)) }, + { &controls::FrameDurationLimits, ControlInfo(INT64_C(1000), INT64_C(1000000000)) }, { &controls::draft::NoiseReductionMode, ControlInfo(controls::draft::NoiseReductionModeValues) }, }; diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp index b32e8be5..0eea2b95 100644 --- a/src/android/camera_device.cpp +++ b/src/android/camera_device.cpp @@ -824,7 +824,7 @@ const camera_metadata_t *CameraDevice::getStaticMetadata() int64_t minFrameDurationNsec = -1; int64_t maxFrameDurationNsec = -1; - const auto frameDurationsInfo = controlsInfo.find(&controls::FrameDurations); + 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; diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp index 52d91db2..79a327e2 100644 --- a/src/ipa/raspberrypi/raspberrypi.cpp +++ b/src/ipa/raspberrypi/raspberrypi.cpp @@ -860,7 +860,7 @@ void IPARPi::queueRequest(const ControlList &controls) break; } - case controls::FRAME_DURATIONS: { + case controls::FRAME_DURATION_LIMITS: { auto frameDurations = ctrl.second.get<Span<const int64_t>>(); applyFrameDurations(frameDurations[0], frameDurations[1]); break; @@ -1075,7 +1075,7 @@ void IPARPi::applyFrameDurations(double minFrameDuration, double maxFrameDuratio maxFrameDuration_ = std::max(maxFrameDuration_, minFrameDuration_); /* Return the validated limits via metadata. */ - libcameraMetadata_.set(controls::FrameDurations, + libcameraMetadata_.set(controls::FrameDurationLimits, { static_cast<int64_t>(minFrameDuration_), static_cast<int64_t>(maxFrameDuration_) }); diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml index 88d81ac4..338fbdc9 100644 --- a/src/libcamera/control_ids.yaml +++ b/src/libcamera/control_ids.yaml @@ -323,7 +323,13 @@ controls: step to respect the received gain factor and shall report their total value in the request metadata. - - FrameDurations: + - FrameDuration: + type: int64_t + description: | + The nominal frame duration from start of frame exposure to start of + next exposure, expressed in microseconds. + + - FrameDurationLimits: type: int64_t description: | The minimum and maximum (in that order) frame duration, diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp index 8ae47c6d..6e17753f 100644 --- a/src/libcamera/pipeline/ipu3/ipu3.cpp +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp @@ -980,9 +980,9 @@ int PipelineHandlerIPU3::initControls(IPU3CameraData *data) frameDurations[i] = frameSize / (sensorInfo.pixelRate / 1000000U); } - controls[&controls::FrameDurations] = ControlInfo(frameDurations[0], - frameDurations[1], - frameDurations[2]); + controls[&controls::FrameDuration] = ControlInfo(frameDurations[0], + frameDurations[1], + frameDurations[2]); /* * Compute the scaler crop limits.
We need a separate control to report the nominal frame duration, but it's also useful to report the min/max frame duration values that will be used. Split the FrameDurations control into FrameDuration and FrameDurationLimits respectively to support both of these. Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> --- include/libcamera/ipa/raspberrypi.h | 2 +- src/android/camera_device.cpp | 2 +- src/ipa/raspberrypi/raspberrypi.cpp | 4 ++-- src/libcamera/control_ids.yaml | 8 +++++++- src/libcamera/pipeline/ipu3/ipu3.cpp | 6 +++--- 5 files changed, 14 insertions(+), 8 deletions(-)