[libcamera-devel] [RFC PATCH v1 09/12] libcamera: v4l2_videodevice: Cache PixelFormatInfo

[Kieran Bingham]

On 02/09/2021 19:26, Laurent Pinchart wrote:
> Hi Kieran,
> 
> On Thu, Sep 02, 2021 at 11:21:29AM +0100, Kieran Bingham wrote:
>> On 02/09/2021 05:23, Laurent Pinchart wrote:
>>> Cache the PixelFormatInfo instead of looking it up in every call to
>>> createBuffer(). This prepare for usage of the info in queueBuffer(), to
>>
>> s/prepare/prepares/
>>
>>> avoid a looking every time a buffer is queued.
>>>
>>> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
>>> ---
>>>  include/libcamera/internal/v4l2_videodevice.h |  1 +
>>>  src/libcamera/v4l2_videodevice.cpp            | 19 +++++++++++--------
>>>  2 files changed, 12 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/include/libcamera/internal/v4l2_videodevice.h b/include/libcamera/internal/v4l2_videodevice.h
>>> index 7a145f608a5b..6096cd609b97 100644
>>> --- a/include/libcamera/internal/v4l2_videodevice.h
>>> +++ b/include/libcamera/internal/v4l2_videodevice.h
>>> @@ -243,6 +243,7 @@ private:
>>>  
>>>  	V4L2Capability caps_;
>>>  	V4L2DeviceFormat format_;
>>> +	const PixelFormatInfo *formatInfo_;
>>>  
>>>  	enum v4l2_buf_type bufferType_;
>>>  	enum v4l2_memory memoryType_;
>>> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp
>>> index 82ddaed3656f..2d9a94c3c974 100644
>>> --- a/src/libcamera/v4l2_videodevice.cpp
>>> +++ b/src/libcamera/v4l2_videodevice.cpp
>>> @@ -482,8 +482,8 @@ const std::string V4L2DeviceFormat::toString() const
>>>   * \param[in] deviceNode The file-system path to the video device node
>>>   */
>>>  V4L2VideoDevice::V4L2VideoDevice(const std::string &deviceNode)
>>> -	: V4L2Device(deviceNode), cache_(nullptr), fdBufferNotifier_(nullptr),
>>> -	  streaming_(false)
>>> +	: V4L2Device(deviceNode), formatInfo_(nullptr), cache_(nullptr),
>>> +	  fdBufferNotifier_(nullptr), streaming_(false)
>>>  {
>>>  	/*
>>>  	 * We default to an MMAP based CAPTURE video device, however this will
>>> @@ -586,6 +586,7 @@ int V4L2VideoDevice::open()
>>>  		return ret;
>>>  	}
>>>  
>>> +	formatInfo_ = &PixelFormatInfo::info(format_.fourcc);
>>
>> I shudder a little seeing code hugging the return like that ;-)
>>
>> But it's purely subjective, same below of course.
> 
> We have various patterns indeed :-) I haven't been able to establish yet
> what my mental process to deal with these are, sometimes it bothers me
> too, but not always.
> 
>>>  	return 0;
>>>  }
>>>  
>>> @@ -681,6 +682,7 @@ int V4L2VideoDevice::open(int handle, enum v4l2_buf_type type)
>>>  		return ret;
>>>  	}
>>>  
>>> +	formatInfo_ = &PixelFormatInfo::info(format_.fourcc);
>>>  	return 0;
>>>  }
>>>  
>>> @@ -695,6 +697,8 @@ void V4L2VideoDevice::close()
>>>  	releaseBuffers();
>>>  	delete fdBufferNotifier_;
>>>  
>>> +	formatInfo_ = nullptr;
>>> +
>>>  	V4L2Device::close();
>>>  }
>>>  
>>> @@ -787,6 +791,7 @@ int V4L2VideoDevice::setFormat(V4L2DeviceFormat *format)
>>>  		return ret;
>>>  
>>>  	format_ = *format;
>>> +	formatInfo_ = &PixelFormatInfo::info(format_.fourcc);
>>
>> Looks like we were already using that pattern though ;-)
>>
>>>  	return 0;
>>>  }
>>>  
>>> @@ -1325,19 +1330,17 @@ std::unique_ptr<FrameBuffer> V4L2VideoDevice::createBuffer(unsigned int index)
>>>  		planes.push_back(std::move(plane));
>>>  	}
>>>  
>>> -	const auto &info = PixelFormatInfo::info(format_.fourcc);
>>> -	if (info.isValid() && info.numPlanes() != numPlanes) {
>>> +	if (formatInfo_->isValid() && formatInfo_->numPlanes() != numPlanes) {
>>>  		ASSERT(numPlanes == 1u);
>>
>> Not for this patch, but I guess this assert needed a
>>   \todo Multiplanar support
>>
>> around it?
> 
> No, when formatInfo_->numPlanes() != numPlanes, it means that we have a
> multi-planar FrameBuffer with a single-planar V4L2 buffer format (e.g.
> V4L2_PIX_FMT_NV12 as opposed to V4L2_PIX_FMT_NV12M). numPlanes must be 1
> in that case, otherwise it's a kernel bug.

As you're updating the conditional there anyway, could you add a short
brief to explain that before the ASSERT please?

I don't think that extra context is clear from just the if statement.
(Perhaps there is more context outside of the hunk maybe, but it's not
clear from just this diff).

> 
>> However, this patch is just for caching the formatInfo, which seems
>> reasonable so
>>
>> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>>
>>> -		const size_t numColorPlanes = info.numPlanes();
>>> -		planes.resize(numColorPlanes);
>>> +		planes.resize(formatInfo_->numPlanes());
>>>  		const FileDescriptor &fd = planes[0].fd;
>>>  		size_t offset = 0;
>>> -		for (size_t i = 0; i < numColorPlanes; ++i) {
>>> +		for (size_t i = 0; i < planes.size(); ++i) {
>>>  			planes[i].fd = fd;
>>>  			planes[i].offset = offset;
>>>  
>>>  			/* \todo Take the V4L2 stride into account */
>>> -			planes[i].length = info.planeSize(format_.size, i);
>>> +			planes[i].length = formatInfo_->planeSize(format_.size, i);
>>>  			offset += planes[i].length;
>>>  		}
>>>  	}
>