[libcamera-devel] test: buffer-import: Fix false-positive failure
diff mbox series

Message ID 20210202173405.1688089-1-kieran.bingham@ideasonboard.com
State Accepted
Commit 7fa1a82811976681fde68a3336b466bf86389f59
Headers show
Series
  • [libcamera-devel] test: buffer-import: Fix false-positive failure
Related show

Commit Message

Kieran Bingham Feb. 2, 2021, 5:34 p.m. UTC 
Running the tests failed with the following error on buffer import:
  "Failed to capture enough frames (got 8 expected at least 8)"

This indicates that the test did in fact capture enough frames as
desired by the test. Update the comparison on both buffer-import and
capture tests accordingly.

Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
---
 test/camera/buffer_import.cpp | 2 +-
 test/camera/capture.cpp       | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

Comments

Jacopo Mondi Feb. 2, 2021, 5:46 p.m. UTC | #1 
Hi Kieran,

On Tue, Feb 02, 2021 at 05:34:05PM +0000, Kieran Bingham wrote:
> Running the tests failed with the following error on buffer import:
>   "Failed to capture enough frames (got 8 expected at least 8)"
>
> This indicates that the test did in fact capture enough frames as
> desired by the test. Update the comparison on both buffer-import and
> capture tests accordingly.
>
> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

How come we didn't notice ? Don't we run tests frequently enough :) ?

Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>

Thanks
   j

> ---
>  test/camera/buffer_import.cpp | 2 +-
>  test/camera/capture.cpp       | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/test/camera/buffer_import.cpp b/test/camera/buffer_import.cpp
> index 7ff628269c47..61f4eb92ae95 100644
> --- a/test/camera/buffer_import.cpp
> +++ b/test/camera/buffer_import.cpp
> @@ -138,7 +138,7 @@ protected:
>  		while (timer.isRunning())
>  			dispatcher->processEvents();
>
> -		if (completeRequestsCount_ <= cfg.bufferCount * 2) {
> +		if (completeRequestsCount_ < cfg.bufferCount * 2) {
>  			std::cout << "Failed to capture enough frames (got "
>  				  << completeRequestsCount_ << " expected at least "
>  				  << cfg.bufferCount * 2 << ")" << std::endl;
> diff --git a/test/camera/capture.cpp b/test/camera/capture.cpp
> index 6d564fe453ac..c4bc21100777 100644
> --- a/test/camera/capture.cpp
> +++ b/test/camera/capture.cpp
> @@ -142,7 +142,7 @@ protected:
>
>  		unsigned int nbuffers = allocator_->buffers(stream).size();
>
> -		if (completeRequestsCount_ <= nbuffers * 2) {
> +		if (completeRequestsCount_ < nbuffers * 2) {
>  			cout << "Failed to capture enough frames (got "
>  			     << completeRequestsCount_ << " expected at least "
>  			     << nbuffers * 2 << ")" << endl;
> --
> 2.25.1
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
Niklas Söderlund Feb. 2, 2021, 10:06 p.m. UTC | #2 
Hi Kieran,

On 2021-02-02 17:34:05 +0000, Kieran Bingham wrote:
> Running the tests failed with the following error on buffer import:
>   "Failed to capture enough frames (got 8 expected at least 8)"
> 
> This indicates that the test did in fact capture enough frames as
> desired by the test. Update the comparison on both buffer-import and
> capture tests accordingly.
> 
> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

First off I agree with the change,

Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>

Second is this finding not telling us we are cutting the test criteria 
quiet close? I think we should in a follow up patch either increase the 
time the capture loop is allowed to run or do something more clever as 
capture N frames but fail if FPS drops bellow ~2 or something.

Out of curiosity what device was this observed on?

> ---
>  test/camera/buffer_import.cpp | 2 +-
>  test/camera/capture.cpp       | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/test/camera/buffer_import.cpp b/test/camera/buffer_import.cpp
> index 7ff628269c47..61f4eb92ae95 100644
> --- a/test/camera/buffer_import.cpp
> +++ b/test/camera/buffer_import.cpp
> @@ -138,7 +138,7 @@ protected:
>  		while (timer.isRunning())
>  			dispatcher->processEvents();
>  
> -		if (completeRequestsCount_ <= cfg.bufferCount * 2) {
> +		if (completeRequestsCount_ < cfg.bufferCount * 2) {
>  			std::cout << "Failed to capture enough frames (got "
>  				  << completeRequestsCount_ << " expected at least "
>  				  << cfg.bufferCount * 2 << ")" << std::endl;
> diff --git a/test/camera/capture.cpp b/test/camera/capture.cpp
> index 6d564fe453ac..c4bc21100777 100644
> --- a/test/camera/capture.cpp
> +++ b/test/camera/capture.cpp
> @@ -142,7 +142,7 @@ protected:
>  
>  		unsigned int nbuffers = allocator_->buffers(stream).size();
>  
> -		if (completeRequestsCount_ <= nbuffers * 2) {
> +		if (completeRequestsCount_ < nbuffers * 2) {
>  			cout << "Failed to capture enough frames (got "
>  			     << completeRequestsCount_ << " expected at least "
>  			     << nbuffers * 2 << ")" << endl;
> -- 
> 2.25.1
> 
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
Paul Elder Feb. 3, 2021, 3:10 a.m. UTC | #3 
Hi Kieran,

On Tue, Feb 02, 2021 at 05:34:05PM +0000, Kieran Bingham wrote:
> Running the tests failed with the following error on buffer import:
>   "Failed to capture enough frames (got 8 expected at least 8)"
> 
> This indicates that the test did in fact capture enough frames as
> desired by the test. Update the comparison on both buffer-import and
> capture tests accordingly.
> 
> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>

> ---
>  test/camera/buffer_import.cpp | 2 +-
>  test/camera/capture.cpp       | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/test/camera/buffer_import.cpp b/test/camera/buffer_import.cpp
> index 7ff628269c47..61f4eb92ae95 100644
> --- a/test/camera/buffer_import.cpp
> +++ b/test/camera/buffer_import.cpp
> @@ -138,7 +138,7 @@ protected:
>  		while (timer.isRunning())
>  			dispatcher->processEvents();
>  
> -		if (completeRequestsCount_ <= cfg.bufferCount * 2) {
> +		if (completeRequestsCount_ < cfg.bufferCount * 2) {
>  			std::cout << "Failed to capture enough frames (got "
>  				  << completeRequestsCount_ << " expected at least "
>  				  << cfg.bufferCount * 2 << ")" << std::endl;
> diff --git a/test/camera/capture.cpp b/test/camera/capture.cpp
> index 6d564fe453ac..c4bc21100777 100644
> --- a/test/camera/capture.cpp
> +++ b/test/camera/capture.cpp
> @@ -142,7 +142,7 @@ protected:
>  
>  		unsigned int nbuffers = allocator_->buffers(stream).size();
>  
> -		if (completeRequestsCount_ <= nbuffers * 2) {
> +		if (completeRequestsCount_ < nbuffers * 2) {
>  			cout << "Failed to capture enough frames (got "
>  			     << completeRequestsCount_ << " expected at least "
>  			     << nbuffers * 2 << ")" << endl;
> -- 
> 2.25.1
> 
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
Kieran Bingham Feb. 4, 2021, 2:15 p.m. UTC | #4 
On 02/02/2021 17:46, Jacopo Mondi wrote:
> Hi Kieran,
> 
> On Tue, Feb 02, 2021 at 05:34:05PM +0000, Kieran Bingham wrote:
>> Running the tests failed with the following error on buffer import:
>>   "Failed to capture enough frames (got 8 expected at least 8)"
>>
>> This indicates that the test did in fact capture enough frames as
>> desired by the test. Update the comparison on both buffer-import and
>> capture tests accordingly.
>>
>> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> 
> How come we didn't notice ? Don't we run tests frequently enough :) ?

I'm not sure - perhaps this was just a close race, and normally more
frames run - but for some reason this just about captured enough in time
- but failed because of the check?

I saw the failure on buffer-import, and investigated because at first I
assumed it was the kernel regression (which is fixed in my running
kernel) so I feared that had come back.

But alas then I saw the message ...
--
Kieran




> 
> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
> 
> Thanks
>    j
> 
>> ---
>>  test/camera/buffer_import.cpp | 2 +-
>>  test/camera/capture.cpp       | 2 +-
>>  2 files changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/test/camera/buffer_import.cpp b/test/camera/buffer_import.cpp
>> index 7ff628269c47..61f4eb92ae95 100644
>> --- a/test/camera/buffer_import.cpp
>> +++ b/test/camera/buffer_import.cpp
>> @@ -138,7 +138,7 @@ protected:
>>  		while (timer.isRunning())
>>  			dispatcher->processEvents();
>>
>> -		if (completeRequestsCount_ <= cfg.bufferCount * 2) {
>> +		if (completeRequestsCount_ < cfg.bufferCount * 2) {
>>  			std::cout << "Failed to capture enough frames (got "
>>  				  << completeRequestsCount_ << " expected at least "
>>  				  << cfg.bufferCount * 2 << ")" << std::endl;
>> diff --git a/test/camera/capture.cpp b/test/camera/capture.cpp
>> index 6d564fe453ac..c4bc21100777 100644
>> --- a/test/camera/capture.cpp
>> +++ b/test/camera/capture.cpp
>> @@ -142,7 +142,7 @@ protected:
>>
>>  		unsigned int nbuffers = allocator_->buffers(stream).size();
>>
>> -		if (completeRequestsCount_ <= nbuffers * 2) {
>> +		if (completeRequestsCount_ < nbuffers * 2) {
>>  			cout << "Failed to capture enough frames (got "
>>  			     << completeRequestsCount_ << " expected at least "
>>  			     << nbuffers * 2 << ")" << endl;
>> --
>> 2.25.1
>>
>> _______________________________________________
>> libcamera-devel mailing list
>> libcamera-devel@lists.libcamera.org
>> https://lists.libcamera.org/listinfo/libcamera-devel
Kieran Bingham Feb. 4, 2021, 2:16 p.m. UTC | #5 
Hi Niklas,

On 02/02/2021 22:06, Niklas Söderlund wrote:
> Hi Kieran,
> 
> On 2021-02-02 17:34:05 +0000, Kieran Bingham wrote:
>> Running the tests failed with the following error on buffer import:
>>   "Failed to capture enough frames (got 8 expected at least 8)"
>>
>> This indicates that the test did in fact capture enough frames as
>> desired by the test. Update the comparison on both buffer-import and
>> capture tests accordingly.
>>
>> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> 
> First off I agree with the change,
> 
> Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> 
> Second is this finding not telling us we are cutting the test criteria 
> quiet close? I think we should in a follow up patch either increase the 
> time the capture loop is allowed to run or do something more clever as 
> capture N frames but fail if FPS drops bellow ~2 or something.
> 
> Out of curiosity what device was this observed on?


Maybe, it was running on my HP Spectre development laptop. It may have
been loaded doing other things at the time, I'm unsure - The issue
occurred while building my daily libcamera which runs the tests after.

--
Kieran


> 
>> ---
>>  test/camera/buffer_import.cpp | 2 +-
>>  test/camera/capture.cpp       | 2 +-
>>  2 files changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/test/camera/buffer_import.cpp b/test/camera/buffer_import.cpp
>> index 7ff628269c47..61f4eb92ae95 100644
>> --- a/test/camera/buffer_import.cpp
>> +++ b/test/camera/buffer_import.cpp
>> @@ -138,7 +138,7 @@ protected:
>>  		while (timer.isRunning())
>>  			dispatcher->processEvents();
>>  
>> -		if (completeRequestsCount_ <= cfg.bufferCount * 2) {
>> +		if (completeRequestsCount_ < cfg.bufferCount * 2) {
>>  			std::cout << "Failed to capture enough frames (got "
>>  				  << completeRequestsCount_ << " expected at least "
>>  				  << cfg.bufferCount * 2 << ")" << std::endl;
>> diff --git a/test/camera/capture.cpp b/test/camera/capture.cpp
>> index 6d564fe453ac..c4bc21100777 100644
>> --- a/test/camera/capture.cpp
>> +++ b/test/camera/capture.cpp
>> @@ -142,7 +142,7 @@ protected:
>>  
>>  		unsigned int nbuffers = allocator_->buffers(stream).size();
>>  
>> -		if (completeRequestsCount_ <= nbuffers * 2) {
>> +		if (completeRequestsCount_ < nbuffers * 2) {
>>  			cout << "Failed to capture enough frames (got "
>>  			     << completeRequestsCount_ << " expected at least "
>>  			     << nbuffers * 2 << ")" << endl;
>> -- 
>> 2.25.1
>>
>> _______________________________________________
>> libcamera-devel mailing list
>> libcamera-devel@lists.libcamera.org
>> https://lists.libcamera.org/listinfo/libcamera-devel
>

Patch
diff mbox series 

diff --git a/test/camera/buffer_import.cpp b/test/camera/buffer_import.cpp
index 7ff628269c47..61f4eb92ae95 100644
--- a/test/camera/buffer_import.cpp
+++ b/test/camera/buffer_import.cpp
@@ -138,7 +138,7 @@  protected:
 		while (timer.isRunning())
 			dispatcher->processEvents();
 
-		if (completeRequestsCount_ <= cfg.bufferCount * 2) {
+		if (completeRequestsCount_ < cfg.bufferCount * 2) {
 			std::cout << "Failed to capture enough frames (got "
 				  << completeRequestsCount_ << " expected at least "
 				  << cfg.bufferCount * 2 << ")" << std::endl;
diff --git a/test/camera/capture.cpp b/test/camera/capture.cpp
index 6d564fe453ac..c4bc21100777 100644
--- a/test/camera/capture.cpp
+++ b/test/camera/capture.cpp
@@ -142,7 +142,7 @@  protected:
 
 		unsigned int nbuffers = allocator_->buffers(stream).size();
 
-		if (completeRequestsCount_ <= nbuffers * 2) {
+		if (completeRequestsCount_ < nbuffers * 2) {
 			cout << "Failed to capture enough frames (got "
 			     << completeRequestsCount_ << " expected at least "
 			     << nbuffers * 2 << ")" << endl;