{"id":10605,"url":"https://patchwork.libcamera.org/api/1.1/patches/10605/?format=json","web_url":"https://patchwork.libcamera.org/patch/10605/","project":{"id":1,"url":"https://patchwork.libcamera.org/api/1.1/projects/1/?format=json","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=json","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=json","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":"","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 ;\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 ;\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 ;\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 ","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":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "},"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 \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\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 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 output_;\n \n \tstd::vector> buffers_;\n \tstd::queue 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 isp_;\n-\tV4L2VideoDevice *param_;\n-\tV4L2VideoDevice *stat_;\n+\tstd::unique_ptr param_;\n+\tstd::unique_ptr 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 &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 &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 resizer_;\n-\tV4L2VideoDevice *video_;\n+\tstd::unique_ptr 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\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(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 video{ V4L2VideoDevice::fromEntityName(media_.get(), videoDeviceName) };\n+\tstd::unique_ptr 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"]}