[libcamera-devel] [PATCH v2 3/5] test: control serialization: Test lookup by ControlId
Jacopo Mondi
jacopo at jmondi.org
Mon Aug 9 16:14:19 CEST 2021
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 at 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 at 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
More information about the libcamera-devel
mailing list