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

[Kieran Bingham]

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.


>  	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?

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;
>  		}
>  	}
>