| Message ID | 20210728161116.64489-4-jacopo@jmondi.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Jacopo, On Wed, Jul 28, 2021 at 06:11:14PM +0200, Jacopo Mondi wrote: > Test that lookup by ControlId reference works in the control > serialization test. > > Also make sure that the control limits are not changed by > de-serialization. > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> Reviewed-by: Paul Elder <paul.elder@ideasonboard.com> > --- > test/serialization/control_serialization.cpp | 22 ++++++++++++++++++++ > 1 file changed, 22 insertions(+) > > diff --git a/test/serialization/control_serialization.cpp b/test/serialization/control_serialization.cpp > index e23383d13bd6..45a706d27ba7 100644 > --- a/test/serialization/control_serialization.cpp > +++ b/test/serialization/control_serialization.cpp > @@ -140,6 +140,28 @@ protected: > return TestFail; > } > > + /* Test lookup by ControlId * on the de-serialized info map. */ > + auto newLimitsIter = newInfoMap.find(&controls::Brightness); > + if (newLimitsIter == newInfoMap.end()) { > + cerr << "Lookup by ControlId * failed" << endl; > + return TestFail; > + } > + > + auto initialLimitsIter = infoMap.find(&controls::Brightness); > + if (initialLimitsIter == infoMap.end()) { > + cerr << "Unable to retrieve the original control limits" << endl; > + return TestFail; > + } > + > + /* Make sure control limits looked up by id are not changed. */ > + const ControlInfo &newLimits = newLimitsIter->second; > + const ControlInfo &initialLimits = initialLimitsIter->second; > + if (newLimits.min().get<float>() != initialLimits.min().get<float>() || > + newLimits.max().get<float>() != initialLimits.max().get<float>()) { > + cerr << "The brightness control limits have changed" << endl; > + return TestFail; > + } > + > /* Deserialize the control list and verify the contents. */ > buffer = ByteStreamBuffer(const_cast<const uint8_t *>(listData.data()), > listData.size()); > -- > 2.32.0 >
Hi Jacopo, Thank you for the patch. On Wed, Jul 28, 2021 at 06:11:14PM +0200, Jacopo Mondi wrote: > Test that lookup by ControlId reference works in the control > serialization test. > > Also make sure that the control limits are not changed by > de-serialization. > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > --- > test/serialization/control_serialization.cpp | 22 ++++++++++++++++++++ > 1 file changed, 22 insertions(+) > > diff --git a/test/serialization/control_serialization.cpp b/test/serialization/control_serialization.cpp > index e23383d13bd6..45a706d27ba7 100644 > --- a/test/serialization/control_serialization.cpp > +++ b/test/serialization/control_serialization.cpp > @@ -140,6 +140,28 @@ protected: > return TestFail; > } > > + /* Test lookup by ControlId * on the de-serialized info map. */ > + auto newLimitsIter = newInfoMap.find(&controls::Brightness); > + if (newLimitsIter == newInfoMap.end()) { > + cerr << "Lookup by ControlId * failed" << endl; > + return TestFail; > + } > + > + auto initialLimitsIter = infoMap.find(&controls::Brightness); > + if (initialLimitsIter == infoMap.end()) { > + cerr << "Unable to retrieve the original control limits" << endl; > + return TestFail; > + } I think you can drop the check here, as this test is about serialization, not about testing basic usage of ControlInfoMap. You can thus write const ControlInfo &initialLimits = infoMap[&controls::Brightness]; below and drop initialLimitsIter. > + > + /* Make sure control limits looked up by id are not changed. */ > + const ControlInfo &newLimits = newLimitsIter->second; > + const ControlInfo &initialLimits = initialLimitsIter->second; > + if (newLimits.min().get<float>() != initialLimits.min().get<float>() || > + newLimits.max().get<float>() != initialLimits.max().get<float>()) { You could write if (newLimits.min() != initialLimits.min() || newLimits.max()) != initialLimits.max()) { Could you also move this patch to the beginning of the series to make sure the test fails ? Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > + cerr << "The brightness control limits have changed" << endl; > + return TestFail; > + } > + > /* Deserialize the control list and verify the contents. */ > buffer = ByteStreamBuffer(const_cast<const uint8_t *>(listData.data()), > listData.size());
Hi Laurent, On Tue, Aug 03, 2021 at 07:37:50PM +0300, Laurent Pinchart wrote: > Hi Jacopo, > > Thank you for the patch. > > On Wed, Jul 28, 2021 at 06:11:14PM +0200, Jacopo Mondi wrote: > > Test that lookup by ControlId reference works in the control > > serialization test. > > > > Also make sure that the control limits are not changed by > > de-serialization. > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > --- > > test/serialization/control_serialization.cpp | 22 ++++++++++++++++++++ > > 1 file changed, 22 insertions(+) > > > > diff --git a/test/serialization/control_serialization.cpp b/test/serialization/control_serialization.cpp > > index e23383d13bd6..45a706d27ba7 100644 > > --- a/test/serialization/control_serialization.cpp > > +++ b/test/serialization/control_serialization.cpp > > @@ -140,6 +140,28 @@ protected: > > return TestFail; > > } > > > > + /* Test lookup by ControlId * on the de-serialized info map. */ > > + auto newLimitsIter = newInfoMap.find(&controls::Brightness); > > + if (newLimitsIter == newInfoMap.end()) { > > + cerr << "Lookup by ControlId * failed" << endl; > > + return TestFail; > > + } > > + > > + auto initialLimitsIter = infoMap.find(&controls::Brightness); > > + if (initialLimitsIter == infoMap.end()) { > > + cerr << "Unable to retrieve the original control limits" << endl; > > + return TestFail; > > + } > > I think you can drop the check here, as this test is about > serialization, not about testing basic usage of ControlInfoMap. You can > thus write > > const ControlInfo &initialLimits = infoMap[&controls::Brightness]; No I can't, we do not expose std::unordered_map::operator[] in ControlInfoMap as it would allow to modify the ControlInfoMap. So I have to go through at(). I wonder if all the emphasis on making ControlInfoMap un-mutable once constructed is still justified. The next item on my list is to update the ControlInfo limits in response to a camera configuration, and we can only do so by overwriting the full ControlInfoMap because of that... > > below and drop initialLimitsIter. > > > + > > + /* Make sure control limits looked up by id are not changed. */ > > + const ControlInfo &newLimits = newLimitsIter->second; > > + const ControlInfo &initialLimits = initialLimitsIter->second; > > + if (newLimits.min().get<float>() != initialLimits.min().get<float>() || > > + newLimits.max().get<float>() != initialLimits.max().get<float>()) { > > You could write > > if (newLimits.min() != initialLimits.min() || > newLimits.max()) != initialLimits.max()) { > > Could you also move this patch to the beginning of the series to make > sure the test fails ? Sure > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Thanks j > > > + cerr << "The brightness control limits have changed" << endl; > > + return TestFail; > > + } > > + > > /* Deserialize the control list and verify the contents. */ > > buffer = ByteStreamBuffer(const_cast<const uint8_t *>(listData.data()), > > listData.size()); > > -- > Regards, > > Laurent Pinchart
Hi Jacopo, On Mon, Aug 09, 2021 at 04:14:19PM +0200, Jacopo Mondi wrote: > On Tue, Aug 03, 2021 at 07:37:50PM +0300, Laurent Pinchart wrote: > > On Wed, Jul 28, 2021 at 06:11:14PM +0200, Jacopo Mondi wrote: > > > Test that lookup by ControlId reference works in the control > > > serialization test. > > > > > > Also make sure that the control limits are not changed by > > > de-serialization. > > > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > > --- > > > test/serialization/control_serialization.cpp | 22 ++++++++++++++++++++ > > > 1 file changed, 22 insertions(+) > > > > > > diff --git a/test/serialization/control_serialization.cpp b/test/serialization/control_serialization.cpp > > > index e23383d13bd6..45a706d27ba7 100644 > > > --- a/test/serialization/control_serialization.cpp > > > +++ b/test/serialization/control_serialization.cpp > > > @@ -140,6 +140,28 @@ protected: > > > return TestFail; > > > } > > > > > > + /* Test lookup by ControlId * on the de-serialized info map. */ > > > + auto newLimitsIter = newInfoMap.find(&controls::Brightness); > > > + if (newLimitsIter == newInfoMap.end()) { > > > + cerr << "Lookup by ControlId * failed" << endl; > > > + return TestFail; > > > + } > > > + > > > + auto initialLimitsIter = infoMap.find(&controls::Brightness); > > > + if (initialLimitsIter == infoMap.end()) { > > > + cerr << "Unable to retrieve the original control limits" << endl; > > > + return TestFail; > > > + } > > > > I think you can drop the check here, as this test is about > > serialization, not about testing basic usage of ControlInfoMap. You can > > thus write > > > > const ControlInfo &initialLimits = infoMap[&controls::Brightness]; > > No I can't, we do not expose std::unordered_map::operator[] in > ControlInfoMap as it would allow to modify the ControlInfoMap. So I > have to go through at(). Oops indeed. > I wonder if all the emphasis on making ControlInfoMap un-mutable once > constructed is still justified. The next item on my list is to update > the ControlInfo limits in response to a camera configuration, and we can only > do so by overwriting the full ControlInfoMap because of that... That's a good question. It should certainly be read-only from an application point of view, but that's enforced by returning a const reference from Camera::controls(). It would also be nice if the API could lower the risk of making modifications by mistake in pipeline handlers, but there we'll need to find a middle-ground anyway, as modifications are needed. I believe we'll revisit this in any case, given that the updates to Camera::controls() that we're doing now if a bit of a hack and needs to be better designed. > > below and drop initialLimitsIter. > > > > > + > > > + /* Make sure control limits looked up by id are not changed. */ > > > + const ControlInfo &newLimits = newLimitsIter->second; > > > + const ControlInfo &initialLimits = initialLimitsIter->second; > > > + if (newLimits.min().get<float>() != initialLimits.min().get<float>() || > > > + newLimits.max().get<float>() != initialLimits.max().get<float>()) { > > > > You could write > > > > if (newLimits.min() != initialLimits.min() || > > newLimits.max()) != initialLimits.max()) { > > > > Could you also move this patch to the beginning of the series to make > > sure the test fails ? > > Sure > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > Thanks > > > > + cerr << "The brightness control limits have changed" << endl; > > > + return TestFail; > > > + } > > > + > > > /* Deserialize the control list and verify the contents. */ > > > buffer = ByteStreamBuffer(const_cast<const uint8_t *>(listData.data()), > > > listData.size());
diff --git a/test/serialization/control_serialization.cpp b/test/serialization/control_serialization.cpp index e23383d13bd6..45a706d27ba7 100644 --- a/test/serialization/control_serialization.cpp +++ b/test/serialization/control_serialization.cpp @@ -140,6 +140,28 @@ protected: return TestFail; } + /* Test lookup by ControlId * on the de-serialized info map. */ + auto newLimitsIter = newInfoMap.find(&controls::Brightness); + if (newLimitsIter == newInfoMap.end()) { + cerr << "Lookup by ControlId * failed" << endl; + return TestFail; + } + + auto initialLimitsIter = infoMap.find(&controls::Brightness); + if (initialLimitsIter == infoMap.end()) { + cerr << "Unable to retrieve the original control limits" << endl; + return TestFail; + } + + /* Make sure control limits looked up by id are not changed. */ + const ControlInfo &newLimits = newLimitsIter->second; + const ControlInfo &initialLimits = initialLimitsIter->second; + if (newLimits.min().get<float>() != initialLimits.min().get<float>() || + newLimits.max().get<float>() != initialLimits.max().get<float>()) { + cerr << "The brightness control limits have changed" << endl; + return TestFail; + } + /* Deserialize the control list and verify the contents. */ buffer = ByteStreamBuffer(const_cast<const uint8_t *>(listData.data()), listData.size());
Test that lookup by ControlId reference works in the control serialization test. Also make sure that the control limits are not changed by de-serialization. Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> --- test/serialization/control_serialization.cpp | 22 ++++++++++++++++++++ 1 file changed, 22 insertions(+)