[libcamera-devel] [PATCH] libcamera: controls: Remove merge assertion

[Jacopo Mondi]

Hi Kieran,
   thanks for handling this and sorry for introducing it in first
   place

On Fri, May 07, 2021 at 07:11:50PM +0300, Laurent Pinchart wrote:
> Hi Kieran,
>
> Thank you for the patch.
>
> On Fri, May 07, 2021 at 01:44:44PM +0100, Kieran Bingham wrote:
> > The ControlList merge operation is protected with an ASSERT to guarantee
> > that the two lists are compatible.
> >
> > Unfortunately this assertion fails when we run IPAs in an isolated case
> > as while the lists are compatible, the isolated IPA has a unique
> > instance of the id map. This breaks the pointer comparison, and the
> > assertion fails with a false positive.
>
> Paul, is this caused by the deserializer using a deserialized idmap
> instead of a cached pointer to an idmap previously serialized ?
>
> > Remove the assertion, leaving only a todo in it's place as this breaks
> > active users of the library.
> >
> > Bugzilla: https://bugs.libcamera.org/show_bug.cgi?id=31
>
> How about "Bug:" to not make it depend on any particular tool ?
>
> > Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

With either Bug: or Bugzilla: (with a slight preference for the first)
Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>

Thanks
   j


>
> > ---
> >  src/libcamera/controls.cpp | 12 +++++++++++-
> >  1 file changed, 11 insertions(+), 1 deletion(-)
> >
> > diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp
> > index b763148d4391..5aef4e7145bd 100644
> > --- a/src/libcamera/controls.cpp
> > +++ b/src/libcamera/controls.cpp
> > @@ -890,7 +890,17 @@ ControlList::ControlList(const ControlInfoMap &infoMap, ControlValidator *valida
> >   */
> >  void ControlList::merge(const ControlList &source)
> >  {
> > -	ASSERT(idmap_ == source.idmap_);
> > +	/**
> > +	 * \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
> > +	 */
> >
> >  	for (const auto &ctrl : source) {
> >  		if (contains(ctrl.first)) {
>
> --
> Regards,
>
> Laurent Pinchart
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel