[libcamera-devel] [PATCH v4 2/6] libcamera: pipeline: ipu3: frames: Fail if the FrameInfo can't be found

[Kieran Bingham]

Hi Laurent,

On 20/04/2021 23:19, Laurent Pinchart wrote:
> Hi Kieran,
> 
> Thank you for the patch.
> 
> On Tue, Apr 20, 2021 at 02:07:37PM +0100, Kieran Bingham wrote:
>> The FrameInfo structure associates the data sent to the IPA
>> and is essential for handling events.
>>
>> If it can not be found, this is a fatal error which must be fixed.
>>
>> Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>> ---
>> v3:
>>  - Make all occurrences of failing to find a frame info fatal.
>> ---
>>  src/libcamera/pipeline/ipu3/frames.cpp | 6 ++++--
>>  1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/libcamera/pipeline/ipu3/frames.cpp b/src/libcamera/pipeline/ipu3/frames.cpp
>> index 03e8131c4829..a1b014eed8d7 100644
>> --- a/src/libcamera/pipeline/ipu3/frames.cpp
>> +++ b/src/libcamera/pipeline/ipu3/frames.cpp
>> @@ -113,7 +113,8 @@ IPU3Frames::Info *IPU3Frames::find(unsigned int id)
>>  	if (itInfo != frameInfo_.end())
>>  		return itInfo->second.get();
>>  
>> -	LOG(IPU3, Error) << "Can't find tracking informaton for frame " << id;
>> +	LOG(IPU3, Fatal) << "Can't find tracking informaton for frame " << id;
>> +
>>  	return nullptr;
>>  }
>>  
>> @@ -131,7 +132,8 @@ IPU3Frames::Info *IPU3Frames::find(FrameBuffer *buffer)
>>  			return info;
>>  	}
>>  
>> -	LOG(IPU3, Error) << "Can't find tracking informaton from buffer";
>> +	LOG(IPU3, Fatal) << "Can't find tracking informaton from buffer";
> 
> In addition to the informaton typo, should this read s/from buffer/for
> buffer/ ?
> 

Both make sense, and both are true, even though they are slightly different.

Can't find tracking information 'for' the buffer
  - I can't identify which tracking state is used for this buffer

Can't find tracking information 'from' buffer
  - Using this buffer, I was unable to find any tracking information


I won't bother changing this now, as I hope in the (near?) future to be
able to drop these anyway, and have the tracking information obtained
from the Request, which itself would be obtained from the buffer (via
Buffer->request())

Then we can turn 'searching/lookups' into simply following two pointers,
by having a private cookie in the now extensible Request.

With that - it won't be possible to 'not find' the entry.
(Of course we will still have to guarantee that it is correct).

> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> 
>> +
>>  	return nullptr;
>>  }
>>  
> 

-- 
Regards
--
Kieran