| Message ID | 20250521125327.6378-2-david.plowman@raspberrypi.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Quoting David Plowman (2025-05-21 14:53:27) > In Camera::queueRequest() the control list is updated transparently by > converting AeEnable into ExposureTimeMode and AnalogueGainMode > controls. > > However, this was not happening during Camera::start(), meaning that > setting AeEnable there was having no effect. It should behave the same > here too. > > Fixes: 7abd4139051c ("libcamera: camera: Pre-process AeEnable control") > Signed-off-by: David Plowman <david.plowman@raspberrypi.com> Looks good to me. Reviewed-by: Paul Elder <paul.elder@ideasonboard.com> > --- > include/libcamera/camera.h | 2 ++ > src/libcamera/camera.cpp | 57 +++++++++++++++++++++++++------------- > 2 files changed, 40 insertions(+), 19 deletions(-) > > diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h > index 94cee7bd..b24a2974 100644 > --- a/include/libcamera/camera.h > +++ b/include/libcamera/camera.h > @@ -165,6 +165,8 @@ private: > friend class FrameBufferAllocator; > int exportFrameBuffers(Stream *stream, > std::vector<std::unique_ptr<FrameBuffer>> *buffers); > + > + void patchControlList(ControlList &controls); > }; > > } /* namespace libcamera */ > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp > index c180a5fd..23b9207a 100644 > --- a/src/libcamera/camera.cpp > +++ b/src/libcamera/camera.cpp > @@ -1266,6 +1266,33 @@ std::unique_ptr<Request> Camera::createRequest(uint64_t cookie) > return request; > } > > +/** > + * \brief Patch a control list that contains the AeEnable control > + * \param[inout] controls The control list to be patched > + * > + * The control list is patched in place, turning the AeEnable control into > + * the equivalent ExposureTimeMode/AnalogueGainMode controls. > + */ > +void Camera::patchControlList(ControlList &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); > + } > + } > +} > + > /** > * \brief Queue a request to the camera > * \param[in] request The request to queue to the camera > @@ -1329,23 +1356,7 @@ 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); > - } > - } > + patchControlList(request->controls()); > > d->pipe_->invokeMethod(&PipelineHandler::queueRequest, > ConnectionTypeQueued, request); > @@ -1383,8 +1394,16 @@ int Camera::start(const ControlList *controls) > > ASSERT(d->requestSequence_ == 0); > > - ret = d->pipe_->invokeMethod(&PipelineHandler::start, > - ConnectionTypeBlocking, this, controls); > + if (controls) { > + ControlList copy(*controls); > + patchControlList(copy); > + ret = d->pipe_->invokeMethod(&PipelineHandler::start, > + ConnectionTypeBlocking, this, ©); > + } > + else > + ret = d->pipe_->invokeMethod(&PipelineHandler::start, > + ConnectionTypeBlocking, this, nullptr); > + > if (ret) > return ret; > > -- > 2.39.5 >
Hi 2025. 05. 21. 14:53 keltezéssel, David Plowman írta: > In Camera::queueRequest() the control list is updated transparently by > converting AeEnable into ExposureTimeMode and AnalogueGainMode > controls. > > However, this was not happening during Camera::start(), meaning that > setting AeEnable there was having no effect. It should behave the same > here too. > > Fixes: 7abd4139051c ("libcamera: camera: Pre-process AeEnable control") > Signed-off-by: David Plowman <david.plowman@raspberrypi.com> > --- > include/libcamera/camera.h | 2 ++ > src/libcamera/camera.cpp | 57 +++++++++++++++++++++++++------------- > 2 files changed, 40 insertions(+), 19 deletions(-) > > diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h > index 94cee7bd..b24a2974 100644 > --- a/include/libcamera/camera.h > +++ b/include/libcamera/camera.h > @@ -165,6 +165,8 @@ private: > friend class FrameBufferAllocator; > int exportFrameBuffers(Stream *stream, > std::vector<std::unique_ptr<FrameBuffer>> *buffers); > + > + void patchControlList(ControlList &controls); Maybe it's just me, but my preference is usually to hide such internal functions completely, i.e. in an anonymous namespace in the source file it is used. > }; > > } /* namespace libcamera */ > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp > index c180a5fd..23b9207a 100644 > --- a/src/libcamera/camera.cpp > +++ b/src/libcamera/camera.cpp > @@ -1266,6 +1266,33 @@ std::unique_ptr<Request> Camera::createRequest(uint64_t cookie) > return request; > } > > +/** > + * \brief Patch a control list that contains the AeEnable control > + * \param[inout] controls The control list to be patched > + * > + * The control list is patched in place, turning the AeEnable control into > + * the equivalent ExposureTimeMode/AnalogueGainMode controls. > + */ > +void Camera::patchControlList(ControlList &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); > + } > + } > +} > + > /** > * \brief Queue a request to the camera > * \param[in] request The request to queue to the camera > @@ -1329,23 +1356,7 @@ 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); > - } > - } > + patchControlList(request->controls()); > > d->pipe_->invokeMethod(&PipelineHandler::queueRequest, > ConnectionTypeQueued, request); > @@ -1383,8 +1394,16 @@ int Camera::start(const ControlList *controls) > > ASSERT(d->requestSequence_ == 0); > > - ret = d->pipe_->invokeMethod(&PipelineHandler::start, > - ConnectionTypeBlocking, this, controls); > + if (controls) { > + ControlList copy(*controls); I would like to present an alternative approach that generates a separate "patch" `ControlList` and then applies it to the main one: diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp index 1075474ba..520d72184 100644 --- a/src/libcamera/camera.cpp +++ b/src/libcamera/camera.cpp @@ -1324,6 +1335,38 @@ std::unique_ptr<Request> Camera::createRequest(uint64_t cookie) return request; } +namespace { + +[[nodiscard]] +ControlList patchControlList(const ControlInfoMap &controlInfo, const ControlList *controls) +{ + ControlList patch; + + if (!controls) + return patch; + + const auto &aeEnable = controls->get(controls::AeEnable); + if (aeEnable) { + if (controlInfo.count(controls::AnalogueGainMode.id()) && + !controls->contains(controls::AnalogueGainMode.id())) { + patch.set(controls::AnalogueGainMode, + *aeEnable ? controls::AnalogueGainModeAuto + : controls::AnalogueGainModeManual); + } + + if (controlInfo.count(controls::ExposureTimeMode.id()) && + !controls->contains(controls::ExposureTimeMode.id())) { + patch.set(controls::ExposureTimeMode, + *aeEnable ? controls::ExposureTimeModeAuto + : controls::ExposureTimeModeManual); + } + } + + return patch; +} + +} /* namespace */ + /** * \brief Queue a request to the camera * \param[in] request The request to queue to the camera @@ -1387,23 +1430,7 @@ 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); - } - } + request->controls().merge(patchControlList(_d()->controlInfo_, &request->controls())); d->pipe_->invokeMethod(&PipelineHandler::queueRequest, ConnectionTypeQueued, request); @@ -1441,8 +1468,16 @@ int Camera::start(const ControlList *controls) ASSERT(d->requestSequence_ == 0); - ret = d->pipe_->invokeMethod(&PipelineHandler::start, - ConnectionTypeBlocking, this, controls); + auto patch = patchControlList(d->controlInfo_, controls); + if (!patch.empty()) { + patch.merge(*controls); + ret = d->pipe_->invokeMethod(&PipelineHandler::start, + ConnectionTypeBlocking, this, &patch); + } else { + ret = d->pipe_->invokeMethod(&PipelineHandler::start, + ConnectionTypeBlocking, this, controls); + } + if (ret) return ret; The above also needs an rvalue overload for ControlList::merge, which can be implemented as follows: diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp index 1a618801a..7d9778217 100644 --- a/src/libcamera/controls.cpp +++ b/src/libcamera/controls.cpp @@ -1192,6 +1224,45 @@ void ControlList::merge(const ControlList &source, MergePolicy policy) } } +/** + * \brief Merge the \a source into the ControlList + * \param[in] source The ControlList to merge into this object + * \param[in] policy Controls if existing elements in *this shall be + * overwritten + * + * Merging two control lists moves elements from the \a source and inserts + * them in *this. If the \a source contains elements whose key is already + * present in *this, then those elements are only overwritten if + * \a policy is MergePolicy::OverwriteExisting. + * + * Only control lists created from the same ControlIdMap or ControlInfoMap may + * be merged. Attempting to do otherwise results in undefined behaviour. + */ +void ControlList::merge(ControlList &&source, MergePolicy policy) +{ + /** + * \todo ASSERT that the current and source ControlList are derived + * from a compatible ControlIdMap, to prevent undefined behaviour due to + * id collisions. + * + * This can not currently be a direct pointer comparison due to the + * duplication of the ControlIdMaps in the isolated IPA use cases. + * Furthermore, manually checking each entry of the id map is identical + * is expensive. + * See https://bugs.libcamera.org/show_bug.cgi?id=31 for further details + */ + + switch (policy) { + case MergePolicy::KeepExisting: + controls_.merge(source.controls_); + break; + case MergePolicy::OverwriteExisting: + source.controls_.merge(controls_); + controls_.swap(source.controls_); + break; + } +} + The slight advantage of the above is that it only makes a copy if it is absolutely necessary. I don't know if anything better can be done with the current design (i.e. `Camera::start(const ControlList *)`) wrt. copying. In any case, since the extra copy is only present in `Camera::start()`, and only if a control list is supplied (although recently pipewire has started supplying an initial control list), so I don't think that it is a too significant issue. Reviewed-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> Regards, Barnabás Pőcze > + patchControlList(copy); > + ret = d->pipe_->invokeMethod(&PipelineHandler::start, > + ConnectionTypeBlocking, this, ©); > + } > + else > + ret = d->pipe_->invokeMethod(&PipelineHandler::start, > + ConnectionTypeBlocking, this, nullptr); > + > if (ret) > return ret; >
Quoting Barnabás Pőcze (2025-07-10 12:02:48) > Hi > > 2025. 05. 21. 14:53 keltezéssel, David Plowman írta: > > In Camera::queueRequest() the control list is updated transparently by > > converting AeEnable into ExposureTimeMode and AnalogueGainMode > > controls. > > > > However, this was not happening during Camera::start(), meaning that > > setting AeEnable there was having no effect. It should behave the same > > here too. > > > > Fixes: 7abd4139051c ("libcamera: camera: Pre-process AeEnable control") > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com> > > --- > > include/libcamera/camera.h | 2 ++ > > src/libcamera/camera.cpp | 57 +++++++++++++++++++++++++------------- > > 2 files changed, 40 insertions(+), 19 deletions(-) > > > > diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h > > index 94cee7bd..b24a2974 100644 > > --- a/include/libcamera/camera.h > > +++ b/include/libcamera/camera.h > > @@ -165,6 +165,8 @@ private: > > friend class FrameBufferAllocator; > > int exportFrameBuffers(Stream *stream, > > std::vector<std::unique_ptr<FrameBuffer>> *buffers); > > + > > + void patchControlList(ControlList &controls); > > Maybe it's just me, but my preference is usually to hide such internal functions > completely, i.e. in an anonymous namespace in the source file it is used. > > > > }; > > > > } /* namespace libcamera */ > > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp > > index c180a5fd..23b9207a 100644 > > --- a/src/libcamera/camera.cpp > > +++ b/src/libcamera/camera.cpp > > @@ -1266,6 +1266,33 @@ std::unique_ptr<Request> Camera::createRequest(uint64_t cookie) > > return request; > > } > > > > +/** > > + * \brief Patch a control list that contains the AeEnable control > > + * \param[inout] controls The control list to be patched > > + * > > + * The control list is patched in place, turning the AeEnable control into > > + * the equivalent ExposureTimeMode/AnalogueGainMode controls. > > + */ > > +void Camera::patchControlList(ControlList &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); > > + } > > + } > > +} > > + > > /** > > * \brief Queue a request to the camera > > * \param[in] request The request to queue to the camera > > @@ -1329,23 +1356,7 @@ 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); > > - } > > - } > > + patchControlList(request->controls()); > > > > d->pipe_->invokeMethod(&PipelineHandler::queueRequest, > > ConnectionTypeQueued, request); > > @@ -1383,8 +1394,16 @@ int Camera::start(const ControlList *controls) > > > > ASSERT(d->requestSequence_ == 0); > > > > - ret = d->pipe_->invokeMethod(&PipelineHandler::start, > > - ConnectionTypeBlocking, this, controls); > > + if (controls) { > > + ControlList copy(*controls); > > > I would like to present an alternative approach that generates a separate > "patch" `ControlList` and then applies it to the main one: > > > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp > index 1075474ba..520d72184 100644 > --- a/src/libcamera/camera.cpp > +++ b/src/libcamera/camera.cpp > @@ -1324,6 +1335,38 @@ std::unique_ptr<Request> Camera::createRequest(uint64_t cookie) > return request; > } > > +namespace { > + > +[[nodiscard]] > +ControlList patchControlList(const ControlInfoMap &controlInfo, const ControlList *controls) > +{ > + ControlList patch; > + > + if (!controls) > + return patch; > + > + const auto &aeEnable = controls->get(controls::AeEnable); > + if (aeEnable) { > + if (controlInfo.count(controls::AnalogueGainMode.id()) && > + !controls->contains(controls::AnalogueGainMode.id())) { > + patch.set(controls::AnalogueGainMode, > + *aeEnable ? controls::AnalogueGainModeAuto > + : controls::AnalogueGainModeManual); > + } > + > + if (controlInfo.count(controls::ExposureTimeMode.id()) && > + !controls->contains(controls::ExposureTimeMode.id())) { > + patch.set(controls::ExposureTimeMode, > + *aeEnable ? controls::ExposureTimeModeAuto > + : controls::ExposureTimeModeManual); > + } > + } > + > + return patch; > +} > + > +} /* namespace */ > + > /** > * \brief Queue a request to the camera > * \param[in] request The request to queue to the camera > @@ -1387,23 +1430,7 @@ 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); > - } > - } > + request->controls().merge(patchControlList(_d()->controlInfo_, &request->controls())); > > d->pipe_->invokeMethod(&PipelineHandler::queueRequest, > ConnectionTypeQueued, request); > @@ -1441,8 +1468,16 @@ int Camera::start(const ControlList *controls) > > ASSERT(d->requestSequence_ == 0); > > - ret = d->pipe_->invokeMethod(&PipelineHandler::start, > - ConnectionTypeBlocking, this, controls); > + auto patch = patchControlList(d->controlInfo_, controls); > + if (!patch.empty()) { > + patch.merge(*controls); > + ret = d->pipe_->invokeMethod(&PipelineHandler::start, > + ConnectionTypeBlocking, this, &patch); > + } else { > + ret = d->pipe_->invokeMethod(&PipelineHandler::start, > + ConnectionTypeBlocking, this, controls); > + } > + > if (ret) > return ret; > > > The above also needs an rvalue overload for ControlList::merge, which can be implemented > as follows: > > > diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp > index 1a618801a..7d9778217 100644 > --- a/src/libcamera/controls.cpp > +++ b/src/libcamera/controls.cpp > @@ -1192,6 +1224,45 @@ void ControlList::merge(const ControlList &source, MergePolicy policy) > } > } > > +/** > + * \brief Merge the \a source into the ControlList > + * \param[in] source The ControlList to merge into this object > + * \param[in] policy Controls if existing elements in *this shall be > + * overwritten > + * > + * Merging two control lists moves elements from the \a source and inserts > + * them in *this. If the \a source contains elements whose key is already > + * present in *this, then those elements are only overwritten if > + * \a policy is MergePolicy::OverwriteExisting. > + * > + * Only control lists created from the same ControlIdMap or ControlInfoMap may > + * be merged. Attempting to do otherwise results in undefined behaviour. > + */ > +void ControlList::merge(ControlList &&source, MergePolicy policy) > +{ > + /** > + * \todo ASSERT that the current and source ControlList are derived > + * from a compatible ControlIdMap, to prevent undefined behaviour due to > + * id collisions. > + * > + * This can not currently be a direct pointer comparison due to the > + * duplication of the ControlIdMaps in the isolated IPA use cases. > + * Furthermore, manually checking each entry of the id map is identical > + * is expensive. > + * See https://bugs.libcamera.org/show_bug.cgi?id=31 for further details > + */ > + > + switch (policy) { > + case MergePolicy::KeepExisting: > + controls_.merge(source.controls_); > + break; > + case MergePolicy::OverwriteExisting: > + source.controls_.merge(controls_); > + controls_.swap(source.controls_); > + break; > + } > +} > + > > > The slight advantage of the above is that it only makes a copy if it is absolutely > necessary. I don't know if anything better can be done with the current design > (i.e. `Camera::start(const ControlList *)`) wrt. copying. > > In any case, since the extra copy is only present in `Camera::start()`, and only > if a control list is supplied (although recently pipewire has started supplying > an initial control list), so I don't think that it is a too significant issue. > > > Reviewed-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> Given this is a fix - I'd like to get it in before the next release - and this already passes CI. If there are improvement that could be done on top I think here. Especially if they require implementing new merge functions. > > > Regards, > Barnabás Pőcze > > > + patchControlList(copy); > > + ret = d->pipe_->invokeMethod(&PipelineHandler::start, > > + ConnectionTypeBlocking, this, ©); > > + } > > + else > > + ret = d->pipe_->invokeMethod(&PipelineHandler::start, > > + ConnectionTypeBlocking, this, nullptr); > > + I'll wrap this else statement with matching { } to make it clear it's tied to the above if though. -- Kieran > > if (ret) > > return ret; > > >
diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h index 94cee7bd..b24a2974 100644 --- a/include/libcamera/camera.h +++ b/include/libcamera/camera.h @@ -165,6 +165,8 @@ private: friend class FrameBufferAllocator; int exportFrameBuffers(Stream *stream, std::vector<std::unique_ptr<FrameBuffer>> *buffers); + + void patchControlList(ControlList &controls); }; } /* namespace libcamera */ diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp index c180a5fd..23b9207a 100644 --- a/src/libcamera/camera.cpp +++ b/src/libcamera/camera.cpp @@ -1266,6 +1266,33 @@ std::unique_ptr<Request> Camera::createRequest(uint64_t cookie) return request; } +/** + * \brief Patch a control list that contains the AeEnable control + * \param[inout] controls The control list to be patched + * + * The control list is patched in place, turning the AeEnable control into + * the equivalent ExposureTimeMode/AnalogueGainMode controls. + */ +void Camera::patchControlList(ControlList &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); + } + } +} + /** * \brief Queue a request to the camera * \param[in] request The request to queue to the camera @@ -1329,23 +1356,7 @@ 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); - } - } + patchControlList(request->controls()); d->pipe_->invokeMethod(&PipelineHandler::queueRequest, ConnectionTypeQueued, request); @@ -1383,8 +1394,16 @@ int Camera::start(const ControlList *controls) ASSERT(d->requestSequence_ == 0); - ret = d->pipe_->invokeMethod(&PipelineHandler::start, - ConnectionTypeBlocking, this, controls); + if (controls) { + ControlList copy(*controls); + patchControlList(copy); + ret = d->pipe_->invokeMethod(&PipelineHandler::start, + ConnectionTypeBlocking, this, ©); + } + else + ret = d->pipe_->invokeMethod(&PipelineHandler::start, + ConnectionTypeBlocking, this, nullptr); + if (ret) return ret;
In Camera::queueRequest() the control list is updated transparently by converting AeEnable into ExposureTimeMode and AnalogueGainMode controls. However, this was not happening during Camera::start(), meaning that setting AeEnable there was having no effect. It should behave the same here too. Fixes: 7abd4139051c ("libcamera: camera: Pre-process AeEnable control") Signed-off-by: David Plowman <david.plowman@raspberrypi.com> --- include/libcamera/camera.h | 2 ++ src/libcamera/camera.cpp | 57 +++++++++++++++++++++++++------------- 2 files changed, 40 insertions(+), 19 deletions(-)