[{"id":18889,"web_url":"https://patchwork.libcamera.org/comment/18889/","msgid":"","date":"2021-08-18T01:51:54","subject":"Re: [libcamera-devel] [RFC PATCH 09/10] android: camera_device:\n\tFill offset and right length in CreateFrameBuffer()","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Hiro,\n\nThank you for the patch.\n\nOn Mon, Aug 16, 2021 at 01:31:37PM +0900, Hirokazu Honda wrote:\n> CameraDevice::CreateFrameBuffer() fills the length of the buffer to\n> each FrameBuffer::Plane::length. It should rather be the length of\n> plane. This also changes CreateFrameBuffer() to fill offset of\n> FrameBuffer::Plane.\n> \n> Signed-off-by: Hirokazu Honda \n> ---\n> src/android/camera_device.cpp | 38 ++++++++++++++++++-----------------\n> 1 file changed, 20 insertions(+), 18 deletions(-)\n> \n> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> index a69b687a..1ded5cb1 100644\n> --- a/src/android/camera_device.cpp\n> +++ b/src/android/camera_device.cpp\n> @@ -12,6 +12,7 @@\n> \n> #include \n> #include \n> +#include \n> #include \n> #include \n> \n> @@ -746,29 +747,30 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)\n> \n> FrameBuffer *CameraDevice::createFrameBuffer(const buffer_handle_t camera3buffer)\n> {\n> -\tstd::vector planes;\n> +\tFileDescriptor fd;\n> +\t/* This assumes all the planes are in the same buffer. */\n\nShould we ASSERT() if that's not the case ? Also, does this assumption\nalways hold ?\n\n> \tfor (int i = 0; i < camera3buffer->numFds; i++) {\n> -\t\t/* Skip unused planes. */\n> -\t\tif (camera3buffer->data[i] == -1)\n> +\t\tif (camera3buffer->data[i] != -1) {\n> +\t\t\tfd = FileDescriptor(camera3buffer->data[i]);\n> \t\t\tbreak;\n> -\n> -\t\tFrameBuffer::Plane plane;\n> -\t\tplane.fd = FileDescriptor(camera3buffer->data[i]);\n> -\t\tif (!plane.fd.isValid()) {\n> -\t\t\tLOG(HAL, Error) << \"Failed to obtain FileDescriptor (\"\n> -\t\t\t\t\t<< camera3buffer->data[i] << \") \"\n> -\t\t\t\t\t<< \" on plane \" << i;\n> -\t\t\treturn nullptr;\n> \t\t}\n> +\t}\n\nBlank line.\n\n> +\tif (!fd.isValid()) {\n> +\t\tLOG(HAL, Fatal) << \"No valid fd\";\n> +\t\treturn nullptr;\n> +\t}\n> \n> -\t\toff_t length = lseek(plane.fd.fd(), 0, SEEK_END);\n> -\t\tif (length == -1) {\n> -\t\t\tLOG(HAL, Error) << \"Failed to query plane length\";\n> -\t\t\treturn nullptr;\n> -\t\t}\n> +\tCameraBuffer buf(camera3buffer, PROT_READ);\n\nI suppose there's no other way than using the cros::CameraBufferManager\nto get the necessary information. I'm annoyed that this involves mapping\nthe buffer to the CPU just to retrieve sizes, as that's an expensive\noperation :-( It's of course not a prerequisite for this series, but I'm\nwondering if the cros::CameraBufferManager API could be improved to\nseparate retrieving plane information from mapping the buffer. What do\nyou think ?\n\nAlso, should generic_camera_buffer.cpp be updated to provide the correct\ninformation ?\n\n> +\tif (!buf.isValid()) {\n> +\t\tLOG(HAL, Fatal) << \"Failed mapping buffer\";\n> +\t\treturn nullptr;\n> +\t}\n> \n> -\t\tplane.length = length;\n> -\t\tplanes.push_back(std::move(plane));\n> +\tstd::vector planes(buf.numPlanes());\n> +\tfor (size_t i = 0; i < buf.numPlanes(); ++i) {\n> +\t\tplanes[i].fd = fd;\n> +\t\tplanes[i].offset = buf.plane(i).data() - buf.plane(0).data();\n> +\t\tplanes[i].length = buf.plane(i).size();\n> \t}\n> \n> \treturn new FrameBuffer(std::move(planes));","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 1B620BD87C\n\tfor ;\n\tWed, 18 Aug 2021 01:52:04 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7198B68894;\n\tWed, 18 Aug 2021 03:52:03 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 0E5F16025D\n\tfor ;\n\tWed, 18 Aug 2021 03:52:02 +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 88A90466;\n\tWed, 18 Aug 2021 03:52:01 +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=\"XnVhG6/W\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1629251521;\n\tbh=wfhc6GWx32fQALifMEur+PkD5YtTJLrKuLrB07q2wfU=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=XnVhG6/WuV1MGzvEo46G7HNJtY2k76hI/nA2gH5iCBQWU9eqAbCDEoI1WB73LZXiG\n\tuzBowhZdFmCYojR6Ebo96suwGrqTIBcFIDobyiewyWqqOi1250B+TeoHsbAhBxcLEN\n\thcwzRexC+jh6qloD5GgvFqjhJgBIntsY6T8Wk7GM=","Date":"Wed, 18 Aug 2021 04:51:54 +0300","From":"Laurent Pinchart ","To":"Hirokazu Honda ","Message-ID":"","References":"<20210816043138.957984-1-hiroh@chromium.org>\n\t<20210816043138.957984-10-hiroh@chromium.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210816043138.957984-10-hiroh@chromium.org>","Subject":"Re: [libcamera-devel] [RFC PATCH 09/10] android: camera_device:\n\tFill offset and right length in CreateFrameBuffer()","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":18903,"web_url":"https://patchwork.libcamera.org/comment/18903/","msgid":"","date":"2021-08-18T09:30:01","subject":"Re: [libcamera-devel] [RFC PATCH 09/10] android: camera_device:\n\tFill offset and right length in CreateFrameBuffer()","submitter":{"id":9,"url":"https://patchwork.libcamera.org/api/people/9/","name":"Tomasz Figa","email":"tfiga@chromium.org"},"content":"On Wed, Aug 18, 2021 at 10:52 AM Laurent Pinchart\n wrote:\n>\n> Hi Hiro,\n>\n> Thank you for the patch.\n>\n> On Mon, Aug 16, 2021 at 01:31:37PM +0900, Hirokazu Honda wrote:\n> > CameraDevice::CreateFrameBuffer() fills the length of the buffer to\n> > each FrameBuffer::Plane::length. It should rather be the length of\n> > plane. This also changes CreateFrameBuffer() to fill offset of\n> > FrameBuffer::Plane.\n> >\n> > Signed-off-by: Hirokazu Honda \n> > ---\n> > src/android/camera_device.cpp | 38 ++++++++++++++++++-----------------\n> > 1 file changed, 20 insertions(+), 18 deletions(-)\n> >\n> > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > index a69b687a..1ded5cb1 100644\n> > --- a/src/android/camera_device.cpp\n> > +++ b/src/android/camera_device.cpp\n> > @@ -12,6 +12,7 @@\n> >\n> > #include \n> > #include \n> > +#include \n> > #include \n> > #include \n> >\n> > @@ -746,29 +747,30 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)\n> >\n> > FrameBuffer *CameraDevice::createFrameBuffer(const buffer_handle_t camera3buffer)\n> > {\n> > - std::vector planes;\n> > + FileDescriptor fd;\n> > + /* This assumes all the planes are in the same buffer. */\n>\n> Should we ASSERT() if that's not the case ? Also, does this assumption\n> always hold ?\n>\n> > for (int i = 0; i < camera3buffer->numFds; i++) {\n> > - /* Skip unused planes. */\n> > - if (camera3buffer->data[i] == -1)\n> > + if (camera3buffer->data[i] != -1) {\n> > + fd = FileDescriptor(camera3buffer->data[i]);\n> > break;\n> > -\n> > - FrameBuffer::Plane plane;\n> > - plane.fd = FileDescriptor(camera3buffer->data[i]);\n> > - if (!plane.fd.isValid()) {\n> > - LOG(HAL, Error) << \"Failed to obtain FileDescriptor (\"\n> > - << camera3buffer->data[i] << \") \"\n> > - << \" on plane \" << i;\n> > - return nullptr;\n> > }\n> > + }\n>\n> Blank line.\n>\n> > + if (!fd.isValid()) {\n> > + LOG(HAL, Fatal) << \"No valid fd\";\n> > + return nullptr;\n> > + }\n> >\n> > - off_t length = lseek(plane.fd.fd(), 0, SEEK_END);\n> > - if (length == -1) {\n> > - LOG(HAL, Error) << \"Failed to query plane length\";\n> > - return nullptr;\n> > - }\n> > + CameraBuffer buf(camera3buffer, PROT_READ);\n>\n> I suppose there's no other way than using the cros::CameraBufferManager\n> to get the necessary information. I'm annoyed that this involves mapping\n> the buffer to the CPU just to retrieve sizes, as that's an expensive\n> operation :-( It's of course not a prerequisite for this series, but I'm\n> wondering if the cros::CameraBufferManager API could be improved to\n> separate retrieving plane information from mapping the buffer. What do\n> you think ?\n\nWhich mapping are you referring to?\n\nAlso, there are already methods to retrieve this information, see\nGetNumPlanes() and GetPlane*() in the CameraBufferManager class [1].\n\n[1] https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/platform2/camera/include/cros-camera/camera_buffer_manager.h;drc=6cd1330d01ad2fe4122faa0b2708ecf6267eb58e;l=305\n\n>\n> Also, should generic_camera_buffer.cpp be updated to provide the correct\n> information ?\n>\n> > + if (!buf.isValid()) {\n> > + LOG(HAL, Fatal) << \"Failed mapping buffer\";\n> > + return nullptr;\n> > + }\n> >\n> > - plane.length = length;\n> > - planes.push_back(std::move(plane));\n> > + std::vector planes(buf.numPlanes());\n> > + for (size_t i = 0; i < buf.numPlanes(); ++i) {\n> > + planes[i].fd = fd;\n> > + planes[i].offset = buf.plane(i).data() - buf.plane(0).data();\n> > + planes[i].length = buf.plane(i).size();\n> > }\n> >\n> > return new FrameBuffer(std::move(planes));\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 4C61FBD87C\n\tfor ;\n\tWed, 18 Aug 2021 09:30:18 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id BCC9C68895;\n\tWed, 18 Aug 2021 11:30:17 +0200 (CEST)","from mail-ed1-x52e.google.com (mail-ed1-x52e.google.com\n\t[IPv6:2a00:1450:4864:20::52e])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 7561E6025E\n\tfor ;\n\tWed, 18 Aug 2021 11:30:15 +0200 (CEST)","by mail-ed1-x52e.google.com with SMTP id cq23so2085481edb.12\n\tfor ;\n\tWed, 18 Aug 2021 02:30:15 -0700 (PDT)","from mail-wr1-f43.google.com (mail-wr1-f43.google.com.\n\t[209.85.221.43]) by smtp.gmail.com with ESMTPSA id\n\tr16sm1756321ejz.41.2021.08.18.02.30.13\n\tfor \n\t(version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128);\n\tWed, 18 Aug 2021 02:30:14 -0700 (PDT)","by mail-wr1-f43.google.com with SMTP id u16so2450196wrn.5\n\tfor ;\n\tWed, 18 Aug 2021 02:30:13 -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=\"UdyodO2x\"; 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=AS8ipt1ypR2hqn+C4gYImRgvpY0zmT9t8taSfVAPEuM=;\n\tb=UdyodO2xD1EnaV77hPQI04wTX93RCevaFsaWC9CN4fokBcSgjHD7T695xAuqrX43AD\n\tX+zcswk2vXr8QJV1uYN0p5gKPYZzh7oZzH+HR/aniYmdSm1/OobG12/UUPfj2HjKUB3D\n\tmrqeV1Tij0mzT2RkttReA43YBpqZAxgTR8ZRI=","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=AS8ipt1ypR2hqn+C4gYImRgvpY0zmT9t8taSfVAPEuM=;\n\tb=uLrIgtXwmf7FdrPb31UdBYVcm/oyW5Xn9CwafIuTBYWb85zSNOO5KvFtpQb53enaKo\n\tmbW4Zc9tbtz+0X/cBT5F2rxAmlHB5VGIlzuptV46sq50BRPnYuhYnI5p+fSBWZaMzBpq\n\t4gucRFQaBq9mEEzNYaHXN9dxMVRSAqXrYv7UsU9ycVnnreGau3yKAyDEddPuTyVF7a8G\n\tCM3E2d8qQ+ni1I3j2ZanNCPBoTyjzqA36C7FRXnXOSNpqJOoIDmct+c4CZ9FAwzGJepL\n\tEfEiq+jZvUb6wpLk4cJ3rT/p4PLNgIVs1VJnmtSXhEH0pBUFZi+Zu5/aSMsiAqLXzhNK\n\tjzyQ==","X-Gm-Message-State":"AOAM531J76GGo8B4/kl8bv69QBUTKtb3BqSHFR8jU4Ey11IKV+Xq7DZl\n\tmqdFf9nkfNCwRPPzUH+sClarYVfn9O9gcQ==","X-Google-Smtp-Source":"ABdhPJzng/tQMwkhc+fi5oJ1KwIS+n/2Gb2wdMOmMVZIxbp1hiBeXvktawTaArnERsMIFlnzhxjGaQ==","X-Received":["by 2002:a05:6402:1057:: with SMTP id\n\te23mr9316519edu.352.1629279014583; \n\tWed, 18 Aug 2021 02:30:14 -0700 (PDT)","by 2002:a5d:6483:: with SMTP id o3mr9320067wri.197.1629279013270;\n\tWed, 18 Aug 2021 02:30:13 -0700 (PDT)"],"MIME-Version":"1.0","References":"<20210816043138.957984-1-hiroh@chromium.org>\n\t<20210816043138.957984-10-hiroh@chromium.org>\n\t","In-Reply-To":"","From":"Tomasz Figa ","Date":"Wed, 18 Aug 2021 18:30:01 +0900","X-Gmail-Original-Message-ID":"","Message-ID":"","To":"Laurent Pinchart ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [RFC PATCH 09/10] android: camera_device:\n\tFill offset and right length in CreateFrameBuffer()","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":18907,"web_url":"https://patchwork.libcamera.org/comment/18907/","msgid":"","date":"2021-08-18T10:26:36","subject":"Re: [libcamera-devel] [RFC PATCH 09/10] android: camera_device:\n\tFill offset and right length in CreateFrameBuffer()","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Tomasz,\n\nOn Wed, Aug 18, 2021 at 06:30:01PM +0900, Tomasz Figa wrote:\n> On Wed, Aug 18, 2021 at 10:52 AM Laurent Pinchart wrote:\n> > On Mon, Aug 16, 2021 at 01:31:37PM +0900, Hirokazu Honda wrote:\n> > > CameraDevice::CreateFrameBuffer() fills the length of the buffer to\n> > > each FrameBuffer::Plane::length. It should rather be the length of\n> > > plane. This also changes CreateFrameBuffer() to fill offset of\n> > > FrameBuffer::Plane.\n> > >\n> > > Signed-off-by: Hirokazu Honda \n> > > ---\n> > > src/android/camera_device.cpp | 38 ++++++++++++++++++-----------------\n> > > 1 file changed, 20 insertions(+), 18 deletions(-)\n> > >\n> > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > > index a69b687a..1ded5cb1 100644\n> > > --- a/src/android/camera_device.cpp\n> > > +++ b/src/android/camera_device.cpp\n> > > @@ -12,6 +12,7 @@\n> > >\n> > > #include \n> > > #include \n> > > +#include \n> > > #include \n> > > #include \n> > >\n> > > @@ -746,29 +747,30 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)\n> > >\n> > > FrameBuffer *CameraDevice::createFrameBuffer(const buffer_handle_t camera3buffer)\n> > > {\n> > > - std::vector planes;\n> > > + FileDescriptor fd;\n> > > + /* This assumes all the planes are in the same buffer. */\n> >\n> > Should we ASSERT() if that's not the case ? Also, does this assumption\n> > always hold ?\n> >\n> > > for (int i = 0; i < camera3buffer->numFds; i++) {\n> > > - /* Skip unused planes. */\n> > > - if (camera3buffer->data[i] == -1)\n> > > + if (camera3buffer->data[i] != -1) {\n> > > + fd = FileDescriptor(camera3buffer->data[i]);\n> > > break;\n> > > -\n> > > - FrameBuffer::Plane plane;\n> > > - plane.fd = FileDescriptor(camera3buffer->data[i]);\n> > > - if (!plane.fd.isValid()) {\n> > > - LOG(HAL, Error) << \"Failed to obtain FileDescriptor (\"\n> > > - << camera3buffer->data[i] << \") \"\n> > > - << \" on plane \" << i;\n> > > - return nullptr;\n> > > }\n> > > + }\n> >\n> > Blank line.\n> >\n> > > + if (!fd.isValid()) {\n> > > + LOG(HAL, Fatal) << \"No valid fd\";\n> > > + return nullptr;\n> > > + }\n> > >\n> > > - off_t length = lseek(plane.fd.fd(), 0, SEEK_END);\n> > > - if (length == -1) {\n> > > - LOG(HAL, Error) << \"Failed to query plane length\";\n> > > - return nullptr;\n> > > - }\n> > > + CameraBuffer buf(camera3buffer, PROT_READ);\n> >\n> > I suppose there's no other way than using the cros::CameraBufferManager\n> > to get the necessary information. I'm annoyed that this involves mapping\n> > the buffer to the CPU just to retrieve sizes, as that's an expensive\n> > operation :-( It's of course not a prerequisite for this series, but I'm\n> > wondering if the cros::CameraBufferManager API could be improved to\n> > separate retrieving plane information from mapping the buffer. What do\n> > you think ?\n> \n> Which mapping are you referring to?\n> \n> Also, there are already methods to retrieve this information, see\n> GetNumPlanes() and GetPlane*() in the CameraBufferManager class [1].\n\nYou're right. We currently call Lock or LockYCbCr in the CameraBuffer\nconstructor, but that's not required to get the plane size or offset. We\nshould thus be able to improve the implementation here, without any\nchange on the CrOS side. Thanks for the feedback.\n\n> [1] https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/platform2/camera/include/cros-camera/camera_buffer_manager.h;drc=6cd1330d01ad2fe4122faa0b2708ecf6267eb58e;l=305\n> \n> > Also, should generic_camera_buffer.cpp be updated to provide the correct\n> > information ?\n> >\n> > > + if (!buf.isValid()) {\n> > > + LOG(HAL, Fatal) << \"Failed mapping buffer\";\n> > > + return nullptr;\n> > > + }\n> > >\n> > > - plane.length = length;\n> > > - planes.push_back(std::move(plane));\n> > > + std::vector planes(buf.numPlanes());\n> > > + for (size_t i = 0; i < buf.numPlanes(); ++i) {\n> > > + planes[i].fd = fd;\n> > > + planes[i].offset = buf.plane(i).data() - buf.plane(0).data();\n> > > + planes[i].length = buf.plane(i).size();\n> > > }\n> > >\n> > > return new FrameBuffer(std::move(planes));","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 4AE64BD87C\n\tfor ;\n\tWed, 18 Aug 2021 10:26:46 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C655068895;\n\tWed, 18 Aug 2021 12:26:45 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D517C6888A\n\tfor ;\n\tWed, 18 Aug 2021 12:26:43 +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 4612D466;\n\tWed, 18 Aug 2021 12:26: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=\"boGL872m\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1629282403;\n\tbh=0mmbeYlu7Gg+KWOLlXjOldXL6s/vhr2/7AbLTE0AroA=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=boGL872m9ijr4qJHzyRAXtVX/TMvcxX8jHsYeSc5K8P7LkrVKPQLjme/14w6+0EA5\n\tOA0EtWH/rNxSHv4KensoHEYMaYjuszRY2kGvYmHkIB/4GqUMBTcZus4Iuh/0S0I9Wq\n\tWtq4CFL0HbiHsGphUqgQtQ2CBAa5fzYLomJNJTGk=","Date":"Wed, 18 Aug 2021 13:26:36 +0300","From":"Laurent Pinchart ","To":"Tomasz Figa ","Message-ID":"","References":"<20210816043138.957984-1-hiroh@chromium.org>\n\t<20210816043138.957984-10-hiroh@chromium.org>\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] [RFC PATCH 09/10] android: camera_device:\n\tFill offset and right length in CreateFrameBuffer()","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":18971,"web_url":"https://patchwork.libcamera.org/comment/18971/","msgid":"","date":"2021-08-20T11:13:28","subject":"Re: [libcamera-devel] [RFC PATCH 09/10] android: camera_device:\n\tFill offset and right length in CreateFrameBuffer()","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"Hi Laurent, thank you for reviewing.\n\nOn Wed, Aug 18, 2021 at 7:26 PM Laurent Pinchart\n wrote:\n>\n> Hi Tomasz,\n>\n> On Wed, Aug 18, 2021 at 06:30:01PM +0900, Tomasz Figa wrote:\n> > On Wed, Aug 18, 2021 at 10:52 AM Laurent Pinchart wrote:\n> > > On Mon, Aug 16, 2021 at 01:31:37PM +0900, Hirokazu Honda wrote:\n> > > > CameraDevice::CreateFrameBuffer() fills the length of the buffer to\n> > > > each FrameBuffer::Plane::length. It should rather be the length of\n> > > > plane. This also changes CreateFrameBuffer() to fill offset of\n> > > > FrameBuffer::Plane.\n> > > >\n> > > > Signed-off-by: Hirokazu Honda \n> > > > ---\n> > > > src/android/camera_device.cpp | 38 ++++++++++++++++++-----------------\n> > > > 1 file changed, 20 insertions(+), 18 deletions(-)\n> > > >\n> > > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > > > index a69b687a..1ded5cb1 100644\n> > > > --- a/src/android/camera_device.cpp\n> > > > +++ b/src/android/camera_device.cpp\n> > > > @@ -12,6 +12,7 @@\n> > > >\n> > > > #include \n> > > > #include \n> > > > +#include \n> > > > #include \n> > > > #include \n> > > >\n> > > > @@ -746,29 +747,30 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)\n> > > >\n> > > > FrameBuffer *CameraDevice::createFrameBuffer(const buffer_handle_t camera3buffer)\n> > > > {\n> > > > - std::vector planes;\n> > > > + FileDescriptor fd;\n> > > > + /* This assumes all the planes are in the same buffer. */\n> > >\n> > > Should we ASSERT() if that's not the case ? Also, does this assumption\n> > > always hold ?\n> > >\n\nI think this assumption holds at least on ChromeOS.\nHmm, ASSERT() is difficult because fds are duplicated and thus\ndifferent from each other in terms of integer.\n\n> > > > for (int i = 0; i < camera3buffer->numFds; i++) {\n> > > > - /* Skip unused planes. */\n> > > > - if (camera3buffer->data[i] == -1)\n> > > > + if (camera3buffer->data[i] != -1) {\n> > > > + fd = FileDescriptor(camera3buffer->data[i]);\n> > > > break;\n> > > > -\n> > > > - FrameBuffer::Plane plane;\n> > > > - plane.fd = FileDescriptor(camera3buffer->data[i]);\n> > > > - if (!plane.fd.isValid()) {\n> > > > - LOG(HAL, Error) << \"Failed to obtain FileDescriptor (\"\n> > > > - << camera3buffer->data[i] << \") \"\n> > > > - << \" on plane \" << i;\n> > > > - return nullptr;\n> > > > }\n> > > > + }\n> > >\n> > > Blank line.\n> > >\n> > > > + if (!fd.isValid()) {\n> > > > + LOG(HAL, Fatal) << \"No valid fd\";\n> > > > + return nullptr;\n> > > > + }\n> > > >\n> > > > - off_t length = lseek(plane.fd.fd(), 0, SEEK_END);\n> > > > - if (length == -1) {\n> > > > - LOG(HAL, Error) << \"Failed to query plane length\";\n> > > > - return nullptr;\n> > > > - }\n> > > > + CameraBuffer buf(camera3buffer, PROT_READ);\n> > >\n> > > I suppose there's no other way than using the cros::CameraBufferManager\n> > > to get the necessary information. I'm annoyed that this involves mapping\n> > > the buffer to the CPU just to retrieve sizes, as that's an expensive\n> > > operation :-( It's of course not a prerequisite for this series, but I'm\n> > > wondering if the cros::CameraBufferManager API could be improved to\n> > > separate retrieving plane information from mapping the buffer. What do\n> > > you think ?\n> >\n> > Which mapping are you referring to?\n> >\n> > Also, there are already methods to retrieve this information, see\n> > GetNumPlanes() and GetPlane*() in the CameraBufferManager class [1].\n>\n> You're right. We currently call Lock or LockYCbCr in the CameraBuffer\n> constructor, but that's not required to get the plane size or offset. We\n> should thus be able to improve the implementation here, without any\n> change on the CrOS side. Thanks for the feedback.\n>\n\nI actually tried to add the option to not map in the constructor\nhttps://patchwork.libcamera.org/patch/11837/.\nWould you tell me your preferred way?\n\nBest Regards,\n-Hiro\n> > [1] https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/platform2/camera/include/cros-camera/camera_buffer_manager.h;drc=6cd1330d01ad2fe4122faa0b2708ecf6267eb58e;l=305\n> >\n> > > Also, should generic_camera_buffer.cpp be updated to provide the correct\n> > > information ?\n> > >\n> > > > + if (!buf.isValid()) {\n> > > > + LOG(HAL, Fatal) << \"Failed mapping buffer\";\n> > > > + return nullptr;\n> > > > + }\n> > > >\n> > > > - plane.length = length;\n> > > > - planes.push_back(std::move(plane));\n> > > > + std::vector planes(buf.numPlanes());\n> > > > + for (size_t i = 0; i < buf.numPlanes(); ++i) {\n> > > > + planes[i].fd = fd;\n> > > > + planes[i].offset = buf.plane(i).data() - buf.plane(0).data();\n> > > > + planes[i].length = buf.plane(i).size();\n> > > > }\n> > > >\n> > > > return new FrameBuffer(std::move(planes));\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 20DF4BD87C\n\tfor ;\n\tFri, 20 Aug 2021 11:13:43 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 861A268895;\n\tFri, 20 Aug 2021 13:13:42 +0200 (CEST)","from mail-ed1-x535.google.com (mail-ed1-x535.google.com\n\t[IPv6:2a00:1450:4864:20::535])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C0B996025E\n\tfor ;\n\tFri, 20 Aug 2021 13:13:40 +0200 (CEST)","by mail-ed1-x535.google.com with SMTP id v2so13446682edq.10\n\tfor ;\n\tFri, 20 Aug 2021 04:13:40 -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=\"LLui5/Wt\"; 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=ISNnJZGn5c09AZJfdZUt4C4SVnvgnox4PN6d/W5XjGA=;\n\tb=LLui5/WtHY8vlegIaCqwTKLu9DiBoEYmXeEAfINeaAyn1Gq4KugDb6+IwwuS2o0hDF\n\tvYo4illjLZVz9PDWrCzpKRzz43LBrnjHQw3CrGt7nofGyUMXHXVQDLRbdHyWawGK8yy8\n\tFfjWppHaz7xc9YSp0FaZLZoAJ43tsOvlSODYM=","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=ISNnJZGn5c09AZJfdZUt4C4SVnvgnox4PN6d/W5XjGA=;\n\tb=mGdxry3PenMIYTXjOzgY8HOYxhbtLapky2bbpxyFWshRnv+oBomp1yFIHqEbyB8oI0\n\ttq5yFVd8N/q02TOcbI/+aY7ksrL54EbY3xa175FTiDkKxpiS9wR8c6TwPzN4bQVyrlPS\n\tHD5dhpAxonwqiR7B9r58srlSGK6cwMuVV2Vb5gD6odlrGcdEkOMtNodsDt7ME57FLVrR\n\tuxUOLkFtdNINbP9oa0ONzlNAjSbfqVsVUJqZEOUqvk3ETVhsBsC6l14KDf3M7AtoSZQq\n\tk+7G/NeZkFOP9Bes5J0V7ZCUrddC8Hi73ZK3ZV3rQkdhqd6bDwFEQiaKBxHLgwqF0rVS\n\tOYxA==","X-Gm-Message-State":"AOAM532cWSdwrCjhF5SREdGPGwBeqHTozBOALznF4CudJmFfZnz3h1uQ\n\t5Z5KUNiOpFcjVUuqlb5+W2ZSrVDGz3b1xAlVIGlfoA==","X-Google-Smtp-Source":"ABdhPJx4QS/bu/YZl8/pOfhtUfPcpyQykTxx1WsYhamBeU3dumdvlZg4y27tM09C10AUM2rfpb1wqueRU78QKV0WycI=","X-Received":"by 2002:aa7:cd92:: with SMTP id\n\tx18mr21415946edv.325.1629458020441; \n\tFri, 20 Aug 2021 04:13:40 -0700 (PDT)","MIME-Version":"1.0","References":"<20210816043138.957984-1-hiroh@chromium.org>\n\t<20210816043138.957984-10-hiroh@chromium.org>\n\t\n\t\n\t","In-Reply-To":"","From":"Hirokazu Honda ","Date":"Fri, 20 Aug 2021 20:13:28 +0900","Message-ID":"","To":"Laurent Pinchart ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [RFC PATCH 09/10] android: camera_device:\n\tFill offset and right length in CreateFrameBuffer()","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":18976,"web_url":"https://patchwork.libcamera.org/comment/18976/","msgid":"","date":"2021-08-20T11:26:01","subject":"Re: [libcamera-devel] [RFC PATCH 09/10] android: camera_device:\n\tFill offset and right length in CreateFrameBuffer()","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Hiro,\n\nOn Fri, Aug 20, 2021 at 08:13:28PM +0900, Hirokazu Honda wrote:\n> On Wed, Aug 18, 2021 at 7:26 PM Laurent Pinchart wrote:\n> > On Wed, Aug 18, 2021 at 06:30:01PM +0900, Tomasz Figa wrote:\n> > > On Wed, Aug 18, 2021 at 10:52 AM Laurent Pinchart wrote:\n> > > > On Mon, Aug 16, 2021 at 01:31:37PM +0900, Hirokazu Honda wrote:\n> > > > > CameraDevice::CreateFrameBuffer() fills the length of the buffer to\n> > > > > each FrameBuffer::Plane::length. It should rather be the length of\n> > > > > plane. This also changes CreateFrameBuffer() to fill offset of\n> > > > > FrameBuffer::Plane.\n> > > > >\n> > > > > Signed-off-by: Hirokazu Honda \n> > > > > ---\n> > > > > src/android/camera_device.cpp | 38 ++++++++++++++++++-----------------\n> > > > > 1 file changed, 20 insertions(+), 18 deletions(-)\n> > > > >\n> > > > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > > > > index a69b687a..1ded5cb1 100644\n> > > > > --- a/src/android/camera_device.cpp\n> > > > > +++ b/src/android/camera_device.cpp\n> > > > > @@ -12,6 +12,7 @@\n> > > > >\n> > > > > #include \n> > > > > #include \n> > > > > +#include \n> > > > > #include \n> > > > > #include \n> > > > >\n> > > > > @@ -746,29 +747,30 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)\n> > > > >\n> > > > > FrameBuffer *CameraDevice::createFrameBuffer(const buffer_handle_t camera3buffer)\n> > > > > {\n> > > > > - std::vector planes;\n> > > > > + FileDescriptor fd;\n> > > > > + /* This assumes all the planes are in the same buffer. */\n> > > >\n> > > > Should we ASSERT() if that's not the case ? Also, does this assumption\n> > > > always hold ?\n> > > >\n> \n> I think this assumption holds at least on ChromeOS.\n> Hmm, ASSERT() is difficult because fds are duplicated and thus\n> different from each other in terms of integer.\n\nSo Chrome OS will give different fds, all referring to the same dmabuf ?\n\n> > > > > for (int i = 0; i < camera3buffer->numFds; i++) {\n> > > > > - /* Skip unused planes. */\n> > > > > - if (camera3buffer->data[i] == -1)\n> > > > > + if (camera3buffer->data[i] != -1) {\n> > > > > + fd = FileDescriptor(camera3buffer->data[i]);\n> > > > > break;\n> > > > > -\n> > > > > - FrameBuffer::Plane plane;\n> > > > > - plane.fd = FileDescriptor(camera3buffer->data[i]);\n> > > > > - if (!plane.fd.isValid()) {\n> > > > > - LOG(HAL, Error) << \"Failed to obtain FileDescriptor (\"\n> > > > > - << camera3buffer->data[i] << \") \"\n> > > > > - << \" on plane \" << i;\n> > > > > - return nullptr;\n> > > > > }\n> > > > > + }\n> > > >\n> > > > Blank line.\n> > > >\n> > > > > + if (!fd.isValid()) {\n> > > > > + LOG(HAL, Fatal) << \"No valid fd\";\n> > > > > + return nullptr;\n> > > > > + }\n> > > > >\n> > > > > - off_t length = lseek(plane.fd.fd(), 0, SEEK_END);\n> > > > > - if (length == -1) {\n> > > > > - LOG(HAL, Error) << \"Failed to query plane length\";\n> > > > > - return nullptr;\n> > > > > - }\n> > > > > + CameraBuffer buf(camera3buffer, PROT_READ);\n> > > >\n> > > > I suppose there's no other way than using the cros::CameraBufferManager\n> > > > to get the necessary information. I'm annoyed that this involves mapping\n> > > > the buffer to the CPU just to retrieve sizes, as that's an expensive\n> > > > operation :-( It's of course not a prerequisite for this series, but I'm\n> > > > wondering if the cros::CameraBufferManager API could be improved to\n> > > > separate retrieving plane information from mapping the buffer. What do\n> > > > you think ?\n> > >\n> > > Which mapping are you referring to?\n> > >\n> > > Also, there are already methods to retrieve this information, see\n> > > GetNumPlanes() and GetPlane*() in the CameraBufferManager class [1].\n> >\n> > You're right. We currently call Lock or LockYCbCr in the CameraBuffer\n> > constructor, but that's not required to get the plane size or offset. We\n> > should thus be able to improve the implementation here, without any\n> > change on the CrOS side. Thanks for the feedback.\n> \n> I actually tried to add the option to not map in the constructor\n> https://patchwork.libcamera.org/patch/11837/.\n> Would you tell me your preferred way?\n\nI had missed that, sorry. I've now replied to that patch.\n\n> > > [1] https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/platform2/camera/include/cros-camera/camera_buffer_manager.h;drc=6cd1330d01ad2fe4122faa0b2708ecf6267eb58e;l=305\n> > >\n> > > > Also, should generic_camera_buffer.cpp be updated to provide the correct\n> > > > information ?\n> > > >\n> > > > > + if (!buf.isValid()) {\n> > > > > + LOG(HAL, Fatal) << \"Failed mapping buffer\";\n> > > > > + return nullptr;\n> > > > > + }\n> > > > >\n> > > > > - plane.length = length;\n> > > > > - planes.push_back(std::move(plane));\n> > > > > + std::vector planes(buf.numPlanes());\n> > > > > + for (size_t i = 0; i < buf.numPlanes(); ++i) {\n> > > > > + planes[i].fd = fd;\n> > > > > + planes[i].offset = buf.plane(i).data() - buf.plane(0).data();\n> > > > > + planes[i].length = buf.plane(i).size();\n> > > > > }\n> > > > >\n> > > > > return new FrameBuffer(std::move(planes));","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 5B49ABD87C\n\tfor ;\n\tFri, 20 Aug 2021 11:26:12 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1D762688A5;\n\tFri, 20 Aug 2021 13:26:12 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 62AEC6025E\n\tfor ;\n\tFri, 20 Aug 2021 13:26:10 +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 002638C8;\n\tFri, 20 Aug 2021 13:26:09 +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=\"UyCTq6F7\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1629458770;\n\tbh=7qyLD3/sD2+o4ViynUV4jnTaxEoHlnjtY+NN3uEMtns=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=UyCTq6F7dnP3ujHYNUxKJqHd3LZnupsDxm73O58KXhfd/hC9g1hDanh+7EgmlTcj5\n\tSEnfgw1xjE1hjxkeDuyhk0EI9eOPrFKS+UNLoExyo2murgO9KbkEKXIdaSDU/MHVPM\n\tdE/VnpSDGVOf95taXdM9F7Bfuh/UhW1H7p6no57o=","Date":"Fri, 20 Aug 2021 14:26:01 +0300","From":"Laurent Pinchart ","To":"Hirokazu Honda ","Message-ID":"","References":"<20210816043138.957984-1-hiroh@chromium.org>\n\t<20210816043138.957984-10-hiroh@chromium.org>\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] [RFC PATCH 09/10] android: camera_device:\n\tFill offset and right length in CreateFrameBuffer()","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":18977,"web_url":"https://patchwork.libcamera.org/comment/18977/","msgid":"","date":"2021-08-20T11:31:59","subject":"Re: [libcamera-devel] [RFC PATCH 09/10] android: camera_device:\n\tFill offset and right length in CreateFrameBuffer()","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"Hi Laurent,\n\nOn Fri, Aug 20, 2021 at 8:26 PM Laurent Pinchart\n wrote:\n>\n> Hi Hiro,\n>\n> On Fri, Aug 20, 2021 at 08:13:28PM +0900, Hirokazu Honda wrote:\n> > On Wed, Aug 18, 2021 at 7:26 PM Laurent Pinchart wrote:\n> > > On Wed, Aug 18, 2021 at 06:30:01PM +0900, Tomasz Figa wrote:\n> > > > On Wed, Aug 18, 2021 at 10:52 AM Laurent Pinchart wrote:\n> > > > > On Mon, Aug 16, 2021 at 01:31:37PM +0900, Hirokazu Honda wrote:\n> > > > > > CameraDevice::CreateFrameBuffer() fills the length of the buffer to\n> > > > > > each FrameBuffer::Plane::length. It should rather be the length of\n> > > > > > plane. This also changes CreateFrameBuffer() to fill offset of\n> > > > > > FrameBuffer::Plane.\n> > > > > >\n> > > > > > Signed-off-by: Hirokazu Honda \n> > > > > > ---\n> > > > > > src/android/camera_device.cpp | 38 ++++++++++++++++++-----------------\n> > > > > > 1 file changed, 20 insertions(+), 18 deletions(-)\n> > > > > >\n> > > > > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > > > > > index a69b687a..1ded5cb1 100644\n> > > > > > --- a/src/android/camera_device.cpp\n> > > > > > +++ b/src/android/camera_device.cpp\n> > > > > > @@ -12,6 +12,7 @@\n> > > > > >\n> > > > > > #include \n> > > > > > #include \n> > > > > > +#include \n> > > > > > #include \n> > > > > > #include \n> > > > > >\n> > > > > > @@ -746,29 +747,30 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)\n> > > > > >\n> > > > > > FrameBuffer *CameraDevice::createFrameBuffer(const buffer_handle_t camera3buffer)\n> > > > > > {\n> > > > > > - std::vector planes;\n> > > > > > + FileDescriptor fd;\n> > > > > > + /* This assumes all the planes are in the same buffer. */\n> > > > >\n> > > > > Should we ASSERT() if that's not the case ? Also, does this assumption\n> > > > > always hold ?\n> > > > >\n> >\n> > I think this assumption holds at least on ChromeOS.\n> > Hmm, ASSERT() is difficult because fds are duplicated and thus\n> > different from each other in terms of integer.\n>\n> So Chrome OS will give different fds, all referring to the same dmabuf ?\n>\n\nI didn't know they are always dup()ed. It depends on HAL client implementation.\n+Ricky, do you know about it?\n\n> > > > > > for (int i = 0; i < camera3buffer->numFds; i++) {\n> > > > > > - /* Skip unused planes. */\n> > > > > > - if (camera3buffer->data[i] == -1)\n> > > > > > + if (camera3buffer->data[i] != -1) {\n> > > > > > + fd = FileDescriptor(camera3buffer->data[i]);\n> > > > > > break;\n> > > > > > -\n> > > > > > - FrameBuffer::Plane plane;\n> > > > > > - plane.fd = FileDescriptor(camera3buffer->data[i]);\n> > > > > > - if (!plane.fd.isValid()) {\n> > > > > > - LOG(HAL, Error) << \"Failed to obtain FileDescriptor (\"\n> > > > > > - << camera3buffer->data[i] << \") \"\n> > > > > > - << \" on plane \" << i;\n> > > > > > - return nullptr;\n> > > > > > }\n> > > > > > + }\n> > > > >\n> > > > > Blank line.\n> > > > >\n> > > > > > + if (!fd.isValid()) {\n> > > > > > + LOG(HAL, Fatal) << \"No valid fd\";\n> > > > > > + return nullptr;\n> > > > > > + }\n> > > > > >\n> > > > > > - off_t length = lseek(plane.fd.fd(), 0, SEEK_END);\n> > > > > > - if (length == -1) {\n> > > > > > - LOG(HAL, Error) << \"Failed to query plane length\";\n> > > > > > - return nullptr;\n> > > > > > - }\n> > > > > > + CameraBuffer buf(camera3buffer, PROT_READ);\n> > > > >\n> > > > > I suppose there's no other way than using the cros::CameraBufferManager\n> > > > > to get the necessary information. I'm annoyed that this involves mapping\n> > > > > the buffer to the CPU just to retrieve sizes, as that's an expensive\n> > > > > operation :-( It's of course not a prerequisite for this series, but I'm\n> > > > > wondering if the cros::CameraBufferManager API could be improved to\n> > > > > separate retrieving plane information from mapping the buffer. What do\n> > > > > you think ?\n> > > >\n> > > > Which mapping are you referring to?\n> > > >\n> > > > Also, there are already methods to retrieve this information, see\n> > > > GetNumPlanes() and GetPlane*() in the CameraBufferManager class [1].\n> > >\n> > > You're right. We currently call Lock or LockYCbCr in the CameraBuffer\n> > > constructor, but that's not required to get the plane size or offset. We\n> > > should thus be able to improve the implementation here, without any\n> > > change on the CrOS side. Thanks for the feedback.\n> >\n> > I actually tried to add the option to not map in the constructor\n> > https://patchwork.libcamera.org/patch/11837/.\n> > Would you tell me your preferred way?\n>\n> I had missed that, sorry. I've now replied to that patch.\n>\n> > > > [1] https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/platform2/camera/include/cros-camera/camera_buffer_manager.h;drc=6cd1330d01ad2fe4122faa0b2708ecf6267eb58e;l=305\n> > > >\n> > > > > Also, should generic_camera_buffer.cpp be updated to provide the correct\n> > > > > information ?\n> > > > >\n> > > > > > + if (!buf.isValid()) {\n> > > > > > + LOG(HAL, Fatal) << \"Failed mapping buffer\";\n> > > > > > + return nullptr;\n> > > > > > + }\n> > > > > >\n> > > > > > - plane.length = length;\n> > > > > > - planes.push_back(std::move(plane));\n> > > > > > + std::vector planes(buf.numPlanes());\n> > > > > > + for (size_t i = 0; i < buf.numPlanes(); ++i) {\n> > > > > > + planes[i].fd = fd;\n> > > > > > + planes[i].offset = buf.plane(i).data() - buf.plane(0).data();\n> > > > > > + planes[i].length = buf.plane(i).size();\n> > > > > > }\n> > > > > >\n> > > > > > return new FrameBuffer(std::move(planes));\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 ABC04BD87D\n\tfor ;\n\tFri, 20 Aug 2021 11:32:15 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3549C68895;\n\tFri, 20 Aug 2021 13:32:15 +0200 (CEST)","from mail-ej1-x62f.google.com (mail-ej1-x62f.google.com\n\t[IPv6:2a00:1450:4864:20::62f])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 8B5B16025E\n\tfor ;\n\tFri, 20 Aug 2021 13:32:14 +0200 (CEST)","by mail-ej1-x62f.google.com with SMTP id mf2so1054883ejb.9\n\tfor ;\n\tFri, 20 Aug 2021 04:32:14 -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=\"LTQnW3Bq\"; 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=yKk50gfGBj/I3l4oi6gCygc4jI5lVO/I2dkgEbwRsGc=;\n\tb=LTQnW3BqIsfBm3Z9xTDS35ewxXQ14GbfQ6th5D/DvnzLY+C5xesOFBj33NeVMvkI82\n\tX4DUCoFmxaMfZij9ttf8t1r0E0wKy6TCnZIykVbey6kPaVEa4jLto/kUTtTyOdua+qxv\n\tGzyfkPU2vCtvkpa2Rf79NXW/YgzL+V/h92IkU=","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=yKk50gfGBj/I3l4oi6gCygc4jI5lVO/I2dkgEbwRsGc=;\n\tb=qrQ6TUywxJwUJ8DWf/i2gY4Qs5jhyqaaXp6/srwbxS9Jxn6cBbEbtGTjnEAD5dGiCg\n\t5LBTffXYRt40gLUKkc5i5ZgAgMCAd1IMLHytfOIIbOUYhbyAB8SC5PU7lnLMsCWIXHFd\n\tQRVQAuFKgOXXwQ3vzWU6Owcs5oKVCcbwd3TpVhHnD1VONhcbaCCweFHXJ9rrmed62imV\n\tOE32CYQbHLXlX25W8xmES5OieaWYZd77WoU1PoWJVxb1rOh2AkAgSvHo+rGD5MCt04Hi\n\tzw9aDjVuxE0clMpn1qqSHvFvsK7C3HIMKzJoSlHbXTWcDmrO6Cu+W6EHb6lSXP2el8Cz\n\tzeKQ==","X-Gm-Message-State":"AOAM533lCJTazkCsoX2vOz1la20ggTTOXR85phUYfNK5ijDHbyPN0w11\n\tCPp7ItDGL9DgU3k4aGTM4QBUkJzLxt7eSt5s2NRuOQ==","X-Google-Smtp-Source":"ABdhPJxInDsjjIzwkXh6Riptf5tFn8HB3bgAh5SWTh+dCQgHczfxQHAKXkc0hEWy+NjNaAJy7GhnfUENQlWJob/dP8E=","X-Received":"by 2002:a17:906:bcdb:: with SMTP id\n\tlw27mr21642102ejb.292.1629459131760; \n\tFri, 20 Aug 2021 04:32:11 -0700 (PDT)","MIME-Version":"1.0","References":"<20210816043138.957984-1-hiroh@chromium.org>\n\t<20210816043138.957984-10-hiroh@chromium.org>\n\t\n\t\n\t\n\t\n\t","In-Reply-To":"","From":"Hirokazu Honda ","Date":"Fri, 20 Aug 2021 20:31:59 +0900","Message-ID":"","To":"Laurent Pinchart ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [RFC PATCH 09/10] android: camera_device:\n\tFill offset and right length in CreateFrameBuffer()","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":18988,"web_url":"https://patchwork.libcamera.org/comment/18988/","msgid":"","date":"2021-08-20T16:25:37","subject":"Re: [libcamera-devel] [RFC PATCH 09/10] android: camera_device:\n\tFill offset and right length in CreateFrameBuffer()","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Hiro,\n\nOn Fri, Aug 20, 2021 at 08:31:59PM +0900, Hirokazu Honda wrote:\n> On Fri, Aug 20, 2021 at 8:26 PM Laurent Pinchart wrote:\n> > On Fri, Aug 20, 2021 at 08:13:28PM +0900, Hirokazu Honda wrote:\n> > > On Wed, Aug 18, 2021 at 7:26 PM Laurent Pinchart wrote:\n> > > > On Wed, Aug 18, 2021 at 06:30:01PM +0900, Tomasz Figa wrote:\n> > > > > On Wed, Aug 18, 2021 at 10:52 AM Laurent Pinchart wrote:\n> > > > > > On Mon, Aug 16, 2021 at 01:31:37PM +0900, Hirokazu Honda wrote:\n> > > > > > > CameraDevice::CreateFrameBuffer() fills the length of the buffer to\n> > > > > > > each FrameBuffer::Plane::length. It should rather be the length of\n> > > > > > > plane. This also changes CreateFrameBuffer() to fill offset of\n> > > > > > > FrameBuffer::Plane.\n> > > > > > >\n> > > > > > > Signed-off-by: Hirokazu Honda \n> > > > > > > ---\n> > > > > > > src/android/camera_device.cpp | 38 ++++++++++++++++++-----------------\n> > > > > > > 1 file changed, 20 insertions(+), 18 deletions(-)\n> > > > > > >\n> > > > > > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > > > > > > index a69b687a..1ded5cb1 100644\n> > > > > > > --- a/src/android/camera_device.cpp\n> > > > > > > +++ b/src/android/camera_device.cpp\n> > > > > > > @@ -12,6 +12,7 @@\n> > > > > > >\n> > > > > > > #include \n> > > > > > > #include \n> > > > > > > +#include \n> > > > > > > #include \n> > > > > > > #include \n> > > > > > >\n> > > > > > > @@ -746,29 +747,30 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)\n> > > > > > >\n> > > > > > > FrameBuffer *CameraDevice::createFrameBuffer(const buffer_handle_t camera3buffer)\n> > > > > > > {\n> > > > > > > - std::vector planes;\n> > > > > > > + FileDescriptor fd;\n> > > > > > > + /* This assumes all the planes are in the same buffer. */\n> > > > > >\n> > > > > > Should we ASSERT() if that's not the case ? Also, does this assumption\n> > > > > > always hold ?\n> > > > > >\n> > >\n> > > I think this assumption holds at least on ChromeOS.\n> > > Hmm, ASSERT() is difficult because fds are duplicated and thus\n> > > different from each other in terms of integer.\n> >\n> > So Chrome OS will give different fds, all referring to the same dmabuf ?\n> \n> I didn't know they are always dup()ed. It depends on HAL client implementation.\n> +Ricky, do you know about it?\n\nFor what it's worth, I believe you can check if two different fds refer\nto the same dmabuf by comparing their inode number, as returned by\nfstat(). This is something that should likely be done in\nV4L2VideoDevice::queueBuffer(), as from the point of view of the\nlibcamera public API, we want to support different buffers and offsets.\n\n> > > > > > > for (int i = 0; i < camera3buffer->numFds; i++) {\n> > > > > > > - /* Skip unused planes. */\n> > > > > > > - if (camera3buffer->data[i] == -1)\n> > > > > > > + if (camera3buffer->data[i] != -1) {\n> > > > > > > + fd = FileDescriptor(camera3buffer->data[i]);\n> > > > > > > break;\n> > > > > > > -\n> > > > > > > - FrameBuffer::Plane plane;\n> > > > > > > - plane.fd = FileDescriptor(camera3buffer->data[i]);\n> > > > > > > - if (!plane.fd.isValid()) {\n> > > > > > > - LOG(HAL, Error) << \"Failed to obtain FileDescriptor (\"\n> > > > > > > - << camera3buffer->data[i] << \") \"\n> > > > > > > - << \" on plane \" << i;\n> > > > > > > - return nullptr;\n> > > > > > > }\n> > > > > > > + }\n> > > > > >\n> > > > > > Blank line.\n> > > > > >\n> > > > > > > + if (!fd.isValid()) {\n> > > > > > > + LOG(HAL, Fatal) << \"No valid fd\";\n> > > > > > > + return nullptr;\n> > > > > > > + }\n> > > > > > >\n> > > > > > > - off_t length = lseek(plane.fd.fd(), 0, SEEK_END);\n> > > > > > > - if (length == -1) {\n> > > > > > > - LOG(HAL, Error) << \"Failed to query plane length\";\n> > > > > > > - return nullptr;\n> > > > > > > - }\n> > > > > > > + CameraBuffer buf(camera3buffer, PROT_READ);\n> > > > > >\n> > > > > > I suppose there's no other way than using the cros::CameraBufferManager\n> > > > > > to get the necessary information. I'm annoyed that this involves mapping\n> > > > > > the buffer to the CPU just to retrieve sizes, as that's an expensive\n> > > > > > operation :-( It's of course not a prerequisite for this series, but I'm\n> > > > > > wondering if the cros::CameraBufferManager API could be improved to\n> > > > > > separate retrieving plane information from mapping the buffer. What do\n> > > > > > you think ?\n> > > > >\n> > > > > Which mapping are you referring to?\n> > > > >\n> > > > > Also, there are already methods to retrieve this information, see\n> > > > > GetNumPlanes() and GetPlane*() in the CameraBufferManager class [1].\n> > > >\n> > > > You're right. We currently call Lock or LockYCbCr in the CameraBuffer\n> > > > constructor, but that's not required to get the plane size or offset. We\n> > > > should thus be able to improve the implementation here, without any\n> > > > change on the CrOS side. Thanks for the feedback.\n> > >\n> > > I actually tried to add the option to not map in the constructor\n> > > https://patchwork.libcamera.org/patch/11837/.\n> > > Would you tell me your preferred way?\n> >\n> > I had missed that, sorry. I've now replied to that patch.\n> >\n> > > > > [1] https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/platform2/camera/include/cros-camera/camera_buffer_manager.h;drc=6cd1330d01ad2fe4122faa0b2708ecf6267eb58e;l=305\n> > > > >\n> > > > > > Also, should generic_camera_buffer.cpp be updated to provide the correct\n> > > > > > information ?\n> > > > > >\n> > > > > > > + if (!buf.isValid()) {\n> > > > > > > + LOG(HAL, Fatal) << \"Failed mapping buffer\";\n> > > > > > > + return nullptr;\n> > > > > > > + }\n> > > > > > >\n> > > > > > > - plane.length = length;\n> > > > > > > - planes.push_back(std::move(plane));\n> > > > > > > + std::vector planes(buf.numPlanes());\n> > > > > > > + for (size_t i = 0; i < buf.numPlanes(); ++i) {\n> > > > > > > + planes[i].fd = fd;\n> > > > > > > + planes[i].offset = buf.plane(i).data() - buf.plane(0).data();\n> > > > > > > + planes[i].length = buf.plane(i).size();\n> > > > > > > }\n> > > > > > >\n> > > > > > > return new FrameBuffer(std::move(planes));","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 CFBF1BD87C\n\tfor ;\n\tFri, 20 Aug 2021 16:25:48 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 28193688AC;\n\tFri, 20 Aug 2021 18:25:48 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 740296888E\n\tfor ;\n\tFri, 20 Aug 2021 18:25:47 +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 F3A808C8;\n\tFri, 20 Aug 2021 18:25:46 +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=\"PrHJkhwK\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1629476747;\n\tbh=kmNTu73bevcuWfuA7AKWwHGxIfIIWzagttAenAXL5AM=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=PrHJkhwKSsjPCHOnFxugwFwtDiwpHz3dYW1LDPtfZryCcNHt/QYf/GXwpBufhN3eI\n\tdECjcxvTwv4lgpmwqQql5EBbxPeW78LvTscRFkhQogJNiqknRjcKDsvfhdzHEwZvDv\n\tL6ZCJNIownLg4G5ej8L4zFt6GSv8bk2a4zyW47UQ=","Date":"Fri, 20 Aug 2021 19:25:37 +0300","From":"Laurent Pinchart ","To":"Hirokazu Honda ","Message-ID":"","References":"<20210816043138.957984-1-hiroh@chromium.org>\n\t<20210816043138.957984-10-hiroh@chromium.org>\n\t\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] [RFC PATCH 09/10] android: camera_device:\n\tFill offset and right length in CreateFrameBuffer()","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":19086,"web_url":"https://patchwork.libcamera.org/comment/19086/","msgid":"","date":"2021-08-26T05:16:51","subject":"Re: [libcamera-devel] [RFC PATCH 09/10] android: camera_device:\n\tFill offset and right length in CreateFrameBuffer()","submitter":{"id":9,"url":"https://patchwork.libcamera.org/api/people/9/","name":"Tomasz Figa","email":"tfiga@chromium.org"},"content":"On Sat, Aug 21, 2021 at 1:25 AM Laurent Pinchart\n wrote:\n>\n> Hi Hiro,\n>\n> On Fri, Aug 20, 2021 at 08:31:59PM +0900, Hirokazu Honda wrote:\n> > On Fri, Aug 20, 2021 at 8:26 PM Laurent Pinchart wrote:\n> > > On Fri, Aug 20, 2021 at 08:13:28PM +0900, Hirokazu Honda wrote:\n> > > > On Wed, Aug 18, 2021 at 7:26 PM Laurent Pinchart wrote:\n> > > > > On Wed, Aug 18, 2021 at 06:30:01PM +0900, Tomasz Figa wrote:\n> > > > > > On Wed, Aug 18, 2021 at 10:52 AM Laurent Pinchart wrote:\n> > > > > > > On Mon, Aug 16, 2021 at 01:31:37PM +0900, Hirokazu Honda wrote:\n> > > > > > > > CameraDevice::CreateFrameBuffer() fills the length of the buffer to\n> > > > > > > > each FrameBuffer::Plane::length. It should rather be the length of\n> > > > > > > > plane. This also changes CreateFrameBuffer() to fill offset of\n> > > > > > > > FrameBuffer::Plane.\n> > > > > > > >\n> > > > > > > > Signed-off-by: Hirokazu Honda \n> > > > > > > > ---\n> > > > > > > > src/android/camera_device.cpp | 38 ++++++++++++++++++-----------------\n> > > > > > > > 1 file changed, 20 insertions(+), 18 deletions(-)\n> > > > > > > >\n> > > > > > > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > > > > > > > index a69b687a..1ded5cb1 100644\n> > > > > > > > --- a/src/android/camera_device.cpp\n> > > > > > > > +++ b/src/android/camera_device.cpp\n> > > > > > > > @@ -12,6 +12,7 @@\n> > > > > > > >\n> > > > > > > > #include \n> > > > > > > > #include \n> > > > > > > > +#include \n> > > > > > > > #include \n> > > > > > > > #include \n> > > > > > > >\n> > > > > > > > @@ -746,29 +747,30 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)\n> > > > > > > >\n> > > > > > > > FrameBuffer *CameraDevice::createFrameBuffer(const buffer_handle_t camera3buffer)\n> > > > > > > > {\n> > > > > > > > - std::vector planes;\n> > > > > > > > + FileDescriptor fd;\n> > > > > > > > + /* This assumes all the planes are in the same buffer. */\n> > > > > > >\n> > > > > > > Should we ASSERT() if that's not the case ? Also, does this assumption\n> > > > > > > always hold ?\n> > > > > > >\n> > > >\n> > > > I think this assumption holds at least on ChromeOS.\n> > > > Hmm, ASSERT() is difficult because fds are duplicated and thus\n> > > > different from each other in terms of integer.\n> > >\n> > > So Chrome OS will give different fds, all referring to the same dmabuf ?\n> >\n> > I didn't know they are always dup()ed. It depends on HAL client implementation.\n> > +Ricky, do you know about it?\n>\n> For what it's worth, I believe you can check if two different fds refer\n> to the same dmabuf by comparing their inode number, as returned by\n> fstat(). This is something that should likely be done in\n> V4L2VideoDevice::queueBuffer(), as from the point of view of the\n> libcamera public API, we want to support different buffers and offsets.\n>\n\nAs I mentioned in another reply, there are only 2 patterns that occur\nin practice - 1 DMA-buf with offsets for color planes OR multiple\nDMA-bufs with 0 offsets. Chrome OS actually ends up using only the\nformer.\n\nBest regards,\nTomasz\n\n> > > > > > > > for (int i = 0; i < camera3buffer->numFds; i++) {\n> > > > > > > > - /* Skip unused planes. */\n> > > > > > > > - if (camera3buffer->data[i] == -1)\n> > > > > > > > + if (camera3buffer->data[i] != -1) {\n> > > > > > > > + fd = FileDescriptor(camera3buffer->data[i]);\n> > > > > > > > break;\n> > > > > > > > -\n> > > > > > > > - FrameBuffer::Plane plane;\n> > > > > > > > - plane.fd = FileDescriptor(camera3buffer->data[i]);\n> > > > > > > > - if (!plane.fd.isValid()) {\n> > > > > > > > - LOG(HAL, Error) << \"Failed to obtain FileDescriptor (\"\n> > > > > > > > - << camera3buffer->data[i] << \") \"\n> > > > > > > > - << \" on plane \" << i;\n> > > > > > > > - return nullptr;\n> > > > > > > > }\n> > > > > > > > + }\n> > > > > > >\n> > > > > > > Blank line.\n> > > > > > >\n> > > > > > > > + if (!fd.isValid()) {\n> > > > > > > > + LOG(HAL, Fatal) << \"No valid fd\";\n> > > > > > > > + return nullptr;\n> > > > > > > > + }\n> > > > > > > >\n> > > > > > > > - off_t length = lseek(plane.fd.fd(), 0, SEEK_END);\n> > > > > > > > - if (length == -1) {\n> > > > > > > > - LOG(HAL, Error) << \"Failed to query plane length\";\n> > > > > > > > - return nullptr;\n> > > > > > > > - }\n> > > > > > > > + CameraBuffer buf(camera3buffer, PROT_READ);\n> > > > > > >\n> > > > > > > I suppose there's no other way than using the cros::CameraBufferManager\n> > > > > > > to get the necessary information. I'm annoyed that this involves mapping\n> > > > > > > the buffer to the CPU just to retrieve sizes, as that's an expensive\n> > > > > > > operation :-( It's of course not a prerequisite for this series, but I'm\n> > > > > > > wondering if the cros::CameraBufferManager API could be improved to\n> > > > > > > separate retrieving plane information from mapping the buffer. What do\n> > > > > > > you think ?\n> > > > > >\n> > > > > > Which mapping are you referring to?\n> > > > > >\n> > > > > > Also, there are already methods to retrieve this information, see\n> > > > > > GetNumPlanes() and GetPlane*() in the CameraBufferManager class [1].\n> > > > >\n> > > > > You're right. We currently call Lock or LockYCbCr in the CameraBuffer\n> > > > > constructor, but that's not required to get the plane size or offset. We\n> > > > > should thus be able to improve the implementation here, without any\n> > > > > change on the CrOS side. Thanks for the feedback.\n> > > >\n> > > > I actually tried to add the option to not map in the constructor\n> > > > https://patchwork.libcamera.org/patch/11837/.\n> > > > Would you tell me your preferred way?\n> > >\n> > > I had missed that, sorry. I've now replied to that patch.\n> > >\n> > > > > > [1] https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/platform2/camera/include/cros-camera/camera_buffer_manager.h;drc=6cd1330d01ad2fe4122faa0b2708ecf6267eb58e;l=305\n> > > > > >\n> > > > > > > Also, should generic_camera_buffer.cpp be updated to provide the correct\n> > > > > > > information ?\n> > > > > > >\n> > > > > > > > + if (!buf.isValid()) {\n> > > > > > > > + LOG(HAL, Fatal) << \"Failed mapping buffer\";\n> > > > > > > > + return nullptr;\n> > > > > > > > + }\n> > > > > > > >\n> > > > > > > > - plane.length = length;\n> > > > > > > > - planes.push_back(std::move(plane));\n> > > > > > > > + std::vector planes(buf.numPlanes());\n> > > > > > > > + for (size_t i = 0; i < buf.numPlanes(); ++i) {\n> > > > > > > > + planes[i].fd = fd;\n> > > > > > > > + planes[i].offset = buf.plane(i).data() - buf.plane(0).data();\n> > > > > > > > + planes[i].length = buf.plane(i).size();\n> > > > > > > > }\n> > > > > > > >\n> > > > > > > > return new FrameBuffer(std::move(planes));\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 CCA67BD87D\n\tfor ;\n\tThu, 26 Aug 2021 05:17:07 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3604B68893;\n\tThu, 26 Aug 2021 07:17:07 +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 DEE4A60256\n\tfor ;\n\tThu, 26 Aug 2021 07:17:05 +0200 (CEST)","by mail-ej1-x62e.google.com with SMTP id bt14so3486771ejb.3\n\tfor ;\n\tWed, 25 Aug 2021 22:17:05 -0700 (PDT)","from mail-wm1-f52.google.com (mail-wm1-f52.google.com.\n\t[209.85.128.52]) by smtp.gmail.com with ESMTPSA id\n\tk12sm1028296edq.59.2021.08.25.22.17.03\n\tfor \n\t(version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128);\n\tWed, 25 Aug 2021 22:17:04 -0700 (PDT)","by mail-wm1-f52.google.com with SMTP id g138so1014819wmg.4\n\tfor ;\n\tWed, 25 Aug 2021 22:17:03 -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=\"Z+ZyIbrP\"; 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=lxY7UX+2XzJLWcNuWNXR/7AXuIC+tR1dKeMm6NrrgN8=;\n\tb=Z+ZyIbrPEY2Hid1BEexb3wlFsFPovjGDMnBp+BV2vt//Jsg4Gn9Z8+uwPT7OAt5j8b\n\tUTNy1BDicOD40g7IiA2nilVL2p70bwZxziP+Bic4WTQkETnZhVF+8AUppB+6IPqfwbvv\n\tUbBxflNQ9ihEu4DMwaOh123AX2U74pYE4wFYg=","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=lxY7UX+2XzJLWcNuWNXR/7AXuIC+tR1dKeMm6NrrgN8=;\n\tb=G6h8v2QgGM+wI/7GgGgn9B3WThNJBqpfe9aXtkoRLXRZ4TEDEiSNScrj7gO5bq+x1X\n\tO+yqYrrDF2Dw0zbY/CnR7ZS6uNu8nrPVL1owk7NfyyWBDpncU6hTf5oTdv8o8Qnks9cz\n\t4YsinAzHNlC2ahTB6ZV++G4056Tmz6xkUfKi9XEWskMAAYDv29QJehTvleFAQIZiqdF2\n\tREdE70LRTxPishGBWcpx+LC1hUsuS4qx3nSFdBUGHKN/x9NYk/ViPvPFydBDjkD1JOsi\n\tv8mhGKJm5N/+prZbngJBq3hFAhDovLB6LOqxbNi0FYqul9BsMy6ebL2JIBJCc3gvY7jZ\n\tCsWw==","X-Gm-Message-State":"AOAM532lNz2DEC7Do/1qG5veBVFjibH5lD/R7W3bpOCSyA7YJsX//m9y\n\tUi5rvreWMRA8JC/okcFBnwH/6DU/JLjQ0Q==","X-Google-Smtp-Source":"ABdhPJxPIdzTyiV8f62aSXWDfNxCJ6C6Dj/+RGl8Cj9WNFp4iJOgt1jPmKmMpuydiukXq6ItYWOwWg==","X-Received":["by 2002:a17:907:61b0:: with SMTP id\n\tmt48mr2349712ejc.64.1629955025080; \n\tWed, 25 Aug 2021 22:17:05 -0700 (PDT)","by 2002:a7b:cc16:: with SMTP id\n\tf22mr12237977wmh.99.1629955023098; \n\tWed, 25 Aug 2021 22:17:03 -0700 (PDT)"],"MIME-Version":"1.0","References":"<20210816043138.957984-1-hiroh@chromium.org>\n\t<20210816043138.957984-10-hiroh@chromium.org>\n\t\n\t\n\t\n\t\n\t\n\t\n\t","In-Reply-To":"","From":"Tomasz Figa ","Date":"Thu, 26 Aug 2021 14:16:51 +0900","X-Gmail-Original-Message-ID":"","Message-ID":"","To":"Laurent Pinchart ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [RFC PATCH 09/10] android: camera_device:\n\tFill offset and right length in CreateFrameBuffer()","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\" "}}]