[libcamera-devel] [PATCH 06/10] libcamera: v4l2_videodevice: Prevent aliasing of V4L2BufferCache members

[Kieran Bingham]

Hi Laurent,

On 15/10/2020 14:38, Laurent Pinchart wrote:
> Hi Kieran,
> 
> Thank you for the patch.
> 
> On Tue, Oct 13, 2020 at 04:12:37PM +0100, Kieran Bingham wrote:
>> The members free, and lastUsed were not following the libcamera coding
>> style, and were producing an aliased parameter on the construction.
>>
>> Rename them to be marked as member variables with the _ postfix
>> accordingly.
>>
>> Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>> ---
>>  include/libcamera/internal/v4l2_videodevice.h |  4 ++--
>>  src/libcamera/v4l2_videodevice.cpp            | 14 +++++++-------
>>  2 files changed, 9 insertions(+), 9 deletions(-)
>>
>> diff --git a/include/libcamera/internal/v4l2_videodevice.h b/include/libcamera/internal/v4l2_videodevice.h
>> index 40ed87e17cfa..53f6a2d5515b 100644
>> --- a/include/libcamera/internal/v4l2_videodevice.h
>> +++ b/include/libcamera/internal/v4l2_videodevice.h
>> @@ -127,8 +127,8 @@ private:
>>  
>>  		bool operator==(const FrameBuffer &buffer) const;
>>  
>> -		bool free;
>> -		uint64_t lastUsed;
>> +		bool free_;
>> +		uint64_t lastUsed_;
>>  
>>  	private:
>>  		struct Plane {
>> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp
>> index 508522ef42bb..96952b26c634 100644
>> --- a/src/libcamera/v4l2_videodevice.cpp
>> +++ b/src/libcamera/v4l2_videodevice.cpp
>> @@ -212,7 +212,7 @@ int V4L2BufferCache::get(const FrameBuffer &buffer)
>>  	for (unsigned int index = 0; index < cache_.size(); index++) {
>>  		const Entry &entry = cache_[index];
>>  
>> -		if (!entry.free)
>> +		if (!entry.free_)
>>  			continue;
>>  
>>  		/* Try to find a cache hit by comparing the planes. */
>> @@ -222,9 +222,9 @@ int V4L2BufferCache::get(const FrameBuffer &buffer)
>>  			break;
>>  		}
>>  
>> -		if (entry.lastUsed < oldest) {
>> +		if (entry.lastUsed_ < oldest) {
>>  			use = index;
>> -			oldest = entry.lastUsed;
>> +			oldest = entry.lastUsed_;
>>  		}
>>  	}
>>  
>> @@ -248,16 +248,16 @@ int V4L2BufferCache::get(const FrameBuffer &buffer)
>>  void V4L2BufferCache::put(unsigned int index)
>>  {
>>  	ASSERT(index < cache_.size());
>> -	cache_[index].free = true;
>> +	cache_[index].free_ = true;
>>  }
>>  
>>  V4L2BufferCache::Entry::Entry()
>> -	: free(true), lastUsed(0)
>> +	: free_(true), lastUsed_(0)
>>  {
>>  }
>>  
>> -V4L2BufferCache::Entry::Entry(bool free, uint64_t lastUsed, const FrameBuffer &buffer)
>> -	: free(free), lastUsed(lastUsed)
>> +V4L2BufferCache::Entry::Entry(bool free, uint64_t age, const FrameBuffer &buffer)
>> +	: free_(free), lastUsed_(age)
> 
> Any specific reason to rename the function parameter here, or is it a
> leftover ? I assume the latter, so without the rename,

Good spot - indeed, this is a leftover from before I saw adding the _
was the correct way forwards.

I'll fix for v2.


> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

Thanks.


> 
>>  {
>>  	for (const FrameBuffer::Plane &plane : buffer.planes())
>>  		planes_.emplace_back(plane);
> 

-- 
Regards
--
Kieran