[libcamera-devel,1/2] libtest: camera_test: Plumb constructor to set LIBCAMERA_IPA_FORCE_ISOLATION
diff mbox series

Message ID 20210817122646.185978-2-umang.jain@ideasonboard.com
State Superseded
Delegated to: Umang Jain
Headers show
Series
  • Fd leak test
Related show

Commit Message

Umang Jain Aug. 17, 2021, 12:26 p.m. UTC 
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@ideasonboard.com>
---
 test/libtest/camera_test.cpp | 5 ++++-
 test/libtest/camera_test.h   | 2 +-
 2 files changed, 5 insertions(+), 2 deletions(-)

Comments

Kieran Bingham Aug. 17, 2021, 12:29 p.m. UTC | #1 
Hi Umang,

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@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)
>  {
>  	cm_ = new CameraManager();
>  
> +	if (isolate)
> +		setenv("LIBCAMERA_IPA_FORCE_ISOLATION", "TRUE", 1);
> +

Ahh nice. Good so we can now isolate IPA's from tests!

Reviewed-by: Kieran Bingham <kieran.bingham@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:
>
Kieran Bingham Aug. 17, 2021, 12:44 p.m. UTC | #2 
On 17/08/2021 13:29, Kieran Bingham wrote:
> Hi Umang,
> 
> 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@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.


>>  {
>>  	cm_ = new CameraManager();
>>  
>> +	if (isolate)
>> +		setenv("LIBCAMERA_IPA_FORCE_ISOLATION", "TRUE", 1);
>> +
> 
> Ahh nice. Good so we can now isolate IPA's from tests!
> 
> Reviewed-by: Kieran Bingham <kieran.bingham@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:
>>
Laurent Pinchart Aug. 17, 2021, 12:53 p.m. UTC | #3 
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@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@ideasonboard.com>

> >> +
> > 
> > Ahh nice. Good so we can now isolate IPA's from tests!
> > 
> > Reviewed-by: Kieran Bingham <kieran.bingham@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:
> >>
Umang Jain Aug. 17, 2021, 2:44 p.m. UTC | #4 
Hi Kieran

On 8/17/21 6:14 PM, Kieran Bingham wrote:
> On 17/08/2021 13:29, Kieran Bingham wrote:
>> Hi Umang,
>>
>> 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@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.


In process of writing this, I actually envisioned that a map of 
environment variables that can be passed in to CameraTest that needed to 
be setenv() before CameraManager::start()

However, my thought process was the same, that is overkill for one 
environment variable.


>
>
>>>   {
>>>   	cm_ = new CameraManager();
>>>   
>>> +	if (isolate)
>>> +		setenv("LIBCAMERA_IPA_FORCE_ISOLATION", "TRUE", 1);
>>> +
>> Ahh nice. Good so we can now isolate IPA's from tests!
>>
>> Reviewed-by: Kieran Bingham <kieran.bingham@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:
>>>
Paul Elder Aug. 18, 2021, 3:13 a.m. UTC | #5 
Hi Umang,

On Tue, Aug 17, 2021 at 05:56:45PM +0530, 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@ideasonboard.com>

Reviewed-by: Paul Elder <paul.elder@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)
>  {
>  	cm_ = new CameraManager();
>  
> +	if (isolate)
> +		setenv("LIBCAMERA_IPA_FORCE_ISOLATION", "TRUE", 1);
> +
>  	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:
> -- 
> 2.31.1
>

Patch
diff mbox series 

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)
 {
 	cm_ = new CameraManager();
 
+	if (isolate)
+		setenv("LIBCAMERA_IPA_FORCE_ISOLATION", "TRUE", 1);
+
 	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: