| Message ID | 20250110235737.1524733-11-paul.elder@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Paul, Thank you for the patch. On Fri, Jan 10, 2025 at 05:57:35PM -0600, Paul Elder wrote: > Handle the AeEnable under the hood in the Camera class, such that > AeEnable activates ExposureTimeMode and AnalogueGain together. This > allows applications the convenience of setting auto/manual mode of all > of the AE-related controls, as well as protecting applications against a > nasty behavior change if an aperture control is added in the future. > This also moves common handling code out of the IPA. > > While we also want to inject AeEnable in Camera::controls() so that IPAs > don't have to report it, it is technically difficult at the moment as > ControlInfoMaps are not easily modifiable. > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > Changes in v7: > - check that the camera supports the ae sub-controls before setting them > when handling AeEnable > > New in v6 > --- > src/libcamera/camera.cpp | 20 ++++++++++++++++++++ > 1 file changed, 20 insertions(+) > > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp > index 69a7ee535..56c585199 100644 > --- a/src/libcamera/camera.cpp > +++ b/src/libcamera/camera.cpp > @@ -19,6 +19,7 @@ > #include <libcamera/base/thread.h> > > #include <libcamera/color_space.h> > +#include <libcamera/control_ids.h> > #include <libcamera/framebuffer_allocator.h> > #include <libcamera/request.h> > #include <libcamera/stream.h> > @@ -1325,6 +1326,25 @@ int Camera::queueRequest(Request *request) > } > } > > + /* Pre-process AeEnable. */ > + ControlList &controls = request->controls(); > + const auto &aeEnable = controls.get(controls::AeEnable); > + if (aeEnable) { > + if (_d()->controlInfo_.count(controls::AnalogueGainMode.id()) && > + !controls.contains(controls::AnalogueGainMode.id())) { > + controls.set(controls::AnalogueGainMode, > + *aeEnable ? controls::AnalogueGainModeAuto > + : controls::AnalogueGainModeManual); > + } > + > + if (_d()->controlInfo_.count(controls::ExposureTimeMode.id()) && > + !controls.contains(controls::ExposureTimeMode.id())) { > + controls.set(controls::ExposureTimeMode, > + *aeEnable ? controls::ExposureTimeModeAuto > + : controls::ExposureTimeModeManual); > + } > + } > + > d->pipe_->invokeMethod(&PipelineHandler::queueRequest, > ConnectionTypeQueued, request); >
Hi Paul, Thank you for the patch. On Fri, Jan 10, 2025 at 05:57:35PM -0600, Paul Elder wrote: > Handle the AeEnable under the hood in the Camera class, such that > AeEnable activates ExposureTimeMode and AnalogueGain together. This > allows applications the convenience of setting auto/manual mode of all > of the AE-related controls, as well as protecting applications against a > nasty behavior change if an aperture control is added in the future. > This also moves common handling code out of the IPA. > > While we also want to inject AeEnable in Camera::controls() so that IPAs > don't have to report it, it is technically difficult at the moment as > ControlInfoMaps are not easily modifiable. > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> > > --- > Changes in v7: > - check that the camera supports the ae sub-controls before setting them > when handling AeEnable > > New in v6 > --- > src/libcamera/camera.cpp | 20 ++++++++++++++++++++ > 1 file changed, 20 insertions(+) > > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp > index 69a7ee535..56c585199 100644 > --- a/src/libcamera/camera.cpp > +++ b/src/libcamera/camera.cpp > @@ -19,6 +19,7 @@ > #include <libcamera/base/thread.h> > > #include <libcamera/color_space.h> > +#include <libcamera/control_ids.h> > #include <libcamera/framebuffer_allocator.h> > #include <libcamera/request.h> > #include <libcamera/stream.h> > @@ -1325,6 +1326,25 @@ int Camera::queueRequest(Request *request) > } > } > > + /* Pre-process AeEnable. */ > + ControlList &controls = request->controls(); > + const auto &aeEnable = controls.get(controls::AeEnable); > + if (aeEnable) { > + if (_d()->controlInfo_.count(controls::AnalogueGainMode.id()) && > + !controls.contains(controls::AnalogueGainMode.id())) { > + controls.set(controls::AnalogueGainMode, > + *aeEnable ? controls::AnalogueGainModeAuto > + : controls::AnalogueGainModeManual); > + } > + > + if (_d()->controlInfo_.count(controls::ExposureTimeMode.id()) && > + !controls.contains(controls::ExposureTimeMode.id())) { > + controls.set(controls::ExposureTimeMode, > + *aeEnable ? controls::ExposureTimeModeAuto > + : controls::ExposureTimeModeManual); > + } > + } > + It feels a bit bad to modify the controls list that was filled by the user. But there is no way around it atm. So Reviewed-by: Stefan Klug <stefan.klug@ideasonboard.com> Cheers, Stefan > d->pipe_->invokeMethod(&PipelineHandler::queueRequest, > ConnectionTypeQueued, request); > > -- > 2.39.2 >
diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp index 69a7ee535..56c585199 100644 --- a/src/libcamera/camera.cpp +++ b/src/libcamera/camera.cpp @@ -19,6 +19,7 @@ #include <libcamera/base/thread.h> #include <libcamera/color_space.h> +#include <libcamera/control_ids.h> #include <libcamera/framebuffer_allocator.h> #include <libcamera/request.h> #include <libcamera/stream.h> @@ -1325,6 +1326,25 @@ int Camera::queueRequest(Request *request) } } + /* Pre-process AeEnable. */ + ControlList &controls = request->controls(); + const auto &aeEnable = controls.get(controls::AeEnable); + if (aeEnable) { + if (_d()->controlInfo_.count(controls::AnalogueGainMode.id()) && + !controls.contains(controls::AnalogueGainMode.id())) { + controls.set(controls::AnalogueGainMode, + *aeEnable ? controls::AnalogueGainModeAuto + : controls::AnalogueGainModeManual); + } + + if (_d()->controlInfo_.count(controls::ExposureTimeMode.id()) && + !controls.contains(controls::ExposureTimeMode.id())) { + controls.set(controls::ExposureTimeMode, + *aeEnable ? controls::ExposureTimeModeAuto + : controls::ExposureTimeModeManual); + } + } + d->pipe_->invokeMethod(&PipelineHandler::queueRequest, ConnectionTypeQueued, request);
Handle the AeEnable under the hood in the Camera class, such that AeEnable activates ExposureTimeMode and AnalogueGain together. This allows applications the convenience of setting auto/manual mode of all of the AE-related controls, as well as protecting applications against a nasty behavior change if an aperture control is added in the future. This also moves common handling code out of the IPA. While we also want to inject AeEnable in Camera::controls() so that IPAs don't have to report it, it is technically difficult at the moment as ControlInfoMaps are not easily modifiable. Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> --- Changes in v7: - check that the camera supports the ae sub-controls before setting them when handling AeEnable New in v6 --- src/libcamera/camera.cpp | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+)