[{"id":19236,"web_url":"https://patchwork.libcamera.org/comment/19236/","msgid":"","date":"2021-08-31T21:45:01","subject":"Re: [libcamera-devel] [PATCH 3/5] mapped_framebuffer: Introduce\n\tMappedVectorBuffer","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 Tue, Aug 31, 2021 at 03:34:36PM +0900, Hirokazu Honda wrote:\n> MappedFrameBuffer is a main class of deriving MappedBuffer. It works\n> with FrameBuffer, so the source planes must be given as file\n> descriptors.\n> MappedBuffer interface is a generic class of providing accessing\n> planes in memory. This introduces a new class deriving MappedBuffer,\n> MappedVectorBuffer. It is created with the plane data in heap,\n> concretely, std::vector and owns it.\n\nThe MappedBuffer interface is geared towards performing memory mappings\n(hence the maps_ member in the base class), which isn't applicable here.\nI see why you want this in this series, but I'm not sure it's the best\nsolution. It feels that what you need is a class that represents an\nimage accessible by the CPU, regardless of how the memory was obtained.\nI fear that creating such a generic image class will quickly grow out of\ncontrol and become a fully fledged CPU image API.\n\nI'll have to sleep over this.\n\n> Signed-off-by: Hirokazu Honda \n> ---\n> include/libcamera/internal/mapped_framebuffer.h | 9 +++++++++\n> src/libcamera/mapped_framebuffer.cpp | 17 +++++++++++++++++\n> 2 files changed, 26 insertions(+)\n> \n> diff --git a/include/libcamera/internal/mapped_framebuffer.h b/include/libcamera/internal/mapped_framebuffer.h\n> index 70ffbcbe..7deecded 100644\n> --- a/include/libcamera/internal/mapped_framebuffer.h\n> +++ b/include/libcamera/internal/mapped_framebuffer.h\n> @@ -59,6 +59,15 @@ public:\n> \n> LIBCAMERA_FLAGS_ENABLE_OPERATORS(MappedFrameBuffer::MapFlag)\n> \n> +class MappedVectorBuffer : public MappedBuffer\n> +{\n> +public:\n> +\tMappedVectorBuffer(std::vector> &&planes);\n> +\n> +private:\n> +\tstd::vector> data_;\n> +};\n> +\n> } /* namespace libcamera */\n> \n> #endif /* __LIBCAMERA_INTERNAL_MAPPED_FRAMEBUFFER_H__ */\n> diff --git a/src/libcamera/mapped_framebuffer.cpp b/src/libcamera/mapped_framebuffer.cpp\n> index 464d35fe..b09ad41a 100644\n> --- a/src/libcamera/mapped_framebuffer.cpp\n> +++ b/src/libcamera/mapped_framebuffer.cpp\n> @@ -240,4 +240,21 @@ MappedFrameBuffer::MappedFrameBuffer(const FrameBuffer *buffer, MapFlags flags)\n> \t}\n> }\n> \n> +/**\n> + * \\class MappedVectorBuffer\n> + * \\brief Owns planes data in heap and provides the MappedBuffer interface\n> + */\n> +\n> +/**\n> + * \\brief Takes the ownership of the passed \\a planes\n> + * \\param[in] planes the plane data that shall be owned by MappedVectorBuffer\n> + */\n> +MappedVectorBuffer::MappedVectorBuffer(std::vector> &&planes)\n> +\t: data_(std::move(planes))\n> +{\n> +\tplanes_.reserve(data_.size());\n> +\tfor (auto &plane : data_)\n> +\t\tplanes_.emplace_back(plane.data(), plane.size());\n> +}\n> +\n> } /* namespace libcamera */","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 C4E74BDC71\n\tfor ;\n\tTue, 31 Aug 2021 21:45:18 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2CB9069166;\n\tTue, 31 Aug 2021 23:45:18 +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 B587C68890\n\tfor ;\n\tTue, 31 Aug 2021 23:45:16 +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 2219B24F;\n\tTue, 31 Aug 2021 23:45:16 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"dxmGwN7r\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1630446316;\n\tbh=G1YUFyrvTSGofCZRoe9yXm5c3oI5eJWufT5z6Eluytc=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=dxmGwN7rcWNYtdlDKWo9tw1ynDd+QFj6fFtbrj8SLrg+aPivfTpEdPn6gdIlESa3n\n\trEKNbTjzv+VGQIg0LAlL3YeOnWCyTZRY2gXv/0Z8t1xjrsdcxYGSA92XljClDdSk9U\n\tfxqBPiK1pUro3JDuuoHuqlJsfPNSF17RkOHylXYo=","Date":"Wed, 1 Sep 2021 00:45:01 +0300","From":"Laurent Pinchart ","To":"Hirokazu Honda ","Message-ID":"","References":"<20210831063438.785767-1-hiroh@chromium.org>\n\t<20210831063438.785767-4-hiroh@chromium.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210831063438.785767-4-hiroh@chromium.org>","Subject":"Re: [libcamera-devel] [PATCH 3/5] mapped_framebuffer: Introduce\n\tMappedVectorBuffer","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":20164,"web_url":"https://patchwork.libcamera.org/comment/20164/","msgid":"","date":"2021-10-13T03:51:07","subject":"Re: [libcamera-devel] [PATCH 3/5] mapped_framebuffer: Introduce\n\tMappedVectorBuffer","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, Sep 01, 2021 at 12:45:02AM +0300, Laurent Pinchart wrote:\n> On Tue, Aug 31, 2021 at 03:34:36PM +0900, Hirokazu Honda wrote:\n> > MappedFrameBuffer is a main class of deriving MappedBuffer. It works\n> > with FrameBuffer, so the source planes must be given as file\n> > descriptors.\n> > MappedBuffer interface is a generic class of providing accessing\n> > planes in memory. This introduces a new class deriving MappedBuffer,\n> > MappedVectorBuffer. It is created with the plane data in heap,\n> > concretely, std::vector and owns it.\n> \n> The MappedBuffer interface is geared towards performing memory mappings\n> (hence the maps_ member in the base class), which isn't applicable here.\n> I see why you want this in this series, but I'm not sure it's the best\n> solution. It feels that what you need is a class that represents an\n> image accessible by the CPU, regardless of how the memory was obtained.\n> I fear that creating such a generic image class will quickly grow out of\n> control and become a fully fledged CPU image API.\n> \n> I'll have to sleep over this.\n\nI think an Image class would be the best way forward. We have an initial\nimplementation in the cam application, but it needs some rework. Here's\na copy of the relevant part of the discussion during the review of that\nclass:\n\n\n The Image class is an interface to provide access to pixel data. In\n its current form it's constructed from a FrameBuffer, but I'd like the\n ability to construct it from a byte array as well. [...] It would\n allow the JPEG compression in the Android HAL to use an Image as the\n source, regardless of whether the compresses the still capture (coming\n from libcamera in a FrameBuffer) or the thumbnail (downscaled in\n software and stored in a std::vector).\n\n What I still haven't determined is whether the Image class should be\n an interface with pure virtual functions only, implemented by\n subclasses such as FrameBufferImage or MemoryImage, or if it should\n contain the data as well, populated by the different constructors.\n\n I've also started to think about how to perform the mapping. For\n FrameBuffer objects constructed from Android buffers, the mapping\n should be delegated to gralloc on Android and to the\n CameraBufferManager on Chrome OS. For FrameBuffer objects constructed\n internally by the V4L2VideoDevice (and in particular the ones exposes\n to applications with FrameBufferAllocator), the code [in the Image\n class] should be correct. For other types of FrameBuffer objects\n supplied by applications, another method of mapping may be needed. I'm\n not sure yet how to best handle that, and if we'll need a\n FrameBufferMapper object that FrameBuffer instances will reference.\n\n\nI'm afraid I don't have answers to the open question, I think they\nrequire prototyping, especially when it comes to how to map a\nFrameBuffer provided by an application.\n\n> > Signed-off-by: Hirokazu Honda \n> > ---\n> > include/libcamera/internal/mapped_framebuffer.h | 9 +++++++++\n> > src/libcamera/mapped_framebuffer.cpp | 17 +++++++++++++++++\n> > 2 files changed, 26 insertions(+)\n> > \n> > diff --git a/include/libcamera/internal/mapped_framebuffer.h b/include/libcamera/internal/mapped_framebuffer.h\n> > index 70ffbcbe..7deecded 100644\n> > --- a/include/libcamera/internal/mapped_framebuffer.h\n> > +++ b/include/libcamera/internal/mapped_framebuffer.h\n> > @@ -59,6 +59,15 @@ public:\n> > \n> > LIBCAMERA_FLAGS_ENABLE_OPERATORS(MappedFrameBuffer::MapFlag)\n> > \n> > +class MappedVectorBuffer : public MappedBuffer\n> > +{\n> > +public:\n> > +\tMappedVectorBuffer(std::vector> &&planes);\n> > +\n> > +private:\n> > +\tstd::vector> data_;\n> > +};\n> > +\n> > } /* namespace libcamera */\n> > \n> > #endif /* __LIBCAMERA_INTERNAL_MAPPED_FRAMEBUFFER_H__ */\n> > diff --git a/src/libcamera/mapped_framebuffer.cpp b/src/libcamera/mapped_framebuffer.cpp\n> > index 464d35fe..b09ad41a 100644\n> > --- a/src/libcamera/mapped_framebuffer.cpp\n> > +++ b/src/libcamera/mapped_framebuffer.cpp\n> > @@ -240,4 +240,21 @@ MappedFrameBuffer::MappedFrameBuffer(const FrameBuffer *buffer, MapFlags flags)\n> > \t}\n> > }\n> > \n> > +/**\n> > + * \\class MappedVectorBuffer\n> > + * \\brief Owns planes data in heap and provides the MappedBuffer interface\n> > + */\n> > +\n> > +/**\n> > + * \\brief Takes the ownership of the passed \\a planes\n> > + * \\param[in] planes the plane data that shall be owned by MappedVectorBuffer\n> > + */\n> > +MappedVectorBuffer::MappedVectorBuffer(std::vector> &&planes)\n> > +\t: data_(std::move(planes))\n> > +{\n> > +\tplanes_.reserve(data_.size());\n> > +\tfor (auto &plane : data_)\n> > +\t\tplanes_.emplace_back(plane.data(), plane.size());\n> > +}\n> > +\n> > } /* namespace libcamera */","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 E89D7C323E\n\tfor ;\n\tWed, 13 Oct 2021 03:51:24 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 648C368F4F;\n\tWed, 13 Oct 2021 05:51:24 +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 A2C3C604FF\n\tfor ;\n\tWed, 13 Oct 2021 05:51:22 +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 18EE9291;\n\tWed, 13 Oct 2021 05:51:22 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"WsxSixWk\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1634097082;\n\tbh=OcZlbCyoaXaEIR4QZYSd5UdQErWQX5+376k7o5hS1hM=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=WsxSixWkfYsccnlt7bbYO46tHk4wqaqk3vQTH1Q6OWeAzeEDiy/mAx46JFLAFiKF2\n\t6YsjuAOCyUAsonotl6VAblkFX9c1E5I79484yKn1U57ao5xsf3mljWSSAwKf8Hdb+L\n\tSIKlldkLt613Cn3Rfnkhh77EvnSGOw4y4AqXrWu4=","Date":"Wed, 13 Oct 2021 06:51:07 +0300","From":"Laurent Pinchart ","To":"Hirokazu Honda ","Message-ID":"","References":"<20210831063438.785767-1-hiroh@chromium.org>\n\t<20210831063438.785767-4-hiroh@chromium.org>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH 3/5] mapped_framebuffer: Introduce\n\tMappedVectorBuffer","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\" "}}]