| Message ID | 20210507124444.1089347-1-kieran.bingham@ideasonboard.com |
|---|---|
| State | Accepted |
| Commit | 414babb60b5453bf2a2d206088c0b4a1b48da15e |
| Headers | show |
| Series |
|
| Related | show |
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@ideasonboard.com> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > 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)) {
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@ideasonboard.com> > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> With either Bug: or Bugzilla: (with a slight preference for the first) Reviewed-by: Jacopo Mondi <jacopo@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@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Jacopo, On 08/05/2021 07:32, Jacopo Mondi wrote: > Hi Kieran, > thanks for handling this and sorry for introducing it in first > place No problem, > 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@ideasonboard.com> >> >> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > With either Bug: or Bugzilla: (with a slight preference for the first) > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> But I'm afraid I had already merged it... -- Kieran > > 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@lists.libcamera.org >> https://lists.libcamera.org/listinfo/libcamera-devel
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)) {
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. 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 Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> --- src/libcamera/controls.cpp | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-)