[{"id":19037,"web_url":"https://patchwork.libcamera.org/comment/19037/","msgid":"<20210825083507.5bodpgew6lsllx77@uno.localdomain>","date":"2021-08-25T08:35:07","subject":"Re: [libcamera-devel] [PATCH v2 1/3] android:\n\tgeneric_camera_buffer: Correct buffer mapping","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Hiro,\n\nOn Wed, Aug 25, 2021 at 01:44:08PM +0900, Hirokazu Honda wrote:\n> buffer_hanlde_t doesn't provide sufficient info to map a buffer\n> properly. cros::CameraBufferManager enables handling the buffer on\n> ChromeOS, but no way is provided for other platforms.\n>\n> Therefore, we put the assumption that planes are in the same buffer\n> and they are consecutive. This modifies the way of mapping in\n> generic_camera_buffer with the assumption.\n>\n> Signed-off-by: Hirokazu Honda \n> ---\n> src/android/camera_buffer.h | 14 ++-\n> src/android/camera_stream.cpp | 4 +-\n> src/android/mm/cros_camera_buffer.cpp | 7 +-\n> src/android/mm/generic_camera_buffer.cpp | 104 +++++++++++++++++------\n> 4 files changed, 99 insertions(+), 30 deletions(-)\n>\n> diff --git a/src/android/camera_buffer.h b/src/android/camera_buffer.h\n> index c4e3a9e7..87df2570 100644\n> --- a/src/android/camera_buffer.h\n> +++ b/src/android/camera_buffer.h\n> @@ -11,13 +11,17 @@\n>\n> #include \n> #include \n> +#include \n> +#include \n>\n> class CameraBuffer final : public libcamera::Extensible\n> {\n> \tLIBCAMERA_DECLARE_PRIVATE()\n>\n> public:\n> -\tCameraBuffer(buffer_handle_t camera3Buffer, int flags);\n> +\tCameraBuffer(buffer_handle_t camera3Buffer,\n> +\t\t libcamera::PixelFormat pixelFormat,\n> +\t\t const libcamera::Size &size, int flags);\n> \t~CameraBuffer();\n>\n> \tbool isValid() const;\n> @@ -31,8 +35,12 @@ public:\n> };\n>\n> #define PUBLIC_CAMERA_BUFFER_IMPLEMENTATION\t\t\t\t\\\n> -CameraBuffer::CameraBuffer(buffer_handle_t camera3Buffer, int flags)\t\\\n> -\t: Extensible(std::make_unique(this, camera3Buffer, flags)) \\\n> +CameraBuffer::CameraBuffer(buffer_handle_t camera3Buffer,\t\t\\\n> +\t\t\t libcamera::PixelFormat pixelFormat,\t\t\\\n> +\t\t\t const libcamera::Size &size, int flags)\t\\\n> +\t: Extensible(std::make_unique(this, camera3Buffer,\t\\\n> +\t\t\t\t\t pixelFormat, size,\t\\\n> +\t\t\t\t\t flags))\t\t\t\\\n> {\t\t\t\t\t\t\t\t\t\\\n> }\t\t\t\t\t\t\t\t\t\\\n> CameraBuffer::~CameraBuffer()\t\t\t\t\t\t\\\n> diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp\n> index 61b44183..01909ec7 100644\n> --- a/src/android/camera_stream.cpp\n> +++ b/src/android/camera_stream.cpp\n> @@ -110,7 +110,9 @@ int CameraStream::process(const libcamera::FrameBuffer &source,\n> \t * \\todo Buffer mapping and processing should be moved to a\n> \t * separate thread.\n> \t */\n> -\tCameraBuffer dest(camera3Dest, PROT_READ | PROT_WRITE);\n> +\tconst StreamConfiguration &output = configuration();\n> +\tCameraBuffer dest(camera3Dest, formats::MJPEG, output.size,\n> +\t\t\t PROT_READ | PROT_WRITE);\n> \tif (!dest.isValid()) {\n> \t\tLOG(HAL, Error) << \"Failed to map android blob buffer\";\n> \t\treturn -EINVAL;\n> diff --git a/src/android/mm/cros_camera_buffer.cpp b/src/android/mm/cros_camera_buffer.cpp\n> index e8783ff8..50732637 100644\n> --- a/src/android/mm/cros_camera_buffer.cpp\n> +++ b/src/android/mm/cros_camera_buffer.cpp\n> @@ -20,8 +20,9 @@ class CameraBuffer::Private : public Extensible::Private\n> \tLIBCAMERA_DECLARE_PUBLIC(CameraBuffer)\n>\n> public:\n> -\tPrivate(CameraBuffer *cameraBuffer,\n> -\t\tbuffer_handle_t camera3Buffer, int flags);\n> +\tPrivate(CameraBuffer *cameraBuffer, buffer_handle_t camera3Buffer,\n> +\t\tlibcamera::PixelFormat pixelFormat, const libcamera::Size &size,\n> +\t\tint flags);\n> \t~Private();\n>\n> \tbool isValid() const { return valid_; }\n> @@ -46,6 +47,8 @@ private:\n>\n> CameraBuffer::Private::Private([[maybe_unused]] CameraBuffer *cameraBuffer,\n> \t\t\t buffer_handle_t camera3Buffer,\n> +\t\t\t [[maybe_unused]] libcamera::PixelFormat pixelFormat,\n> +\t\t\t [[maybe_unused]] const libcamera::Size &size,\n> \t\t\t [[maybe_unused]] int flags)\n> \t: handle_(camera3Buffer), numPlanes_(0), valid_(false),\n> \t registered_(false)\n> diff --git a/src/android/mm/generic_camera_buffer.cpp b/src/android/mm/generic_camera_buffer.cpp\n> index b3af194c..0bfdc2ba 100644\n> --- a/src/android/mm/generic_camera_buffer.cpp\n> +++ b/src/android/mm/generic_camera_buffer.cpp\n> @@ -12,6 +12,7 @@\n>\n> #include \n>\n> +#include \"libcamera/internal/formats.h\"\n> #include \"libcamera/internal/mapped_framebuffer.h\"\n>\n> using namespace libcamera;\n> @@ -24,8 +25,9 @@ class CameraBuffer::Private : public Extensible::Private,\n> \tLIBCAMERA_DECLARE_PUBLIC(CameraBuffer)\n>\n> public:\n> -\tPrivate(CameraBuffer *cameraBuffer,\n> -\t\tbuffer_handle_t camera3Buffer, int flags);\n> +\tPrivate(CameraBuffer *cameraBuffer, buffer_handle_t camera3Buffer,\n> +\t\tlibcamera::PixelFormat pixelFormat, const libcamera::Size &size,\n> +\t\tint flags);\n> \t~Private();\n>\n> \tunsigned int numPlanes() const;\n> @@ -33,35 +35,89 @@ public:\n> \tSpan plane(unsigned int plane);\n>\n> \tsize_t jpegBufferSize(size_t maxJpegBufferSize) const;\n> +\n> +private:\n> +\t/* \\todo Remove planes_ when it will be added to MappedBuffer */\n> +\tstd::vector> planes_;\n> };\n>\n> CameraBuffer::Private::Private([[maybe_unused]] CameraBuffer *cameraBuffer,\n> -\t\t\t buffer_handle_t camera3Buffer, int flags)\n> +\t\t\t buffer_handle_t camera3Buffer,\n> +\t\t\t libcamera::PixelFormat pixelFormat,\n> +\t\t\t const libcamera::Size &size, int flags)\n> {\n> -\tmaps_.reserve(camera3Buffer->numFds);\n> \terror_ = 0;\n> -\n> +\t/*\n> +\t * As Android doesn't offer an API to query buffer layouts, assume for\n> +\t * now that the buffer is backed by a single dmabuf, with planes being\n> +\t * stored contiguously.\n> +\t */\n> +\tint fd = -1;\n> \tfor (int i = 0; i < camera3Buffer->numFds; i++) {\n\nunsigned int i\n\n> -\t\tif (camera3Buffer->data[i] == -1)\n> +\t\tif (camera3Buffer->data[i] == -1 ||\n> +\t\t camera3Buffer->data[i] == fd) {\n> \t\t\tcontinue;\n> -\n> -\t\toff_t length = lseek(camera3Buffer->data[i], 0, SEEK_END);\n> -\t\tif (length < 0) {\n> -\t\t\terror_ = -errno;\n> -\t\t\tLOG(HAL, Error) << \"Failed to query plane length\";\n> -\t\t\tbreak;\n> \t\t}\n>\n> -\t\tvoid *address = mmap(nullptr, length, flags, MAP_SHARED,\n> -\t\t\t\t camera3Buffer->data[i], 0);\n> -\t\tif (address == MAP_FAILED) {\n> -\t\t\terror_ = -errno;\n> -\t\t\tLOG(HAL, Error) << \"Failed to mmap plane\";\n> -\t\t\tbreak;\n> -\t\t}\n> +\t\tif (fd != -1)\n> +\t\t\tLOG(HAL, Fatal) << \"Discontiguous planes are not supported\";\n\nDo we just warn ? Or exit with error ? See the below question\n\n> +\t\tfd = camera3Buffer->data[i];\n\nThis assigns fd to the 'next' valid data[i].fd. I might have missed\nwhat is the policy here if we have more than one valid dmabuf fds.\n\nCurrently you warn and use the last valid one. I would instead error out,\nor warn but use the first available one (even if that would result in\nundefined behaviour, or most probably an access to non valid memory when\ncreating planes below).\n\nDo you have a use case for formats with planes allocated in different\nbuffers that you need to support ? Or can we error out without causing\nregressions ?\n\n> +\t}\n\nThe loop is pretty convoluted. If I'm not mistaken what you want to\nverify is that\n1) Either all data[i].fd are the same\n2) All but the data[0].fd are -1\n\nWouldn't it be nicer as\n\n int fd = camera3Buffer->data[0].fd;\n for (unsigned int i = 1; i < camera3Buffer->numFds; ++i) {\n if (camera3Buffer->data[i].fd != -1)\n continue;\n\n if (camera3Buffer->data[i].fd != fd)\n /* Here I would error out */\n }\n if (fd == -1) {\n /* No valid fds, error out */\n }\n\n> +\n> +\tif (fd != -1) {\n\nShouldn't this be fd == -1 ? As in \"no valid fd found ?\"\nIf I'm not mistaken checking for fd != -1 should fail all the times as\nat least one valid fd should be found. On which platform have you\ntested this patch ?\n\n> +\t\terror_ = -EINVAL;\n> +\t\tLOG(HAL, Error) << \"No valid file descriptor\";\n> +\t\treturn;\n> +\t}\n>\n> -\t\tmaps_.emplace_back(static_cast(address),\n> -\t\t\t\t static_cast(length));\n> +\tconst auto &info = libcamera::PixelFormatInfo::info(pixelFormat);\n> +\tif (!info.isValid()) {\n> +\t\terror_ = -EINVAL;\n> +\t\tLOG(HAL, Error) << \"Invalid pixel format: \"\n> +\t\t\t\t<< pixelFormat.toString();\n> +\t\treturn;\n> +\t}\n\nWe could check this at the very beginning of the function and avoid\nlooping on fds if the format is invalid.\n\n> +\n> +\toff_t bufferLength = lseek(fd, 0, SEEK_END);\n> +\tif (bufferLength < 0) {\n> +\t\terror_ = -errno;\n> +\t\tLOG(HAL, Error) << \"Failed to get buffer length\";\n> +\t\treturn;\n> +\t}\n> +\n> +\tvoid *address = mmap(nullptr, bufferLength, flags, MAP_SHARED, fd, 0);\n> +\tif (address == MAP_FAILED) {\n> +\t\terror_ = -errno;\n> +\t\tLOG(HAL, Error) << \"Failed to mmap plane\";\n> +\t\treturn;\n> +\t}\n> +\tmaps_.emplace_back(static_cast(address), bufferLength);\n> +\n> +\tconst unsigned int numPlanes = info.numPlanes();\n> +\tplanes_.resize(numPlanes);\n> +\tunsigned int offset = 0;\n> +\tfor (unsigned int i = 0; i < numPlanes; ++i) {\n> +\t\t/*\n> +\t\t * \\todo Remove if this plane size computation function is\n> +\t\t * added to PixelFormatInfo.\n> +\t\t */\n> +\t\tconst unsigned int vertSubSample = info.planes[i].verticalSubSampling;\n> +\t\tconst unsigned int stride = info.stride(size.width, i, 1u);\n> +\t\tconst unsigned int planeSize =\n> +\t\t\tstride * ((size.height + vertSubSample - 1) / vertSubSample);\n> +\n> +\t\tplanes_[i] = libcamera::Span(\n> +\t\t\tstatic_cast(address) + offset, planeSize);\n> +\n> +\t\tif (bufferLength < offset + planeSize) {\n> +\t\t\terror_ = -EINVAL;\n> +\t\t\tLOG(HAL, Error) << \"Plane \" << i << \" is out of buffer\"\n> +\t\t\t\t\t<< \", buffer length=\" << bufferLength\n> +\t\t\t\t\t<< \", offset=\" << offset\n> +\t\t\t\t\t<< \", size=\" << planeSize;\n> +\t\t\treturn;\n> +\t\t}\n> +\t\toffset += planeSize;\n> \t}\n> }\n>\n> @@ -71,15 +127,15 @@ CameraBuffer::Private::~Private()\n>\n> unsigned int CameraBuffer::Private::numPlanes() const\n> {\n> -\treturn maps_.size();\n> +\treturn planes_.size();\n> }\n>\n> Span CameraBuffer::Private::plane(unsigned int plane)\n> {\n> -\tif (plane >= maps_.size())\n> +\tif (plane >= planes_.size())\n> \t\treturn {};\n>\n> -\treturn maps_[plane];\n> +\treturn planes_[plane];\n> }\n>\n> size_t CameraBuffer::Private::jpegBufferSize(size_t maxJpegBufferSize) const\n> --\n> 2.33.0.rc2.250.ged5fa647cd-goog\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id B2BC3BD87D\n\tfor ;\n\tWed, 25 Aug 2021 08:34:27 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 53BFB688A3;\n\tWed, 25 Aug 2021 10:34:21 +0200 (CEST)","from relay9-d.mail.gandi.net (relay9-d.mail.gandi.net\n\t[217.70.183.199])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6EF5560259\n\tfor ;\n\tWed, 25 Aug 2021 10:34:20 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay9-d.mail.gandi.net (Postfix) with ESMTPSA id 2A2B8FF805;\n\tWed, 25 Aug 2021 08:34:18 +0000 (UTC)"],"Date":"Wed, 25 Aug 2021 10:35:07 +0200","From":"Jacopo Mondi ","To":"Hirokazu Honda ","Message-ID":"<20210825083507.5bodpgew6lsllx77@uno.localdomain>","References":"<20210825044410.2787433-1-hiroh@chromium.org>\n\t<20210825044410.2787433-2-hiroh@chromium.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210825044410.2787433-2-hiroh@chromium.org>","Subject":"Re: [libcamera-devel] [PATCH v2 1/3] android:\n\tgeneric_camera_buffer: Correct buffer mapping","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":19048,"web_url":"https://patchwork.libcamera.org/comment/19048/","msgid":"<20210825100048.hyppw5eychr3thmq@uno.localdomain>","date":"2021-08-25T10:00:48","subject":"Re: [libcamera-devel] [PATCH v2 1/3] android:\n\tgeneric_camera_buffer: Correct buffer mapping","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Ups,\n\nOn Wed, Aug 25, 2021 at 10:35:07AM +0200, Jacopo Mondi wrote:\n> Hi Hiro,\n>\n> On Wed, Aug 25, 2021 at 01:44:08PM +0900, Hirokazu Honda wrote:\n> > buffer_hanlde_t doesn't provide sufficient info to map a buffer\n> > properly. cros::CameraBufferManager enables handling the buffer on\n> > ChromeOS, but no way is provided for other platforms.\n> >\n> > Therefore, we put the assumption that planes are in the same buffer\n> > and they are consecutive. This modifies the way of mapping in\n> > generic_camera_buffer with the assumption.\n> >\n> > Signed-off-by: Hirokazu Honda \n> > ---\n> > src/android/camera_buffer.h | 14 ++-\n> > src/android/camera_stream.cpp | 4 +-\n> > src/android/mm/cros_camera_buffer.cpp | 7 +-\n> > src/android/mm/generic_camera_buffer.cpp | 104 +++++++++++++++++------\n> > 4 files changed, 99 insertions(+), 30 deletions(-)\n> >\n> > diff --git a/src/android/camera_buffer.h b/src/android/camera_buffer.h\n> > index c4e3a9e7..87df2570 100644\n> > --- a/src/android/camera_buffer.h\n> > +++ b/src/android/camera_buffer.h\n> > @@ -11,13 +11,17 @@\n> >\n> > #include \n> > #include \n> > +#include \n> > +#include \n> >\n> > class CameraBuffer final : public libcamera::Extensible\n> > {\n> > \tLIBCAMERA_DECLARE_PRIVATE()\n> >\n> > public:\n> > -\tCameraBuffer(buffer_handle_t camera3Buffer, int flags);\n> > +\tCameraBuffer(buffer_handle_t camera3Buffer,\n> > +\t\t libcamera::PixelFormat pixelFormat,\n> > +\t\t const libcamera::Size &size, int flags);\n> > \t~CameraBuffer();\n> >\n> > \tbool isValid() const;\n> > @@ -31,8 +35,12 @@ public:\n> > };\n> >\n> > #define PUBLIC_CAMERA_BUFFER_IMPLEMENTATION\t\t\t\t\\\n> > -CameraBuffer::CameraBuffer(buffer_handle_t camera3Buffer, int flags)\t\\\n> > -\t: Extensible(std::make_unique(this, camera3Buffer, flags)) \\\n> > +CameraBuffer::CameraBuffer(buffer_handle_t camera3Buffer,\t\t\\\n> > +\t\t\t libcamera::PixelFormat pixelFormat,\t\t\\\n> > +\t\t\t const libcamera::Size &size, int flags)\t\\\n> > +\t: Extensible(std::make_unique(this, camera3Buffer,\t\\\n> > +\t\t\t\t\t pixelFormat, size,\t\\\n> > +\t\t\t\t\t flags))\t\t\t\\\n> > {\t\t\t\t\t\t\t\t\t\\\n> > }\t\t\t\t\t\t\t\t\t\\\n> > CameraBuffer::~CameraBuffer()\t\t\t\t\t\t\\\n> > diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp\n> > index 61b44183..01909ec7 100644\n> > --- a/src/android/camera_stream.cpp\n> > +++ b/src/android/camera_stream.cpp\n> > @@ -110,7 +110,9 @@ int CameraStream::process(const libcamera::FrameBuffer &source,\n> > \t * \\todo Buffer mapping and processing should be moved to a\n> > \t * separate thread.\n> > \t */\n> > -\tCameraBuffer dest(camera3Dest, PROT_READ | PROT_WRITE);\n> > +\tconst StreamConfiguration &output = configuration();\n> > +\tCameraBuffer dest(camera3Dest, formats::MJPEG, output.size,\n> > +\t\t\t PROT_READ | PROT_WRITE);\n> > \tif (!dest.isValid()) {\n> > \t\tLOG(HAL, Error) << \"Failed to map android blob buffer\";\n> > \t\treturn -EINVAL;\n> > diff --git a/src/android/mm/cros_camera_buffer.cpp b/src/android/mm/cros_camera_buffer.cpp\n> > index e8783ff8..50732637 100644\n> > --- a/src/android/mm/cros_camera_buffer.cpp\n> > +++ b/src/android/mm/cros_camera_buffer.cpp\n> > @@ -20,8 +20,9 @@ class CameraBuffer::Private : public Extensible::Private\n> > \tLIBCAMERA_DECLARE_PUBLIC(CameraBuffer)\n> >\n> > public:\n> > -\tPrivate(CameraBuffer *cameraBuffer,\n> > -\t\tbuffer_handle_t camera3Buffer, int flags);\n> > +\tPrivate(CameraBuffer *cameraBuffer, buffer_handle_t camera3Buffer,\n> > +\t\tlibcamera::PixelFormat pixelFormat, const libcamera::Size &size,\n> > +\t\tint flags);\n> > \t~Private();\n> >\n> > \tbool isValid() const { return valid_; }\n> > @@ -46,6 +47,8 @@ private:\n> >\n> > CameraBuffer::Private::Private([[maybe_unused]] CameraBuffer *cameraBuffer,\n> > \t\t\t buffer_handle_t camera3Buffer,\n> > +\t\t\t [[maybe_unused]] libcamera::PixelFormat pixelFormat,\n> > +\t\t\t [[maybe_unused]] const libcamera::Size &size,\n> > \t\t\t [[maybe_unused]] int flags)\n> > \t: handle_(camera3Buffer), numPlanes_(0), valid_(false),\n> > \t registered_(false)\n> > diff --git a/src/android/mm/generic_camera_buffer.cpp b/src/android/mm/generic_camera_buffer.cpp\n> > index b3af194c..0bfdc2ba 100644\n> > --- a/src/android/mm/generic_camera_buffer.cpp\n> > +++ b/src/android/mm/generic_camera_buffer.cpp\n> > @@ -12,6 +12,7 @@\n> >\n> > #include \n> >\n> > +#include \"libcamera/internal/formats.h\"\n> > #include \"libcamera/internal/mapped_framebuffer.h\"\n> >\n> > using namespace libcamera;\n> > @@ -24,8 +25,9 @@ class CameraBuffer::Private : public Extensible::Private,\n> > \tLIBCAMERA_DECLARE_PUBLIC(CameraBuffer)\n> >\n> > public:\n> > -\tPrivate(CameraBuffer *cameraBuffer,\n> > -\t\tbuffer_handle_t camera3Buffer, int flags);\n> > +\tPrivate(CameraBuffer *cameraBuffer, buffer_handle_t camera3Buffer,\n> > +\t\tlibcamera::PixelFormat pixelFormat, const libcamera::Size &size,\n> > +\t\tint flags);\n> > \t~Private();\n> >\n> > \tunsigned int numPlanes() const;\n> > @@ -33,35 +35,89 @@ public:\n> > \tSpan plane(unsigned int plane);\n> >\n> > \tsize_t jpegBufferSize(size_t maxJpegBufferSize) const;\n> > +\n> > +private:\n> > +\t/* \\todo Remove planes_ when it will be added to MappedBuffer */\n> > +\tstd::vector> planes_;\n> > };\n> >\n> > CameraBuffer::Private::Private([[maybe_unused]] CameraBuffer *cameraBuffer,\n> > -\t\t\t buffer_handle_t camera3Buffer, int flags)\n> > +\t\t\t buffer_handle_t camera3Buffer,\n> > +\t\t\t libcamera::PixelFormat pixelFormat,\n> > +\t\t\t const libcamera::Size &size, int flags)\n> > {\n> > -\tmaps_.reserve(camera3Buffer->numFds);\n> > \terror_ = 0;\n> > -\n> > +\t/*\n> > +\t * As Android doesn't offer an API to query buffer layouts, assume for\n> > +\t * now that the buffer is backed by a single dmabuf, with planes being\n> > +\t * stored contiguously.\n> > +\t */\n> > +\tint fd = -1;\n> > \tfor (int i = 0; i < camera3Buffer->numFds; i++) {\n>\n> unsigned int i\n>\n> > -\t\tif (camera3Buffer->data[i] == -1)\n> > +\t\tif (camera3Buffer->data[i] == -1 ||\n> > +\t\t camera3Buffer->data[i] == fd) {\n> > \t\t\tcontinue;\n> > -\n> > -\t\toff_t length = lseek(camera3Buffer->data[i], 0, SEEK_END);\n> > -\t\tif (length < 0) {\n> > -\t\t\terror_ = -errno;\n> > -\t\t\tLOG(HAL, Error) << \"Failed to query plane length\";\n> > -\t\t\tbreak;\n> > \t\t}\n> >\n> > -\t\tvoid *address = mmap(nullptr, length, flags, MAP_SHARED,\n> > -\t\t\t\t camera3Buffer->data[i], 0);\n> > -\t\tif (address == MAP_FAILED) {\n> > -\t\t\terror_ = -errno;\n> > -\t\t\tLOG(HAL, Error) << \"Failed to mmap plane\";\n> > -\t\t\tbreak;\n> > -\t\t}\n> > +\t\tif (fd != -1)\n> > +\t\t\tLOG(HAL, Fatal) << \"Discontiguous planes are not supported\";\n>\n> Do we just warn ? Or exit with error ? See the below question\n>\n> > +\t\tfd = camera3Buffer->data[i];\n>\n> This assigns fd to the 'next' valid data[i].fd. I might have missed\n> what is the policy here if we have more than one valid dmabuf fds.\n>\n> Currently you warn and use the last valid one. I would instead error out,\n> or warn but use the first available one (even if that would result in\n> undefined behaviour, or most probably an access to non valid memory when\n> creating planes below).\n>\n> Do you have a use case for formats with planes allocated in different\n> buffers that you need to support ? Or can we error out without causing\n> regressions ?\n>\n> > +\t}\n>\n> The loop is pretty convoluted. If I'm not mistaken what you want to\n> verify is that\n> 1) Either all data[i].fd are the same\n> 2) All but the data[0].fd are -1\n>\n> Wouldn't it be nicer as\n>\n> int fd = camera3Buffer->data[0].fd;\n> for (unsigned int i = 1; i < camera3Buffer->numFds; ++i) {\n> if (camera3Buffer->data[i].fd != -1)\n\n I meant == -1\n> continue;\n>\n> if (camera3Buffer->data[i].fd != fd)\n> /* Here I would error out */\n> }\n> if (fd == -1) {\n> /* No valid fds, error out */\n> }\n>\n> > +\n> > +\tif (fd != -1) {\n>\n> Shouldn't this be fd == -1 ? As in \"no valid fd found ?\"\n> If I'm not mistaken checking for fd != -1 should fail all the times as\n> at least one valid fd should be found. On which platform have you\n> tested this patch ?\n>\n> > +\t\terror_ = -EINVAL;\n> > +\t\tLOG(HAL, Error) << \"No valid file descriptor\";\n> > +\t\treturn;\n> > +\t}\n> >\n> > -\t\tmaps_.emplace_back(static_cast(address),\n> > -\t\t\t\t static_cast(length));\n> > +\tconst auto &info = libcamera::PixelFormatInfo::info(pixelFormat);\n> > +\tif (!info.isValid()) {\n> > +\t\terror_ = -EINVAL;\n> > +\t\tLOG(HAL, Error) << \"Invalid pixel format: \"\n> > +\t\t\t\t<< pixelFormat.toString();\n> > +\t\treturn;\n> > +\t}\n>\n> We could check this at the very beginning of the function and avoid\n> looping on fds if the format is invalid.\n>\n> > +\n> > +\toff_t bufferLength = lseek(fd, 0, SEEK_END);\n> > +\tif (bufferLength < 0) {\n> > +\t\terror_ = -errno;\n> > +\t\tLOG(HAL, Error) << \"Failed to get buffer length\";\n> > +\t\treturn;\n> > +\t}\n> > +\n> > +\tvoid *address = mmap(nullptr, bufferLength, flags, MAP_SHARED, fd, 0);\n> > +\tif (address == MAP_FAILED) {\n> > +\t\terror_ = -errno;\n> > +\t\tLOG(HAL, Error) << \"Failed to mmap plane\";\n> > +\t\treturn;\n> > +\t}\n> > +\tmaps_.emplace_back(static_cast(address), bufferLength);\n> > +\n> > +\tconst unsigned int numPlanes = info.numPlanes();\n> > +\tplanes_.resize(numPlanes);\n> > +\tunsigned int offset = 0;\n> > +\tfor (unsigned int i = 0; i < numPlanes; ++i) {\n> > +\t\t/*\n> > +\t\t * \\todo Remove if this plane size computation function is\n> > +\t\t * added to PixelFormatInfo.\n> > +\t\t */\n> > +\t\tconst unsigned int vertSubSample = info.planes[i].verticalSubSampling;\n> > +\t\tconst unsigned int stride = info.stride(size.width, i, 1u);\n> > +\t\tconst unsigned int planeSize =\n> > +\t\t\tstride * ((size.height + vertSubSample - 1) / vertSubSample);\n> > +\n> > +\t\tplanes_[i] = libcamera::Span(\n> > +\t\t\tstatic_cast(address) + offset, planeSize);\n> > +\n> > +\t\tif (bufferLength < offset + planeSize) {\n> > +\t\t\terror_ = -EINVAL;\n> > +\t\t\tLOG(HAL, Error) << \"Plane \" << i << \" is out of buffer\"\n> > +\t\t\t\t\t<< \", buffer length=\" << bufferLength\n> > +\t\t\t\t\t<< \", offset=\" << offset\n> > +\t\t\t\t\t<< \", size=\" << planeSize;\n> > +\t\t\treturn;\n> > +\t\t}\n> > +\t\toffset += planeSize;\n> > \t}\n> > }\n> >\n> > @@ -71,15 +127,15 @@ CameraBuffer::Private::~Private()\n> >\n> > unsigned int CameraBuffer::Private::numPlanes() const\n> > {\n> > -\treturn maps_.size();\n> > +\treturn planes_.size();\n> > }\n> >\n> > Span CameraBuffer::Private::plane(unsigned int plane)\n> > {\n> > -\tif (plane >= maps_.size())\n> > +\tif (plane >= planes_.size())\n> > \t\treturn {};\n> >\n> > -\treturn maps_[plane];\n> > +\treturn planes_[plane];\n> > }\n> >\n> > size_t CameraBuffer::Private::jpegBufferSize(size_t maxJpegBufferSize) const\n> > --\n> > 2.33.0.rc2.250.ged5fa647cd-goog\n> >","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id CC16DBD87D\n\tfor ;\n\tWed, 25 Aug 2021 10:00:02 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id ECAEB688A3;\n\tWed, 25 Aug 2021 12:00:01 +0200 (CEST)","from relay6-d.mail.gandi.net (relay6-d.mail.gandi.net\n\t[217.70.183.198])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 9600560259\n\tfor ;\n\tWed, 25 Aug 2021 12:00:00 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay6-d.mail.gandi.net (Postfix) with ESMTPSA id 6D19DC0005;\n\tWed, 25 Aug 2021 09:59:59 +0000 (UTC)"],"Date":"Wed, 25 Aug 2021 12:00:48 +0200","From":"Jacopo Mondi ","To":"Hirokazu Honda ","Message-ID":"<20210825100048.hyppw5eychr3thmq@uno.localdomain>","References":"<20210825044410.2787433-1-hiroh@chromium.org>\n\t<20210825044410.2787433-2-hiroh@chromium.org>\n\t<20210825083507.5bodpgew6lsllx77@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210825083507.5bodpgew6lsllx77@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH v2 1/3] android:\n\tgeneric_camera_buffer: Correct buffer mapping","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":19052,"web_url":"https://patchwork.libcamera.org/comment/19052/","msgid":"","date":"2021-08-25T10:57:23","subject":"Re: [libcamera-devel] [PATCH v2 1/3] android:\n\tgeneric_camera_buffer: Correct buffer mapping","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"Hi Jacopo, thank you for reviewing.\n\n\nOn Wed, Aug 25, 2021 at 7:00 PM Jacopo Mondi wrote:\n>\n> Ups,\n>\n> On Wed, Aug 25, 2021 at 10:35:07AM +0200, Jacopo Mondi wrote:\n> > Hi Hiro,\n> >\n> > On Wed, Aug 25, 2021 at 01:44:08PM +0900, Hirokazu Honda wrote:\n> > > buffer_hanlde_t doesn't provide sufficient info to map a buffer\n> > > properly. cros::CameraBufferManager enables handling the buffer on\n> > > ChromeOS, but no way is provided for other platforms.\n> > >\n> > > Therefore, we put the assumption that planes are in the same buffer\n> > > and they are consecutive. This modifies the way of mapping in\n> > > generic_camera_buffer with the assumption.\n> > >\n> > > Signed-off-by: Hirokazu Honda \n> > > ---\n> > > src/android/camera_buffer.h | 14 ++-\n> > > src/android/camera_stream.cpp | 4 +-\n> > > src/android/mm/cros_camera_buffer.cpp | 7 +-\n> > > src/android/mm/generic_camera_buffer.cpp | 104 +++++++++++++++++------\n> > > 4 files changed, 99 insertions(+), 30 deletions(-)\n> > >\n> > > diff --git a/src/android/camera_buffer.h b/src/android/camera_buffer.h\n> > > index c4e3a9e7..87df2570 100644\n> > > --- a/src/android/camera_buffer.h\n> > > +++ b/src/android/camera_buffer.h\n> > > @@ -11,13 +11,17 @@\n> > >\n> > > #include \n> > > #include \n> > > +#include \n> > > +#include \n> > >\n> > > class CameraBuffer final : public libcamera::Extensible\n> > > {\n> > > LIBCAMERA_DECLARE_PRIVATE()\n> > >\n> > > public:\n> > > - CameraBuffer(buffer_handle_t camera3Buffer, int flags);\n> > > + CameraBuffer(buffer_handle_t camera3Buffer,\n> > > + libcamera::PixelFormat pixelFormat,\n> > > + const libcamera::Size &size, int flags);\n> > > ~CameraBuffer();\n> > >\n> > > bool isValid() const;\n> > > @@ -31,8 +35,12 @@ public:\n> > > };\n> > >\n> > > #define PUBLIC_CAMERA_BUFFER_IMPLEMENTATION \\\n> > > -CameraBuffer::CameraBuffer(buffer_handle_t camera3Buffer, int flags) \\\n> > > - : Extensible(std::make_unique(this, camera3Buffer, flags)) \\\n> > > +CameraBuffer::CameraBuffer(buffer_handle_t camera3Buffer, \\\n> > > + libcamera::PixelFormat pixelFormat, \\\n> > > + const libcamera::Size &size, int flags) \\\n> > > + : Extensible(std::make_unique(this, camera3Buffer, \\\n> > > + pixelFormat, size, \\\n> > > + flags)) \\\n> > > { \\\n> > > } \\\n> > > CameraBuffer::~CameraBuffer() \\\n> > > diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp\n> > > index 61b44183..01909ec7 100644\n> > > --- a/src/android/camera_stream.cpp\n> > > +++ b/src/android/camera_stream.cpp\n> > > @@ -110,7 +110,9 @@ int CameraStream::process(const libcamera::FrameBuffer &source,\n> > > * \\todo Buffer mapping and processing should be moved to a\n> > > * separate thread.\n> > > */\n> > > - CameraBuffer dest(camera3Dest, PROT_READ | PROT_WRITE);\n> > > + const StreamConfiguration &output = configuration();\n> > > + CameraBuffer dest(camera3Dest, formats::MJPEG, output.size,\n> > > + PROT_READ | PROT_WRITE);\n> > > if (!dest.isValid()) {\n> > > LOG(HAL, Error) << \"Failed to map android blob buffer\";\n> > > return -EINVAL;\n> > > diff --git a/src/android/mm/cros_camera_buffer.cpp b/src/android/mm/cros_camera_buffer.cpp\n> > > index e8783ff8..50732637 100644\n> > > --- a/src/android/mm/cros_camera_buffer.cpp\n> > > +++ b/src/android/mm/cros_camera_buffer.cpp\n> > > @@ -20,8 +20,9 @@ class CameraBuffer::Private : public Extensible::Private\n> > > LIBCAMERA_DECLARE_PUBLIC(CameraBuffer)\n> > >\n> > > public:\n> > > - Private(CameraBuffer *cameraBuffer,\n> > > - buffer_handle_t camera3Buffer, int flags);\n> > > + Private(CameraBuffer *cameraBuffer, buffer_handle_t camera3Buffer,\n> > > + libcamera::PixelFormat pixelFormat, const libcamera::Size &size,\n> > > + int flags);\n> > > ~Private();\n> > >\n> > > bool isValid() const { return valid_; }\n> > > @@ -46,6 +47,8 @@ private:\n> > >\n> > > CameraBuffer::Private::Private([[maybe_unused]] CameraBuffer *cameraBuffer,\n> > > buffer_handle_t camera3Buffer,\n> > > + [[maybe_unused]] libcamera::PixelFormat pixelFormat,\n> > > + [[maybe_unused]] const libcamera::Size &size,\n> > > [[maybe_unused]] int flags)\n> > > : handle_(camera3Buffer), numPlanes_(0), valid_(false),\n> > > registered_(false)\n> > > diff --git a/src/android/mm/generic_camera_buffer.cpp b/src/android/mm/generic_camera_buffer.cpp\n> > > index b3af194c..0bfdc2ba 100644\n> > > --- a/src/android/mm/generic_camera_buffer.cpp\n> > > +++ b/src/android/mm/generic_camera_buffer.cpp\n> > > @@ -12,6 +12,7 @@\n> > >\n> > > #include \n> > >\n> > > +#include \"libcamera/internal/formats.h\"\n> > > #include \"libcamera/internal/mapped_framebuffer.h\"\n> > >\n> > > using namespace libcamera;\n> > > @@ -24,8 +25,9 @@ class CameraBuffer::Private : public Extensible::Private,\n> > > LIBCAMERA_DECLARE_PUBLIC(CameraBuffer)\n> > >\n> > > public:\n> > > - Private(CameraBuffer *cameraBuffer,\n> > > - buffer_handle_t camera3Buffer, int flags);\n> > > + Private(CameraBuffer *cameraBuffer, buffer_handle_t camera3Buffer,\n> > > + libcamera::PixelFormat pixelFormat, const libcamera::Size &size,\n> > > + int flags);\n> > > ~Private();\n> > >\n> > > unsigned int numPlanes() const;\n> > > @@ -33,35 +35,89 @@ public:\n> > > Span plane(unsigned int plane);\n> > >\n> > > size_t jpegBufferSize(size_t maxJpegBufferSize) const;\n> > > +\n> > > +private:\n> > > + /* \\todo Remove planes_ when it will be added to MappedBuffer */\n> > > + std::vector> planes_;\n> > > };\n> > >\n> > > CameraBuffer::Private::Private([[maybe_unused]] CameraBuffer *cameraBuffer,\n> > > - buffer_handle_t camera3Buffer, int flags)\n> > > + buffer_handle_t camera3Buffer,\n> > > + libcamera::PixelFormat pixelFormat,\n> > > + const libcamera::Size &size, int flags)\n> > > {\n> > > - maps_.reserve(camera3Buffer->numFds);\n> > > error_ = 0;\n> > > -\n> > > + /*\n> > > + * As Android doesn't offer an API to query buffer layouts, assume for\n> > > + * now that the buffer is backed by a single dmabuf, with planes being\n> > > + * stored contiguously.\n> > > + */\n> > > + int fd = -1;\n> > > for (int i = 0; i < camera3Buffer->numFds; i++) {\n> >\n> > unsigned int i\n\nThe type of buffer_handle_t's numFds is int strangely. :-S\nIf I use unsigned int here, a compiler complains the comparison against numFds.\n\n> >\n> > > - if (camera3Buffer->data[i] == -1)\n> > > + if (camera3Buffer->data[i] == -1 ||\n> > > + camera3Buffer->data[i] == fd) {\n> > > continue;\n> > > -\n> > > - off_t length = lseek(camera3Buffer->data[i], 0, SEEK_END);\n> > > - if (length < 0) {\n> > > - error_ = -errno;\n> > > - LOG(HAL, Error) << \"Failed to query plane length\";\n> > > - break;\n> > > }\n> > >\n> > > - void *address = mmap(nullptr, length, flags, MAP_SHARED,\n> > > - camera3Buffer->data[i], 0);\n> > > - if (address == MAP_FAILED) {\n> > > - error_ = -errno;\n> > > - LOG(HAL, Error) << \"Failed to mmap plane\";\n> > > - break;\n> > > - }\n> > > + if (fd != -1)\n> > > + LOG(HAL, Fatal) << \"Discontiguous planes are not supported\";\n> >\n> > Do we just warn ? Or exit with error ? See the below question\n> >\n> > > + fd = camera3Buffer->data[i];\n> >\n> > This assigns fd to the 'next' valid data[i].fd. I might have missed\n> > what is the policy here if we have more than one valid dmabuf fds.\n> >\n> > Currently you warn and use the last valid one. I would instead error out,\n> > or warn but use the first available one (even if that would result in\n> > undefined behaviour, or most probably an access to non valid memory when\n> > creating planes below).\n> >\n> > Do you have a use case for formats with planes allocated in different\n> > buffers that you need to support ? Or can we error out without causing\n> > regressions ?\n> >\n\nThis is just from Laurent's comment.\nI think no platform except ChromeOS has used libcamera Android HAL.\nTo catch something bad easily, we implement so.\n\n> > > + }\n> >\n> > The loop is pretty convoluted. If I'm not mistaken what you want to\n> > verify is that\n> > 1) Either all data[i].fd are the same\n> > 2) All but the data[0].fd are -1\n> >\n> > Wouldn't it be nicer as\n> >\n> > int fd = camera3Buffer->data[0].fd;\n> > for (unsigned int i = 1; i < camera3Buffer->numFds; ++i) {\n> > if (camera3Buffer->data[i].fd != -1)\n>\n> I meant == -1\n> > continue;\n> >\n> > if (camera3Buffer->data[i].fd != fd)\n> > /* Here I would error out */\n> > }\n> > if (fd == -1) {\n> > /* No valid fds, error out */\n> > }\n> >\n> > > +\n> > > + if (fd != -1) {\n> >\n> > Shouldn't this be fd == -1 ? As in \"no valid fd found ?\"\n> > If I'm not mistaken checking for fd != -1 should fail all the times as\n> > at least one valid fd should be found. On which platform have you\n> > tested this patch ?\n> >\n\nThanks for catching this. I haven't tested this implementation as I am\ntesting on ChromeOS.\nIs there any easy way of testing android HAL implementation on Linux?\n\n-Hiro\n> > > + error_ = -EINVAL;\n> > > + LOG(HAL, Error) << \"No valid file descriptor\";\n> > > + return;\n> > > + }\n> > >\n> > > - maps_.emplace_back(static_cast(address),\n> > > - static_cast(length));\n> > > + const auto &info = libcamera::PixelFormatInfo::info(pixelFormat);\n> > > + if (!info.isValid()) {\n> > > + error_ = -EINVAL;\n> > > + LOG(HAL, Error) << \"Invalid pixel format: \"\n> > > + << pixelFormat.toString();\n> > > + return;\n> > > + }\n> >\n> > We could check this at the very beginning of the function and avoid\n> > looping on fds if the format is invalid.\n> >\n> > > +\n> > > + off_t bufferLength = lseek(fd, 0, SEEK_END);\n> > > + if (bufferLength < 0) {\n> > > + error_ = -errno;\n> > > + LOG(HAL, Error) << \"Failed to get buffer length\";\n> > > + return;\n> > > + }\n> > > +\n> > > + void *address = mmap(nullptr, bufferLength, flags, MAP_SHARED, fd, 0);\n> > > + if (address == MAP_FAILED) {\n> > > + error_ = -errno;\n> > > + LOG(HAL, Error) << \"Failed to mmap plane\";\n> > > + return;\n> > > + }\n> > > + maps_.emplace_back(static_cast(address), bufferLength);\n> > > +\n> > > + const unsigned int numPlanes = info.numPlanes();\n> > > + planes_.resize(numPlanes);\n> > > + unsigned int offset = 0;\n> > > + for (unsigned int i = 0; i < numPlanes; ++i) {\n> > > + /*\n> > > + * \\todo Remove if this plane size computation function is\n> > > + * added to PixelFormatInfo.\n> > > + */\n> > > + const unsigned int vertSubSample = info.planes[i].verticalSubSampling;\n> > > + const unsigned int stride = info.stride(size.width, i, 1u);\n> > > + const unsigned int planeSize =\n> > > + stride * ((size.height + vertSubSample - 1) / vertSubSample);\n> > > +\n> > > + planes_[i] = libcamera::Span(\n> > > + static_cast(address) + offset, planeSize);\n> > > +\n> > > + if (bufferLength < offset + planeSize) {\n> > > + error_ = -EINVAL;\n> > > + LOG(HAL, Error) << \"Plane \" << i << \" is out of buffer\"\n> > > + << \", buffer length=\" << bufferLength\n> > > + << \", offset=\" << offset\n> > > + << \", size=\" << planeSize;\n> > > + return;\n> > > + }\n> > > + offset += planeSize;\n> > > }\n> > > }\n> > >\n> > > @@ -71,15 +127,15 @@ CameraBuffer::Private::~Private()\n> > >\n> > > unsigned int CameraBuffer::Private::numPlanes() const\n> > > {\n> > > - return maps_.size();\n> > > + return planes_.size();\n> > > }\n> > >\n> > > Span CameraBuffer::Private::plane(unsigned int plane)\n> > > {\n> > > - if (plane >= maps_.size())\n> > > + if (plane >= planes_.size())\n> > > return {};\n> > >\n> > > - return maps_[plane];\n> > > + return planes_[plane];\n> > > }\n> > >\n> > > size_t CameraBuffer::Private::jpegBufferSize(size_t maxJpegBufferSize) const\n> > > --\n> > > 2.33.0.rc2.250.ged5fa647cd-goog\n> > >","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 7D9DABD87D\n\tfor ;\n\tWed, 25 Aug 2021 10:57:36 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E4477688A5;\n\tWed, 25 Aug 2021 12:57:35 +0200 (CEST)","from mail-ej1-x62e.google.com (mail-ej1-x62e.google.com\n\t[IPv6:2a00:1450:4864:20::62e])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E5B0860259\n\tfor ;\n\tWed, 25 Aug 2021 12:57:34 +0200 (CEST)","by mail-ej1-x62e.google.com with SMTP id n27so18687172eja.5\n\tfor ;\n\tWed, 25 Aug 2021 03:57:34 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"nKVjdukO\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org;\n\ts=google; \n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=NGzP5UM4Pqp/vAtk51Wk18NK8VEKH2hYcDqEojA3huk=;\n\tb=nKVjdukOinWl5KWb1lCbjj+Jvs1ZRCGUB04VOTAU0Eov3FUG0GdXD8A/HbSFmOI6tH\n\trZ/UPSmIdNiGjqbEO0wuzasC0n5JxCPs5u/1fVFUULMkSwGcZqjM8h+M3BQudXwGzeOQ\n\tT+LNdDgsVf85Xg4CzpsINQp/jwzoQqWA2mlXE=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=NGzP5UM4Pqp/vAtk51Wk18NK8VEKH2hYcDqEojA3huk=;\n\tb=YJ1AdY2f9Sp40N5d8VLnrFYZXoNCfrw4jGejZ8Rm7lzPTo2KUqgDtOMFVIZk4nQ1O1\n\tTRYgeyECXqY9ouKTi1oAQvelrTMoxe0oM1gBfR3MHRwHOjPRx9lz7ysiXPlye1mQcLE7\n\tKhICWK1ajbCk6gKBrGNaeOOP/lcJhRYUWWiuBGyc8BwhQyCdWPAQjQQ4VZMccY3HkPE7\n\tZCw5T8rMbWVzua7PJCCtNZTAnRDa6lfoP0PnIZ0cFe1tU5NeCtwXyJxDCA9Z2oy14VX5\n\tXhMeHfYbXb2izdFyprvctV6NpZQx25Xoemz2zUjdBvn95hCxTZ4bG02hAoYZ3CwVaFKP\n\tJtUA==","X-Gm-Message-State":"AOAM533bcUijFvgsWB/Yio80n++undbAXrvFR1xxQVvRFGUXR9ZhrNrF\n\tpiWEpMTeVa0Oin+RfZDhsWjysYowzPkzGlkEZ+Dw/A==","X-Google-Smtp-Source":"ABdhPJzBZ1bttZIvFOZr7+1FZEVuaJnkWl0WZUCxUk/zOb13/6PoI9Hz6UdQAVIwF5N0RcPyHBvcTyQw9FKKtznsUzE=","X-Received":"by 2002:a17:907:969f:: with SMTP id\n\thd31mr23495456ejc.475.1629889054314; \n\tWed, 25 Aug 2021 03:57:34 -0700 (PDT)","MIME-Version":"1.0","References":"<20210825044410.2787433-1-hiroh@chromium.org>\n\t<20210825044410.2787433-2-hiroh@chromium.org>\n\t<20210825083507.5bodpgew6lsllx77@uno.localdomain>\n\t<20210825100048.hyppw5eychr3thmq@uno.localdomain>","In-Reply-To":"<20210825100048.hyppw5eychr3thmq@uno.localdomain>","From":"Hirokazu Honda ","Date":"Wed, 25 Aug 2021 19:57:23 +0900","Message-ID":"","To":"Jacopo Mondi ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH v2 1/3] android:\n\tgeneric_camera_buffer: Correct buffer mapping","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":19072,"web_url":"https://patchwork.libcamera.org/comment/19072/","msgid":"","date":"2021-08-25T20:18:01","subject":"Re: [libcamera-devel] [PATCH v2 1/3] android:\n\tgeneric_camera_buffer: Correct buffer mapping","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Hiro,\n\nOn Wed, Aug 25, 2021 at 07:57:23PM +0900, Hirokazu Honda wrote:\n> On Wed, Aug 25, 2021 at 7:00 PM Jacopo Mondi wrote:\n> > On Wed, Aug 25, 2021 at 10:35:07AM +0200, Jacopo Mondi wrote:\n> > > On Wed, Aug 25, 2021 at 01:44:08PM +0900, Hirokazu Honda wrote:\n> > > > buffer_hanlde_t doesn't provide sufficient info to map a buffer\n\ns/buffer_hanlde_t/buffer_handle_t/\n\n> > > > properly. cros::CameraBufferManager enables handling the buffer on\n> > > > ChromeOS, but no way is provided for other platforms.\n> > > >\n> > > > Therefore, we put the assumption that planes are in the same buffer\n> > > > and they are consecutive. This modifies the way of mapping in\n> > > > generic_camera_buffer with the assumption.\n> > > >\n> > > > Signed-off-by: Hirokazu Honda \n> > > > ---\n> > > > src/android/camera_buffer.h | 14 ++-\n> > > > src/android/camera_stream.cpp | 4 +-\n> > > > src/android/mm/cros_camera_buffer.cpp | 7 +-\n> > > > src/android/mm/generic_camera_buffer.cpp | 104 +++++++++++++++++------\n> > > > 4 files changed, 99 insertions(+), 30 deletions(-)\n> > > >\n> > > > diff --git a/src/android/camera_buffer.h b/src/android/camera_buffer.h\n> > > > index c4e3a9e7..87df2570 100644\n> > > > --- a/src/android/camera_buffer.h\n> > > > +++ b/src/android/camera_buffer.h\n> > > > @@ -11,13 +11,17 @@\n> > > >\n> > > > #include \n> > > > #include \n> > > > +#include \n> > > > +#include \n> > > >\n> > > > class CameraBuffer final : public libcamera::Extensible\n> > > > {\n> > > > LIBCAMERA_DECLARE_PRIVATE()\n> > > >\n> > > > public:\n> > > > - CameraBuffer(buffer_handle_t camera3Buffer, int flags);\n> > > > + CameraBuffer(buffer_handle_t camera3Buffer,\n> > > > + libcamera::PixelFormat pixelFormat,\n> > > > + const libcamera::Size &size, int flags);\n> > > > ~CameraBuffer();\n> > > >\n> > > > bool isValid() const;\n> > > > @@ -31,8 +35,12 @@ public:\n> > > > };\n> > > >\n> > > > #define PUBLIC_CAMERA_BUFFER_IMPLEMENTATION \\\n> > > > -CameraBuffer::CameraBuffer(buffer_handle_t camera3Buffer, int flags) \\\n> > > > - : Extensible(std::make_unique(this, camera3Buffer, flags)) \\\n> > > > +CameraBuffer::CameraBuffer(buffer_handle_t camera3Buffer, \\\n> > > > + libcamera::PixelFormat pixelFormat, \\\n> > > > + const libcamera::Size &size, int flags) \\\n> > > > + : Extensible(std::make_unique(this, camera3Buffer, \\\n> > > > + pixelFormat, size, \\\n> > > > + flags)) \\\n> > > > { \\\n> > > > } \\\n> > > > CameraBuffer::~CameraBuffer() \\\n> > > > diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp\n> > > > index 61b44183..01909ec7 100644\n> > > > --- a/src/android/camera_stream.cpp\n> > > > +++ b/src/android/camera_stream.cpp\n> > > > @@ -110,7 +110,9 @@ int CameraStream::process(const libcamera::FrameBuffer &source,\n> > > > * \\todo Buffer mapping and processing should be moved to a\n> > > > * separate thread.\n> > > > */\n> > > > - CameraBuffer dest(camera3Dest, PROT_READ | PROT_WRITE);\n> > > > + const StreamConfiguration &output = configuration();\n> > > > + CameraBuffer dest(camera3Dest, formats::MJPEG, output.size,\n> > > > + PROT_READ | PROT_WRITE);\n> > > > if (!dest.isValid()) {\n> > > > LOG(HAL, Error) << \"Failed to map android blob buffer\";\n> > > > return -EINVAL;\n> > > > diff --git a/src/android/mm/cros_camera_buffer.cpp b/src/android/mm/cros_camera_buffer.cpp\n> > > > index e8783ff8..50732637 100644\n> > > > --- a/src/android/mm/cros_camera_buffer.cpp\n> > > > +++ b/src/android/mm/cros_camera_buffer.cpp\n> > > > @@ -20,8 +20,9 @@ class CameraBuffer::Private : public Extensible::Private\n> > > > LIBCAMERA_DECLARE_PUBLIC(CameraBuffer)\n> > > >\n> > > > public:\n> > > > - Private(CameraBuffer *cameraBuffer,\n> > > > - buffer_handle_t camera3Buffer, int flags);\n> > > > + Private(CameraBuffer *cameraBuffer, buffer_handle_t camera3Buffer,\n> > > > + libcamera::PixelFormat pixelFormat, const libcamera::Size &size,\n> > > > + int flags);\n> > > > ~Private();\n> > > >\n> > > > bool isValid() const { return valid_; }\n> > > > @@ -46,6 +47,8 @@ private:\n> > > >\n> > > > CameraBuffer::Private::Private([[maybe_unused]] CameraBuffer *cameraBuffer,\n> > > > buffer_handle_t camera3Buffer,\n> > > > + [[maybe_unused]] libcamera::PixelFormat pixelFormat,\n> > > > + [[maybe_unused]] const libcamera::Size &size,\n> > > > [[maybe_unused]] int flags)\n> > > > : handle_(camera3Buffer), numPlanes_(0), valid_(false),\n> > > > registered_(false)\n> > > > diff --git a/src/android/mm/generic_camera_buffer.cpp b/src/android/mm/generic_camera_buffer.cpp\n> > > > index b3af194c..0bfdc2ba 100644\n> > > > --- a/src/android/mm/generic_camera_buffer.cpp\n> > > > +++ b/src/android/mm/generic_camera_buffer.cpp\n> > > > @@ -12,6 +12,7 @@\n> > > >\n> > > > #include \n> > > >\n> > > > +#include \"libcamera/internal/formats.h\"\n> > > > #include \"libcamera/internal/mapped_framebuffer.h\"\n> > > >\n> > > > using namespace libcamera;\n> > > > @@ -24,8 +25,9 @@ class CameraBuffer::Private : public Extensible::Private,\n> > > > LIBCAMERA_DECLARE_PUBLIC(CameraBuffer)\n> > > >\n> > > > public:\n> > > > - Private(CameraBuffer *cameraBuffer,\n> > > > - buffer_handle_t camera3Buffer, int flags);\n> > > > + Private(CameraBuffer *cameraBuffer, buffer_handle_t camera3Buffer,\n> > > > + libcamera::PixelFormat pixelFormat, const libcamera::Size &size,\n> > > > + int flags);\n> > > > ~Private();\n> > > >\n> > > > unsigned int numPlanes() const;\n> > > > @@ -33,35 +35,89 @@ public:\n> > > > Span plane(unsigned int plane);\n> > > >\n> > > > size_t jpegBufferSize(size_t maxJpegBufferSize) const;\n> > > > +\n> > > > +private:\n> > > > + /* \\todo Remove planes_ when it will be added to MappedBuffer */\n> > > > + std::vector> planes_;\n> > > > };\n> > > >\n> > > > CameraBuffer::Private::Private([[maybe_unused]] CameraBuffer *cameraBuffer,\n> > > > - buffer_handle_t camera3Buffer, int flags)\n> > > > + buffer_handle_t camera3Buffer,\n> > > > + libcamera::PixelFormat pixelFormat,\n> > > > + const libcamera::Size &size, int flags)\n> > > > {\n> > > > - maps_.reserve(camera3Buffer->numFds);\n> > > > error_ = 0;\n> > > > -\n> > > > + /*\n> > > > + * As Android doesn't offer an API to query buffer layouts, assume for\n> > > > + * now that the buffer is backed by a single dmabuf, with planes being\n> > > > + * stored contiguously.\n> > > > + */\n> > > > + int fd = -1;\n> > > > for (int i = 0; i < camera3Buffer->numFds; i++) {\n> > >\n> > > unsigned int i\n> \n> The type of buffer_handle_t's numFds is int strangely. :-S\n> If I use unsigned int here, a compiler complains the comparison against numFds.\n\nAgreed, we have to use a signed int to avoid signed vs. unsigned\ncomparison warnings.\n\n> > > > - if (camera3Buffer->data[i] == -1)\n> > > > + if (camera3Buffer->data[i] == -1 ||\n> > > > + camera3Buffer->data[i] == fd) {\n> > > > continue;\n> > > > -\n> > > > - off_t length = lseek(camera3Buffer->data[i], 0, SEEK_END);\n> > > > - if (length < 0) {\n> > > > - error_ = -errno;\n> > > > - LOG(HAL, Error) << \"Failed to query plane length\";\n> > > > - break;\n> > > > }\n\nNo need for curly braces around the continue.\n\n> > > >\n> > > > - void *address = mmap(nullptr, length, flags, MAP_SHARED,\n> > > > - camera3Buffer->data[i], 0);\n> > > > - if (address == MAP_FAILED) {\n> > > > - error_ = -errno;\n> > > > - LOG(HAL, Error) << \"Failed to mmap plane\";\n> > > > - break;\n> > > > - }\n> > > > + if (fd != -1)\n> > > > + LOG(HAL, Fatal) << \"Discontiguous planes are not supported\";\n> > >\n> > > Do we just warn ? Or exit with error ? See the below question\n\nWe could indeed error out instead of aborting.\n\n\t\tif (fd != -1) {\n\t\t\terror_ = -EINVAL;\n\t\t\tLOG(HAL, Error) << \"Discontiguous planes are not supported\";\n\t\t\treturn;\n\t\t}\n\n> > >\n> > > > + fd = camera3Buffer->data[i];\n> > >\n> > > This assigns fd to the 'next' valid data[i].fd. I might have missed\n> > > what is the policy here if we have more than one valid dmabuf fds.\n> > >\n> > > Currently you warn and use the last valid one. I would instead error out,\n> > > or warn but use the first available one (even if that would result in\n> > > undefined behaviour, or most probably an access to non valid memory when\n> > > creating planes below).\n> > >\n> > > Do you have a use case for formats with planes allocated in different\n> > > buffers that you need to support ? Or can we error out without causing\n> > > regressions ?\n> \n> This is just from Laurent's comment.\n> I think no platform except ChromeOS has used libcamera Android HAL.\n> To catch something bad easily, we implement so.\n>\n> > > > + }\n> > >\n> > > The loop is pretty convoluted. If I'm not mistaken what you want to\n> > > verify is that\n> > > 1) Either all data[i].fd are the same\n> > > 2) All but the data[0].fd are -1\n> > >\n> > > Wouldn't it be nicer as\n> > >\n> > > int fd = camera3Buffer->data[0].fd;\n> > > for (unsigned int i = 1; i < camera3Buffer->numFds; ++i) {\n> > > if (camera3Buffer->data[i].fd != -1)\n> >\n> > I meant == -1\n> > > continue;\n> > >\n> > > if (camera3Buffer->data[i].fd != fd)\n> > > /* Here I would error out */\n> > > }\n> > > if (fd == -1) {\n> > > /* No valid fds, error out */\n> > > }\n\nWe could do this too, I like my version, but if Hiro you think this is\nbetter, that's fine with me too.\n\n> > > > +\n> > > > + if (fd != -1) {\n> > >\n> > > Shouldn't this be fd == -1 ? As in \"no valid fd found ?\"\n> > > If I'm not mistaken checking for fd != -1 should fail all the times as\n> > > at least one valid fd should be found. On which platform have you\n> > > tested this patch ?\n> \n> Thanks for catching this. I haven't tested this implementation as I am\n> testing on ChromeOS.\n> Is there any easy way of testing android HAL implementation on Linux?\n\nNot easily yet, but I'm working on this.\n\n> > > > + error_ = -EINVAL;\n> > > > + LOG(HAL, Error) << \"No valid file descriptor\";\n> > > > + return;\n> > > > + }\n> > > >\n> > > > - maps_.emplace_back(static_cast(address),\n> > > > - static_cast(length));\n> > > > + const auto &info = libcamera::PixelFormatInfo::info(pixelFormat);\n> > > > + if (!info.isValid()) {\n> > > > + error_ = -EINVAL;\n> > > > + LOG(HAL, Error) << \"Invalid pixel format: \"\n> > > > + << pixelFormat.toString();\n> > > > + return;\n> > > > + }\n> > >\n> > > We could check this at the very beginning of the function and avoid\n> > > looping on fds if the format is invalid.\n\nI don't mind either way. This shouldn't happen in any case, so there's\nno performance impact, I'd thus favour what you think produces the\ncleanest code.\n\n> > > > +\n> > > > + off_t bufferLength = lseek(fd, 0, SEEK_END);\n> > > > + if (bufferLength < 0) {\n> > > > + error_ = -errno;\n> > > > + LOG(HAL, Error) << \"Failed to get buffer length\";\n> > > > + return;\n> > > > + }\n> > > > +\n> > > > + void *address = mmap(nullptr, bufferLength, flags, MAP_SHARED, fd, 0);\n> > > > + if (address == MAP_FAILED) {\n> > > > + error_ = -errno;\n> > > > + LOG(HAL, Error) << \"Failed to mmap plane\";\n> > > > + return;\n> > > > + }\n> > > > + maps_.emplace_back(static_cast(address), bufferLength);\n> > > > +\n> > > > + const unsigned int numPlanes = info.numPlanes();\n> > > > + planes_.resize(numPlanes);\n> > > > + unsigned int offset = 0;\n> > > > + for (unsigned int i = 0; i < numPlanes; ++i) {\n> > > > + /*\n> > > > + * \\todo Remove if this plane size computation function is\n> > > > + * added to PixelFormatInfo.\n> > > > + */\n> > > > + const unsigned int vertSubSample = info.planes[i].verticalSubSampling;\n> > > > + const unsigned int stride = info.stride(size.width, i, 1u);\n> > > > + const unsigned int planeSize =\n> > > > + stride * ((size.height + vertSubSample - 1) / vertSubSample);\n> > > > +\n> > > > + planes_[i] = libcamera::Span(\n> > > > + static_cast(address) + offset, planeSize);\n> > > > +\n> > > > + if (bufferLength < offset + planeSize) {\n> > > > + error_ = -EINVAL;\n> > > > + LOG(HAL, Error) << \"Plane \" << i << \" is out of buffer\"\n> > > > + << \", buffer length=\" << bufferLength\n> > > > + << \", offset=\" << offset\n> > > > + << \", size=\" << planeSize;\n> > > > + return;\n> > > > + }\n> > > > + offset += planeSize;\n> > > > }\n> > > > }\n> > > >\n> > > > @@ -71,15 +127,15 @@ CameraBuffer::Private::~Private()\n> > > >\n> > > > unsigned int CameraBuffer::Private::numPlanes() const\n> > > > {\n> > > > - return maps_.size();\n> > > > + return planes_.size();\n> > > > }\n> > > >\n> > > > Span CameraBuffer::Private::plane(unsigned int plane)\n> > > > {\n> > > > - if (plane >= maps_.size())\n> > > > + if (plane >= planes_.size())\n> > > > return {};\n> > > >\n> > > > - return maps_[plane];\n> > > > + return planes_[plane];\n> > > > }\n> > > >\n> > > > size_t CameraBuffer::Private::jpegBufferSize(size_t maxJpegBufferSize) const","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 704B5BD87C\n\tfor ;\n\tWed, 25 Aug 2021 20:18:16 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D773E60504;\n\tWed, 25 Aug 2021 22:18:15 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2292B60288\n\tfor ;\n\tWed, 25 Aug 2021 22:18:14 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 8AA0224F;\n\tWed, 25 Aug 2021 22:18:13 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"VubgJa/8\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1629922693;\n\tbh=wDZSzOzwq75YA8XNVTHMh0FT01MyHbN3pm1C84Kn0Xo=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=VubgJa/8Qm1kQYRu3jLxkNZ3o9NOI8R/fas1FyeHIdqrZdDnI1/8DedacwaVefGy0\n\tpU4tEmW5N7lEOFq4NtL/XwzPUOnPB+Y6dDuh6mbBscSZThiTexERHKJ8VvZ+o3PaUB\n\tkY7B3DXwtMhRi+vgqkZRbZLOBZ8rtQ3YkO320bec=","Date":"Wed, 25 Aug 2021 23:18:01 +0300","From":"Laurent Pinchart ","To":"Hirokazu Honda ","Message-ID":"","References":"<20210825044410.2787433-1-hiroh@chromium.org>\n\t<20210825044410.2787433-2-hiroh@chromium.org>\n\t<20210825083507.5bodpgew6lsllx77@uno.localdomain>\n\t<20210825100048.hyppw5eychr3thmq@uno.localdomain>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v2 1/3] android:\n\tgeneric_camera_buffer: Correct buffer mapping","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":19131,"web_url":"https://patchwork.libcamera.org/comment/19131/","msgid":"","date":"2021-08-27T10:08:30","subject":"Re: [libcamera-devel] [PATCH v2 1/3] android:\n\tgeneric_camera_buffer: Correct buffer mapping","submitter":{"id":9,"url":"https://patchwork.libcamera.org/api/people/9/","name":"Tomasz Figa","email":"tfiga@chromium.org"},"content":"On Wed, Aug 25, 2021 at 5:34 PM Jacopo Mondi wrote:\n>\n> Hi Hiro,\n>\n> On Wed, Aug 25, 2021 at 01:44:08PM +0900, Hirokazu Honda wrote:\n> > buffer_hanlde_t doesn't provide sufficient info to map a buffer\n> > properly. cros::CameraBufferManager enables handling the buffer on\n> > ChromeOS, but no way is provided for other platforms.\n> >\n> > Therefore, we put the assumption that planes are in the same buffer\n> > and they are consecutive. This modifies the way of mapping in\n> > generic_camera_buffer with the assumption.\n> >\n> > Signed-off-by: Hirokazu Honda \n> > ---\n> > src/android/camera_buffer.h | 14 ++-\n> > src/android/camera_stream.cpp | 4 +-\n> > src/android/mm/cros_camera_buffer.cpp | 7 +-\n> > src/android/mm/generic_camera_buffer.cpp | 104 +++++++++++++++++------\n> > 4 files changed, 99 insertions(+), 30 deletions(-)\n> >\n> > diff --git a/src/android/camera_buffer.h b/src/android/camera_buffer.h\n> > index c4e3a9e7..87df2570 100644\n> > --- a/src/android/camera_buffer.h\n> > +++ b/src/android/camera_buffer.h\n> > @@ -11,13 +11,17 @@\n> >\n> > #include \n> > #include \n> > +#include \n> > +#include \n> >\n> > class CameraBuffer final : public libcamera::Extensible\n> > {\n> > LIBCAMERA_DECLARE_PRIVATE()\n> >\n> > public:\n> > - CameraBuffer(buffer_handle_t camera3Buffer, int flags);\n> > + CameraBuffer(buffer_handle_t camera3Buffer,\n> > + libcamera::PixelFormat pixelFormat,\n> > + const libcamera::Size &size, int flags);\n> > ~CameraBuffer();\n> >\n> > bool isValid() const;\n> > @@ -31,8 +35,12 @@ public:\n> > };\n> >\n> > #define PUBLIC_CAMERA_BUFFER_IMPLEMENTATION \\\n> > -CameraBuffer::CameraBuffer(buffer_handle_t camera3Buffer, int flags) \\\n> > - : Extensible(std::make_unique(this, camera3Buffer, flags)) \\\n> > +CameraBuffer::CameraBuffer(buffer_handle_t camera3Buffer, \\\n> > + libcamera::PixelFormat pixelFormat, \\\n> > + const libcamera::Size &size, int flags) \\\n> > + : Extensible(std::make_unique(this, camera3Buffer, \\\n> > + pixelFormat, size, \\\n> > + flags)) \\\n> > { \\\n> > } \\\n> > CameraBuffer::~CameraBuffer() \\\n> > diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp\n> > index 61b44183..01909ec7 100644\n> > --- a/src/android/camera_stream.cpp\n> > +++ b/src/android/camera_stream.cpp\n> > @@ -110,7 +110,9 @@ int CameraStream::process(const libcamera::FrameBuffer &source,\n> > * \\todo Buffer mapping and processing should be moved to a\n> > * separate thread.\n> > */\n> > - CameraBuffer dest(camera3Dest, PROT_READ | PROT_WRITE);\n> > + const StreamConfiguration &output = configuration();\n> > + CameraBuffer dest(camera3Dest, formats::MJPEG, output.size,\n> > + PROT_READ | PROT_WRITE);\n> > if (!dest.isValid()) {\n> > LOG(HAL, Error) << \"Failed to map android blob buffer\";\n> > return -EINVAL;\n> > diff --git a/src/android/mm/cros_camera_buffer.cpp b/src/android/mm/cros_camera_buffer.cpp\n> > index e8783ff8..50732637 100644\n> > --- a/src/android/mm/cros_camera_buffer.cpp\n> > +++ b/src/android/mm/cros_camera_buffer.cpp\n> > @@ -20,8 +20,9 @@ class CameraBuffer::Private : public Extensible::Private\n> > LIBCAMERA_DECLARE_PUBLIC(CameraBuffer)\n> >\n> > public:\n> > - Private(CameraBuffer *cameraBuffer,\n> > - buffer_handle_t camera3Buffer, int flags);\n> > + Private(CameraBuffer *cameraBuffer, buffer_handle_t camera3Buffer,\n> > + libcamera::PixelFormat pixelFormat, const libcamera::Size &size,\n> > + int flags);\n> > ~Private();\n> >\n> > bool isValid() const { return valid_; }\n> > @@ -46,6 +47,8 @@ private:\n> >\n> > CameraBuffer::Private::Private([[maybe_unused]] CameraBuffer *cameraBuffer,\n> > buffer_handle_t camera3Buffer,\n> > + [[maybe_unused]] libcamera::PixelFormat pixelFormat,\n> > + [[maybe_unused]] const libcamera::Size &size,\n> > [[maybe_unused]] int flags)\n> > : handle_(camera3Buffer), numPlanes_(0), valid_(false),\n> > registered_(false)\n> > diff --git a/src/android/mm/generic_camera_buffer.cpp b/src/android/mm/generic_camera_buffer.cpp\n> > index b3af194c..0bfdc2ba 100644\n> > --- a/src/android/mm/generic_camera_buffer.cpp\n> > +++ b/src/android/mm/generic_camera_buffer.cpp\n> > @@ -12,6 +12,7 @@\n> >\n> > #include \n> >\n> > +#include \"libcamera/internal/formats.h\"\n> > #include \"libcamera/internal/mapped_framebuffer.h\"\n> >\n> > using namespace libcamera;\n> > @@ -24,8 +25,9 @@ class CameraBuffer::Private : public Extensible::Private,\n> > LIBCAMERA_DECLARE_PUBLIC(CameraBuffer)\n> >\n> > public:\n> > - Private(CameraBuffer *cameraBuffer,\n> > - buffer_handle_t camera3Buffer, int flags);\n> > + Private(CameraBuffer *cameraBuffer, buffer_handle_t camera3Buffer,\n> > + libcamera::PixelFormat pixelFormat, const libcamera::Size &size,\n> > + int flags);\n> > ~Private();\n> >\n> > unsigned int numPlanes() const;\n> > @@ -33,35 +35,89 @@ public:\n> > Span plane(unsigned int plane);\n> >\n> > size_t jpegBufferSize(size_t maxJpegBufferSize) const;\n> > +\n> > +private:\n> > + /* \\todo Remove planes_ when it will be added to MappedBuffer */\n> > + std::vector> planes_;\n> > };\n> >\n> > CameraBuffer::Private::Private([[maybe_unused]] CameraBuffer *cameraBuffer,\n> > - buffer_handle_t camera3Buffer, int flags)\n> > + buffer_handle_t camera3Buffer,\n> > + libcamera::PixelFormat pixelFormat,\n> > + const libcamera::Size &size, int flags)\n> > {\n> > - maps_.reserve(camera3Buffer->numFds);\n> > error_ = 0;\n> > -\n> > + /*\n> > + * As Android doesn't offer an API to query buffer layouts, assume for\n> > + * now that the buffer is backed by a single dmabuf, with planes being\n> > + * stored contiguously.\n> > + */\n> > + int fd = -1;\n> > for (int i = 0; i < camera3Buffer->numFds; i++) {\n>\n> unsigned int i\n>\n> > - if (camera3Buffer->data[i] == -1)\n> > + if (camera3Buffer->data[i] == -1 ||\n> > + camera3Buffer->data[i] == fd) {\n> > continue;\n> > -\n> > - off_t length = lseek(camera3Buffer->data[i], 0, SEEK_END);\n> > - if (length < 0) {\n> > - error_ = -errno;\n> > - LOG(HAL, Error) << \"Failed to query plane length\";\n> > - break;\n> > }\n> >\n> > - void *address = mmap(nullptr, length, flags, MAP_SHARED,\n> > - camera3Buffer->data[i], 0);\n> > - if (address == MAP_FAILED) {\n> > - error_ = -errno;\n> > - LOG(HAL, Error) << \"Failed to mmap plane\";\n> > - break;\n> > - }\n> > + if (fd != -1)\n> > + LOG(HAL, Fatal) << \"Discontiguous planes are not supported\";\n>\n> Do we just warn ? Or exit with error ? See the below question\n>\n> > + fd = camera3Buffer->data[i];\n>\n> This assigns fd to the 'next' valid data[i].fd. I might have missed\n> what is the policy here if we have more than one valid dmabuf fds.\n>\n> Currently you warn and use the last valid one. I would instead error out,\n> or warn but use the first available one (even if that would result in\n> undefined behaviour, or most probably an access to non valid memory when\n> creating planes below).\n>\n> Do you have a use case for formats with planes allocated in different\n> buffers that you need to support ? Or can we error out without causing\n> regressions ?\n>\n> > + }\n>\n> The loop is pretty convoluted. If I'm not mistaken what you want to\n> verify is that\n> 1) Either all data[i].fd are the same\n\nThis is actually not something that would happen in practice, because\nthe handles are passed via Binder and even if the source puts the same\nFD for all the planes, the receiver would see all different FDs,\nbecause Binder would create a new FD for each array element.\n\n> 2) All but the data[0].fd are -1\n>\n> Wouldn't it be nicer as\n>\n> int fd = camera3Buffer->data[0].fd;\n> for (unsigned int i = 1; i < camera3Buffer->numFds; ++i) {\n> if (camera3Buffer->data[i].fd != -1)\n> continue;\n>\n> if (camera3Buffer->data[i].fd != fd)\n> /* Here I would error out */\n> }\n> if (fd == -1) {\n> /* No valid fds, error out */\n> }\n>\n> > +\n> > + if (fd != -1) {\n>\n> Shouldn't this be fd == -1 ? As in \"no valid fd found ?\"\n> If I'm not mistaken checking for fd != -1 should fail all the times as\n> at least one valid fd should be found. On which platform have you\n> tested this patch ?\n>\n> > + error_ = -EINVAL;\n> > + LOG(HAL, Error) << \"No valid file descriptor\";\n> > + return;\n> > + }\n> >\n> > - maps_.emplace_back(static_cast(address),\n> > - static_cast(length));\n> > + const auto &info = libcamera::PixelFormatInfo::info(pixelFormat);\n> > + if (!info.isValid()) {\n> > + error_ = -EINVAL;\n> > + LOG(HAL, Error) << \"Invalid pixel format: \"\n> > + << pixelFormat.toString();\n> > + return;\n> > + }\n>\n> We could check this at the very beginning of the function and avoid\n> looping on fds if the format is invalid.\n>\n> > +\n> > + off_t bufferLength = lseek(fd, 0, SEEK_END);\n> > + if (bufferLength < 0) {\n> > + error_ = -errno;\n> > + LOG(HAL, Error) << \"Failed to get buffer length\";\n> > + return;\n> > + }\n> > +\n> > + void *address = mmap(nullptr, bufferLength, flags, MAP_SHARED, fd, 0);\n> > + if (address == MAP_FAILED) {\n> > + error_ = -errno;\n> > + LOG(HAL, Error) << \"Failed to mmap plane\";\n> > + return;\n> > + }\n> > + maps_.emplace_back(static_cast(address), bufferLength);\n> > +\n> > + const unsigned int numPlanes = info.numPlanes();\n> > + planes_.resize(numPlanes);\n> > + unsigned int offset = 0;\n> > + for (unsigned int i = 0; i < numPlanes; ++i) {\n> > + /*\n> > + * \\todo Remove if this plane size computation function is\n> > + * added to PixelFormatInfo.\n> > + */\n> > + const unsigned int vertSubSample = info.planes[i].verticalSubSampling;\n> > + const unsigned int stride = info.stride(size.width, i, 1u);\n> > + const unsigned int planeSize =\n> > + stride * ((size.height + vertSubSample - 1) / vertSubSample);\n> > +\n> > + planes_[i] = libcamera::Span(\n> > + static_cast(address) + offset, planeSize);\n> > +\n> > + if (bufferLength < offset + planeSize) {\n> > + error_ = -EINVAL;\n> > + LOG(HAL, Error) << \"Plane \" << i << \" is out of buffer\"\n> > + << \", buffer length=\" << bufferLength\n> > + << \", offset=\" << offset\n> > + << \", size=\" << planeSize;\n> > + return;\n> > + }\n> > + offset += planeSize;\n> > }\n> > }\n> >\n> > @@ -71,15 +127,15 @@ CameraBuffer::Private::~Private()\n> >\n> > unsigned int CameraBuffer::Private::numPlanes() const\n> > {\n> > - return maps_.size();\n> > + return planes_.size();\n> > }\n> >\n> > Span CameraBuffer::Private::plane(unsigned int plane)\n> > {\n> > - if (plane >= maps_.size())\n> > + if (plane >= planes_.size())\n> > return {};\n> >\n> > - return maps_[plane];\n> > + return planes_[plane];\n> > }\n> >\n> > size_t CameraBuffer::Private::jpegBufferSize(size_t maxJpegBufferSize) const\n> > --\n> > 2.33.0.rc2.250.ged5fa647cd-goog\n> >","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 7FAFFBD87D\n\tfor ;\n\tFri, 27 Aug 2021 10:08:45 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 03E026891F;\n\tFri, 27 Aug 2021 12:08:45 +0200 (CEST)","from mail-ej1-x633.google.com (mail-ej1-x633.google.com\n\t[IPv6:2a00:1450:4864:20::633])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 7EEB860256\n\tfor ;\n\tFri, 27 Aug 2021 12:08:44 +0200 (CEST)","by mail-ej1-x633.google.com with SMTP id h9so12708864ejs.4\n\tfor ;\n\tFri, 27 Aug 2021 03:08:44 -0700 (PDT)","from mail-wm1-f50.google.com (mail-wm1-f50.google.com.\n\t[209.85.128.50]) by smtp.gmail.com with ESMTPSA id\n\to21sm2672593eji.29.2021.08.27.03.08.42\n\tfor \n\t(version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128);\n\tFri, 27 Aug 2021 03:08:43 -0700 (PDT)","by mail-wm1-f50.google.com with SMTP id\n\tj17-20020a05600c1c1100b002e754875260so3946401wms.4\n\tfor ;\n\tFri, 27 Aug 2021 03:08:42 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"HrG4cZFn\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org;\n\ts=google; \n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=WJ/x5Imx7bGmleQEOMysqpYNAKgLaFbWrZBp6D0kbIc=;\n\tb=HrG4cZFnETKHARikPdj1nnL4Io9YseNuXN7QVBzcv0wgnD7d46GS5XhuQz15K8YGGM\n\tDUmI+Tt14WPu4uNUyOE75UT/S5eHUKL20QjTYvPPoA5QYku99SmBNjGhWk5eS14/TN+N\n\tNv+BbiEBFBwNQBwbAJkWy86wCKjy9T7FmImvE=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=WJ/x5Imx7bGmleQEOMysqpYNAKgLaFbWrZBp6D0kbIc=;\n\tb=KEQT4Mnixxeve8KRtJ94eLi/hkshXtH3imcASPSTBDZwRawLhrSJXzuT9f/YqRmXkC\n\tTcy7DZmknr7jhu14FJVdX8o6DEx2mbhD4rNJIXy8/nsxd4RWQmBLtSaDfLGIKdLweyXc\n\tdzubuCu+W/igqblYCzthnKZIVDeKz5lM7/nDFVKFgmv4QzPwzIHBX1GldRckbyRN5Fml\n\tNPkDSSaaoiOfZbYt9oErMLEstJiMCqNna9jt3+J/gix/gj0FrlgKup9RGZF3TGPWlhTI\n\tzeL7Bq91q/SNr2gIRBfTuqPsrtCM1bJ73BTsNfJl+VVN11RForyZCczQqjlPn5p4HigT\n\tF1lg==","X-Gm-Message-State":"AOAM533zWeW5KOY7kO1Z0wC88j4U3TslZMHzEAD5w9JbKe8awVdT7xcU\n\tBVVzYy6Qk41wCnPhZAl0D+Pu1tL9wONHLw==","X-Google-Smtp-Source":"ABdhPJz+E45OmxwmyBJCSIY1TmoDuYRuTgee9PhCgEXmszfr2P0dkJb8Es4lmGwns363s6hIUhR/mw==","X-Received":["by 2002:a17:906:c317:: with SMTP id\n\ts23mr9194085ejz.83.1630058923599; \n\tFri, 27 Aug 2021 03:08:43 -0700 (PDT)","by 2002:a1c:91:: with SMTP id 139mr1629739wma.116.1630058922047; \n\tFri, 27 Aug 2021 03:08:42 -0700 (PDT)"],"MIME-Version":"1.0","References":"<20210825044410.2787433-1-hiroh@chromium.org>\n\t<20210825044410.2787433-2-hiroh@chromium.org>\n\t<20210825083507.5bodpgew6lsllx77@uno.localdomain>","In-Reply-To":"<20210825083507.5bodpgew6lsllx77@uno.localdomain>","From":"Tomasz Figa ","Date":"Fri, 27 Aug 2021 19:08:30 +0900","X-Gmail-Original-Message-ID":"","Message-ID":"","To":"Jacopo Mondi ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH v2 1/3] android:\n\tgeneric_camera_buffer: Correct buffer mapping","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":19137,"web_url":"https://patchwork.libcamera.org/comment/19137/","msgid":"","date":"2021-08-27T11:36:40","subject":"Re: [libcamera-devel] [PATCH v2 1/3] android:\n\tgeneric_camera_buffer: Correct buffer mapping","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Tomasz,\n\nOn Fri, Aug 27, 2021 at 07:08:30PM +0900, Tomasz Figa wrote:\n> On Wed, Aug 25, 2021 at 5:34 PM Jacopo Mondi wrote:\n> > On Wed, Aug 25, 2021 at 01:44:08PM +0900, Hirokazu Honda wrote:\n> > > buffer_hanlde_t doesn't provide sufficient info to map a buffer\n> > > properly. cros::CameraBufferManager enables handling the buffer on\n> > > ChromeOS, but no way is provided for other platforms.\n> > >\n> > > Therefore, we put the assumption that planes are in the same buffer\n> > > and they are consecutive. This modifies the way of mapping in\n> > > generic_camera_buffer with the assumption.\n> > >\n> > > Signed-off-by: Hirokazu Honda \n> > > ---\n> > > src/android/camera_buffer.h | 14 ++-\n> > > src/android/camera_stream.cpp | 4 +-\n> > > src/android/mm/cros_camera_buffer.cpp | 7 +-\n> > > src/android/mm/generic_camera_buffer.cpp | 104 +++++++++++++++++------\n> > > 4 files changed, 99 insertions(+), 30 deletions(-)\n> > >\n> > > diff --git a/src/android/camera_buffer.h b/src/android/camera_buffer.h\n> > > index c4e3a9e7..87df2570 100644\n> > > --- a/src/android/camera_buffer.h\n> > > +++ b/src/android/camera_buffer.h\n> > > @@ -11,13 +11,17 @@\n> > >\n> > > #include \n> > > #include \n> > > +#include \n> > > +#include \n> > >\n> > > class CameraBuffer final : public libcamera::Extensible\n> > > {\n> > > LIBCAMERA_DECLARE_PRIVATE()\n> > >\n> > > public:\n> > > - CameraBuffer(buffer_handle_t camera3Buffer, int flags);\n> > > + CameraBuffer(buffer_handle_t camera3Buffer,\n> > > + libcamera::PixelFormat pixelFormat,\n> > > + const libcamera::Size &size, int flags);\n> > > ~CameraBuffer();\n> > >\n> > > bool isValid() const;\n> > > @@ -31,8 +35,12 @@ public:\n> > > };\n> > >\n> > > #define PUBLIC_CAMERA_BUFFER_IMPLEMENTATION \\\n> > > -CameraBuffer::CameraBuffer(buffer_handle_t camera3Buffer, int flags) \\\n> > > - : Extensible(std::make_unique(this, camera3Buffer, flags)) \\\n> > > +CameraBuffer::CameraBuffer(buffer_handle_t camera3Buffer, \\\n> > > + libcamera::PixelFormat pixelFormat, \\\n> > > + const libcamera::Size &size, int flags) \\\n> > > + : Extensible(std::make_unique(this, camera3Buffer, \\\n> > > + pixelFormat, size, \\\n> > > + flags)) \\\n> > > { \\\n> > > } \\\n> > > CameraBuffer::~CameraBuffer() \\\n> > > diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp\n> > > index 61b44183..01909ec7 100644\n> > > --- a/src/android/camera_stream.cpp\n> > > +++ b/src/android/camera_stream.cpp\n> > > @@ -110,7 +110,9 @@ int CameraStream::process(const libcamera::FrameBuffer &source,\n> > > * \\todo Buffer mapping and processing should be moved to a\n> > > * separate thread.\n> > > */\n> > > - CameraBuffer dest(camera3Dest, PROT_READ | PROT_WRITE);\n> > > + const StreamConfiguration &output = configuration();\n> > > + CameraBuffer dest(camera3Dest, formats::MJPEG, output.size,\n> > > + PROT_READ | PROT_WRITE);\n> > > if (!dest.isValid()) {\n> > > LOG(HAL, Error) << \"Failed to map android blob buffer\";\n> > > return -EINVAL;\n> > > diff --git a/src/android/mm/cros_camera_buffer.cpp b/src/android/mm/cros_camera_buffer.cpp\n> > > index e8783ff8..50732637 100644\n> > > --- a/src/android/mm/cros_camera_buffer.cpp\n> > > +++ b/src/android/mm/cros_camera_buffer.cpp\n> > > @@ -20,8 +20,9 @@ class CameraBuffer::Private : public Extensible::Private\n> > > LIBCAMERA_DECLARE_PUBLIC(CameraBuffer)\n> > >\n> > > public:\n> > > - Private(CameraBuffer *cameraBuffer,\n> > > - buffer_handle_t camera3Buffer, int flags);\n> > > + Private(CameraBuffer *cameraBuffer, buffer_handle_t camera3Buffer,\n> > > + libcamera::PixelFormat pixelFormat, const libcamera::Size &size,\n> > > + int flags);\n> > > ~Private();\n> > >\n> > > bool isValid() const { return valid_; }\n> > > @@ -46,6 +47,8 @@ private:\n> > >\n> > > CameraBuffer::Private::Private([[maybe_unused]] CameraBuffer *cameraBuffer,\n> > > buffer_handle_t camera3Buffer,\n> > > + [[maybe_unused]] libcamera::PixelFormat pixelFormat,\n> > > + [[maybe_unused]] const libcamera::Size &size,\n> > > [[maybe_unused]] int flags)\n> > > : handle_(camera3Buffer), numPlanes_(0), valid_(false),\n> > > registered_(false)\n> > > diff --git a/src/android/mm/generic_camera_buffer.cpp b/src/android/mm/generic_camera_buffer.cpp\n> > > index b3af194c..0bfdc2ba 100644\n> > > --- a/src/android/mm/generic_camera_buffer.cpp\n> > > +++ b/src/android/mm/generic_camera_buffer.cpp\n> > > @@ -12,6 +12,7 @@\n> > >\n> > > #include \n> > >\n> > > +#include \"libcamera/internal/formats.h\"\n> > > #include \"libcamera/internal/mapped_framebuffer.h\"\n> > >\n> > > using namespace libcamera;\n> > > @@ -24,8 +25,9 @@ class CameraBuffer::Private : public Extensible::Private,\n> > > LIBCAMERA_DECLARE_PUBLIC(CameraBuffer)\n> > >\n> > > public:\n> > > - Private(CameraBuffer *cameraBuffer,\n> > > - buffer_handle_t camera3Buffer, int flags);\n> > > + Private(CameraBuffer *cameraBuffer, buffer_handle_t camera3Buffer,\n> > > + libcamera::PixelFormat pixelFormat, const libcamera::Size &size,\n> > > + int flags);\n> > > ~Private();\n> > >\n> > > unsigned int numPlanes() const;\n> > > @@ -33,35 +35,89 @@ public:\n> > > Span plane(unsigned int plane);\n> > >\n> > > size_t jpegBufferSize(size_t maxJpegBufferSize) const;\n> > > +\n> > > +private:\n> > > + /* \\todo Remove planes_ when it will be added to MappedBuffer */\n> > > + std::vector> planes_;\n> > > };\n> > >\n> > > CameraBuffer::Private::Private([[maybe_unused]] CameraBuffer *cameraBuffer,\n> > > - buffer_handle_t camera3Buffer, int flags)\n> > > + buffer_handle_t camera3Buffer,\n> > > + libcamera::PixelFormat pixelFormat,\n> > > + const libcamera::Size &size, int flags)\n> > > {\n> > > - maps_.reserve(camera3Buffer->numFds);\n> > > error_ = 0;\n> > > -\n> > > + /*\n> > > + * As Android doesn't offer an API to query buffer layouts, assume for\n> > > + * now that the buffer is backed by a single dmabuf, with planes being\n> > > + * stored contiguously.\n> > > + */\n> > > + int fd = -1;\n> > > for (int i = 0; i < camera3Buffer->numFds; i++) {\n> >\n> > unsigned int i\n> >\n> > > - if (camera3Buffer->data[i] == -1)\n> > > + if (camera3Buffer->data[i] == -1 ||\n> > > + camera3Buffer->data[i] == fd) {\n> > > continue;\n> > > -\n> > > - off_t length = lseek(camera3Buffer->data[i], 0, SEEK_END);\n> > > - if (length < 0) {\n> > > - error_ = -errno;\n> > > - LOG(HAL, Error) << \"Failed to query plane length\";\n> > > - break;\n> > > }\n> > >\n> > > - void *address = mmap(nullptr, length, flags, MAP_SHARED,\n> > > - camera3Buffer->data[i], 0);\n> > > - if (address == MAP_FAILED) {\n> > > - error_ = -errno;\n> > > - LOG(HAL, Error) << \"Failed to mmap plane\";\n> > > - break;\n> > > - }\n> > > + if (fd != -1)\n> > > + LOG(HAL, Fatal) << \"Discontiguous planes are not supported\";\n> >\n> > Do we just warn ? Or exit with error ? See the below question\n> >\n> > > + fd = camera3Buffer->data[i];\n> >\n> > This assigns fd to the 'next' valid data[i].fd. I might have missed\n> > what is the policy here if we have more than one valid dmabuf fds.\n> >\n> > Currently you warn and use the last valid one. I would instead error out,\n> > or warn but use the first available one (even if that would result in\n> > undefined behaviour, or most probably an access to non valid memory when\n> > creating planes below).\n> >\n> > Do you have a use case for formats with planes allocated in different\n> > buffers that you need to support ? Or can we error out without causing\n> > regressions ?\n> >\n> > > + }\n> >\n> > The loop is pretty convoluted. If I'm not mistaken what you want to\n> > verify is that\n> > 1) Either all data[i].fd are the same\n> \n> This is actually not something that would happen in practice, because\n> the handles are passed via Binder and even if the source puts the same\n> FD for all the planes, the receiver would see all different FDs,\n> because Binder would create a new FD for each array element.\n\nSounds like there's room for improvement in binder then. Nevertheless,\nwe certainly want to support the case where the fds will be different\nbut all refer to the same dmabuf. The main blocker for this is the\nability to actually test it on Android, which I'm working on.\n\n> > 2) All but the data[0].fd are -1\n> >\n> > Wouldn't it be nicer as\n> >\n> > int fd = camera3Buffer->data[0].fd;\n> > for (unsigned int i = 1; i < camera3Buffer->numFds; ++i) {\n> > if (camera3Buffer->data[i].fd != -1)\n> > continue;\n> >\n> > if (camera3Buffer->data[i].fd != fd)\n> > /* Here I would error out */\n> > }\n> > if (fd == -1) {\n> > /* No valid fds, error out */\n> > }\n> >\n> > > +\n> > > + if (fd != -1) {\n> >\n> > Shouldn't this be fd == -1 ? As in \"no valid fd found ?\"\n> > If I'm not mistaken checking for fd != -1 should fail all the times as\n> > at least one valid fd should be found. On which platform have you\n> > tested this patch ?\n> >\n> > > + error_ = -EINVAL;\n> > > + LOG(HAL, Error) << \"No valid file descriptor\";\n> > > + return;\n> > > + }\n> > >\n> > > - maps_.emplace_back(static_cast(address),\n> > > - static_cast(length));\n> > > + const auto &info = libcamera::PixelFormatInfo::info(pixelFormat);\n> > > + if (!info.isValid()) {\n> > > + error_ = -EINVAL;\n> > > + LOG(HAL, Error) << \"Invalid pixel format: \"\n> > > + << pixelFormat.toString();\n> > > + return;\n> > > + }\n> >\n> > We could check this at the very beginning of the function and avoid\n> > looping on fds if the format is invalid.\n> >\n> > > +\n> > > + off_t bufferLength = lseek(fd, 0, SEEK_END);\n> > > + if (bufferLength < 0) {\n> > > + error_ = -errno;\n> > > + LOG(HAL, Error) << \"Failed to get buffer length\";\n> > > + return;\n> > > + }\n> > > +\n> > > + void *address = mmap(nullptr, bufferLength, flags, MAP_SHARED, fd, 0);\n> > > + if (address == MAP_FAILED) {\n> > > + error_ = -errno;\n> > > + LOG(HAL, Error) << \"Failed to mmap plane\";\n> > > + return;\n> > > + }\n> > > + maps_.emplace_back(static_cast(address), bufferLength);\n> > > +\n> > > + const unsigned int numPlanes = info.numPlanes();\n> > > + planes_.resize(numPlanes);\n> > > + unsigned int offset = 0;\n> > > + for (unsigned int i = 0; i < numPlanes; ++i) {\n> > > + /*\n> > > + * \\todo Remove if this plane size computation function is\n> > > + * added to PixelFormatInfo.\n> > > + */\n> > > + const unsigned int vertSubSample = info.planes[i].verticalSubSampling;\n> > > + const unsigned int stride = info.stride(size.width, i, 1u);\n> > > + const unsigned int planeSize =\n> > > + stride * ((size.height + vertSubSample - 1) / vertSubSample);\n> > > +\n> > > + planes_[i] = libcamera::Span(\n> > > + static_cast(address) + offset, planeSize);\n> > > +\n> > > + if (bufferLength < offset + planeSize) {\n> > > + error_ = -EINVAL;\n> > > + LOG(HAL, Error) << \"Plane \" << i << \" is out of buffer\"\n> > > + << \", buffer length=\" << bufferLength\n> > > + << \", offset=\" << offset\n> > > + << \", size=\" << planeSize;\n> > > + return;\n> > > + }\n> > > + offset += planeSize;\n> > > }\n> > > }\n> > >\n> > > @@ -71,15 +127,15 @@ CameraBuffer::Private::~Private()\n> > >\n> > > unsigned int CameraBuffer::Private::numPlanes() const\n> > > {\n> > > - return maps_.size();\n> > > + return planes_.size();\n> > > }\n> > >\n> > > Span CameraBuffer::Private::plane(unsigned int plane)\n> > > {\n> > > - if (plane >= maps_.size())\n> > > + if (plane >= planes_.size())\n> > > return {};\n> > >\n> > > - return maps_[plane];\n> > > + return planes_[plane];\n> > > }\n> > >\n> > > size_t CameraBuffer::Private::jpegBufferSize(size_t maxJpegBufferSize) const","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 93305BD87D\n\tfor ;\n\tFri, 27 Aug 2021 11:36:55 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0FEFA6891F;\n\tFri, 27 Aug 2021 13:36:55 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2BDE360256\n\tfor ;\n\tFri, 27 Aug 2021 13:36:54 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 9FF3D5A1;\n\tFri, 27 Aug 2021 13:36:53 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"pepqNAA3\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1630064213;\n\tbh=j2mT94h0UPOow/Kb197hTWt+FMZxP8PIGe+bmmRL2Xs=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=pepqNAA3a20eP8vDnAkC4b2EaW7KwyAOFUmicXK7c4Bft+H8c4Ga6pt9wYSKBbp/P\n\t6kOINTJgCYt+M5UQ/Swl9AjzwknyZeVS0P0LJ1IkkgRZkOdMXQfz0R238q3GXCiV+W\n\tusAlBot/fsYItkDYjWbicpv5W2yc9DbyLFBX7rEo=","Date":"Fri, 27 Aug 2021 14:36:40 +0300","From":"Laurent Pinchart ","To":"Tomasz Figa ","Message-ID":"","References":"<20210825044410.2787433-1-hiroh@chromium.org>\n\t<20210825044410.2787433-2-hiroh@chromium.org>\n\t<20210825083507.5bodpgew6lsllx77@uno.localdomain>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v2 1/3] android:\n\tgeneric_camera_buffer: Correct buffer mapping","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":19140,"web_url":"https://patchwork.libcamera.org/comment/19140/","msgid":"","date":"2021-08-27T12:01:07","subject":"Re: [libcamera-devel] [PATCH v2 1/3] android:\n\tgeneric_camera_buffer: Correct buffer mapping","submitter":{"id":9,"url":"https://patchwork.libcamera.org/api/people/9/","name":"Tomasz Figa","email":"tfiga@chromium.org"},"content":"On Fri, Aug 27, 2021 at 8:36 PM Laurent Pinchart\n wrote:\n>\n> Hi Tomasz,\n>\n> On Fri, Aug 27, 2021 at 07:08:30PM +0900, Tomasz Figa wrote:\n> > On Wed, Aug 25, 2021 at 5:34 PM Jacopo Mondi wrote:\n> > > On Wed, Aug 25, 2021 at 01:44:08PM +0900, Hirokazu Honda wrote:\n> > > > buffer_hanlde_t doesn't provide sufficient info to map a buffer\n> > > > properly. cros::CameraBufferManager enables handling the buffer on\n> > > > ChromeOS, but no way is provided for other platforms.\n> > > >\n> > > > Therefore, we put the assumption that planes are in the same buffer\n> > > > and they are consecutive. This modifies the way of mapping in\n> > > > generic_camera_buffer with the assumption.\n> > > >\n> > > > Signed-off-by: Hirokazu Honda \n> > > > ---\n> > > > src/android/camera_buffer.h | 14 ++-\n> > > > src/android/camera_stream.cpp | 4 +-\n> > > > src/android/mm/cros_camera_buffer.cpp | 7 +-\n> > > > src/android/mm/generic_camera_buffer.cpp | 104 +++++++++++++++++------\n> > > > 4 files changed, 99 insertions(+), 30 deletions(-)\n> > > >\n> > > > diff --git a/src/android/camera_buffer.h b/src/android/camera_buffer.h\n> > > > index c4e3a9e7..87df2570 100644\n> > > > --- a/src/android/camera_buffer.h\n> > > > +++ b/src/android/camera_buffer.h\n> > > > @@ -11,13 +11,17 @@\n> > > >\n> > > > #include \n> > > > #include \n> > > > +#include \n> > > > +#include \n> > > >\n> > > > class CameraBuffer final : public libcamera::Extensible\n> > > > {\n> > > > LIBCAMERA_DECLARE_PRIVATE()\n> > > >\n> > > > public:\n> > > > - CameraBuffer(buffer_handle_t camera3Buffer, int flags);\n> > > > + CameraBuffer(buffer_handle_t camera3Buffer,\n> > > > + libcamera::PixelFormat pixelFormat,\n> > > > + const libcamera::Size &size, int flags);\n> > > > ~CameraBuffer();\n> > > >\n> > > > bool isValid() const;\n> > > > @@ -31,8 +35,12 @@ public:\n> > > > };\n> > > >\n> > > > #define PUBLIC_CAMERA_BUFFER_IMPLEMENTATION \\\n> > > > -CameraBuffer::CameraBuffer(buffer_handle_t camera3Buffer, int flags) \\\n> > > > - : Extensible(std::make_unique(this, camera3Buffer, flags)) \\\n> > > > +CameraBuffer::CameraBuffer(buffer_handle_t camera3Buffer, \\\n> > > > + libcamera::PixelFormat pixelFormat, \\\n> > > > + const libcamera::Size &size, int flags) \\\n> > > > + : Extensible(std::make_unique(this, camera3Buffer, \\\n> > > > + pixelFormat, size, \\\n> > > > + flags)) \\\n> > > > { \\\n> > > > } \\\n> > > > CameraBuffer::~CameraBuffer() \\\n> > > > diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp\n> > > > index 61b44183..01909ec7 100644\n> > > > --- a/src/android/camera_stream.cpp\n> > > > +++ b/src/android/camera_stream.cpp\n> > > > @@ -110,7 +110,9 @@ int CameraStream::process(const libcamera::FrameBuffer &source,\n> > > > * \\todo Buffer mapping and processing should be moved to a\n> > > > * separate thread.\n> > > > */\n> > > > - CameraBuffer dest(camera3Dest, PROT_READ | PROT_WRITE);\n> > > > + const StreamConfiguration &output = configuration();\n> > > > + CameraBuffer dest(camera3Dest, formats::MJPEG, output.size,\n> > > > + PROT_READ | PROT_WRITE);\n> > > > if (!dest.isValid()) {\n> > > > LOG(HAL, Error) << \"Failed to map android blob buffer\";\n> > > > return -EINVAL;\n> > > > diff --git a/src/android/mm/cros_camera_buffer.cpp b/src/android/mm/cros_camera_buffer.cpp\n> > > > index e8783ff8..50732637 100644\n> > > > --- a/src/android/mm/cros_camera_buffer.cpp\n> > > > +++ b/src/android/mm/cros_camera_buffer.cpp\n> > > > @@ -20,8 +20,9 @@ class CameraBuffer::Private : public Extensible::Private\n> > > > LIBCAMERA_DECLARE_PUBLIC(CameraBuffer)\n> > > >\n> > > > public:\n> > > > - Private(CameraBuffer *cameraBuffer,\n> > > > - buffer_handle_t camera3Buffer, int flags);\n> > > > + Private(CameraBuffer *cameraBuffer, buffer_handle_t camera3Buffer,\n> > > > + libcamera::PixelFormat pixelFormat, const libcamera::Size &size,\n> > > > + int flags);\n> > > > ~Private();\n> > > >\n> > > > bool isValid() const { return valid_; }\n> > > > @@ -46,6 +47,8 @@ private:\n> > > >\n> > > > CameraBuffer::Private::Private([[maybe_unused]] CameraBuffer *cameraBuffer,\n> > > > buffer_handle_t camera3Buffer,\n> > > > + [[maybe_unused]] libcamera::PixelFormat pixelFormat,\n> > > > + [[maybe_unused]] const libcamera::Size &size,\n> > > > [[maybe_unused]] int flags)\n> > > > : handle_(camera3Buffer), numPlanes_(0), valid_(false),\n> > > > registered_(false)\n> > > > diff --git a/src/android/mm/generic_camera_buffer.cpp b/src/android/mm/generic_camera_buffer.cpp\n> > > > index b3af194c..0bfdc2ba 100644\n> > > > --- a/src/android/mm/generic_camera_buffer.cpp\n> > > > +++ b/src/android/mm/generic_camera_buffer.cpp\n> > > > @@ -12,6 +12,7 @@\n> > > >\n> > > > #include \n> > > >\n> > > > +#include \"libcamera/internal/formats.h\"\n> > > > #include \"libcamera/internal/mapped_framebuffer.h\"\n> > > >\n> > > > using namespace libcamera;\n> > > > @@ -24,8 +25,9 @@ class CameraBuffer::Private : public Extensible::Private,\n> > > > LIBCAMERA_DECLARE_PUBLIC(CameraBuffer)\n> > > >\n> > > > public:\n> > > > - Private(CameraBuffer *cameraBuffer,\n> > > > - buffer_handle_t camera3Buffer, int flags);\n> > > > + Private(CameraBuffer *cameraBuffer, buffer_handle_t camera3Buffer,\n> > > > + libcamera::PixelFormat pixelFormat, const libcamera::Size &size,\n> > > > + int flags);\n> > > > ~Private();\n> > > >\n> > > > unsigned int numPlanes() const;\n> > > > @@ -33,35 +35,89 @@ public:\n> > > > Span plane(unsigned int plane);\n> > > >\n> > > > size_t jpegBufferSize(size_t maxJpegBufferSize) const;\n> > > > +\n> > > > +private:\n> > > > + /* \\todo Remove planes_ when it will be added to MappedBuffer */\n> > > > + std::vector> planes_;\n> > > > };\n> > > >\n> > > > CameraBuffer::Private::Private([[maybe_unused]] CameraBuffer *cameraBuffer,\n> > > > - buffer_handle_t camera3Buffer, int flags)\n> > > > + buffer_handle_t camera3Buffer,\n> > > > + libcamera::PixelFormat pixelFormat,\n> > > > + const libcamera::Size &size, int flags)\n> > > > {\n> > > > - maps_.reserve(camera3Buffer->numFds);\n> > > > error_ = 0;\n> > > > -\n> > > > + /*\n> > > > + * As Android doesn't offer an API to query buffer layouts, assume for\n> > > > + * now that the buffer is backed by a single dmabuf, with planes being\n> > > > + * stored contiguously.\n> > > > + */\n> > > > + int fd = -1;\n> > > > for (int i = 0; i < camera3Buffer->numFds; i++) {\n> > >\n> > > unsigned int i\n> > >\n> > > > - if (camera3Buffer->data[i] == -1)\n> > > > + if (camera3Buffer->data[i] == -1 ||\n> > > > + camera3Buffer->data[i] == fd) {\n> > > > continue;\n> > > > -\n> > > > - off_t length = lseek(camera3Buffer->data[i], 0, SEEK_END);\n> > > > - if (length < 0) {\n> > > > - error_ = -errno;\n> > > > - LOG(HAL, Error) << \"Failed to query plane length\";\n> > > > - break;\n> > > > }\n> > > >\n> > > > - void *address = mmap(nullptr, length, flags, MAP_SHARED,\n> > > > - camera3Buffer->data[i], 0);\n> > > > - if (address == MAP_FAILED) {\n> > > > - error_ = -errno;\n> > > > - LOG(HAL, Error) << \"Failed to mmap plane\";\n> > > > - break;\n> > > > - }\n> > > > + if (fd != -1)\n> > > > + LOG(HAL, Fatal) << \"Discontiguous planes are not supported\";\n> > >\n> > > Do we just warn ? Or exit with error ? See the below question\n> > >\n> > > > + fd = camera3Buffer->data[i];\n> > >\n> > > This assigns fd to the 'next' valid data[i].fd. I might have missed\n> > > what is the policy here if we have more than one valid dmabuf fds.\n> > >\n> > > Currently you warn and use the last valid one. I would instead error out,\n> > > or warn but use the first available one (even if that would result in\n> > > undefined behaviour, or most probably an access to non valid memory when\n> > > creating planes below).\n> > >\n> > > Do you have a use case for formats with planes allocated in different\n> > > buffers that you need to support ? Or can we error out without causing\n> > > regressions ?\n> > >\n> > > > + }\n> > >\n> > > The loop is pretty convoluted. If I'm not mistaken what you want to\n> > > verify is that\n> > > 1) Either all data[i].fd are the same\n> >\n> > This is actually not something that would happen in practice, because\n> > the handles are passed via Binder and even if the source puts the same\n> > FD for all the planes, the receiver would see all different FDs,\n> > because Binder would create a new FD for each array element.\n>\n> Sounds like there's room for improvement in binder then. Nevertheless,\n> we certainly want to support the case where the fds will be different\n> but all refer to the same dmabuf. The main blocker for this is the\n> ability to actually test it on Android, which I'm working on.\n>\n\nDo we really need that strict validation? I think we could just take\nthe first DMA-buf if the offsets match the non-M format layout. There\nis no practical case when libcamera would get more than 1 DMA-buf to\nbe used with hardware that only supports non-M formats.\n\n> > > 2) All but the data[0].fd are -1\n> > >\n> > > Wouldn't it be nicer as\n> > >\n> > > int fd = camera3Buffer->data[0].fd;\n> > > for (unsigned int i = 1; i < camera3Buffer->numFds; ++i) {\n> > > if (camera3Buffer->data[i].fd != -1)\n> > > continue;\n> > >\n> > > if (camera3Buffer->data[i].fd != fd)\n> > > /* Here I would error out */\n> > > }\n> > > if (fd == -1) {\n> > > /* No valid fds, error out */\n> > > }\n> > >\n> > > > +\n> > > > + if (fd != -1) {\n> > >\n> > > Shouldn't this be fd == -1 ? As in \"no valid fd found ?\"\n> > > If I'm not mistaken checking for fd != -1 should fail all the times as\n> > > at least one valid fd should be found. On which platform have you\n> > > tested this patch ?\n> > >\n> > > > + error_ = -EINVAL;\n> > > > + LOG(HAL, Error) << \"No valid file descriptor\";\n> > > > + return;\n> > > > + }\n> > > >\n> > > > - maps_.emplace_back(static_cast(address),\n> > > > - static_cast(length));\n> > > > + const auto &info = libcamera::PixelFormatInfo::info(pixelFormat);\n> > > > + if (!info.isValid()) {\n> > > > + error_ = -EINVAL;\n> > > > + LOG(HAL, Error) << \"Invalid pixel format: \"\n> > > > + << pixelFormat.toString();\n> > > > + return;\n> > > > + }\n> > >\n> > > We could check this at the very beginning of the function and avoid\n> > > looping on fds if the format is invalid.\n> > >\n> > > > +\n> > > > + off_t bufferLength = lseek(fd, 0, SEEK_END);\n> > > > + if (bufferLength < 0) {\n> > > > + error_ = -errno;\n> > > > + LOG(HAL, Error) << \"Failed to get buffer length\";\n> > > > + return;\n> > > > + }\n> > > > +\n> > > > + void *address = mmap(nullptr, bufferLength, flags, MAP_SHARED, fd, 0);\n> > > > + if (address == MAP_FAILED) {\n> > > > + error_ = -errno;\n> > > > + LOG(HAL, Error) << \"Failed to mmap plane\";\n> > > > + return;\n> > > > + }\n> > > > + maps_.emplace_back(static_cast(address), bufferLength);\n> > > > +\n> > > > + const unsigned int numPlanes = info.numPlanes();\n> > > > + planes_.resize(numPlanes);\n> > > > + unsigned int offset = 0;\n> > > > + for (unsigned int i = 0; i < numPlanes; ++i) {\n> > > > + /*\n> > > > + * \\todo Remove if this plane size computation function is\n> > > > + * added to PixelFormatInfo.\n> > > > + */\n> > > > + const unsigned int vertSubSample = info.planes[i].verticalSubSampling;\n> > > > + const unsigned int stride = info.stride(size.width, i, 1u);\n> > > > + const unsigned int planeSize =\n> > > > + stride * ((size.height + vertSubSample - 1) / vertSubSample);\n> > > > +\n> > > > + planes_[i] = libcamera::Span(\n> > > > + static_cast(address) + offset, planeSize);\n> > > > +\n> > > > + if (bufferLength < offset + planeSize) {\n> > > > + error_ = -EINVAL;\n> > > > + LOG(HAL, Error) << \"Plane \" << i << \" is out of buffer\"\n> > > > + << \", buffer length=\" << bufferLength\n> > > > + << \", offset=\" << offset\n> > > > + << \", size=\" << planeSize;\n> > > > + return;\n> > > > + }\n> > > > + offset += planeSize;\n> > > > }\n> > > > }\n> > > >\n> > > > @@ -71,15 +127,15 @@ CameraBuffer::Private::~Private()\n> > > >\n> > > > unsigned int CameraBuffer::Private::numPlanes() const\n> > > > {\n> > > > - return maps_.size();\n> > > > + return planes_.size();\n> > > > }\n> > > >\n> > > > Span CameraBuffer::Private::plane(unsigned int plane)\n> > > > {\n> > > > - if (plane >= maps_.size())\n> > > > + if (plane >= planes_.size())\n> > > > return {};\n> > > >\n> > > > - return maps_[plane];\n> > > > + return planes_[plane];\n> > > > }\n> > > >\n> > > > size_t CameraBuffer::Private::jpegBufferSize(size_t maxJpegBufferSize) const\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 0EFA1BD87C\n\tfor ;\n\tFri, 27 Aug 2021 12:01:25 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7197D68932;\n\tFri, 27 Aug 2021 14:01:24 +0200 (CEST)","from mail-ed1-x52f.google.com (mail-ed1-x52f.google.com\n\t[IPv6:2a00:1450:4864:20::52f])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 4B2E660256\n\tfor ;\n\tFri, 27 Aug 2021 14:01:22 +0200 (CEST)","by mail-ed1-x52f.google.com with SMTP id dm15so9513206edb.10\n\tfor ;\n\tFri, 27 Aug 2021 05:01:22 -0700 (PDT)","from mail-wr1-f47.google.com (mail-wr1-f47.google.com.\n\t[209.85.221.47]) by smtp.gmail.com with ESMTPSA id\n\tn23sm3363704eds.41.2021.08.27.05.01.20\n\tfor \n\t(version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128);\n\tFri, 27 Aug 2021 05:01:20 -0700 (PDT)","by mail-wr1-f47.google.com with SMTP id g18so2492280wrc.11\n\tfor ;\n\tFri, 27 Aug 2021 05:01:20 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"mZWLPHX9\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org;\n\ts=google; \n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=DBs+0sZ9DRtaFPlSlyEWRj0KM9l1X1X0BO7jwZys2gQ=;\n\tb=mZWLPHX9ywpaLKnNFbhuEsWf3bECJV1zkOSgLG9E1IvT4rrW/vTPTPnpti1UDAOriB\n\tUYTgClU4toy9Lsm7v63xX1bzuvt3c/cAXni2FV9ZBRE4+A8EKRvchghS2sfHpzBogmv2\n\tV2nV3Minvs2Tp3u/kA3A0EeOY0M5K8L3i4qto=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=DBs+0sZ9DRtaFPlSlyEWRj0KM9l1X1X0BO7jwZys2gQ=;\n\tb=M0cBC13NBtBUxeMFhyQwcJf1pZc/FdL6ZV/yoHWwWWElLkrTYoH0DDRauC1dO8cUJa\n\tu/JXE5PcBeKfW7/XBTNgL7S61lR3oJ9wiPzbMjko6YYAwWa81U0AWvOHNrF8Zkdto4EP\n\t8aBCxV8UOEYiEI6buEqfQqwW7bc2JlmnW2Y3F2EYXWXCNmhfOCzfHEn73aqw3xPHIvXl\n\tTNXtz8VY0pyTKakLMhhKU2FC3yNttIbUQjCwLnmwlL2GYRKONrig1SdtgS++VXjqn180\n\tlRbnSSwKUNETNlAtgGfh6fadVI1+xkl0y3QcPpSMVq4T5qtbLpni9L5FGHRM5X7iP0C+\n\tIi6w==","X-Gm-Message-State":"AOAM5331tMjWKcIFonZSTug+vZgDvT6bpHcELPkmxWSmzLWnMzbehxra\n\tcfmIYRK1cnySegpBB2uwGxyz9KCcZA0Xew==","X-Google-Smtp-Source":"ABdhPJz/VqacUywgWJvy0sKNidNu63Z4T5SUMbakCT1395sXwf5JBbZe3oFR9fAZ3K1wV1f72LHx1A==","X-Received":["by 2002:aa7:d9d2:: with SMTP id\n\tv18mr9703333eds.128.1630065681254; \n\tFri, 27 Aug 2021 05:01:21 -0700 (PDT)","by 2002:a5d:5919:: with SMTP id v25mr9728066wrd.32.1630065679689;\n\tFri, 27 Aug 2021 05:01:19 -0700 (PDT)"],"MIME-Version":"1.0","References":"<20210825044410.2787433-1-hiroh@chromium.org>\n\t<20210825044410.2787433-2-hiroh@chromium.org>\n\t<20210825083507.5bodpgew6lsllx77@uno.localdomain>\n\t\n\t","In-Reply-To":"","From":"Tomasz Figa ","Date":"Fri, 27 Aug 2021 21:01:07 +0900","X-Gmail-Original-Message-ID":"","Message-ID":"","To":"Laurent Pinchart ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH v2 1/3] android:\n\tgeneric_camera_buffer: Correct buffer mapping","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":19141,"web_url":"https://patchwork.libcamera.org/comment/19141/","msgid":"","date":"2021-08-27T12:07:30","subject":"Re: [libcamera-devel] [PATCH v2 1/3] android:\n\tgeneric_camera_buffer: Correct buffer mapping","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Tomasz,\n\nOn Fri, Aug 27, 2021 at 09:01:07PM +0900, Tomasz Figa wrote:\n> On Fri, Aug 27, 2021 at 8:36 PM Laurent Pinchart wrote:\n> > On Fri, Aug 27, 2021 at 07:08:30PM +0900, Tomasz Figa wrote:\n> > > On Wed, Aug 25, 2021 at 5:34 PM Jacopo Mondi wrote:\n> > > > On Wed, Aug 25, 2021 at 01:44:08PM +0900, Hirokazu Honda wrote:\n> > > > > buffer_hanlde_t doesn't provide sufficient info to map a buffer\n> > > > > properly. cros::CameraBufferManager enables handling the buffer on\n> > > > > ChromeOS, but no way is provided for other platforms.\n> > > > >\n> > > > > Therefore, we put the assumption that planes are in the same buffer\n> > > > > and they are consecutive. This modifies the way of mapping in\n> > > > > generic_camera_buffer with the assumption.\n> > > > >\n> > > > > Signed-off-by: Hirokazu Honda \n> > > > > ---\n> > > > > src/android/camera_buffer.h | 14 ++-\n> > > > > src/android/camera_stream.cpp | 4 +-\n> > > > > src/android/mm/cros_camera_buffer.cpp | 7 +-\n> > > > > src/android/mm/generic_camera_buffer.cpp | 104 +++++++++++++++++------\n> > > > > 4 files changed, 99 insertions(+), 30 deletions(-)\n> > > > >\n> > > > > diff --git a/src/android/camera_buffer.h b/src/android/camera_buffer.h\n> > > > > index c4e3a9e7..87df2570 100644\n> > > > > --- a/src/android/camera_buffer.h\n> > > > > +++ b/src/android/camera_buffer.h\n> > > > > @@ -11,13 +11,17 @@\n> > > > >\n> > > > > #include \n> > > > > #include \n> > > > > +#include \n> > > > > +#include \n> > > > >\n> > > > > class CameraBuffer final : public libcamera::Extensible\n> > > > > {\n> > > > > LIBCAMERA_DECLARE_PRIVATE()\n> > > > >\n> > > > > public:\n> > > > > - CameraBuffer(buffer_handle_t camera3Buffer, int flags);\n> > > > > + CameraBuffer(buffer_handle_t camera3Buffer,\n> > > > > + libcamera::PixelFormat pixelFormat,\n> > > > > + const libcamera::Size &size, int flags);\n> > > > > ~CameraBuffer();\n> > > > >\n> > > > > bool isValid() const;\n> > > > > @@ -31,8 +35,12 @@ public:\n> > > > > };\n> > > > >\n> > > > > #define PUBLIC_CAMERA_BUFFER_IMPLEMENTATION \\\n> > > > > -CameraBuffer::CameraBuffer(buffer_handle_t camera3Buffer, int flags) \\\n> > > > > - : Extensible(std::make_unique(this, camera3Buffer, flags)) \\\n> > > > > +CameraBuffer::CameraBuffer(buffer_handle_t camera3Buffer, \\\n> > > > > + libcamera::PixelFormat pixelFormat, \\\n> > > > > + const libcamera::Size &size, int flags) \\\n> > > > > + : Extensible(std::make_unique(this, camera3Buffer, \\\n> > > > > + pixelFormat, size, \\\n> > > > > + flags)) \\\n> > > > > { \\\n> > > > > } \\\n> > > > > CameraBuffer::~CameraBuffer() \\\n> > > > > diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp\n> > > > > index 61b44183..01909ec7 100644\n> > > > > --- a/src/android/camera_stream.cpp\n> > > > > +++ b/src/android/camera_stream.cpp\n> > > > > @@ -110,7 +110,9 @@ int CameraStream::process(const libcamera::FrameBuffer &source,\n> > > > > * \\todo Buffer mapping and processing should be moved to a\n> > > > > * separate thread.\n> > > > > */\n> > > > > - CameraBuffer dest(camera3Dest, PROT_READ | PROT_WRITE);\n> > > > > + const StreamConfiguration &output = configuration();\n> > > > > + CameraBuffer dest(camera3Dest, formats::MJPEG, output.size,\n> > > > > + PROT_READ | PROT_WRITE);\n> > > > > if (!dest.isValid()) {\n> > > > > LOG(HAL, Error) << \"Failed to map android blob buffer\";\n> > > > > return -EINVAL;\n> > > > > diff --git a/src/android/mm/cros_camera_buffer.cpp b/src/android/mm/cros_camera_buffer.cpp\n> > > > > index e8783ff8..50732637 100644\n> > > > > --- a/src/android/mm/cros_camera_buffer.cpp\n> > > > > +++ b/src/android/mm/cros_camera_buffer.cpp\n> > > > > @@ -20,8 +20,9 @@ class CameraBuffer::Private : public Extensible::Private\n> > > > > LIBCAMERA_DECLARE_PUBLIC(CameraBuffer)\n> > > > >\n> > > > > public:\n> > > > > - Private(CameraBuffer *cameraBuffer,\n> > > > > - buffer_handle_t camera3Buffer, int flags);\n> > > > > + Private(CameraBuffer *cameraBuffer, buffer_handle_t camera3Buffer,\n> > > > > + libcamera::PixelFormat pixelFormat, const libcamera::Size &size,\n> > > > > + int flags);\n> > > > > ~Private();\n> > > > >\n> > > > > bool isValid() const { return valid_; }\n> > > > > @@ -46,6 +47,8 @@ private:\n> > > > >\n> > > > > CameraBuffer::Private::Private([[maybe_unused]] CameraBuffer *cameraBuffer,\n> > > > > buffer_handle_t camera3Buffer,\n> > > > > + [[maybe_unused]] libcamera::PixelFormat pixelFormat,\n> > > > > + [[maybe_unused]] const libcamera::Size &size,\n> > > > > [[maybe_unused]] int flags)\n> > > > > : handle_(camera3Buffer), numPlanes_(0), valid_(false),\n> > > > > registered_(false)\n> > > > > diff --git a/src/android/mm/generic_camera_buffer.cpp b/src/android/mm/generic_camera_buffer.cpp\n> > > > > index b3af194c..0bfdc2ba 100644\n> > > > > --- a/src/android/mm/generic_camera_buffer.cpp\n> > > > > +++ b/src/android/mm/generic_camera_buffer.cpp\n> > > > > @@ -12,6 +12,7 @@\n> > > > >\n> > > > > #include \n> > > > >\n> > > > > +#include \"libcamera/internal/formats.h\"\n> > > > > #include \"libcamera/internal/mapped_framebuffer.h\"\n> > > > >\n> > > > > using namespace libcamera;\n> > > > > @@ -24,8 +25,9 @@ class CameraBuffer::Private : public Extensible::Private,\n> > > > > LIBCAMERA_DECLARE_PUBLIC(CameraBuffer)\n> > > > >\n> > > > > public:\n> > > > > - Private(CameraBuffer *cameraBuffer,\n> > > > > - buffer_handle_t camera3Buffer, int flags);\n> > > > > + Private(CameraBuffer *cameraBuffer, buffer_handle_t camera3Buffer,\n> > > > > + libcamera::PixelFormat pixelFormat, const libcamera::Size &size,\n> > > > > + int flags);\n> > > > > ~Private();\n> > > > >\n> > > > > unsigned int numPlanes() const;\n> > > > > @@ -33,35 +35,89 @@ public:\n> > > > > Span plane(unsigned int plane);\n> > > > >\n> > > > > size_t jpegBufferSize(size_t maxJpegBufferSize) const;\n> > > > > +\n> > > > > +private:\n> > > > > + /* \\todo Remove planes_ when it will be added to MappedBuffer */\n> > > > > + std::vector> planes_;\n> > > > > };\n> > > > >\n> > > > > CameraBuffer::Private::Private([[maybe_unused]] CameraBuffer *cameraBuffer,\n> > > > > - buffer_handle_t camera3Buffer, int flags)\n> > > > > + buffer_handle_t camera3Buffer,\n> > > > > + libcamera::PixelFormat pixelFormat,\n> > > > > + const libcamera::Size &size, int flags)\n> > > > > {\n> > > > > - maps_.reserve(camera3Buffer->numFds);\n> > > > > error_ = 0;\n> > > > > -\n> > > > > + /*\n> > > > > + * As Android doesn't offer an API to query buffer layouts, assume for\n> > > > > + * now that the buffer is backed by a single dmabuf, with planes being\n> > > > > + * stored contiguously.\n> > > > > + */\n> > > > > + int fd = -1;\n> > > > > for (int i = 0; i < camera3Buffer->numFds; i++) {\n> > > >\n> > > > unsigned int i\n> > > >\n> > > > > - if (camera3Buffer->data[i] == -1)\n> > > > > + if (camera3Buffer->data[i] == -1 ||\n> > > > > + camera3Buffer->data[i] == fd) {\n> > > > > continue;\n> > > > > -\n> > > > > - off_t length = lseek(camera3Buffer->data[i], 0, SEEK_END);\n> > > > > - if (length < 0) {\n> > > > > - error_ = -errno;\n> > > > > - LOG(HAL, Error) << \"Failed to query plane length\";\n> > > > > - break;\n> > > > > }\n> > > > >\n> > > > > - void *address = mmap(nullptr, length, flags, MAP_SHARED,\n> > > > > - camera3Buffer->data[i], 0);\n> > > > > - if (address == MAP_FAILED) {\n> > > > > - error_ = -errno;\n> > > > > - LOG(HAL, Error) << \"Failed to mmap plane\";\n> > > > > - break;\n> > > > > - }\n> > > > > + if (fd != -1)\n> > > > > + LOG(HAL, Fatal) << \"Discontiguous planes are not supported\";\n> > > >\n> > > > Do we just warn ? Or exit with error ? See the below question\n> > > >\n> > > > > + fd = camera3Buffer->data[i];\n> > > >\n> > > > This assigns fd to the 'next' valid data[i].fd. I might have missed\n> > > > what is the policy here if we have more than one valid dmabuf fds.\n> > > >\n> > > > Currently you warn and use the last valid one. I would instead error out,\n> > > > or warn but use the first available one (even if that would result in\n> > > > undefined behaviour, or most probably an access to non valid memory when\n> > > > creating planes below).\n> > > >\n> > > > Do you have a use case for formats with planes allocated in different\n> > > > buffers that you need to support ? Or can we error out without causing\n> > > > regressions ?\n> > > >\n> > > > > + }\n> > > >\n> > > > The loop is pretty convoluted. If I'm not mistaken what you want to\n> > > > verify is that\n> > > > 1) Either all data[i].fd are the same\n> > >\n> > > This is actually not something that would happen in practice, because\n> > > the handles are passed via Binder and even if the source puts the same\n> > > FD for all the planes, the receiver would see all different FDs,\n> > > because Binder would create a new FD for each array element.\n> >\n> > Sounds like there's room for improvement in binder then. Nevertheless,\n> > we certainly want to support the case where the fds will be different\n> > but all refer to the same dmabuf. The main blocker for this is the\n> > ability to actually test it on Android, which I'm working on.\n> \n> Do we really need that strict validation? I think we could just take\n> the first DMA-buf if the offsets match the non-M format layout. There\n> is no practical case when libcamera would get more than 1 DMA-buf to\n> be used with hardware that only supports non-M formats.\n\nIt's an easy check to implement, so I'd rather do it now that my memory\nis fresh than running into a problem in a year and spend days debugging\nit :-)\n\n> > > > 2) All but the data[0].fd are -1\n> > > >\n> > > > Wouldn't it be nicer as\n> > > >\n> > > > int fd = camera3Buffer->data[0].fd;\n> > > > for (unsigned int i = 1; i < camera3Buffer->numFds; ++i) {\n> > > > if (camera3Buffer->data[i].fd != -1)\n> > > > continue;\n> > > >\n> > > > if (camera3Buffer->data[i].fd != fd)\n> > > > /* Here I would error out */\n> > > > }\n> > > > if (fd == -1) {\n> > > > /* No valid fds, error out */\n> > > > }\n> > > >\n> > > > > +\n> > > > > + if (fd != -1) {\n> > > >\n> > > > Shouldn't this be fd == -1 ? As in \"no valid fd found ?\"\n> > > > If I'm not mistaken checking for fd != -1 should fail all the times as\n> > > > at least one valid fd should be found. On which platform have you\n> > > > tested this patch ?\n> > > >\n> > > > > + error_ = -EINVAL;\n> > > > > + LOG(HAL, Error) << \"No valid file descriptor\";\n> > > > > + return;\n> > > > > + }\n> > > > >\n> > > > > - maps_.emplace_back(static_cast(address),\n> > > > > - static_cast(length));\n> > > > > + const auto &info = libcamera::PixelFormatInfo::info(pixelFormat);\n> > > > > + if (!info.isValid()) {\n> > > > > + error_ = -EINVAL;\n> > > > > + LOG(HAL, Error) << \"Invalid pixel format: \"\n> > > > > + << pixelFormat.toString();\n> > > > > + return;\n> > > > > + }\n> > > >\n> > > > We could check this at the very beginning of the function and avoid\n> > > > looping on fds if the format is invalid.\n> > > >\n> > > > > +\n> > > > > + off_t bufferLength = lseek(fd, 0, SEEK_END);\n> > > > > + if (bufferLength < 0) {\n> > > > > + error_ = -errno;\n> > > > > + LOG(HAL, Error) << \"Failed to get buffer length\";\n> > > > > + return;\n> > > > > + }\n> > > > > +\n> > > > > + void *address = mmap(nullptr, bufferLength, flags, MAP_SHARED, fd, 0);\n> > > > > + if (address == MAP_FAILED) {\n> > > > > + error_ = -errno;\n> > > > > + LOG(HAL, Error) << \"Failed to mmap plane\";\n> > > > > + return;\n> > > > > + }\n> > > > > + maps_.emplace_back(static_cast(address), bufferLength);\n> > > > > +\n> > > > > + const unsigned int numPlanes = info.numPlanes();\n> > > > > + planes_.resize(numPlanes);\n> > > > > + unsigned int offset = 0;\n> > > > > + for (unsigned int i = 0; i < numPlanes; ++i) {\n> > > > > + /*\n> > > > > + * \\todo Remove if this plane size computation function is\n> > > > > + * added to PixelFormatInfo.\n> > > > > + */\n> > > > > + const unsigned int vertSubSample = info.planes[i].verticalSubSampling;\n> > > > > + const unsigned int stride = info.stride(size.width, i, 1u);\n> > > > > + const unsigned int planeSize =\n> > > > > + stride * ((size.height + vertSubSample - 1) / vertSubSample);\n> > > > > +\n> > > > > + planes_[i] = libcamera::Span(\n> > > > > + static_cast(address) + offset, planeSize);\n> > > > > +\n> > > > > + if (bufferLength < offset + planeSize) {\n> > > > > + error_ = -EINVAL;\n> > > > > + LOG(HAL, Error) << \"Plane \" << i << \" is out of buffer\"\n> > > > > + << \", buffer length=\" << bufferLength\n> > > > > + << \", offset=\" << offset\n> > > > > + << \", size=\" << planeSize;\n> > > > > + return;\n> > > > > + }\n> > > > > + offset += planeSize;\n> > > > > }\n> > > > > }\n> > > > >\n> > > > > @@ -71,15 +127,15 @@ CameraBuffer::Private::~Private()\n> > > > >\n> > > > > unsigned int CameraBuffer::Private::numPlanes() const\n> > > > > {\n> > > > > - return maps_.size();\n> > > > > + return planes_.size();\n> > > > > }\n> > > > >\n> > > > > Span CameraBuffer::Private::plane(unsigned int plane)\n> > > > > {\n> > > > > - if (plane >= maps_.size())\n> > > > > + if (plane >= planes_.size())\n> > > > > return {};\n> > > > >\n> > > > > - return maps_[plane];\n> > > > > + return planes_[plane];\n> > > > > }\n> > > > >\n> > > > > size_t CameraBuffer::Private::jpegBufferSize(size_t maxJpegBufferSize) const","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id DCF55C324E\n\tfor ;\n\tFri, 27 Aug 2021 12:08:13 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 23ACE6891F;\n\tFri, 27 Aug 2021 14:07:46 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 34F0D60256\n\tfor ;\n\tFri, 27 Aug 2021 14:07:44 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id AA1C0739;\n\tFri, 27 Aug 2021 14:07:43 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"wpy1cCtc\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1630066063;\n\tbh=RRrxYW+Qs3UOK9rWbHkOAkKz7/bZyOKDUxS+zM+lZZg=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=wpy1cCtcOhvkP5x6LufDVywQbXLoKzcBAcUUQWf3sj+hOCbr1SoyO+j2JvsFCHkEe\n\tubNryNUAtfiCbUMHEskkPVmXk3PFnqnjBYeFWeZlU0vHBM9xcgIGSRtVUac1334nAN\n\tzjUSAOQGb6ackcAKxsTcMiWKQnYUXxlJyI/AJXgo=","Date":"Fri, 27 Aug 2021 15:07:30 +0300","From":"Laurent Pinchart ","To":"Tomasz Figa ","Message-ID":"","References":"<20210825044410.2787433-1-hiroh@chromium.org>\n\t<20210825044410.2787433-2-hiroh@chromium.org>\n\t<20210825083507.5bodpgew6lsllx77@uno.localdomain>\n\t\n\t\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v2 1/3] android:\n\tgeneric_camera_buffer: Correct buffer mapping","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":19142,"web_url":"https://patchwork.libcamera.org/comment/19142/","msgid":"","date":"2021-08-27T12:09:37","subject":"Re: [libcamera-devel] [PATCH v2 1/3] android:\n\tgeneric_camera_buffer: Correct buffer mapping","submitter":{"id":9,"url":"https://patchwork.libcamera.org/api/people/9/","name":"Tomasz Figa","email":"tfiga@chromium.org"},"content":"On Fri, Aug 27, 2021 at 9:07 PM Laurent Pinchart\n wrote:\n>\n> Hi Tomasz,\n>\n> On Fri, Aug 27, 2021 at 09:01:07PM +0900, Tomasz Figa wrote:\n> > On Fri, Aug 27, 2021 at 8:36 PM Laurent Pinchart wrote:\n> > > On Fri, Aug 27, 2021 at 07:08:30PM +0900, Tomasz Figa wrote:\n> > > > On Wed, Aug 25, 2021 at 5:34 PM Jacopo Mondi wrote:\n> > > > > On Wed, Aug 25, 2021 at 01:44:08PM +0900, Hirokazu Honda wrote:\n> > > > > > buffer_hanlde_t doesn't provide sufficient info to map a buffer\n> > > > > > properly. cros::CameraBufferManager enables handling the buffer on\n> > > > > > ChromeOS, but no way is provided for other platforms.\n> > > > > >\n> > > > > > Therefore, we put the assumption that planes are in the same buffer\n> > > > > > and they are consecutive. This modifies the way of mapping in\n> > > > > > generic_camera_buffer with the assumption.\n> > > > > >\n> > > > > > Signed-off-by: Hirokazu Honda \n> > > > > > ---\n> > > > > > src/android/camera_buffer.h | 14 ++-\n> > > > > > src/android/camera_stream.cpp | 4 +-\n> > > > > > src/android/mm/cros_camera_buffer.cpp | 7 +-\n> > > > > > src/android/mm/generic_camera_buffer.cpp | 104 +++++++++++++++++------\n> > > > > > 4 files changed, 99 insertions(+), 30 deletions(-)\n> > > > > >\n> > > > > > diff --git a/src/android/camera_buffer.h b/src/android/camera_buffer.h\n> > > > > > index c4e3a9e7..87df2570 100644\n> > > > > > --- a/src/android/camera_buffer.h\n> > > > > > +++ b/src/android/camera_buffer.h\n> > > > > > @@ -11,13 +11,17 @@\n> > > > > >\n> > > > > > #include \n> > > > > > #include \n> > > > > > +#include \n> > > > > > +#include \n> > > > > >\n> > > > > > class CameraBuffer final : public libcamera::Extensible\n> > > > > > {\n> > > > > > LIBCAMERA_DECLARE_PRIVATE()\n> > > > > >\n> > > > > > public:\n> > > > > > - CameraBuffer(buffer_handle_t camera3Buffer, int flags);\n> > > > > > + CameraBuffer(buffer_handle_t camera3Buffer,\n> > > > > > + libcamera::PixelFormat pixelFormat,\n> > > > > > + const libcamera::Size &size, int flags);\n> > > > > > ~CameraBuffer();\n> > > > > >\n> > > > > > bool isValid() const;\n> > > > > > @@ -31,8 +35,12 @@ public:\n> > > > > > };\n> > > > > >\n> > > > > > #define PUBLIC_CAMERA_BUFFER_IMPLEMENTATION \\\n> > > > > > -CameraBuffer::CameraBuffer(buffer_handle_t camera3Buffer, int flags) \\\n> > > > > > - : Extensible(std::make_unique(this, camera3Buffer, flags)) \\\n> > > > > > +CameraBuffer::CameraBuffer(buffer_handle_t camera3Buffer, \\\n> > > > > > + libcamera::PixelFormat pixelFormat, \\\n> > > > > > + const libcamera::Size &size, int flags) \\\n> > > > > > + : Extensible(std::make_unique(this, camera3Buffer, \\\n> > > > > > + pixelFormat, size, \\\n> > > > > > + flags)) \\\n> > > > > > { \\\n> > > > > > } \\\n> > > > > > CameraBuffer::~CameraBuffer() \\\n> > > > > > diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp\n> > > > > > index 61b44183..01909ec7 100644\n> > > > > > --- a/src/android/camera_stream.cpp\n> > > > > > +++ b/src/android/camera_stream.cpp\n> > > > > > @@ -110,7 +110,9 @@ int CameraStream::process(const libcamera::FrameBuffer &source,\n> > > > > > * \\todo Buffer mapping and processing should be moved to a\n> > > > > > * separate thread.\n> > > > > > */\n> > > > > > - CameraBuffer dest(camera3Dest, PROT_READ | PROT_WRITE);\n> > > > > > + const StreamConfiguration &output = configuration();\n> > > > > > + CameraBuffer dest(camera3Dest, formats::MJPEG, output.size,\n> > > > > > + PROT_READ | PROT_WRITE);\n> > > > > > if (!dest.isValid()) {\n> > > > > > LOG(HAL, Error) << \"Failed to map android blob buffer\";\n> > > > > > return -EINVAL;\n> > > > > > diff --git a/src/android/mm/cros_camera_buffer.cpp b/src/android/mm/cros_camera_buffer.cpp\n> > > > > > index e8783ff8..50732637 100644\n> > > > > > --- a/src/android/mm/cros_camera_buffer.cpp\n> > > > > > +++ b/src/android/mm/cros_camera_buffer.cpp\n> > > > > > @@ -20,8 +20,9 @@ class CameraBuffer::Private : public Extensible::Private\n> > > > > > LIBCAMERA_DECLARE_PUBLIC(CameraBuffer)\n> > > > > >\n> > > > > > public:\n> > > > > > - Private(CameraBuffer *cameraBuffer,\n> > > > > > - buffer_handle_t camera3Buffer, int flags);\n> > > > > > + Private(CameraBuffer *cameraBuffer, buffer_handle_t camera3Buffer,\n> > > > > > + libcamera::PixelFormat pixelFormat, const libcamera::Size &size,\n> > > > > > + int flags);\n> > > > > > ~Private();\n> > > > > >\n> > > > > > bool isValid() const { return valid_; }\n> > > > > > @@ -46,6 +47,8 @@ private:\n> > > > > >\n> > > > > > CameraBuffer::Private::Private([[maybe_unused]] CameraBuffer *cameraBuffer,\n> > > > > > buffer_handle_t camera3Buffer,\n> > > > > > + [[maybe_unused]] libcamera::PixelFormat pixelFormat,\n> > > > > > + [[maybe_unused]] const libcamera::Size &size,\n> > > > > > [[maybe_unused]] int flags)\n> > > > > > : handle_(camera3Buffer), numPlanes_(0), valid_(false),\n> > > > > > registered_(false)\n> > > > > > diff --git a/src/android/mm/generic_camera_buffer.cpp b/src/android/mm/generic_camera_buffer.cpp\n> > > > > > index b3af194c..0bfdc2ba 100644\n> > > > > > --- a/src/android/mm/generic_camera_buffer.cpp\n> > > > > > +++ b/src/android/mm/generic_camera_buffer.cpp\n> > > > > > @@ -12,6 +12,7 @@\n> > > > > >\n> > > > > > #include \n> > > > > >\n> > > > > > +#include \"libcamera/internal/formats.h\"\n> > > > > > #include \"libcamera/internal/mapped_framebuffer.h\"\n> > > > > >\n> > > > > > using namespace libcamera;\n> > > > > > @@ -24,8 +25,9 @@ class CameraBuffer::Private : public Extensible::Private,\n> > > > > > LIBCAMERA_DECLARE_PUBLIC(CameraBuffer)\n> > > > > >\n> > > > > > public:\n> > > > > > - Private(CameraBuffer *cameraBuffer,\n> > > > > > - buffer_handle_t camera3Buffer, int flags);\n> > > > > > + Private(CameraBuffer *cameraBuffer, buffer_handle_t camera3Buffer,\n> > > > > > + libcamera::PixelFormat pixelFormat, const libcamera::Size &size,\n> > > > > > + int flags);\n> > > > > > ~Private();\n> > > > > >\n> > > > > > unsigned int numPlanes() const;\n> > > > > > @@ -33,35 +35,89 @@ public:\n> > > > > > Span plane(unsigned int plane);\n> > > > > >\n> > > > > > size_t jpegBufferSize(size_t maxJpegBufferSize) const;\n> > > > > > +\n> > > > > > +private:\n> > > > > > + /* \\todo Remove planes_ when it will be added to MappedBuffer */\n> > > > > > + std::vector> planes_;\n> > > > > > };\n> > > > > >\n> > > > > > CameraBuffer::Private::Private([[maybe_unused]] CameraBuffer *cameraBuffer,\n> > > > > > - buffer_handle_t camera3Buffer, int flags)\n> > > > > > + buffer_handle_t camera3Buffer,\n> > > > > > + libcamera::PixelFormat pixelFormat,\n> > > > > > + const libcamera::Size &size, int flags)\n> > > > > > {\n> > > > > > - maps_.reserve(camera3Buffer->numFds);\n> > > > > > error_ = 0;\n> > > > > > -\n> > > > > > + /*\n> > > > > > + * As Android doesn't offer an API to query buffer layouts, assume for\n> > > > > > + * now that the buffer is backed by a single dmabuf, with planes being\n> > > > > > + * stored contiguously.\n> > > > > > + */\n> > > > > > + int fd = -1;\n> > > > > > for (int i = 0; i < camera3Buffer->numFds; i++) {\n> > > > >\n> > > > > unsigned int i\n> > > > >\n> > > > > > - if (camera3Buffer->data[i] == -1)\n> > > > > > + if (camera3Buffer->data[i] == -1 ||\n> > > > > > + camera3Buffer->data[i] == fd) {\n> > > > > > continue;\n> > > > > > -\n> > > > > > - off_t length = lseek(camera3Buffer->data[i], 0, SEEK_END);\n> > > > > > - if (length < 0) {\n> > > > > > - error_ = -errno;\n> > > > > > - LOG(HAL, Error) << \"Failed to query plane length\";\n> > > > > > - break;\n> > > > > > }\n> > > > > >\n> > > > > > - void *address = mmap(nullptr, length, flags, MAP_SHARED,\n> > > > > > - camera3Buffer->data[i], 0);\n> > > > > > - if (address == MAP_FAILED) {\n> > > > > > - error_ = -errno;\n> > > > > > - LOG(HAL, Error) << \"Failed to mmap plane\";\n> > > > > > - break;\n> > > > > > - }\n> > > > > > + if (fd != -1)\n> > > > > > + LOG(HAL, Fatal) << \"Discontiguous planes are not supported\";\n> > > > >\n> > > > > Do we just warn ? Or exit with error ? See the below question\n> > > > >\n> > > > > > + fd = camera3Buffer->data[i];\n> > > > >\n> > > > > This assigns fd to the 'next' valid data[i].fd. I might have missed\n> > > > > what is the policy here if we have more than one valid dmabuf fds.\n> > > > >\n> > > > > Currently you warn and use the last valid one. I would instead error out,\n> > > > > or warn but use the first available one (even if that would result in\n> > > > > undefined behaviour, or most probably an access to non valid memory when\n> > > > > creating planes below).\n> > > > >\n> > > > > Do you have a use case for formats with planes allocated in different\n> > > > > buffers that you need to support ? Or can we error out without causing\n> > > > > regressions ?\n> > > > >\n> > > > > > + }\n> > > > >\n> > > > > The loop is pretty convoluted. If I'm not mistaken what you want to\n> > > > > verify is that\n> > > > > 1) Either all data[i].fd are the same\n> > > >\n> > > > This is actually not something that would happen in practice, because\n> > > > the handles are passed via Binder and even if the source puts the same\n> > > > FD for all the planes, the receiver would see all different FDs,\n> > > > because Binder would create a new FD for each array element.\n> > >\n> > > Sounds like there's room for improvement in binder then. Nevertheless,\n> > > we certainly want to support the case where the fds will be different\n> > > but all refer to the same dmabuf. The main blocker for this is the\n> > > ability to actually test it on Android, which I'm working on.\n> >\n> > Do we really need that strict validation? I think we could just take\n> > the first DMA-buf if the offsets match the non-M format layout. There\n> > is no practical case when libcamera would get more than 1 DMA-buf to\n> > be used with hardware that only supports non-M formats.\n>\n> It's an easy check to implement, so I'd rather do it now that my memory\n> is fresh than running into a problem in a year and spend days debugging\n> it :-)\n\nIs the check cheap and compatible with older kernel versions?\n\nGenerally I don't think it would ever be possible to run into issues\nwith this in real life, but agreed that if the check is simple it's\nbetter to have it.\n\n>\n> > > > > 2) All but the data[0].fd are -1\n> > > > >\n> > > > > Wouldn't it be nicer as\n> > > > >\n> > > > > int fd = camera3Buffer->data[0].fd;\n> > > > > for (unsigned int i = 1; i < camera3Buffer->numFds; ++i) {\n> > > > > if (camera3Buffer->data[i].fd != -1)\n> > > > > continue;\n> > > > >\n> > > > > if (camera3Buffer->data[i].fd != fd)\n> > > > > /* Here I would error out */\n> > > > > }\n> > > > > if (fd == -1) {\n> > > > > /* No valid fds, error out */\n> > > > > }\n> > > > >\n> > > > > > +\n> > > > > > + if (fd != -1) {\n> > > > >\n> > > > > Shouldn't this be fd == -1 ? As in \"no valid fd found ?\"\n> > > > > If I'm not mistaken checking for fd != -1 should fail all the times as\n> > > > > at least one valid fd should be found. On which platform have you\n> > > > > tested this patch ?\n> > > > >\n> > > > > > + error_ = -EINVAL;\n> > > > > > + LOG(HAL, Error) << \"No valid file descriptor\";\n> > > > > > + return;\n> > > > > > + }\n> > > > > >\n> > > > > > - maps_.emplace_back(static_cast(address),\n> > > > > > - static_cast(length));\n> > > > > > + const auto &info = libcamera::PixelFormatInfo::info(pixelFormat);\n> > > > > > + if (!info.isValid()) {\n> > > > > > + error_ = -EINVAL;\n> > > > > > + LOG(HAL, Error) << \"Invalid pixel format: \"\n> > > > > > + << pixelFormat.toString();\n> > > > > > + return;\n> > > > > > + }\n> > > > >\n> > > > > We could check this at the very beginning of the function and avoid\n> > > > > looping on fds if the format is invalid.\n> > > > >\n> > > > > > +\n> > > > > > + off_t bufferLength = lseek(fd, 0, SEEK_END);\n> > > > > > + if (bufferLength < 0) {\n> > > > > > + error_ = -errno;\n> > > > > > + LOG(HAL, Error) << \"Failed to get buffer length\";\n> > > > > > + return;\n> > > > > > + }\n> > > > > > +\n> > > > > > + void *address = mmap(nullptr, bufferLength, flags, MAP_SHARED, fd, 0);\n> > > > > > + if (address == MAP_FAILED) {\n> > > > > > + error_ = -errno;\n> > > > > > + LOG(HAL, Error) << \"Failed to mmap plane\";\n> > > > > > + return;\n> > > > > > + }\n> > > > > > + maps_.emplace_back(static_cast(address), bufferLength);\n> > > > > > +\n> > > > > > + const unsigned int numPlanes = info.numPlanes();\n> > > > > > + planes_.resize(numPlanes);\n> > > > > > + unsigned int offset = 0;\n> > > > > > + for (unsigned int i = 0; i < numPlanes; ++i) {\n> > > > > > + /*\n> > > > > > + * \\todo Remove if this plane size computation function is\n> > > > > > + * added to PixelFormatInfo.\n> > > > > > + */\n> > > > > > + const unsigned int vertSubSample = info.planes[i].verticalSubSampling;\n> > > > > > + const unsigned int stride = info.stride(size.width, i, 1u);\n> > > > > > + const unsigned int planeSize =\n> > > > > > + stride * ((size.height + vertSubSample - 1) / vertSubSample);\n> > > > > > +\n> > > > > > + planes_[i] = libcamera::Span(\n> > > > > > + static_cast(address) + offset, planeSize);\n> > > > > > +\n> > > > > > + if (bufferLength < offset + planeSize) {\n> > > > > > + error_ = -EINVAL;\n> > > > > > + LOG(HAL, Error) << \"Plane \" << i << \" is out of buffer\"\n> > > > > > + << \", buffer length=\" << bufferLength\n> > > > > > + << \", offset=\" << offset\n> > > > > > + << \", size=\" << planeSize;\n> > > > > > + return;\n> > > > > > + }\n> > > > > > + offset += planeSize;\n> > > > > > }\n> > > > > > }\n> > > > > >\n> > > > > > @@ -71,15 +127,15 @@ CameraBuffer::Private::~Private()\n> > > > > >\n> > > > > > unsigned int CameraBuffer::Private::numPlanes() const\n> > > > > > {\n> > > > > > - return maps_.size();\n> > > > > > + return planes_.size();\n> > > > > > }\n> > > > > >\n> > > > > > Span CameraBuffer::Private::plane(unsigned int plane)\n> > > > > > {\n> > > > > > - if (plane >= maps_.size())\n> > > > > > + if (plane >= planes_.size())\n> > > > > > return {};\n> > > > > >\n> > > > > > - return maps_[plane];\n> > > > > > + return planes_[plane];\n> > > > > > }\n> > > > > >\n> > > > > > size_t CameraBuffer::Private::jpegBufferSize(size_t maxJpegBufferSize) const\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 8A605BD87D\n\tfor ;\n\tFri, 27 Aug 2021 12:09:54 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B368168932;\n\tFri, 27 Aug 2021 14:09:53 +0200 (CEST)","from mail-ej1-x62b.google.com (mail-ej1-x62b.google.com\n\t[IPv6:2a00:1450:4864:20::62b])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 74AB760256\n\tfor ;\n\tFri, 27 Aug 2021 14:09:52 +0200 (CEST)","by mail-ej1-x62b.google.com with SMTP id u3so13255040ejz.1\n\tfor ;\n\tFri, 27 Aug 2021 05:09:52 -0700 (PDT)","from mail-wr1-f50.google.com (mail-wr1-f50.google.com.\n\t[209.85.221.50]) by smtp.gmail.com with ESMTPSA id\n\tkw10sm2749650ejc.111.2021.08.27.05.09.50\n\tfor \n\t(version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128);\n\tFri, 27 Aug 2021 05:09:50 -0700 (PDT)","by mail-wr1-f50.google.com with SMTP id i6so10096514wrv.2\n\tfor ;\n\tFri, 27 Aug 2021 05:09:50 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"MesEzEr7\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org;\n\ts=google; \n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=SxPRPsZx9PSm94R3mvGnYO8Io2OGR8fO2RDL1I6lTtk=;\n\tb=MesEzEr7+m0329TjWGtn4Ta7LF1FibPkJjKcETULXZnAFSO/LrVMjZ+/cgXjOe63QA\n\tcI37Lamm1uw1lG4rnd5jpUaoeXEty7YNMspy9u3iIdE9UutW4yNDkLW2y6qqeIUjn9Oe\n\tYqhT6Sk4MMKbLwkM3bleDsUW2eqc5jTh5qBn4=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=SxPRPsZx9PSm94R3mvGnYO8Io2OGR8fO2RDL1I6lTtk=;\n\tb=j13YVFt+UrfhwFv35moUR6M64e5BQPkFlJkk74+ExNpgW1OgfElpCeX2XxM0eBa1om\n\tftDF3StaDw7ZYEi5p7lt16nsE2ngLTucCVMZeG3CZUtkUd1jHYmSRMhaVLFqmjvSNm9g\n\t/b+/7CSOsamDZjzYYNUarclv8VWgAJxLrPhktSrTqkV+p2E9M1zg61vs6e+Gnw3HqSfs\n\t8UklfQDwomk53R2qJSy9Ib3ra8W6CYH6kVf7PaFUSex4R31Q7p42gcaDTGojtiRKSUmn\n\tA6cfoLN7Ha85bTDCKOcdp/wk6zZW5aJN7mcFfJFnJ1ARLfYWYYvoSVOs2bQDQrfylRDx\n\t8XZg==","X-Gm-Message-State":"AOAM5301xnp3Aj20YKs21iWquQvv1aUkbov7lPqItFZNoI65sjDu7Wfi\n\tWUyaIOOiU0C+v7BkH11C8X5x3ev5RDoc1A==","X-Google-Smtp-Source":"ABdhPJwe8qEk7fsi0F6T6UO79i02KgJt+fh0CoSsdJpcKo9J/gGck+wpQmm7BRD/djc4Q4h7mEv1OQ==","X-Received":["by 2002:a17:906:4d01:: with SMTP id\n\tr1mr9471662eju.471.1630066191330; \n\tFri, 27 Aug 2021 05:09:51 -0700 (PDT)","by 2002:a5d:538e:: with SMTP id\n\td14mr9935385wrv.192.1630066189470; \n\tFri, 27 Aug 2021 05:09:49 -0700 (PDT)"],"MIME-Version":"1.0","References":"<20210825044410.2787433-1-hiroh@chromium.org>\n\t<20210825044410.2787433-2-hiroh@chromium.org>\n\t<20210825083507.5bodpgew6lsllx77@uno.localdomain>\n\t\n\t\n\t\n\t","In-Reply-To":"","From":"Tomasz Figa ","Date":"Fri, 27 Aug 2021 21:09:37 +0900","X-Gmail-Original-Message-ID":"","Message-ID":"","To":"Laurent Pinchart ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH v2 1/3] android:\n\tgeneric_camera_buffer: Correct buffer mapping","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":19180,"web_url":"https://patchwork.libcamera.org/comment/19180/","msgid":"","date":"2021-08-30T16:18:19","subject":"Re: [libcamera-devel] [PATCH v2 1/3] android:\n\tgeneric_camera_buffer: Correct buffer mapping","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Tomasz,\n\nOn Fri, Aug 27, 2021 at 09:09:37PM +0900, Tomasz Figa wrote:\n> On Fri, Aug 27, 2021 at 9:07 PM Laurent Pinchart wrote:\n> > On Fri, Aug 27, 2021 at 09:01:07PM +0900, Tomasz Figa wrote:\n> > > On Fri, Aug 27, 2021 at 8:36 PM Laurent Pinchart wrote:\n> > > > On Fri, Aug 27, 2021 at 07:08:30PM +0900, Tomasz Figa wrote:\n> > > > > On Wed, Aug 25, 2021 at 5:34 PM Jacopo Mondi wrote:\n> > > > > > On Wed, Aug 25, 2021 at 01:44:08PM +0900, Hirokazu Honda wrote:\n> > > > > > > buffer_hanlde_t doesn't provide sufficient info to map a buffer\n> > > > > > > properly. cros::CameraBufferManager enables handling the buffer on\n> > > > > > > ChromeOS, but no way is provided for other platforms.\n> > > > > > >\n> > > > > > > Therefore, we put the assumption that planes are in the same buffer\n> > > > > > > and they are consecutive. This modifies the way of mapping in\n> > > > > > > generic_camera_buffer with the assumption.\n> > > > > > >\n> > > > > > > Signed-off-by: Hirokazu Honda \n> > > > > > > ---\n> > > > > > > src/android/camera_buffer.h | 14 ++-\n> > > > > > > src/android/camera_stream.cpp | 4 +-\n> > > > > > > src/android/mm/cros_camera_buffer.cpp | 7 +-\n> > > > > > > src/android/mm/generic_camera_buffer.cpp | 104 +++++++++++++++++------\n> > > > > > > 4 files changed, 99 insertions(+), 30 deletions(-)\n> > > > > > >\n> > > > > > > diff --git a/src/android/camera_buffer.h b/src/android/camera_buffer.h\n> > > > > > > index c4e3a9e7..87df2570 100644\n> > > > > > > --- a/src/android/camera_buffer.h\n> > > > > > > +++ b/src/android/camera_buffer.h\n> > > > > > > @@ -11,13 +11,17 @@\n> > > > > > >\n> > > > > > > #include \n> > > > > > > #include \n> > > > > > > +#include \n> > > > > > > +#include \n> > > > > > >\n> > > > > > > class CameraBuffer final : public libcamera::Extensible\n> > > > > > > {\n> > > > > > > LIBCAMERA_DECLARE_PRIVATE()\n> > > > > > >\n> > > > > > > public:\n> > > > > > > - CameraBuffer(buffer_handle_t camera3Buffer, int flags);\n> > > > > > > + CameraBuffer(buffer_handle_t camera3Buffer,\n> > > > > > > + libcamera::PixelFormat pixelFormat,\n> > > > > > > + const libcamera::Size &size, int flags);\n> > > > > > > ~CameraBuffer();\n> > > > > > >\n> > > > > > > bool isValid() const;\n> > > > > > > @@ -31,8 +35,12 @@ public:\n> > > > > > > };\n> > > > > > >\n> > > > > > > #define PUBLIC_CAMERA_BUFFER_IMPLEMENTATION \\\n> > > > > > > -CameraBuffer::CameraBuffer(buffer_handle_t camera3Buffer, int flags) \\\n> > > > > > > - : Extensible(std::make_unique(this, camera3Buffer, flags)) \\\n> > > > > > > +CameraBuffer::CameraBuffer(buffer_handle_t camera3Buffer, \\\n> > > > > > > + libcamera::PixelFormat pixelFormat, \\\n> > > > > > > + const libcamera::Size &size, int flags) \\\n> > > > > > > + : Extensible(std::make_unique(this, camera3Buffer, \\\n> > > > > > > + pixelFormat, size, \\\n> > > > > > > + flags)) \\\n> > > > > > > { \\\n> > > > > > > } \\\n> > > > > > > CameraBuffer::~CameraBuffer() \\\n> > > > > > > diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp\n> > > > > > > index 61b44183..01909ec7 100644\n> > > > > > > --- a/src/android/camera_stream.cpp\n> > > > > > > +++ b/src/android/camera_stream.cpp\n> > > > > > > @@ -110,7 +110,9 @@ int CameraStream::process(const libcamera::FrameBuffer &source,\n> > > > > > > * \\todo Buffer mapping and processing should be moved to a\n> > > > > > > * separate thread.\n> > > > > > > */\n> > > > > > > - CameraBuffer dest(camera3Dest, PROT_READ | PROT_WRITE);\n> > > > > > > + const StreamConfiguration &output = configuration();\n> > > > > > > + CameraBuffer dest(camera3Dest, formats::MJPEG, output.size,\n> > > > > > > + PROT_READ | PROT_WRITE);\n> > > > > > > if (!dest.isValid()) {\n> > > > > > > LOG(HAL, Error) << \"Failed to map android blob buffer\";\n> > > > > > > return -EINVAL;\n> > > > > > > diff --git a/src/android/mm/cros_camera_buffer.cpp b/src/android/mm/cros_camera_buffer.cpp\n> > > > > > > index e8783ff8..50732637 100644\n> > > > > > > --- a/src/android/mm/cros_camera_buffer.cpp\n> > > > > > > +++ b/src/android/mm/cros_camera_buffer.cpp\n> > > > > > > @@ -20,8 +20,9 @@ class CameraBuffer::Private : public Extensible::Private\n> > > > > > > LIBCAMERA_DECLARE_PUBLIC(CameraBuffer)\n> > > > > > >\n> > > > > > > public:\n> > > > > > > - Private(CameraBuffer *cameraBuffer,\n> > > > > > > - buffer_handle_t camera3Buffer, int flags);\n> > > > > > > + Private(CameraBuffer *cameraBuffer, buffer_handle_t camera3Buffer,\n> > > > > > > + libcamera::PixelFormat pixelFormat, const libcamera::Size &size,\n> > > > > > > + int flags);\n> > > > > > > ~Private();\n> > > > > > >\n> > > > > > > bool isValid() const { return valid_; }\n> > > > > > > @@ -46,6 +47,8 @@ private:\n> > > > > > >\n> > > > > > > CameraBuffer::Private::Private([[maybe_unused]] CameraBuffer *cameraBuffer,\n> > > > > > > buffer_handle_t camera3Buffer,\n> > > > > > > + [[maybe_unused]] libcamera::PixelFormat pixelFormat,\n> > > > > > > + [[maybe_unused]] const libcamera::Size &size,\n> > > > > > > [[maybe_unused]] int flags)\n> > > > > > > : handle_(camera3Buffer), numPlanes_(0), valid_(false),\n> > > > > > > registered_(false)\n> > > > > > > diff --git a/src/android/mm/generic_camera_buffer.cpp b/src/android/mm/generic_camera_buffer.cpp\n> > > > > > > index b3af194c..0bfdc2ba 100644\n> > > > > > > --- a/src/android/mm/generic_camera_buffer.cpp\n> > > > > > > +++ b/src/android/mm/generic_camera_buffer.cpp\n> > > > > > > @@ -12,6 +12,7 @@\n> > > > > > >\n> > > > > > > #include \n> > > > > > >\n> > > > > > > +#include \"libcamera/internal/formats.h\"\n> > > > > > > #include \"libcamera/internal/mapped_framebuffer.h\"\n> > > > > > >\n> > > > > > > using namespace libcamera;\n> > > > > > > @@ -24,8 +25,9 @@ class CameraBuffer::Private : public Extensible::Private,\n> > > > > > > LIBCAMERA_DECLARE_PUBLIC(CameraBuffer)\n> > > > > > >\n> > > > > > > public:\n> > > > > > > - Private(CameraBuffer *cameraBuffer,\n> > > > > > > - buffer_handle_t camera3Buffer, int flags);\n> > > > > > > + Private(CameraBuffer *cameraBuffer, buffer_handle_t camera3Buffer,\n> > > > > > > + libcamera::PixelFormat pixelFormat, const libcamera::Size &size,\n> > > > > > > + int flags);\n> > > > > > > ~Private();\n> > > > > > >\n> > > > > > > unsigned int numPlanes() const;\n> > > > > > > @@ -33,35 +35,89 @@ public:\n> > > > > > > Span plane(unsigned int plane);\n> > > > > > >\n> > > > > > > size_t jpegBufferSize(size_t maxJpegBufferSize) const;\n> > > > > > > +\n> > > > > > > +private:\n> > > > > > > + /* \\todo Remove planes_ when it will be added to MappedBuffer */\n> > > > > > > + std::vector> planes_;\n> > > > > > > };\n> > > > > > >\n> > > > > > > CameraBuffer::Private::Private([[maybe_unused]] CameraBuffer *cameraBuffer,\n> > > > > > > - buffer_handle_t camera3Buffer, int flags)\n> > > > > > > + buffer_handle_t camera3Buffer,\n> > > > > > > + libcamera::PixelFormat pixelFormat,\n> > > > > > > + const libcamera::Size &size, int flags)\n> > > > > > > {\n> > > > > > > - maps_.reserve(camera3Buffer->numFds);\n> > > > > > > error_ = 0;\n> > > > > > > -\n> > > > > > > + /*\n> > > > > > > + * As Android doesn't offer an API to query buffer layouts, assume for\n> > > > > > > + * now that the buffer is backed by a single dmabuf, with planes being\n> > > > > > > + * stored contiguously.\n> > > > > > > + */\n> > > > > > > + int fd = -1;\n> > > > > > > for (int i = 0; i < camera3Buffer->numFds; i++) {\n> > > > > >\n> > > > > > unsigned int i\n> > > > > >\n> > > > > > > - if (camera3Buffer->data[i] == -1)\n> > > > > > > + if (camera3Buffer->data[i] == -1 ||\n> > > > > > > + camera3Buffer->data[i] == fd) {\n> > > > > > > continue;\n> > > > > > > -\n> > > > > > > - off_t length = lseek(camera3Buffer->data[i], 0, SEEK_END);\n> > > > > > > - if (length < 0) {\n> > > > > > > - error_ = -errno;\n> > > > > > > - LOG(HAL, Error) << \"Failed to query plane length\";\n> > > > > > > - break;\n> > > > > > > }\n> > > > > > >\n> > > > > > > - void *address = mmap(nullptr, length, flags, MAP_SHARED,\n> > > > > > > - camera3Buffer->data[i], 0);\n> > > > > > > - if (address == MAP_FAILED) {\n> > > > > > > - error_ = -errno;\n> > > > > > > - LOG(HAL, Error) << \"Failed to mmap plane\";\n> > > > > > > - break;\n> > > > > > > - }\n> > > > > > > + if (fd != -1)\n> > > > > > > + LOG(HAL, Fatal) << \"Discontiguous planes are not supported\";\n> > > > > >\n> > > > > > Do we just warn ? Or exit with error ? See the below question\n> > > > > >\n> > > > > > > + fd = camera3Buffer->data[i];\n> > > > > >\n> > > > > > This assigns fd to the 'next' valid data[i].fd. I might have missed\n> > > > > > what is the policy here if we have more than one valid dmabuf fds.\n> > > > > >\n> > > > > > Currently you warn and use the last valid one. I would instead error out,\n> > > > > > or warn but use the first available one (even if that would result in\n> > > > > > undefined behaviour, or most probably an access to non valid memory when\n> > > > > > creating planes below).\n> > > > > >\n> > > > > > Do you have a use case for formats with planes allocated in different\n> > > > > > buffers that you need to support ? Or can we error out without causing\n> > > > > > regressions ?\n> > > > > >\n> > > > > > > + }\n> > > > > >\n> > > > > > The loop is pretty convoluted. If I'm not mistaken what you want to\n> > > > > > verify is that\n> > > > > > 1) Either all data[i].fd are the same\n> > > > >\n> > > > > This is actually not something that would happen in practice, because\n> > > > > the handles are passed via Binder and even if the source puts the same\n> > > > > FD for all the planes, the receiver would see all different FDs,\n> > > > > because Binder would create a new FD for each array element.\n> > > >\n> > > > Sounds like there's room for improvement in binder then. Nevertheless,\n> > > > we certainly want to support the case where the fds will be different\n> > > > but all refer to the same dmabuf. The main blocker for this is the\n> > > > ability to actually test it on Android, which I'm working on.\n> > >\n> > > Do we really need that strict validation? I think we could just take\n> > > the first DMA-buf if the offsets match the non-M format layout. There\n> > > is no practical case when libcamera would get more than 1 DMA-buf to\n> > > be used with hardware that only supports non-M formats.\n> >\n> > It's an easy check to implement, so I'd rather do it now that my memory\n> > is fresh than running into a problem in a year and spend days debugging\n> > it :-)\n> \n> Is the check cheap and compatible with older kernel versions?\n\nAs far as I can tell, yes, but I'll double-check of course.\n\n> Generally I don't think it would ever be possible to run into issues\n> with this in real life, but agreed that if the check is simple it's\n> better to have it.\n> \n> > > > > > 2) All but the data[0].fd are -1\n> > > > > >\n> > > > > > Wouldn't it be nicer as\n> > > > > >\n> > > > > > int fd = camera3Buffer->data[0].fd;\n> > > > > > for (unsigned int i = 1; i < camera3Buffer->numFds; ++i) {\n> > > > > > if (camera3Buffer->data[i].fd != -1)\n> > > > > > continue;\n> > > > > >\n> > > > > > if (camera3Buffer->data[i].fd != fd)\n> > > > > > /* Here I would error out */\n> > > > > > }\n> > > > > > if (fd == -1) {\n> > > > > > /* No valid fds, error out */\n> > > > > > }\n> > > > > >\n> > > > > > > +\n> > > > > > > + if (fd != -1) {\n> > > > > >\n> > > > > > Shouldn't this be fd == -1 ? As in \"no valid fd found ?\"\n> > > > > > If I'm not mistaken checking for fd != -1 should fail all the times as\n> > > > > > at least one valid fd should be found. On which platform have you\n> > > > > > tested this patch ?\n> > > > > >\n> > > > > > > + error_ = -EINVAL;\n> > > > > > > + LOG(HAL, Error) << \"No valid file descriptor\";\n> > > > > > > + return;\n> > > > > > > + }\n> > > > > > >\n> > > > > > > - maps_.emplace_back(static_cast(address),\n> > > > > > > - static_cast(length));\n> > > > > > > + const auto &info = libcamera::PixelFormatInfo::info(pixelFormat);\n> > > > > > > + if (!info.isValid()) {\n> > > > > > > + error_ = -EINVAL;\n> > > > > > > + LOG(HAL, Error) << \"Invalid pixel format: \"\n> > > > > > > + << pixelFormat.toString();\n> > > > > > > + return;\n> > > > > > > + }\n> > > > > >\n> > > > > > We could check this at the very beginning of the function and avoid\n> > > > > > looping on fds if the format is invalid.\n> > > > > >\n> > > > > > > +\n> > > > > > > + off_t bufferLength = lseek(fd, 0, SEEK_END);\n> > > > > > > + if (bufferLength < 0) {\n> > > > > > > + error_ = -errno;\n> > > > > > > + LOG(HAL, Error) << \"Failed to get buffer length\";\n> > > > > > > + return;\n> > > > > > > + }\n> > > > > > > +\n> > > > > > > + void *address = mmap(nullptr, bufferLength, flags, MAP_SHARED, fd, 0);\n> > > > > > > + if (address == MAP_FAILED) {\n> > > > > > > + error_ = -errno;\n> > > > > > > + LOG(HAL, Error) << \"Failed to mmap plane\";\n> > > > > > > + return;\n> > > > > > > + }\n> > > > > > > + maps_.emplace_back(static_cast(address), bufferLength);\n> > > > > > > +\n> > > > > > > + const unsigned int numPlanes = info.numPlanes();\n> > > > > > > + planes_.resize(numPlanes);\n> > > > > > > + unsigned int offset = 0;\n> > > > > > > + for (unsigned int i = 0; i < numPlanes; ++i) {\n> > > > > > > + /*\n> > > > > > > + * \\todo Remove if this plane size computation function is\n> > > > > > > + * added to PixelFormatInfo.\n> > > > > > > + */\n> > > > > > > + const unsigned int vertSubSample = info.planes[i].verticalSubSampling;\n> > > > > > > + const unsigned int stride = info.stride(size.width, i, 1u);\n> > > > > > > + const unsigned int planeSize =\n> > > > > > > + stride * ((size.height + vertSubSample - 1) / vertSubSample);\n> > > > > > > +\n> > > > > > > + planes_[i] = libcamera::Span(\n> > > > > > > + static_cast(address) + offset, planeSize);\n> > > > > > > +\n> > > > > > > + if (bufferLength < offset + planeSize) {\n> > > > > > > + error_ = -EINVAL;\n> > > > > > > + LOG(HAL, Error) << \"Plane \" << i << \" is out of buffer\"\n> > > > > > > + << \", buffer length=\" << bufferLength\n> > > > > > > + << \", offset=\" << offset\n> > > > > > > + << \", size=\" << planeSize;\n> > > > > > > + return;\n> > > > > > > + }\n> > > > > > > + offset += planeSize;\n> > > > > > > }\n> > > > > > > }\n> > > > > > >\n> > > > > > > @@ -71,15 +127,15 @@ CameraBuffer::Private::~Private()\n> > > > > > >\n> > > > > > > unsigned int CameraBuffer::Private::numPlanes() const\n> > > > > > > {\n> > > > > > > - return maps_.size();\n> > > > > > > + return planes_.size();\n> > > > > > > }\n> > > > > > >\n> > > > > > > Span CameraBuffer::Private::plane(unsigned int plane)\n> > > > > > > {\n> > > > > > > - if (plane >= maps_.size())\n> > > > > > > + if (plane >= planes_.size())\n> > > > > > > return {};\n> > > > > > >\n> > > > > > > - return maps_[plane];\n> > > > > > > + return planes_[plane];\n> > > > > > > }\n> > > > > > >\n> > > > > > > size_t CameraBuffer::Private::jpegBufferSize(size_t maxJpegBufferSize) const","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 0BDE7BDC71\n\tfor ;\n\tMon, 30 Aug 2021 16:18:36 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6C30069166;\n\tMon, 30 Aug 2021 18:18:35 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 180C460258\n\tfor ;\n\tMon, 30 Aug 2021 18:18:34 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 8C1145A7;\n\tMon, 30 Aug 2021 18:18:33 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"B15OyvZS\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1630340313;\n\tbh=XfCqGa+zDg051CKH4XyeltQa2ReFRvnoIyfPWO8Gsrk=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=B15OyvZSIzGbwFIXfu4KEqTkikMkJFin7fQpHXNaVorCsewKbsnldNuoRxMHdDUvQ\n\tJGoNMif2YxSwKTU0orJFn63MSLluVT9WDwgcBJl2dc/ahI5Vn4hXtyV4yEhB8djsTy\n\toDiQcYTPtCWVCb75DjrI+tJ17B7SJwyLrNRZs/Jc=","Date":"Mon, 30 Aug 2021 19:18:19 +0300","From":"Laurent Pinchart ","To":"Tomasz Figa ","Message-ID":"","References":"<20210825044410.2787433-1-hiroh@chromium.org>\n\t<20210825044410.2787433-2-hiroh@chromium.org>\n\t<20210825083507.5bodpgew6lsllx77@uno.localdomain>\n\t\n\t\n\t\n\t\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v2 1/3] android:\n\tgeneric_camera_buffer: Correct buffer mapping","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]