[libcamera-devel,v4,7/9] libcamera: v4l2_videodevice: Create color-format planes in createBuffer()
diff mbox series

Message ID 20210827064439.879308-1-hiroh@chromium.org
State Accepted
Headers show
Series
  • Untitled series #2404
Related show

Commit Message

Hirokazu Honda Aug. 27, 2021, 6:44 a.m. UTC
V4L2VideDevice::createBuffer() creates the same number of
FrameBuffer::Planes as V4L2 format planes. Therefore, if the v4l2
format single is single-planar format, the created number of
FrameBuffer::Planes is 1.
It should rather create the same number of FrameBuffer::Planes as the
color format planes.

Signed-off-by: Hirokazu Honda <hiroh@chromium.org>

---

Let me send this patch only in v4.

---
 include/libcamera/internal/v4l2_videodevice.h |  1 +
 src/libcamera/v4l2_videodevice.cpp            | 69 ++++++++++++++++---
 2 files changed, 61 insertions(+), 9 deletions(-)

--
2.33.0.259.gc128427fd7-goog

Comments

Laurent Pinchart Aug. 30, 2021, 3:57 p.m. UTC | #1
Hi Hiro,

Thank you for the patch.

On Fri, Aug 27, 2021 at 03:44:39PM +0900, Hirokazu Honda wrote:
> V4L2VideDevice::createBuffer() creates the same number of
> FrameBuffer::Planes as V4L2 format planes. Therefore, if the v4l2
> format single is single-planar format, the created number of
> FrameBuffer::Planes is 1.
> It should rather create the same number of FrameBuffer::Planes as the
> color format planes.
> 
> Signed-off-by: Hirokazu Honda <hiroh@chromium.org>

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

> ---
> 
> Let me send this patch only in v4.
> 
> ---
>  include/libcamera/internal/v4l2_videodevice.h |  1 +
>  src/libcamera/v4l2_videodevice.cpp            | 69 ++++++++++++++++---
>  2 files changed, 61 insertions(+), 9 deletions(-)
> 
> diff --git a/include/libcamera/internal/v4l2_videodevice.h b/include/libcamera/internal/v4l2_videodevice.h
> index e767ec84..4a5d2cad 100644
> --- a/include/libcamera/internal/v4l2_videodevice.h
> +++ b/include/libcamera/internal/v4l2_videodevice.h
> @@ -242,6 +242,7 @@ private:
>  	FrameBuffer *dequeueBuffer();
> 
>  	V4L2Capability caps_;
> +	V4L2DeviceFormat format_;
> 
>  	enum v4l2_buf_type bufferType_;
>  	enum v4l2_memory memoryType_;
> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp
> index 2ff25af2..9d35618d 100644
> --- a/src/libcamera/v4l2_videodevice.cpp
> +++ b/src/libcamera/v4l2_videodevice.cpp
> @@ -25,6 +25,7 @@
> 
>  #include <libcamera/file_descriptor.h>
> 
> +#include "libcamera/internal/formats.h"
>  #include "libcamera/internal/media_device.h"
>  #include "libcamera/internal/media_object.h"
> 
> @@ -579,6 +580,12 @@ int V4L2VideoDevice::open()
>  		<< "Opened device " << caps_.bus_info() << ": "
>  		<< caps_.driver() << ": " << caps_.card();
> 
> +	ret = getFormat(&format_);
> +	if (ret) {
> +		LOG(V4L2, Error) << "Failed to get format";
> +		return ret;
> +	}
> +
>  	return 0;
>  }
> 
> @@ -668,6 +675,12 @@ int V4L2VideoDevice::open(int handle, enum v4l2_buf_type type)
>  		<< "Opened device " << caps_.bus_info() << ": "
>  		<< caps_.driver() << ": " << caps_.card();
> 
> +	ret = getFormat(&format_);
> +	if (ret) {
> +		LOG(V4L2, Error) << "Failed to get format";
> +		return ret;
> +	}
> +
>  	return 0;
>  }
> 
> @@ -761,12 +774,20 @@ int V4L2VideoDevice::tryFormat(V4L2DeviceFormat *format)
>   */
>  int V4L2VideoDevice::setFormat(V4L2DeviceFormat *format)
>  {
> +	int ret = 0;
>  	if (caps_.isMeta())
> -		return trySetFormatMeta(format, true);
> +		ret = trySetFormatMeta(format, true);
>  	else if (caps_.isMultiplanar())
> -		return trySetFormatMultiplane(format, true);
> +		ret = trySetFormatMultiplane(format, true);
>  	else
> -		return trySetFormatSingleplane(format, true);
> +		ret = trySetFormatSingleplane(format, true);
> +
> +	/* Cache the set format on success. */
> +	if (ret)
> +		return ret;
> +
> +	format_ = *format;
> +	return 0;
>  }
> 
>  int V4L2VideoDevice::getFormatMeta(V4L2DeviceFormat *format)
> @@ -1152,8 +1173,13 @@ int V4L2VideoDevice::requestBuffers(unsigned int count,
>   * successful return the driver's internal buffer management is initialized in
>   * MMAP mode, and the video device is ready to accept queueBuffer() calls.
>   *
> - * The number of planes and the plane sizes for the allocation are determined
> - * by the currently active format on the device as set by setFormat().
> + * The number of planes and their offsets and sizes are determined by the
> + * currently active format on the device as set by setFormat(). They do not map
> + * to the V4L2 buffer planes, but to colour planes of the pixel format. For
> + * instance, if the active format is formats::NV12, the allocated FrameBuffer
> + * instances will have two planes, for the luma and chroma components,
> + * regardless of whether the device uses V4L2_PIX_FMT_NV12 or
> + * V4L2_PIX_FMT_NV12M.
>   *
>   * Buffers allocated with this function shall later be free with
>   * releaseBuffers(). If buffers have already been allocated with
> @@ -1190,8 +1216,13 @@ int V4L2VideoDevice::allocateBuffers(unsigned int count,
>   * usable with any V4L2 video device in DMABUF mode, or with other dmabuf
>   * importers.
>   *
> - * The number of planes and the plane sizes for the allocation are determined
> - * by the currently active format on the device as set by setFormat().
> + * The number of planes and their offsets and sizes are determined by the
> + * currently active format on the device as set by setFormat(). They do not map
> + * to the V4L2 buffer planes, but to colour planes of the pixel format. For
> + * instance, if the active format is formats::NV12, the allocated FrameBuffer
> + * instances will have two planes, for the luma and chroma components,
> + * regardless of whether the device uses V4L2_PIX_FMT_NV12 or
> + * V4L2_PIX_FMT_NV12M.
>   *
>   * Multiple independent sets of buffers can be allocated with multiple calls to
>   * this function. Device-specific limitations may apply regarding the minimum
> @@ -1289,12 +1320,32 @@ std::unique_ptr<FrameBuffer> V4L2VideoDevice::createBuffer(unsigned int index)
>  		 * \todo Set the right offset once V4L2 API provides a way.
>  		 */
>  		plane.offset = 0;
> -		plane.length = multiPlanar ?
> -			buf.m.planes[nplane].length : buf.length;
> +		plane.length = multiPlanar ? buf.m.planes[nplane].length : buf.length;
> 
>  		planes.push_back(std::move(plane));
>  	}
> 
> +	const auto &info = PixelFormatInfo::info(format_.fourcc);
> +	if (info.isValid() && info.numPlanes() != numPlanes) {
> +		ASSERT(numPlanes == 1u);
> +		const size_t numColorPlanes = info.numPlanes();
> +		planes.resize(numColorPlanes);
> +		const FileDescriptor &fd = planes[0].fd;
> +		size_t offset = 0;
> +		for (size_t i = 0; i < numColorPlanes; ++i) {
> +			planes[i].fd = fd;
> +			planes[i].offset = offset;
> +
> +			/* \todo Take the V4L2 stride into account */
> +			const unsigned int vertSubSample =
> +				info.planes[i].verticalSubSampling;
> +			planes[i].length =
> +				info.stride(format_.size.width, i, 1u) *
> +				((format_.size.height + vertSubSample - 1) / vertSubSample);
> +			offset += planes[i].length;
> +		}
> +	}
> +
>  	return std::make_unique<FrameBuffer>(std::move(planes));
>  }
>

Patch
diff mbox series

diff --git a/include/libcamera/internal/v4l2_videodevice.h b/include/libcamera/internal/v4l2_videodevice.h
index e767ec84..4a5d2cad 100644
--- a/include/libcamera/internal/v4l2_videodevice.h
+++ b/include/libcamera/internal/v4l2_videodevice.h
@@ -242,6 +242,7 @@  private:
 	FrameBuffer *dequeueBuffer();

 	V4L2Capability caps_;
+	V4L2DeviceFormat format_;

 	enum v4l2_buf_type bufferType_;
 	enum v4l2_memory memoryType_;
diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp
index 2ff25af2..9d35618d 100644
--- a/src/libcamera/v4l2_videodevice.cpp
+++ b/src/libcamera/v4l2_videodevice.cpp
@@ -25,6 +25,7 @@ 

 #include <libcamera/file_descriptor.h>

+#include "libcamera/internal/formats.h"
 #include "libcamera/internal/media_device.h"
 #include "libcamera/internal/media_object.h"

@@ -579,6 +580,12 @@  int V4L2VideoDevice::open()
 		<< "Opened device " << caps_.bus_info() << ": "
 		<< caps_.driver() << ": " << caps_.card();

+	ret = getFormat(&format_);
+	if (ret) {
+		LOG(V4L2, Error) << "Failed to get format";
+		return ret;
+	}
+
 	return 0;
 }

@@ -668,6 +675,12 @@  int V4L2VideoDevice::open(int handle, enum v4l2_buf_type type)
 		<< "Opened device " << caps_.bus_info() << ": "
 		<< caps_.driver() << ": " << caps_.card();

+	ret = getFormat(&format_);
+	if (ret) {
+		LOG(V4L2, Error) << "Failed to get format";
+		return ret;
+	}
+
 	return 0;
 }

@@ -761,12 +774,20 @@  int V4L2VideoDevice::tryFormat(V4L2DeviceFormat *format)
  */
 int V4L2VideoDevice::setFormat(V4L2DeviceFormat *format)
 {
+	int ret = 0;
 	if (caps_.isMeta())
-		return trySetFormatMeta(format, true);
+		ret = trySetFormatMeta(format, true);
 	else if (caps_.isMultiplanar())
-		return trySetFormatMultiplane(format, true);
+		ret = trySetFormatMultiplane(format, true);
 	else
-		return trySetFormatSingleplane(format, true);
+		ret = trySetFormatSingleplane(format, true);
+
+	/* Cache the set format on success. */
+	if (ret)
+		return ret;
+
+	format_ = *format;
+	return 0;
 }

 int V4L2VideoDevice::getFormatMeta(V4L2DeviceFormat *format)
@@ -1152,8 +1173,13 @@  int V4L2VideoDevice::requestBuffers(unsigned int count,
  * successful return the driver's internal buffer management is initialized in
  * MMAP mode, and the video device is ready to accept queueBuffer() calls.
  *
- * The number of planes and the plane sizes for the allocation are determined
- * by the currently active format on the device as set by setFormat().
+ * The number of planes and their offsets and sizes are determined by the
+ * currently active format on the device as set by setFormat(). They do not map
+ * to the V4L2 buffer planes, but to colour planes of the pixel format. For
+ * instance, if the active format is formats::NV12, the allocated FrameBuffer
+ * instances will have two planes, for the luma and chroma components,
+ * regardless of whether the device uses V4L2_PIX_FMT_NV12 or
+ * V4L2_PIX_FMT_NV12M.
  *
  * Buffers allocated with this function shall later be free with
  * releaseBuffers(). If buffers have already been allocated with
@@ -1190,8 +1216,13 @@  int V4L2VideoDevice::allocateBuffers(unsigned int count,
  * usable with any V4L2 video device in DMABUF mode, or with other dmabuf
  * importers.
  *
- * The number of planes and the plane sizes for the allocation are determined
- * by the currently active format on the device as set by setFormat().
+ * The number of planes and their offsets and sizes are determined by the
+ * currently active format on the device as set by setFormat(). They do not map
+ * to the V4L2 buffer planes, but to colour planes of the pixel format. For
+ * instance, if the active format is formats::NV12, the allocated FrameBuffer
+ * instances will have two planes, for the luma and chroma components,
+ * regardless of whether the device uses V4L2_PIX_FMT_NV12 or
+ * V4L2_PIX_FMT_NV12M.
  *
  * Multiple independent sets of buffers can be allocated with multiple calls to
  * this function. Device-specific limitations may apply regarding the minimum
@@ -1289,12 +1320,32 @@  std::unique_ptr<FrameBuffer> V4L2VideoDevice::createBuffer(unsigned int index)
 		 * \todo Set the right offset once V4L2 API provides a way.
 		 */
 		plane.offset = 0;
-		plane.length = multiPlanar ?
-			buf.m.planes[nplane].length : buf.length;
+		plane.length = multiPlanar ? buf.m.planes[nplane].length : buf.length;

 		planes.push_back(std::move(plane));
 	}

+	const auto &info = PixelFormatInfo::info(format_.fourcc);
+	if (info.isValid() && info.numPlanes() != numPlanes) {
+		ASSERT(numPlanes == 1u);
+		const size_t numColorPlanes = info.numPlanes();
+		planes.resize(numColorPlanes);
+		const FileDescriptor &fd = planes[0].fd;
+		size_t offset = 0;
+		for (size_t i = 0; i < numColorPlanes; ++i) {
+			planes[i].fd = fd;
+			planes[i].offset = offset;
+
+			/* \todo Take the V4L2 stride into account */
+			const unsigned int vertSubSample =
+				info.planes[i].verticalSubSampling;
+			planes[i].length =
+				info.stride(format_.size.width, i, 1u) *
+				((format_.size.height + vertSubSample - 1) / vertSubSample);
+			offset += planes[i].length;
+		}
+	}
+
 	return std::make_unique<FrameBuffer>(std::move(planes));
 }