Patch Detail
Show a patch.
GET /api/1.1/patches/10605/?format=api
{ "id": 10605, "url": "https://patchwork.libcamera.org/api/1.1/patches/10605/?format=api", "web_url": "https://patchwork.libcamera.org/patch/10605/", "project": { "id": 1, "url": "https://patchwork.libcamera.org/api/1.1/projects/1/?format=api", "name": "libcamera", "link_name": "libcamera", "list_id": "libcamera_core", "list_email": "libcamera-devel@lists.libcamera.org", "web_url": "", "scm_url": "", "webscm_url": "" }, "msgid": "<20201208014951.30989-2-laurent.pinchart@ideasonboard.com>", "date": "2020-12-08T01:49:51", "name": "[libcamera-devel,2/2] libcamera: v4l2_device: Return a unique pointer from fromEntityName()", "commit_ref": "e638ffde530440ec3515f40aa75a414ea1100231", "pull_url": null, "state": "accepted", "archived": false, "hash": "db082fe95f04d69076fc1dc42617ef5aaed7fcff", "submitter": { "id": 2, "url": "https://patchwork.libcamera.org/api/1.1/people/2/?format=api", "name": "Laurent Pinchart", "email": "laurent.pinchart@ideasonboard.com" }, "delegate": null, "mbox": "https://patchwork.libcamera.org/patch/10605/mbox/", "series": [ { "id": 1512, "url": "https://patchwork.libcamera.org/api/1.1/series/1512/?format=api", "web_url": "https://patchwork.libcamera.org/project/libcamera/list/?series=1512", "date": "2020-12-08T01:49:50", "name": "[libcamera-devel,1/2] libcamera: v4l2_subdevice: Return a unique pointer from fromEntityName()", "version": 1, "mbox": "https://patchwork.libcamera.org/series/1512/mbox/" } ], "comments": "https://patchwork.libcamera.org/api/patches/10605/comments/", "check": "pending", "checks": "https://patchwork.libcamera.org/api/patches/10605/checks/", "tags": {}, "headers": { "Return-Path": "<libcamera-devel-bounces@lists.libcamera.org>", "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 5E607BDB20\n\tfor <parsemail@patchwork.libcamera.org>;\n\tTue, 8 Dec 2020 01:50:34 +0000 (UTC)", "from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B4E1767E71;\n\tTue, 8 Dec 2020 02:50:33 +0100 (CET)", "from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 73C7867E6C\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tTue, 8 Dec 2020 02:50:31 +0100 (CET)", "from pendragon.lan (62-78-145-57.bb.dnainternet.fi [62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 1202D335\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tTue, 8 Dec 2020 02:50:31 +0100 (CET)" ], "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=\"QFEMgxpD\"; dkim-atps=neutral", "DKIM-Signature": "v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1607392231;\n\tbh=BYCVb7YctqWcAMzIAg1K/aYvjrkLga34S/u2Qw4KnFU=;\n\th=From:To:Subject:Date:In-Reply-To:References:From;\n\tb=QFEMgxpDdb6XAE1R74PjD/km5rawi9TG75C9tZmQHVZfPmG+NjwGWpfmKOIe6rAnp\n\tyiE15lLsoBxJkrJSTk3AlQevkc0L07tCqVLqDty11mrafbTHUHnRb7GmTeAS2BoW0M\n\tSlse3yXsLWGPN8ppdEcQSSKpOwuyGSBZA0mniAAQ=", "From": "Laurent Pinchart <laurent.pinchart@ideasonboard.com>", "To": "libcamera-devel@lists.libcamera.org", "Date": "Tue, 8 Dec 2020 03:49:51 +0200", "Message-Id": "<20201208014951.30989-2-laurent.pinchart@ideasonboard.com>", "X-Mailer": "git-send-email 2.27.0", "In-Reply-To": "<20201208014951.30989-1-laurent.pinchart@ideasonboard.com>", "References": "<20201208014951.30989-1-laurent.pinchart@ideasonboard.com>", "MIME-Version": "1.0", "Subject": "[libcamera-devel] [PATCH 2/2] libcamera: v4l2_device: Return a\n\tunique pointer from fromEntityName()", "X-BeenThere": "libcamera-devel@lists.libcamera.org", "X-Mailman-Version": "2.1.29", "Precedence": "list", "List-Id": "<libcamera-devel.lists.libcamera.org>", "List-Unsubscribe": "<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>", "List-Archive": "<https://lists.libcamera.org/pipermail/libcamera-devel/>", "List-Post": "<mailto:libcamera-devel@lists.libcamera.org>", "List-Help": "<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>", "List-Subscribe": "<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>", "Content-Type": "text/plain; charset=\"us-ascii\"", "Content-Transfer-Encoding": "7bit", "Errors-To": "libcamera-devel-bounces@lists.libcamera.org", "Sender": "\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>" }, "content": "The fromEntityName() function returns a pointer to a newly allocated\nV4L2Device instance, which must be deleted by the caller. This opens the\ndoor to memory leaks. Return a unique pointer instead, which conveys the\nAPI semantics better than a sentence in the documentation.\n\nSigned-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n---\n include/libcamera/internal/v4l2_videodevice.h | 4 ++--\n src/libcamera/pipeline/ipu3/cio2.cpp | 3 +--\n src/libcamera/pipeline/ipu3/cio2.h | 2 +-\n src/libcamera/pipeline/ipu3/imgu.cpp | 10 ++++------\n src/libcamera/pipeline/rkisp1/rkisp1.cpp | 13 +++----------\n src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 7 +------\n src/libcamera/pipeline/rkisp1/rkisp1_path.h | 3 +--\n src/libcamera/v4l2_videodevice.cpp | 10 ++++------\n test/libtest/buffer_source.cpp | 2 +-\n 9 files changed, 18 insertions(+), 36 deletions(-)", "diff": "diff --git a/include/libcamera/internal/v4l2_videodevice.h b/include/libcamera/internal/v4l2_videodevice.h\nindex 661503d1565a..529ca0e359d6 100644\n--- a/include/libcamera/internal/v4l2_videodevice.h\n+++ b/include/libcamera/internal/v4l2_videodevice.h\n@@ -210,8 +210,8 @@ public:\n \tint streamOn();\n \tint streamOff();\n \n-\tstatic V4L2VideoDevice *fromEntityName(const MediaDevice *media,\n-\t\t\t\t\t const std::string &entity);\n+\tstatic std::unique_ptr<V4L2VideoDevice>\n+\tfromEntityName(const MediaDevice *media, const std::string &entity);\n \n \tV4L2PixelFormat toV4L2PixelFormat(const PixelFormat &pixelFormat);\n \ndiff --git a/src/libcamera/pipeline/ipu3/cio2.cpp b/src/libcamera/pipeline/ipu3/cio2.cpp\nindex e43ec70fe3e4..821715e325b4 100644\n--- a/src/libcamera/pipeline/ipu3/cio2.cpp\n+++ b/src/libcamera/pipeline/ipu3/cio2.cpp\n@@ -33,13 +33,12 @@ const std::map<uint32_t, PixelFormat> mbusCodesToPixelFormat = {\n } /* namespace */\n \n CIO2Device::CIO2Device()\n-\t: sensor_(nullptr), csi2_(nullptr), output_(nullptr)\n+\t: sensor_(nullptr), csi2_(nullptr)\n {\n }\n \n CIO2Device::~CIO2Device()\n {\n-\tdelete output_;\n \tdelete csi2_;\n \tdelete sensor_;\n }\ndiff --git a/src/libcamera/pipeline/ipu3/cio2.h b/src/libcamera/pipeline/ipu3/cio2.h\nindex fa813a989fd2..0dca9673ca07 100644\n--- a/src/libcamera/pipeline/ipu3/cio2.h\n+++ b/src/libcamera/pipeline/ipu3/cio2.h\n@@ -63,7 +63,7 @@ private:\n \n \tCameraSensor *sensor_;\n \tV4L2Subdevice *csi2_;\n-\tV4L2VideoDevice *output_;\n+\tstd::unique_ptr<V4L2VideoDevice> output_;\n \n \tstd::vector<std::unique_ptr<FrameBuffer>> buffers_;\n \tstd::queue<FrameBuffer *> availableBuffers_;\ndiff --git a/src/libcamera/pipeline/ipu3/imgu.cpp b/src/libcamera/pipeline/ipu3/imgu.cpp\nindex bfe9624c7797..5b1c0318b426 100644\n--- a/src/libcamera/pipeline/ipu3/imgu.cpp\n+++ b/src/libcamera/pipeline/ipu3/imgu.cpp\n@@ -348,24 +348,22 @@ int ImgUDevice::init(MediaDevice *media, unsigned int index)\n \tif (ret)\n \t\treturn ret;\n \n-\tinput_.reset(V4L2VideoDevice::fromEntityName(media, name_ + \" input\"));\n+\tinput_ = V4L2VideoDevice::fromEntityName(media, name_ + \" input\");\n \tret = input_->open();\n \tif (ret)\n \t\treturn ret;\n \n-\toutput_.reset(V4L2VideoDevice::fromEntityName(media,\n-\t\t\t\t\t\t name_ + \" output\"));\n+\toutput_ = V4L2VideoDevice::fromEntityName(media, name_ + \" output\");\n \tret = output_->open();\n \tif (ret)\n \t\treturn ret;\n \n-\tviewfinder_.reset(V4L2VideoDevice::fromEntityName(media,\n-\t\t\t\t\t\t\t name_ + \" viewfinder\"));\n+\tviewfinder_ = V4L2VideoDevice::fromEntityName(media, name_ + \" viewfinder\");\n \tret = viewfinder_->open();\n \tif (ret)\n \t\treturn ret;\n \n-\tstat_.reset(V4L2VideoDevice::fromEntityName(media, name_ + \" 3a stat\"));\n+\tstat_ = V4L2VideoDevice::fromEntityName(media, name_ + \" 3a stat\");\n \tret = stat_->open();\n \tif (ret)\n \t\treturn ret;\ndiff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\nindex bcfe6c0514ab..064ab01057f4 100644\n--- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n+++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n@@ -178,7 +178,6 @@ class PipelineHandlerRkISP1 : public PipelineHandler\n {\n public:\n \tPipelineHandlerRkISP1(CameraManager *manager);\n-\t~PipelineHandlerRkISP1();\n \n \tCameraConfiguration *generateConfiguration(Camera *camera,\n \t\tconst StreamRoles &roles) override;\n@@ -218,8 +217,8 @@ private:\n \n \tMediaDevice *media_;\n \tstd::unique_ptr<V4L2Subdevice> isp_;\n-\tV4L2VideoDevice *param_;\n-\tV4L2VideoDevice *stat_;\n+\tstd::unique_ptr<V4L2VideoDevice> param_;\n+\tstd::unique_ptr<V4L2VideoDevice> stat_;\n \n \tRkISP1MainPath mainPath_;\n \tRkISP1SelfPath selfPath_;\n@@ -599,16 +598,10 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n }\n \n PipelineHandlerRkISP1::PipelineHandlerRkISP1(CameraManager *manager)\n-\t: PipelineHandler(manager), param_(nullptr), stat_(nullptr)\n+\t: PipelineHandler(manager)\n {\n }\n \n-PipelineHandlerRkISP1::~PipelineHandlerRkISP1()\n-{\n-\tdelete param_;\n-\tdelete stat_;\n-}\n-\n /* -----------------------------------------------------------------------------\n * Pipeline Operations\n */\ndiff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp\nindex e05d9dd657cd..25f482eb8d8e 100644\n--- a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp\n+++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp\n@@ -24,15 +24,10 @@ RkISP1Path::RkISP1Path(const char *name, const Span<const PixelFormat> &formats,\n \t\t const Size &minResolution, const Size &maxResolution)\n \t: name_(name), running_(false), formats_(formats),\n \t minResolution_(minResolution), maxResolution_(maxResolution),\n-\t video_(nullptr), link_(nullptr)\n+\t link_(nullptr)\n {\n }\n \n-RkISP1Path::~RkISP1Path()\n-{\n-\tdelete video_;\n-}\n-\n bool RkISP1Path::init(MediaDevice *media)\n {\n \tstd::string resizer = std::string(\"rkisp1_resizer_\") + name_ + \"path\";\ndiff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.h b/src/libcamera/pipeline/rkisp1/rkisp1_path.h\nindex f06ac5a731cc..3b3e37d258d0 100644\n--- a/src/libcamera/pipeline/rkisp1/rkisp1_path.h\n+++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.h\n@@ -31,7 +31,6 @@ class RkISP1Path\n public:\n \tRkISP1Path(const char *name, const Span<const PixelFormat> &formats,\n \t\t const Size &minResolution, const Size &maxResolution);\n-\t~RkISP1Path();\n \n \tbool init(MediaDevice *media);\n \n@@ -67,7 +66,7 @@ private:\n \tconst Size maxResolution_;\n \n \tstd::unique_ptr<V4L2Subdevice> resizer_;\n-\tV4L2VideoDevice *video_;\n+\tstd::unique_ptr<V4L2VideoDevice> video_;\n \tMediaLink *link_;\n };\n \ndiff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp\nindex e76fe2dd1f3a..e2b582842a9b 100644\n--- a/src/libcamera/v4l2_videodevice.cpp\n+++ b/src/libcamera/v4l2_videodevice.cpp\n@@ -1598,19 +1598,17 @@ int V4L2VideoDevice::streamOff()\n * \\param[in] media The media device where the entity is registered\n * \\param[in] entity The media entity name\n *\n- * Releasing memory of the newly created instance is responsibility of the\n- * caller of this function.\n- *\n * \\return A newly created V4L2VideoDevice on success, nullptr otherwise\n */\n-V4L2VideoDevice *V4L2VideoDevice::fromEntityName(const MediaDevice *media,\n-\t\t\t\t\t\t const std::string &entity)\n+std::unique_ptr<V4L2VideoDevice>\n+V4L2VideoDevice::fromEntityName(const MediaDevice *media,\n+\t\t\t\tconst std::string &entity)\n {\n \tMediaEntity *mediaEntity = media->getEntityByName(entity);\n \tif (!mediaEntity)\n \t\treturn nullptr;\n \n-\treturn new V4L2VideoDevice(mediaEntity);\n+\treturn std::make_unique<V4L2VideoDevice>(mediaEntity);\n }\n \n /**\ndiff --git a/test/libtest/buffer_source.cpp b/test/libtest/buffer_source.cpp\nindex ee87c8cd821c..73563f2fc39d 100644\n--- a/test/libtest/buffer_source.cpp\n+++ b/test/libtest/buffer_source.cpp\n@@ -50,7 +50,7 @@ int BufferSource::allocate(const StreamConfiguration &config)\n \t\treturn TestSkip;\n \t}\n \n-\tstd::unique_ptr<V4L2VideoDevice> video{ V4L2VideoDevice::fromEntityName(media_.get(), videoDeviceName) };\n+\tstd::unique_ptr<V4L2VideoDevice> video = V4L2VideoDevice::fromEntityName(media_.get(), videoDeviceName);\n \tif (!video) {\n \t\tstd::cout << \"Failed to get video device from entity \"\n \t\t\t << videoDeviceName << std::endl;\n", "prefixes": [ "libcamera-devel", "2/2" ] }