[1/1] libcamera: camera: Fix up the AeEnable control during Camera::start()
diff mbox series

Message ID 20250521125327.6378-2-david.plowman@raspberrypi.com
State Accepted
Headers show
Series
  • Fix AeEnable when camera starts
Related show

Commit Message

David Plowman May 21, 2025, 12:53 p.m. UTC
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(-)

Comments

Paul Elder May 21, 2025, 5:48 p.m. UTC | #1
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, &copy);
> +       }
> +       else
> +               ret = d->pipe_->invokeMethod(&PipelineHandler::start,
> +                                            ConnectionTypeBlocking, this, nullptr);
> +
>         if (ret)
>                 return ret;
>  
> -- 
> 2.39.5
>
Barnabás Pőcze July 10, 2025, 11:02 a.m. UTC | #2
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, &copy);
> +	}
> +	else
> +		ret = d->pipe_->invokeMethod(&PipelineHandler::start,
> +					     ConnectionTypeBlocking, this, nullptr);
> +
>   	if (ret)
>   		return ret;
>
Kieran Bingham July 21, 2025, 7:07 p.m. UTC | #3
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, &copy);
> > +     }
> > +     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;
> >   
>

Patch
diff mbox series

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, &copy);
+	}
+	else
+		ret = d->pipe_->invokeMethod(&PipelineHandler::start,
+					     ConnectionTypeBlocking, this, nullptr);
+
 	if (ret)
 		return ret;