[libcamera-devel] [PATCH 1/2] libtest: camera_test: Plumb constructor to set LIBCAMERA_IPA_FORCE_ISOLATION
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Tue Aug 17 14:53:53 CEST 2021
Hello,
On Tue, Aug 17, 2021 at 01:44:50PM +0100, Kieran Bingham wrote:
> On 17/08/2021 13:29, Kieran Bingham wrote:
> > On 17/08/2021 13:26, Umang Jain wrote:
> >> Some tests might require to have LIBCAMERA_IPA_FORCE_ISOLATION set
> >> to ensure they can test the IPA running in isolated mode. These tests
> >> are likely to leverage CameraTest. The environment variable should
> >> be set before CameraManager::start() call which happens in CameraTest's
> >> constructor. Hence, plumb the constructor with a flag so that the
> >> LIBCAMERA_IPA_FORCE_ISOLATION can be set before CameraManager::start().
> >>
> >> Signed-off-by: Umang Jain <umang.jain at ideasonboard.com>
> >> ---
> >> test/libtest/camera_test.cpp | 5 ++++-
> >> test/libtest/camera_test.h | 2 +-
> >> 2 files changed, 5 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/test/libtest/camera_test.cpp b/test/libtest/camera_test.cpp
> >> index 2ae4d677..9cbc4d82 100644
> >> --- a/test/libtest/camera_test.cpp
> >> +++ b/test/libtest/camera_test.cpp
> >> @@ -13,10 +13,13 @@
> >> using namespace libcamera;
> >> using namespace std;
> >>
> >> -CameraTest::CameraTest(const char *name)
> >> +CameraTest::CameraTest(const char *name, bool isolate)
>
> Reading this where it is used, I wonder if this flag should be an enum,
> but it's not essential - and more a question for debate.
>
> i.e.
> CameraTest(kCamId_, ISOLATED_IPA)
>
>
> might be clearer than
>
> CameraTest(kCamId_, true);
>
>
> Though the ISOLATED_IPA name is also debatable, and might end up being
> flags? But if it's only the one flag for now ... that's overkill anyway.
Bools are harmful at they're not very explicit when used as flags. If
this was a public API I'd propose fixing it already, but for an internal
API my standards are a bit lower.
If we want an enum, the enumerator should be named in CamelCase, not
SNAKE_CASE.
> >> {
> >> cm_ = new CameraManager();
> >>
> >> + if (isolate)
> >> + setenv("LIBCAMERA_IPA_FORCE_ISOLATION", "TRUE", 1);
The example in the documentation sets the value to "1", not "TRUE", so
I'd do the same here.
Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> >> +
> >
> > Ahh nice. Good so we can now isolate IPA's from tests!
> >
> > Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> >
> >> if (cm_->start()) {
> >> cerr << "Failed to start camera manager" << endl;
> >> status_ = TestFail;
> >> diff --git a/test/libtest/camera_test.h b/test/libtest/camera_test.h
> >> index 7939798f..f56e343e 100644
> >> --- a/test/libtest/camera_test.h
> >> +++ b/test/libtest/camera_test.h
> >> @@ -17,7 +17,7 @@ using namespace libcamera;
> >> class CameraTest
> >> {
> >> public:
> >> - CameraTest(const char *name);
> >> + CameraTest(const char *name, bool isolate = false);
> >> ~CameraTest();
> >>
> >> protected:
> >>
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list