[libcamera-devel,v2,3/5] test: control serialization: Test lookup by ControlId
diff mbox series

Message ID 20210728161116.64489-4-jacopo@jmondi.org
State Superseded
Headers show
Series
  • libcamera: Initialize controls in the IPA
Related show

Commit Message

Jacopo Mondi July 28, 2021, 4:11 p.m. UTC
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(+)

Comments

Paul Elder July 29, 2021, 10:23 a.m. UTC | #1
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
>
Laurent Pinchart Aug. 3, 2021, 4:37 p.m. UTC | #2
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());
Jacopo Mondi Aug. 9, 2021, 2:14 p.m. UTC | #3
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
Laurent Pinchart Aug. 9, 2021, 2:40 p.m. UTC | #4
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());

Patch
diff mbox series

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());