[{"id":556,"web_url":"https://patchwork.libcamera.org/comment/556/","msgid":"<20190124170004.GX4127@bigcity.dyn.berto.se>","date":"2019-01-24T17:00:04","subject":"Re: [libcamera-devel] [PATCH 04/10] libcamera: device_enumerator:\n\tReference-count MediaDevice instances","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Laurent,\n\nThanks for your work.\n\nOn 2019-01-24 12:16:45 +0200, Laurent Pinchart wrote:\n> The MediaDevice class will be the entry point to hot-unplug, as it\n> corresponds to the kernel devices that will report device removal\n> events. The class will signal media device disconnection to pipeline\n> handlers, which will clean up resources as a result.\n> \n> This can't be performed synchronously as references may exist to the\n> related Camera objects in applications. The MediaDevice object thus\n> needs to be reference-counted in order to support unplugging, as\n> otherwise pipeline handlers would be required to drop all the references\n> to the media device they have borrowed synchronously with the\n> disconnection signal handler, which would be very error prone (if even\n> possible at all in a sane way).\n> \n> Handle MedieDevice instances with std::shared_ptr<> to support this.\n> \n> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n\nReviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>\n\n> ---\n>  src/libcamera/device_enumerator.cpp          | 23 ++++++++++----------\n>  src/libcamera/include/device_enumerator.h    |  4 ++--\n>  src/libcamera/pipeline/ipu3/ipu3.cpp         | 11 ++++------\n>  src/libcamera/pipeline/uvcvideo.cpp          |  4 ++--\n>  src/libcamera/pipeline/vimc.cpp              |  4 ++--\n>  test/media_device/media_device_link_test.cpp |  2 +-\n>  test/pipeline/ipu3/ipu3_pipeline_test.cpp    |  2 +-\n>  7 files changed, 23 insertions(+), 27 deletions(-)\n> \n> diff --git a/src/libcamera/device_enumerator.cpp b/src/libcamera/device_enumerator.cpp\n> index 8100972a8d04..149ffbf9aea6 100644\n> --- a/src/libcamera/device_enumerator.cpp\n> +++ b/src/libcamera/device_enumerator.cpp\n> @@ -166,12 +166,10 @@ std::unique_ptr<DeviceEnumerator> DeviceEnumerator::create()\n>  \n>  DeviceEnumerator::~DeviceEnumerator()\n>  {\n> -\tfor (MediaDevice *dev : devices_) {\n> -\t\tif (dev->busy())\n> +\tfor (std::shared_ptr<MediaDevice> media : devices_) {\n> +\t\tif (media->busy())\n>  \t\t\tLOG(DeviceEnumerator, Error)\n>  \t\t\t\t<< \"Removing media device while still in use\";\n> -\n> -\t\tdelete dev;\n>  \t}\n>  }\n>  \n> @@ -211,7 +209,7 @@ DeviceEnumerator::~DeviceEnumerator()\n>   */\n>  int DeviceEnumerator::addDevice(const std::string &deviceNode)\n>  {\n> -\tMediaDevice *media = new MediaDevice(deviceNode);\n> +\tstd::shared_ptr<MediaDevice> media = std::make_shared<MediaDevice>(deviceNode);\n>  \n>  \tint ret = media->open();\n>  \tif (ret < 0)\n> @@ -243,9 +241,10 @@ int DeviceEnumerator::addDevice(const std::string &deviceNode)\n>  \t\t\treturn ret;\n>  \t}\n>  \n> -\tdevices_.push_back(media);\n>  \tmedia->close();\n>  \n> +\tdevices_.push_back(std::move(media));\n> +\n>  \treturn 0;\n>  }\n>  \n> @@ -260,17 +259,17 @@ int DeviceEnumerator::addDevice(const std::string &deviceNode)\n>   *\n>   * \\return pointer to the matching MediaDevice, or nullptr if no match is found\n>   */\n> -MediaDevice *DeviceEnumerator::search(const DeviceMatch &dm)\n> +std::shared_ptr<MediaDevice> DeviceEnumerator::search(const DeviceMatch &dm)\n>  {\n> -\tfor (MediaDevice *dev : devices_) {\n> -\t\tif (dev->busy())\n> +\tfor (std::shared_ptr<MediaDevice> media : devices_) {\n> +\t\tif (media->busy())\n>  \t\t\tcontinue;\n>  \n> -\t\tif (dm.match(dev)) {\n> +\t\tif (dm.match(media.get())) {\n>  \t\t\tLOG(DeviceEnumerator, Debug)\n>  \t\t\t\t<< \"Successful match for media device \\\"\"\n> -\t\t\t\t<< dev->driver() << \"\\\"\";\n> -\t\t\treturn dev;\n> +\t\t\t\t<< media->driver() << \"\\\"\";\n> +\t\t\treturn std::move(media);\n>  \t\t}\n>  \t}\n>  \n> diff --git a/src/libcamera/include/device_enumerator.h b/src/libcamera/include/device_enumerator.h\n> index 40c7750b49fc..3f87a6255303 100644\n> --- a/src/libcamera/include/device_enumerator.h\n> +++ b/src/libcamera/include/device_enumerator.h\n> @@ -42,13 +42,13 @@ public:\n>  \tvirtual int init() = 0;\n>  \tvirtual int enumerate() = 0;\n>  \n> -\tMediaDevice *search(const DeviceMatch &dm);\n> +\tstd::shared_ptr<MediaDevice> search(const DeviceMatch &dm);\n>  \n>  protected:\n>  \tint addDevice(const std::string &deviceNode);\n>  \n>  private:\n> -\tstd::vector<MediaDevice *> devices_;\n> +\tstd::vector<std::shared_ptr<MediaDevice>> devices_;\n>  \n>  \tvirtual std::string lookupDeviceNode(int major, int minor) = 0;\n>  };\n> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> index 13ff7da4c99e..9831f74fe53f 100644\n> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> @@ -29,8 +29,8 @@ public:\n>  \tbool match(DeviceEnumerator *enumerator);\n>  \n>  private:\n> -\tMediaDevice *cio2_;\n> -\tMediaDevice *imgu_;\n> +\tstd::shared_ptr<MediaDevice> cio2_;\n> +\tstd::shared_ptr<MediaDevice> imgu_;\n>  \n>  \tvoid registerCameras();\n>  };\n> @@ -47,9 +47,6 @@ PipelineHandlerIPU3::~PipelineHandlerIPU3()\n>  \n>  \tif (imgu_)\n>  \t\timgu_->release();\n> -\n> -\tcio2_ = nullptr;\n> -\timgu_ = nullptr;\n>  }\n>  \n>  bool PipelineHandlerIPU3::match(DeviceEnumerator *enumerator)\n> @@ -78,11 +75,11 @@ bool PipelineHandlerIPU3::match(DeviceEnumerator *enumerator)\n>  \timgu_dm.add(\"ipu3-imgu 1 viewfinder\");\n>  \timgu_dm.add(\"ipu3-imgu 1 3a stat\");\n>  \n> -\tcio2_ = enumerator->search(cio2_dm);\n> +\tcio2_ = std::move(enumerator->search(cio2_dm));\n>  \tif (!cio2_)\n>  \t\treturn false;\n>  \n> -\timgu_ = enumerator->search(imgu_dm);\n> +\timgu_ = std::move(enumerator->search(imgu_dm));\n>  \tif (!imgu_)\n>  \t\treturn false;\n>  \n> diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp\n> index 3ebc074093ab..73bad6714bb4 100644\n> --- a/src/libcamera/pipeline/uvcvideo.cpp\n> +++ b/src/libcamera/pipeline/uvcvideo.cpp\n> @@ -23,7 +23,7 @@ public:\n>  \tbool match(DeviceEnumerator *enumerator);\n>  \n>  private:\n> -\tMediaDevice *dev_;\n> +\tstd::shared_ptr<MediaDevice> dev_;\n>  };\n>  \n>  PipelineHandlerUVC::PipelineHandlerUVC(CameraManager *manager)\n> @@ -41,7 +41,7 @@ bool PipelineHandlerUVC::match(DeviceEnumerator *enumerator)\n>  {\n>  \tDeviceMatch dm(\"uvcvideo\");\n>  \n> -\tdev_ = enumerator->search(dm);\n> +\tdev_ = std::move(enumerator->search(dm));\n>  \n>  \tif (!dev_)\n>  \t\treturn false;\n> diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp\n> index 68bfe9b12ab6..521b20d3a120 100644\n> --- a/src/libcamera/pipeline/vimc.cpp\n> +++ b/src/libcamera/pipeline/vimc.cpp\n> @@ -23,7 +23,7 @@ public:\n>  \tbool match(DeviceEnumerator *enumerator);\n>  \n>  private:\n> -\tMediaDevice *dev_;\n> +\tstd::shared_ptr<MediaDevice> dev_;\n>  };\n>  \n>  PipeHandlerVimc::PipeHandlerVimc(CameraManager *manager)\n> @@ -51,7 +51,7 @@ bool PipeHandlerVimc::match(DeviceEnumerator *enumerator)\n>  \tdm.add(\"RGB/YUV Input\");\n>  \tdm.add(\"Scaler\");\n>  \n> -\tdev_ = enumerator->search(dm);\n> +\tdev_ = std::move(enumerator->search(dm));\n>  \tif (!dev_)\n>  \t\treturn false;\n>  \n> diff --git a/test/media_device/media_device_link_test.cpp b/test/media_device/media_device_link_test.cpp\n> index ac5b632f8ed1..58a55cdfee63 100644\n> --- a/test/media_device/media_device_link_test.cpp\n> +++ b/test/media_device/media_device_link_test.cpp\n> @@ -240,7 +240,7 @@ class MediaDeviceLinkTest : public Test\n>  \n>  private:\n>  \tunique_ptr<DeviceEnumerator> enumerator;\n> -\tMediaDevice *dev_;\n> +\tshared_ptr<MediaDevice> dev_;\n>  };\n>  \n>  TEST_REGISTER(MediaDeviceLinkTest);\n> diff --git a/test/pipeline/ipu3/ipu3_pipeline_test.cpp b/test/pipeline/ipu3/ipu3_pipeline_test.cpp\n> index 482c12499a86..953f0233cde4 100644\n> --- a/test/pipeline/ipu3/ipu3_pipeline_test.cpp\n> +++ b/test/pipeline/ipu3/ipu3_pipeline_test.cpp\n> @@ -65,7 +65,7 @@ int IPU3PipelineTest::init()\n>  \t\treturn TestSkip;\n>  \t}\n>  \n> -\tMediaDevice *cio2 = enumerator->search(cio2_dm);\n> +\tstd::shared_ptr<MediaDevice> cio2 = enumerator->search(cio2_dm);\n>  \tif (!cio2) {\n>  \t\tcerr << \"Failed to find IPU3 CIO2: test skip\" << endl;\n>  \t\treturn TestSkip;\n> -- \n> Regards,\n> \n> Laurent Pinchart\n> \n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"<niklas.soderlund@ragnatech.se>","Received":["from mail-lf1-x142.google.com (mail-lf1-x142.google.com\n\t[IPv6:2a00:1450:4864:20::142])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B9B1060C82\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tThu, 24 Jan 2019 18:00:06 +0100 (CET)","by mail-lf1-x142.google.com with SMTP id e26so4855503lfc.2\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tThu, 24 Jan 2019 09:00:06 -0800 (PST)","from localhost (89-233-230-99.cust.bredband2.com. [89.233.230.99])\n\tby smtp.gmail.com with ESMTPSA id\n\te19-v6sm1091005ljf.67.2019.01.24.09.00.04\n\t(version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256);\n\tThu, 24 Jan 2019 09:00:05 -0800 (PST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ragnatech-se.20150623.gappssmtp.com; s=20150623;\n\th=date:from:to:cc:subject:message-id:references:mime-version\n\t:content-disposition:content-transfer-encoding:in-reply-to\n\t:user-agent; bh=mRWxyX1WVxs6mR2yGTC5zf96g4KhbYvnZnux6yJlIc8=;\n\tb=fnjyvW+trdM2j5UIYP/mYrdBwv3/NMwhS6QEGqnL2N6t94cpP88pWZ/QQxSy4b5jid\n\tKzNRrSysxrHAEI9JwbtpD5zRcsVmdl7qMImfQj5KzaLbv2x96cL5KayjewbxVERJBqKG\n\tRsKOll722HoIX8H7L3swTs3rfnj6rDTZsoKUEPmshs7syMgn/5MK5tPZ0Gq5JGxyMUsm\n\t4prOz80Ex8QPbAHUhMSZv8Dmo7deiuXxnUjwbRWtMNRpVrIUHyltWtnZMMaTV/L44Rms\n\tnmlQxEex4GDAQ3rPrCy9KblvpWcgiiDODPBY8/tdaPXUBndK2gCXcPcyUbF1f2X/Wvkl\n\tjFoA==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:date:from:to:cc:subject:message-id:references\n\t:mime-version:content-disposition:content-transfer-encoding\n\t:in-reply-to:user-agent;\n\tbh=mRWxyX1WVxs6mR2yGTC5zf96g4KhbYvnZnux6yJlIc8=;\n\tb=h8lHk3hBN+oARue9nUHRFNDFOP1oe/R0GrzJk4bMfc2hyikj675BRUG0oC81s1CTL4\n\tjbcJ7fIyHGHNa5FARolHddya2O6VLfK6pDn8gv46/IU90tnyQcVjJXJwytASEIJLygbs\n\tsDIfq1pwsGI/vRI0XevOQTUGBokayAXpOFCVihWPPmTtp4aNBK9LRUAC/+LEZSvO9bMx\n\taEXwo+ZivHOJH24Qi1LmcYVoERfxOoTPRwyNi0vuo9JrBHbPtIPhUDBoA+A8LFILw4oL\n\t5Sshg02lKlpRFVYzzmqvZdETq70tBRZBDrS9mRBZD8sKaQH+ERvXu76zT8UO+rPdl2fF\n\tViig==","X-Gm-Message-State":"AJcUukerDK145q36XbgjNOK0jFdkmGuOIOEiS4wnKA2bShsxLLi7DCrp\n\tSA4tdOx2w/GIHDxpN2EWtK36bMEu5CM=","X-Google-Smtp-Source":"ALg8bN7keDOFXj97Y1adfZ+tI/6HL4RqyylBt9vS6z9/VqTqi8BGUfv5TBDJEbfI4vEzwSbuCy0H3A==","X-Received":"by 2002:a19:10a4:: with SMTP id 36mr5658469lfq.60.1548349205764; \n\tThu, 24 Jan 2019 09:00:05 -0800 (PST)","Date":"Thu, 24 Jan 2019 18:00:04 +0100","From":"Niklas =?iso-8859-1?q?S=F6derlund?= <niklas.soderlund@ragnatech.se>","To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190124170004.GX4127@bigcity.dyn.berto.se>","References":"<20190124101651.9993-1-laurent.pinchart@ideasonboard.com>\n\t<20190124101651.9993-5-laurent.pinchart@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=iso-8859-1","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20190124101651.9993-5-laurent.pinchart@ideasonboard.com>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH 04/10] libcamera: device_enumerator:\n\tReference-count MediaDevice instances","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","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>","X-List-Received-Date":"Thu, 24 Jan 2019 17:00:07 -0000"}},{"id":570,"web_url":"https://patchwork.libcamera.org/comment/570/","msgid":"<20190124231641.ulw42irklzizewox@uno.localdomain>","date":"2019-01-24T23:16:41","subject":"Re: [libcamera-devel] [PATCH 04/10] libcamera: device_enumerator:\n\tReference-count MediaDevice instances","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent,\n\nOn Thu, Jan 24, 2019 at 12:16:45PM +0200, Laurent Pinchart wrote:\n> The MediaDevice class will be the entry point to hot-unplug, as it\n> corresponds to the kernel devices that will report device removal\n> events. The class will signal media device disconnection to pipeline\n> handlers, which will clean up resources as a result.\n>\n> This can't be performed synchronously as references may exist to the\n> related Camera objects in applications. The MediaDevice object thus\n> needs to be reference-counted in order to support unplugging, as\n> otherwise pipeline handlers would be required to drop all the references\n> to the media device they have borrowed synchronously with the\n> disconnection signal handler, which would be very error prone (if even\n> possible at all in a sane way).\n>\n> Handle MedieDevice instances with std::shared_ptr<> to support this.\n>\n> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n> ---\n>  src/libcamera/device_enumerator.cpp          | 23 ++++++++++----------\n>  src/libcamera/include/device_enumerator.h    |  4 ++--\n>  src/libcamera/pipeline/ipu3/ipu3.cpp         | 11 ++++------\n>  src/libcamera/pipeline/uvcvideo.cpp          |  4 ++--\n>  src/libcamera/pipeline/vimc.cpp              |  4 ++--\n>  test/media_device/media_device_link_test.cpp |  2 +-\n>  test/pipeline/ipu3/ipu3_pipeline_test.cpp    |  2 +-\n>  7 files changed, 23 insertions(+), 27 deletions(-)\n>\n> diff --git a/src/libcamera/device_enumerator.cpp b/src/libcamera/device_enumerator.cpp\n> index 8100972a8d04..149ffbf9aea6 100644\n> --- a/src/libcamera/device_enumerator.cpp\n> +++ b/src/libcamera/device_enumerator.cpp\n> @@ -166,12 +166,10 @@ std::unique_ptr<DeviceEnumerator> DeviceEnumerator::create()\n>\n>  DeviceEnumerator::~DeviceEnumerator()\n>  {\n> -\tfor (MediaDevice *dev : devices_) {\n> -\t\tif (dev->busy())\n> +\tfor (std::shared_ptr<MediaDevice> media : devices_) {\n> +\t\tif (media->busy())\n\nNot sure I got this. The assignement to media in the range-based for\nloop creates a copy, right? Does the device enumerator need to drop\nthat? They are added to a vector of shared_ptr with\n\n\tdevices_.push_back(std::move(media));\n\nSo I'm expecting this to happen. What have I missed?\n\nThanks\n  j\n\n\n\n>  \t\t\tLOG(DeviceEnumerator, Error)\n>  \t\t\t\t<< \"Removing media device while still in use\";\n> -\n> -\t\tdelete dev;\n>  \t}\n>  }\n>\n> @@ -211,7 +209,7 @@ DeviceEnumerator::~DeviceEnumerator()\n>   */\n>  int DeviceEnumerator::addDevice(const std::string &deviceNode)\n>  {\n> -\tMediaDevice *media = new MediaDevice(deviceNode);\n> +\tstd::shared_ptr<MediaDevice> media = std::make_shared<MediaDevice>(deviceNode);\n>\n>  \tint ret = media->open();\n>  \tif (ret < 0)\n> @@ -243,9 +241,10 @@ int DeviceEnumerator::addDevice(const std::string &deviceNode)\n>  \t\t\treturn ret;\n>  \t}\n>\n> -\tdevices_.push_back(media);\n>  \tmedia->close();\n>\n> +\tdevices_.push_back(std::move(media));\n> +\n>  \treturn 0;\n>  }\n>\n> @@ -260,17 +259,17 @@ int DeviceEnumerator::addDevice(const std::string &deviceNode)\n>   *\n>   * \\return pointer to the matching MediaDevice, or nullptr if no match is found\n>   */\n> -MediaDevice *DeviceEnumerator::search(const DeviceMatch &dm)\n> +std::shared_ptr<MediaDevice> DeviceEnumerator::search(const DeviceMatch &dm)\n>  {\n> -\tfor (MediaDevice *dev : devices_) {\n> -\t\tif (dev->busy())\n> +\tfor (std::shared_ptr<MediaDevice> media : devices_) {\n> +\t\tif (media->busy())\n>  \t\t\tcontinue;\n>\n> -\t\tif (dm.match(dev)) {\n> +\t\tif (dm.match(media.get())) {\n>  \t\t\tLOG(DeviceEnumerator, Debug)\n>  \t\t\t\t<< \"Successful match for media device \\\"\"\n> -\t\t\t\t<< dev->driver() << \"\\\"\";\n> -\t\t\treturn dev;\n> +\t\t\t\t<< media->driver() << \"\\\"\";\n> +\t\t\treturn std::move(media);\n>  \t\t}\n>  \t}\n>\n> diff --git a/src/libcamera/include/device_enumerator.h b/src/libcamera/include/device_enumerator.h\n> index 40c7750b49fc..3f87a6255303 100644\n> --- a/src/libcamera/include/device_enumerator.h\n> +++ b/src/libcamera/include/device_enumerator.h\n> @@ -42,13 +42,13 @@ public:\n>  \tvirtual int init() = 0;\n>  \tvirtual int enumerate() = 0;\n>\n> -\tMediaDevice *search(const DeviceMatch &dm);\n> +\tstd::shared_ptr<MediaDevice> search(const DeviceMatch &dm);\n>\n>  protected:\n>  \tint addDevice(const std::string &deviceNode);\n>\n>  private:\n> -\tstd::vector<MediaDevice *> devices_;\n> +\tstd::vector<std::shared_ptr<MediaDevice>> devices_;\n>\n>  \tvirtual std::string lookupDeviceNode(int major, int minor) = 0;\n>  };\n> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> index 13ff7da4c99e..9831f74fe53f 100644\n> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> @@ -29,8 +29,8 @@ public:\n>  \tbool match(DeviceEnumerator *enumerator);\n>\n>  private:\n> -\tMediaDevice *cio2_;\n> -\tMediaDevice *imgu_;\n> +\tstd::shared_ptr<MediaDevice> cio2_;\n> +\tstd::shared_ptr<MediaDevice> imgu_;\n>\n>  \tvoid registerCameras();\n>  };\n> @@ -47,9 +47,6 @@ PipelineHandlerIPU3::~PipelineHandlerIPU3()\n>\n>  \tif (imgu_)\n>  \t\timgu_->release();\n> -\n> -\tcio2_ = nullptr;\n> -\timgu_ = nullptr;\n>  }\n>\n>  bool PipelineHandlerIPU3::match(DeviceEnumerator *enumerator)\n> @@ -78,11 +75,11 @@ bool PipelineHandlerIPU3::match(DeviceEnumerator *enumerator)\n>  \timgu_dm.add(\"ipu3-imgu 1 viewfinder\");\n>  \timgu_dm.add(\"ipu3-imgu 1 3a stat\");\n>\n> -\tcio2_ = enumerator->search(cio2_dm);\n> +\tcio2_ = std::move(enumerator->search(cio2_dm));\n>  \tif (!cio2_)\n>  \t\treturn false;\n>\n> -\timgu_ = enumerator->search(imgu_dm);\n> +\timgu_ = std::move(enumerator->search(imgu_dm));\n>  \tif (!imgu_)\n>  \t\treturn false;\n>\n> diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp\n> index 3ebc074093ab..73bad6714bb4 100644\n> --- a/src/libcamera/pipeline/uvcvideo.cpp\n> +++ b/src/libcamera/pipeline/uvcvideo.cpp\n> @@ -23,7 +23,7 @@ public:\n>  \tbool match(DeviceEnumerator *enumerator);\n>\n>  private:\n> -\tMediaDevice *dev_;\n> +\tstd::shared_ptr<MediaDevice> dev_;\n>  };\n>\n>  PipelineHandlerUVC::PipelineHandlerUVC(CameraManager *manager)\n> @@ -41,7 +41,7 @@ bool PipelineHandlerUVC::match(DeviceEnumerator *enumerator)\n>  {\n>  \tDeviceMatch dm(\"uvcvideo\");\n>\n> -\tdev_ = enumerator->search(dm);\n> +\tdev_ = std::move(enumerator->search(dm));\n>\n>  \tif (!dev_)\n>  \t\treturn false;\n> diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp\n> index 68bfe9b12ab6..521b20d3a120 100644\n> --- a/src/libcamera/pipeline/vimc.cpp\n> +++ b/src/libcamera/pipeline/vimc.cpp\n> @@ -23,7 +23,7 @@ public:\n>  \tbool match(DeviceEnumerator *enumerator);\n>\n>  private:\n> -\tMediaDevice *dev_;\n> +\tstd::shared_ptr<MediaDevice> dev_;\n>  };\n>\n>  PipeHandlerVimc::PipeHandlerVimc(CameraManager *manager)\n> @@ -51,7 +51,7 @@ bool PipeHandlerVimc::match(DeviceEnumerator *enumerator)\n>  \tdm.add(\"RGB/YUV Input\");\n>  \tdm.add(\"Scaler\");\n>\n> -\tdev_ = enumerator->search(dm);\n> +\tdev_ = std::move(enumerator->search(dm));\n>  \tif (!dev_)\n>  \t\treturn false;\n>\n> diff --git a/test/media_device/media_device_link_test.cpp b/test/media_device/media_device_link_test.cpp\n> index ac5b632f8ed1..58a55cdfee63 100644\n> --- a/test/media_device/media_device_link_test.cpp\n> +++ b/test/media_device/media_device_link_test.cpp\n> @@ -240,7 +240,7 @@ class MediaDeviceLinkTest : public Test\n>\n>  private:\n>  \tunique_ptr<DeviceEnumerator> enumerator;\n> -\tMediaDevice *dev_;\n> +\tshared_ptr<MediaDevice> dev_;\n>  };\n>\n>  TEST_REGISTER(MediaDeviceLinkTest);\n> diff --git a/test/pipeline/ipu3/ipu3_pipeline_test.cpp b/test/pipeline/ipu3/ipu3_pipeline_test.cpp\n> index 482c12499a86..953f0233cde4 100644\n> --- a/test/pipeline/ipu3/ipu3_pipeline_test.cpp\n> +++ b/test/pipeline/ipu3/ipu3_pipeline_test.cpp\n> @@ -65,7 +65,7 @@ int IPU3PipelineTest::init()\n>  \t\treturn TestSkip;\n>  \t}\n>\n> -\tMediaDevice *cio2 = enumerator->search(cio2_dm);\n> +\tstd::shared_ptr<MediaDevice> cio2 = enumerator->search(cio2_dm);\n>  \tif (!cio2) {\n>  \t\tcerr << \"Failed to find IPU3 CIO2: test skip\" << endl;\n>  \t\treturn TestSkip;\n> --\n> Regards,\n>\n> Laurent Pinchart\n>\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"<jacopo@jmondi.org>","Received":["from relay8-d.mail.gandi.net (relay8-d.mail.gandi.net\n\t[217.70.183.201])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A121C60C82\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri, 25 Jan 2019 00:16:28 +0100 (CET)","from uno.localdomain (2-224-242-101.ip172.fastwebnet.it\n\t[2.224.242.101]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay8-d.mail.gandi.net (Postfix) with ESMTPSA id 24B691BF205;\n\tThu, 24 Jan 2019 23:16:27 +0000 (UTC)"],"X-Originating-IP":"2.224.242.101","Date":"Fri, 25 Jan 2019 00:16:41 +0100","From":"Jacopo Mondi <jacopo@jmondi.org>","To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190124231641.ulw42irklzizewox@uno.localdomain>","References":"<20190124101651.9993-1-laurent.pinchart@ideasonboard.com>\n\t<20190124101651.9993-5-laurent.pinchart@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"ur27fwleuoizppqp\"","Content-Disposition":"inline","In-Reply-To":"<20190124101651.9993-5-laurent.pinchart@ideasonboard.com>","User-Agent":"NeoMutt/20180716","Subject":"Re: [libcamera-devel] [PATCH 04/10] libcamera: device_enumerator:\n\tReference-count MediaDevice instances","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","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>","X-List-Received-Date":"Thu, 24 Jan 2019 23:16:28 -0000"}},{"id":573,"web_url":"https://patchwork.libcamera.org/comment/573/","msgid":"<20190124235508.GB4399@pendragon.ideasonboard.com>","date":"2019-01-24T23:55:08","subject":"Re: [libcamera-devel] [PATCH 04/10] libcamera: device_enumerator:\n\tReference-count MediaDevice instances","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Fri, Jan 25, 2019 at 12:16:41AM +0100, Jacopo Mondi wrote:\n> On Thu, Jan 24, 2019 at 12:16:45PM +0200, Laurent Pinchart wrote:\n> > The MediaDevice class will be the entry point to hot-unplug, as it\n> > corresponds to the kernel devices that will report device removal\n> > events. The class will signal media device disconnection to pipeline\n> > handlers, which will clean up resources as a result.\n> >\n> > This can't be performed synchronously as references may exist to the\n> > related Camera objects in applications. The MediaDevice object thus\n> > needs to be reference-counted in order to support unplugging, as\n> > otherwise pipeline handlers would be required to drop all the references\n> > to the media device they have borrowed synchronously with the\n> > disconnection signal handler, which would be very error prone (if even\n> > possible at all in a sane way).\n> >\n> > Handle MedieDevice instances with std::shared_ptr<> to support this.\n> >\n> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n> > ---\n> >  src/libcamera/device_enumerator.cpp          | 23 ++++++++++----------\n> >  src/libcamera/include/device_enumerator.h    |  4 ++--\n> >  src/libcamera/pipeline/ipu3/ipu3.cpp         | 11 ++++------\n> >  src/libcamera/pipeline/uvcvideo.cpp          |  4 ++--\n> >  src/libcamera/pipeline/vimc.cpp              |  4 ++--\n> >  test/media_device/media_device_link_test.cpp |  2 +-\n> >  test/pipeline/ipu3/ipu3_pipeline_test.cpp    |  2 +-\n> >  7 files changed, 23 insertions(+), 27 deletions(-)\n> >\n> > diff --git a/src/libcamera/device_enumerator.cpp b/src/libcamera/device_enumerator.cpp\n> > index 8100972a8d04..149ffbf9aea6 100644\n> > --- a/src/libcamera/device_enumerator.cpp\n> > +++ b/src/libcamera/device_enumerator.cpp\n> > @@ -166,12 +166,10 @@ std::unique_ptr<DeviceEnumerator> DeviceEnumerator::create()\n> >\n> >  DeviceEnumerator::~DeviceEnumerator()\n> >  {\n> > -\tfor (MediaDevice *dev : devices_) {\n> > -\t\tif (dev->busy())\n> > +\tfor (std::shared_ptr<MediaDevice> media : devices_) {\n> > +\t\tif (media->busy())\n> \n> Not sure I got this. The assignement to media in the range-based for\n> loop creates a copy, right? Does the device enumerator need to drop\n> that? They are added to a vector of shared_ptr with\n> \n> \tdevices_.push_back(std::move(media));\n> \n> So I'm expecting this to happen. What have I missed?\n\nIt helps to look at how range-based for loops are implemented. The C++\nstandard explains they are equivalent to the following code.\n\n{\n\tauto && __range = range_expression ;\n\tfor (auto __begin = begin_expr, __end = end_expr; __begin != __end; ++__begin) {\n\t\trange_declaration = *__begin;\n\t\tloop_statement\n\t}\n}\n\nwhich, when applied to our code, produces\n\n1. {\n2. \tauto && __range = devices_;\n3. \tfor (auto __begin = begin(__range), __end = end(__range); __begin != __end; ++__begin) {\n4. \t\tstd::shared_ptr<MediaDevice> media = *__begin;\n5. \t\tif (media->busy())\n6.   \t\t\tLOG(DeviceEnumerator, Error)\n7.   \t\t\t\t<< \"Removing media device while still in use\";\n8. \t}\n9. }\n\nOn line 2 a reference to devices_ is created (as an rvalue reference as\nrange_expression could be the return of a function, in which case using\nan lvalue would generate an lvalue reference to temporary object error).\n__range thus references devices_, it doesn't create a copy of the\nvector.\n\nOn line 3 we use a classic iterator-based loop, with __begin taking the\ntype std::vector<std::shared_ptr<MediaDevice>>::iterator.\n\nOn line 4 the media object is created as a copy of the vector entry\nbeing iterated on. This creates a new std::shared_ptr<> instance,\nincrementing the reference count.\n\nOn line 8 the media object gets out of scope, and is thus destroyed. The\nreference count is decremented.\n\nThe for loop thus doesn't modify the reference count on the objects.\n\nAt the end of the destructor all the member variables are destroyed.\nDestroying devices_ will call the destructor of each entry in turn, and\nthen destroy the vector itself. The references we added to devices_ in\nDeviceEnumerator::addDevice() are thus dropped at this point, and\neverything is fine. There is thus no need to destroy the devices_\nentries manually. The \"delete dev\" statement was need because the\ndevices_ vector contained pointers to MediaDevice objects that it owned,\nso they had to be deleted.\n\n> >  \t\t\tLOG(DeviceEnumerator, Error)\n> >  \t\t\t\t<< \"Removing media device while still in use\";\n> > -\n> > -\t\tdelete dev;\n> >  \t}\n> >  }\n> >\n> > @@ -211,7 +209,7 @@ DeviceEnumerator::~DeviceEnumerator()\n> >   */\n> >  int DeviceEnumerator::addDevice(const std::string &deviceNode)\n> >  {\n> > -\tMediaDevice *media = new MediaDevice(deviceNode);\n> > +\tstd::shared_ptr<MediaDevice> media = std::make_shared<MediaDevice>(deviceNode);\n> >\n> >  \tint ret = media->open();\n> >  \tif (ret < 0)\n> > @@ -243,9 +241,10 @@ int DeviceEnumerator::addDevice(const std::string &deviceNode)\n> >  \t\t\treturn ret;\n> >  \t}\n> >\n> > -\tdevices_.push_back(media);\n> >  \tmedia->close();\n> >\n> > +\tdevices_.push_back(std::move(media));\n> > +\n> >  \treturn 0;\n> >  }\n> >\n> > @@ -260,17 +259,17 @@ int DeviceEnumerator::addDevice(const std::string &deviceNode)\n> >   *\n> >   * \\return pointer to the matching MediaDevice, or nullptr if no match is found\n> >   */\n> > -MediaDevice *DeviceEnumerator::search(const DeviceMatch &dm)\n> > +std::shared_ptr<MediaDevice> DeviceEnumerator::search(const DeviceMatch &dm)\n> >  {\n> > -\tfor (MediaDevice *dev : devices_) {\n> > -\t\tif (dev->busy())\n> > +\tfor (std::shared_ptr<MediaDevice> media : devices_) {\n> > +\t\tif (media->busy())\n> >  \t\t\tcontinue;\n> >\n> > -\t\tif (dm.match(dev)) {\n> > +\t\tif (dm.match(media.get())) {\n> >  \t\t\tLOG(DeviceEnumerator, Debug)\n> >  \t\t\t\t<< \"Successful match for media device \\\"\"\n> > -\t\t\t\t<< dev->driver() << \"\\\"\";\n> > -\t\t\treturn dev;\n> > +\t\t\t\t<< media->driver() << \"\\\"\";\n> > +\t\t\treturn std::move(media);\n> >  \t\t}\n> >  \t}\n> >\n> > diff --git a/src/libcamera/include/device_enumerator.h b/src/libcamera/include/device_enumerator.h\n> > index 40c7750b49fc..3f87a6255303 100644\n> > --- a/src/libcamera/include/device_enumerator.h\n> > +++ b/src/libcamera/include/device_enumerator.h\n> > @@ -42,13 +42,13 @@ public:\n> >  \tvirtual int init() = 0;\n> >  \tvirtual int enumerate() = 0;\n> >\n> > -\tMediaDevice *search(const DeviceMatch &dm);\n> > +\tstd::shared_ptr<MediaDevice> search(const DeviceMatch &dm);\n> >\n> >  protected:\n> >  \tint addDevice(const std::string &deviceNode);\n> >\n> >  private:\n> > -\tstd::vector<MediaDevice *> devices_;\n> > +\tstd::vector<std::shared_ptr<MediaDevice>> devices_;\n> >\n> >  \tvirtual std::string lookupDeviceNode(int major, int minor) = 0;\n> >  };\n> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > index 13ff7da4c99e..9831f74fe53f 100644\n> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > @@ -29,8 +29,8 @@ public:\n> >  \tbool match(DeviceEnumerator *enumerator);\n> >\n> >  private:\n> > -\tMediaDevice *cio2_;\n> > -\tMediaDevice *imgu_;\n> > +\tstd::shared_ptr<MediaDevice> cio2_;\n> > +\tstd::shared_ptr<MediaDevice> imgu_;\n> >\n> >  \tvoid registerCameras();\n> >  };\n> > @@ -47,9 +47,6 @@ PipelineHandlerIPU3::~PipelineHandlerIPU3()\n> >\n> >  \tif (imgu_)\n> >  \t\timgu_->release();\n> > -\n> > -\tcio2_ = nullptr;\n> > -\timgu_ = nullptr;\n> >  }\n> >\n> >  bool PipelineHandlerIPU3::match(DeviceEnumerator *enumerator)\n> > @@ -78,11 +75,11 @@ bool PipelineHandlerIPU3::match(DeviceEnumerator *enumerator)\n> >  \timgu_dm.add(\"ipu3-imgu 1 viewfinder\");\n> >  \timgu_dm.add(\"ipu3-imgu 1 3a stat\");\n> >\n> > -\tcio2_ = enumerator->search(cio2_dm);\n> > +\tcio2_ = std::move(enumerator->search(cio2_dm));\n> >  \tif (!cio2_)\n> >  \t\treturn false;\n> >\n> > -\timgu_ = enumerator->search(imgu_dm);\n> > +\timgu_ = std::move(enumerator->search(imgu_dm));\n> >  \tif (!imgu_)\n> >  \t\treturn false;\n> >\n> > diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp\n> > index 3ebc074093ab..73bad6714bb4 100644\n> > --- a/src/libcamera/pipeline/uvcvideo.cpp\n> > +++ b/src/libcamera/pipeline/uvcvideo.cpp\n> > @@ -23,7 +23,7 @@ public:\n> >  \tbool match(DeviceEnumerator *enumerator);\n> >\n> >  private:\n> > -\tMediaDevice *dev_;\n> > +\tstd::shared_ptr<MediaDevice> dev_;\n> >  };\n> >\n> >  PipelineHandlerUVC::PipelineHandlerUVC(CameraManager *manager)\n> > @@ -41,7 +41,7 @@ bool PipelineHandlerUVC::match(DeviceEnumerator *enumerator)\n> >  {\n> >  \tDeviceMatch dm(\"uvcvideo\");\n> >\n> > -\tdev_ = enumerator->search(dm);\n> > +\tdev_ = std::move(enumerator->search(dm));\n> >\n> >  \tif (!dev_)\n> >  \t\treturn false;\n> > diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp\n> > index 68bfe9b12ab6..521b20d3a120 100644\n> > --- a/src/libcamera/pipeline/vimc.cpp\n> > +++ b/src/libcamera/pipeline/vimc.cpp\n> > @@ -23,7 +23,7 @@ public:\n> >  \tbool match(DeviceEnumerator *enumerator);\n> >\n> >  private:\n> > -\tMediaDevice *dev_;\n> > +\tstd::shared_ptr<MediaDevice> dev_;\n> >  };\n> >\n> >  PipeHandlerVimc::PipeHandlerVimc(CameraManager *manager)\n> > @@ -51,7 +51,7 @@ bool PipeHandlerVimc::match(DeviceEnumerator *enumerator)\n> >  \tdm.add(\"RGB/YUV Input\");\n> >  \tdm.add(\"Scaler\");\n> >\n> > -\tdev_ = enumerator->search(dm);\n> > +\tdev_ = std::move(enumerator->search(dm));\n> >  \tif (!dev_)\n> >  \t\treturn false;\n> >\n> > diff --git a/test/media_device/media_device_link_test.cpp b/test/media_device/media_device_link_test.cpp\n> > index ac5b632f8ed1..58a55cdfee63 100644\n> > --- a/test/media_device/media_device_link_test.cpp\n> > +++ b/test/media_device/media_device_link_test.cpp\n> > @@ -240,7 +240,7 @@ class MediaDeviceLinkTest : public Test\n> >\n> >  private:\n> >  \tunique_ptr<DeviceEnumerator> enumerator;\n> > -\tMediaDevice *dev_;\n> > +\tshared_ptr<MediaDevice> dev_;\n> >  };\n> >\n> >  TEST_REGISTER(MediaDeviceLinkTest);\n> > diff --git a/test/pipeline/ipu3/ipu3_pipeline_test.cpp b/test/pipeline/ipu3/ipu3_pipeline_test.cpp\n> > index 482c12499a86..953f0233cde4 100644\n> > --- a/test/pipeline/ipu3/ipu3_pipeline_test.cpp\n> > +++ b/test/pipeline/ipu3/ipu3_pipeline_test.cpp\n> > @@ -65,7 +65,7 @@ int IPU3PipelineTest::init()\n> >  \t\treturn TestSkip;\n> >  \t}\n> >\n> > -\tMediaDevice *cio2 = enumerator->search(cio2_dm);\n> > +\tstd::shared_ptr<MediaDevice> cio2 = enumerator->search(cio2_dm);\n> >  \tif (!cio2) {\n> >  \t\tcerr << \"Failed to find IPU3 CIO2: test skip\" << endl;\n> >  \t\treturn TestSkip;","headers":{"Return-Path":"<laurent.pinchart@ideasonboard.com>","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 8D06160C82\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri, 25 Jan 2019 00:55:09 +0100 (CET)","from pendragon.ideasonboard.com (unknown\n\t[IPv6:2a02:a03f:4499:2700:1060:1d4c:d6a:8e80])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id D2EC32F6;\n\tFri, 25 Jan 2019 00:55:08 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1548374108;\n\tbh=BPUjdxCOHnTwTtVoKenyEqFF5U3COX8Xf1aWM3kHgg0=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=jQlrCoojCBxHwjypIwNgtwD9pN5+h9HY1AFMS0YkgCtVs/jxmJ8VjapGXXFHUUuQJ\n\t60ltkDV8Rck2t4gCrmRf/RTL7u2v9CuzvzU2OJshS9biCZPDz5D+POqi1lILsC9TlV\n\tBYQt+t61HN29f/tWnvkcF9dtU+wPjopz7WsBSzAs=","Date":"Fri, 25 Jan 2019 01:55:08 +0200","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Jacopo Mondi <jacopo@jmondi.org>","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190124235508.GB4399@pendragon.ideasonboard.com>","References":"<20190124101651.9993-1-laurent.pinchart@ideasonboard.com>\n\t<20190124101651.9993-5-laurent.pinchart@ideasonboard.com>\n\t<20190124231641.ulw42irklzizewox@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20190124231641.ulw42irklzizewox@uno.localdomain>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH 04/10] libcamera: device_enumerator:\n\tReference-count MediaDevice instances","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","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>","X-List-Received-Date":"Thu, 24 Jan 2019 23:55:09 -0000"}},{"id":576,"web_url":"https://patchwork.libcamera.org/comment/576/","msgid":"<20190125085024.fhz3as2gfr24p4rt@uno.localdomain>","date":"2019-01-25T08:50:24","subject":"Re: [libcamera-devel] [PATCH 04/10] libcamera: device_enumerator:\n\tReference-count MediaDevice instances","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent,\n   thanks for the explanation\n\nOn Fri, Jan 25, 2019 at 01:55:08AM +0200, Laurent Pinchart wrote:\n> Hi Jacopo,\n>\n> On Fri, Jan 25, 2019 at 12:16:41AM +0100, Jacopo Mondi wrote:\n> > On Thu, Jan 24, 2019 at 12:16:45PM +0200, Laurent Pinchart wrote:\n> > > The MediaDevice class will be the entry point to hot-unplug, as it\n> > > corresponds to the kernel devices that will report device removal\n> > > events. The class will signal media device disconnection to pipeline\n> > > handlers, which will clean up resources as a result.\n> > >\n> > > This can't be performed synchronously as references may exist to the\n> > > related Camera objects in applications. The MediaDevice object thus\n> > > needs to be reference-counted in order to support unplugging, as\n> > > otherwise pipeline handlers would be required to drop all the references\n> > > to the media device they have borrowed synchronously with the\n> > > disconnection signal handler, which would be very error prone (if even\n> > > possible at all in a sane way).\n> > >\n> > > Handle MedieDevice instances with std::shared_ptr<> to support this.\n> > >\n> > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n> > > ---\n> > >  src/libcamera/device_enumerator.cpp          | 23 ++++++++++----------\n> > >  src/libcamera/include/device_enumerator.h    |  4 ++--\n> > >  src/libcamera/pipeline/ipu3/ipu3.cpp         | 11 ++++------\n> > >  src/libcamera/pipeline/uvcvideo.cpp          |  4 ++--\n> > >  src/libcamera/pipeline/vimc.cpp              |  4 ++--\n> > >  test/media_device/media_device_link_test.cpp |  2 +-\n> > >  test/pipeline/ipu3/ipu3_pipeline_test.cpp    |  2 +-\n> > >  7 files changed, 23 insertions(+), 27 deletions(-)\n> > >\n> > > diff --git a/src/libcamera/device_enumerator.cpp b/src/libcamera/device_enumerator.cpp\n> > > index 8100972a8d04..149ffbf9aea6 100644\n> > > --- a/src/libcamera/device_enumerator.cpp\n> > > +++ b/src/libcamera/device_enumerator.cpp\n> > > @@ -166,12 +166,10 @@ std::unique_ptr<DeviceEnumerator> DeviceEnumerator::create()\n> > >\n> > >  DeviceEnumerator::~DeviceEnumerator()\n> > >  {\n> > > -\tfor (MediaDevice *dev : devices_) {\n> > > -\t\tif (dev->busy())\n> > > +\tfor (std::shared_ptr<MediaDevice> media : devices_) {\n> > > +\t\tif (media->busy())\n> >\n> > Not sure I got this. The assignement to media in the range-based for\n> > loop creates a copy, right? Does the device enumerator need to drop\n> > that? They are added to a vector of shared_ptr with\n> >\n> > \tdevices_.push_back(std::move(media));\n> >\n> > So I'm expecting this to happen. What have I missed?\n>\n> It helps to look at how range-based for loops are implemented. The C++\n> standard explains they are equivalent to the following code.\n>\n> {\n> \tauto && __range = range_expression ;\n> \tfor (auto __begin = begin_expr, __end = end_expr; __begin != __end; ++__begin) {\n> \t\trange_declaration = *__begin;\n> \t\tloop_statement\n> \t}\n> }\n>\n> which, when applied to our code, produces\n>\n> 1. {\n> 2. \tauto && __range = devices_;\n> 3. \tfor (auto __begin = begin(__range), __end = end(__range); __begin != __end; ++__begin) {\n> 4. \t\tstd::shared_ptr<MediaDevice> media = *__begin;\n> 5. \t\tif (media->busy())\n> 6.   \t\t\tLOG(DeviceEnumerator, Error)\n> 7.   \t\t\t\t<< \"Removing media device while still in use\";\n> 8. \t}\n> 9. }\n>\n> On line 2 a reference to devices_ is created (as an rvalue reference as\n> range_expression could be the return of a function, in which case using\n> an lvalue would generate an lvalue reference to temporary object error).\n> __range thus references devices_, it doesn't create a copy of the\n> vector.\n>\n> On line 3 we use a classic iterator-based loop, with __begin taking the\n> type std::vector<std::shared_ptr<MediaDevice>>::iterator.\n>\n> On line 4 the media object is created as a copy of the vector entry\n> being iterated on. This creates a new std::shared_ptr<> instance,\n> incrementing the reference count.\n>\n> On line 8 the media object gets out of scope, and is thus destroyed. The\n> reference count is decremented.\n>\n> The for loop thus doesn't modify the reference count on the objects.\n>\n\nThanks. What I wanted to express was \"shouldn't we delete the elements\ninside the container\" ? But yes, when the object gets deleted, the\nvector gets deleted too, as all the elements it contains, dropping the\nreference to the shared object.\n\nThanks for the detailed explanation!\n   j\n\n> At the end of the destructor all the member variables are destroyed.\n> Destroying devices_ will call the destructor of each entry in turn, and\n> then destroy the vector itself. The references we added to devices_ in\n> DeviceEnumerator::addDevice() are thus dropped at this point, and\n> everything is fine. There is thus no need to destroy the devices_\n> entries manually. The \"delete dev\" statement was need because the\n> devices_ vector contained pointers to MediaDevice objects that it owned,\n> so they had to be deleted.\n>\n> > >  \t\t\tLOG(DeviceEnumerator, Error)\n> > >  \t\t\t\t<< \"Removing media device while still in use\";\n> > > -\n> > > -\t\tdelete dev;\n> > >  \t}\n> > >  }\n> > >\n> > > @@ -211,7 +209,7 @@ DeviceEnumerator::~DeviceEnumerator()\n> > >   */\n> > >  int DeviceEnumerator::addDevice(const std::string &deviceNode)\n> > >  {\n> > > -\tMediaDevice *media = new MediaDevice(deviceNode);\n> > > +\tstd::shared_ptr<MediaDevice> media = std::make_shared<MediaDevice>(deviceNode);\n> > >\n> > >  \tint ret = media->open();\n> > >  \tif (ret < 0)\n> > > @@ -243,9 +241,10 @@ int DeviceEnumerator::addDevice(const std::string &deviceNode)\n> > >  \t\t\treturn ret;\n> > >  \t}\n> > >\n> > > -\tdevices_.push_back(media);\n> > >  \tmedia->close();\n> > >\n> > > +\tdevices_.push_back(std::move(media));\n> > > +\n> > >  \treturn 0;\n> > >  }\n> > >\n> > > @@ -260,17 +259,17 @@ int DeviceEnumerator::addDevice(const std::string &deviceNode)\n> > >   *\n> > >   * \\return pointer to the matching MediaDevice, or nullptr if no match is found\n> > >   */\n> > > -MediaDevice *DeviceEnumerator::search(const DeviceMatch &dm)\n> > > +std::shared_ptr<MediaDevice> DeviceEnumerator::search(const DeviceMatch &dm)\n> > >  {\n> > > -\tfor (MediaDevice *dev : devices_) {\n> > > -\t\tif (dev->busy())\n> > > +\tfor (std::shared_ptr<MediaDevice> media : devices_) {\n> > > +\t\tif (media->busy())\n> > >  \t\t\tcontinue;\n> > >\n> > > -\t\tif (dm.match(dev)) {\n> > > +\t\tif (dm.match(media.get())) {\n> > >  \t\t\tLOG(DeviceEnumerator, Debug)\n> > >  \t\t\t\t<< \"Successful match for media device \\\"\"\n> > > -\t\t\t\t<< dev->driver() << \"\\\"\";\n> > > -\t\t\treturn dev;\n> > > +\t\t\t\t<< media->driver() << \"\\\"\";\n> > > +\t\t\treturn std::move(media);\n> > >  \t\t}\n> > >  \t}\n> > >\n> > > diff --git a/src/libcamera/include/device_enumerator.h b/src/libcamera/include/device_enumerator.h\n> > > index 40c7750b49fc..3f87a6255303 100644\n> > > --- a/src/libcamera/include/device_enumerator.h\n> > > +++ b/src/libcamera/include/device_enumerator.h\n> > > @@ -42,13 +42,13 @@ public:\n> > >  \tvirtual int init() = 0;\n> > >  \tvirtual int enumerate() = 0;\n> > >\n> > > -\tMediaDevice *search(const DeviceMatch &dm);\n> > > +\tstd::shared_ptr<MediaDevice> search(const DeviceMatch &dm);\n> > >\n> > >  protected:\n> > >  \tint addDevice(const std::string &deviceNode);\n> > >\n> > >  private:\n> > > -\tstd::vector<MediaDevice *> devices_;\n> > > +\tstd::vector<std::shared_ptr<MediaDevice>> devices_;\n> > >\n> > >  \tvirtual std::string lookupDeviceNode(int major, int minor) = 0;\n> > >  };\n> > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > index 13ff7da4c99e..9831f74fe53f 100644\n> > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > @@ -29,8 +29,8 @@ public:\n> > >  \tbool match(DeviceEnumerator *enumerator);\n> > >\n> > >  private:\n> > > -\tMediaDevice *cio2_;\n> > > -\tMediaDevice *imgu_;\n> > > +\tstd::shared_ptr<MediaDevice> cio2_;\n> > > +\tstd::shared_ptr<MediaDevice> imgu_;\n> > >\n> > >  \tvoid registerCameras();\n> > >  };\n> > > @@ -47,9 +47,6 @@ PipelineHandlerIPU3::~PipelineHandlerIPU3()\n> > >\n> > >  \tif (imgu_)\n> > >  \t\timgu_->release();\n> > > -\n> > > -\tcio2_ = nullptr;\n> > > -\timgu_ = nullptr;\n> > >  }\n> > >\n> > >  bool PipelineHandlerIPU3::match(DeviceEnumerator *enumerator)\n> > > @@ -78,11 +75,11 @@ bool PipelineHandlerIPU3::match(DeviceEnumerator *enumerator)\n> > >  \timgu_dm.add(\"ipu3-imgu 1 viewfinder\");\n> > >  \timgu_dm.add(\"ipu3-imgu 1 3a stat\");\n> > >\n> > > -\tcio2_ = enumerator->search(cio2_dm);\n> > > +\tcio2_ = std::move(enumerator->search(cio2_dm));\n> > >  \tif (!cio2_)\n> > >  \t\treturn false;\n> > >\n> > > -\timgu_ = enumerator->search(imgu_dm);\n> > > +\timgu_ = std::move(enumerator->search(imgu_dm));\n> > >  \tif (!imgu_)\n> > >  \t\treturn false;\n> > >\n> > > diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp\n> > > index 3ebc074093ab..73bad6714bb4 100644\n> > > --- a/src/libcamera/pipeline/uvcvideo.cpp\n> > > +++ b/src/libcamera/pipeline/uvcvideo.cpp\n> > > @@ -23,7 +23,7 @@ public:\n> > >  \tbool match(DeviceEnumerator *enumerator);\n> > >\n> > >  private:\n> > > -\tMediaDevice *dev_;\n> > > +\tstd::shared_ptr<MediaDevice> dev_;\n> > >  };\n> > >\n> > >  PipelineHandlerUVC::PipelineHandlerUVC(CameraManager *manager)\n> > > @@ -41,7 +41,7 @@ bool PipelineHandlerUVC::match(DeviceEnumerator *enumerator)\n> > >  {\n> > >  \tDeviceMatch dm(\"uvcvideo\");\n> > >\n> > > -\tdev_ = enumerator->search(dm);\n> > > +\tdev_ = std::move(enumerator->search(dm));\n> > >\n> > >  \tif (!dev_)\n> > >  \t\treturn false;\n> > > diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp\n> > > index 68bfe9b12ab6..521b20d3a120 100644\n> > > --- a/src/libcamera/pipeline/vimc.cpp\n> > > +++ b/src/libcamera/pipeline/vimc.cpp\n> > > @@ -23,7 +23,7 @@ public:\n> > >  \tbool match(DeviceEnumerator *enumerator);\n> > >\n> > >  private:\n> > > -\tMediaDevice *dev_;\n> > > +\tstd::shared_ptr<MediaDevice> dev_;\n> > >  };\n> > >\n> > >  PipeHandlerVimc::PipeHandlerVimc(CameraManager *manager)\n> > > @@ -51,7 +51,7 @@ bool PipeHandlerVimc::match(DeviceEnumerator *enumerator)\n> > >  \tdm.add(\"RGB/YUV Input\");\n> > >  \tdm.add(\"Scaler\");\n> > >\n> > > -\tdev_ = enumerator->search(dm);\n> > > +\tdev_ = std::move(enumerator->search(dm));\n> > >  \tif (!dev_)\n> > >  \t\treturn false;\n> > >\n> > > diff --git a/test/media_device/media_device_link_test.cpp b/test/media_device/media_device_link_test.cpp\n> > > index ac5b632f8ed1..58a55cdfee63 100644\n> > > --- a/test/media_device/media_device_link_test.cpp\n> > > +++ b/test/media_device/media_device_link_test.cpp\n> > > @@ -240,7 +240,7 @@ class MediaDeviceLinkTest : public Test\n> > >\n> > >  private:\n> > >  \tunique_ptr<DeviceEnumerator> enumerator;\n> > > -\tMediaDevice *dev_;\n> > > +\tshared_ptr<MediaDevice> dev_;\n> > >  };\n> > >\n> > >  TEST_REGISTER(MediaDeviceLinkTest);\n> > > diff --git a/test/pipeline/ipu3/ipu3_pipeline_test.cpp b/test/pipeline/ipu3/ipu3_pipeline_test.cpp\n> > > index 482c12499a86..953f0233cde4 100644\n> > > --- a/test/pipeline/ipu3/ipu3_pipeline_test.cpp\n> > > +++ b/test/pipeline/ipu3/ipu3_pipeline_test.cpp\n> > > @@ -65,7 +65,7 @@ int IPU3PipelineTest::init()\n> > >  \t\treturn TestSkip;\n> > >  \t}\n> > >\n> > > -\tMediaDevice *cio2 = enumerator->search(cio2_dm);\n> > > +\tstd::shared_ptr<MediaDevice> cio2 = enumerator->search(cio2_dm);\n> > >  \tif (!cio2) {\n> > >  \t\tcerr << \"Failed to find IPU3 CIO2: test skip\" << endl;\n> > >  \t\treturn TestSkip;\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","headers":{"Return-Path":"<jacopo@jmondi.org>","Received":["from relay11.mail.gandi.net (relay11.mail.gandi.net\n\t[217.70.178.231])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 90AAB60C7F\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri, 25 Jan 2019 09:50:11 +0100 (CET)","from uno.localdomain (2-224-242-101.ip172.fastwebnet.it\n\t[2.224.242.101]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay11.mail.gandi.net (Postfix) with ESMTPSA id 0EF5410000C;\n\tFri, 25 Jan 2019 08:50:10 +0000 (UTC)"],"Date":"Fri, 25 Jan 2019 09:50:24 +0100","From":"Jacopo Mondi <jacopo@jmondi.org>","To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190125085024.fhz3as2gfr24p4rt@uno.localdomain>","References":"<20190124101651.9993-1-laurent.pinchart@ideasonboard.com>\n\t<20190124101651.9993-5-laurent.pinchart@ideasonboard.com>\n\t<20190124231641.ulw42irklzizewox@uno.localdomain>\n\t<20190124235508.GB4399@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"jhv5rv3a3sgbzut4\"","Content-Disposition":"inline","In-Reply-To":"<20190124235508.GB4399@pendragon.ideasonboard.com>","User-Agent":"NeoMutt/20180716","Subject":"Re: [libcamera-devel] [PATCH 04/10] libcamera: device_enumerator:\n\tReference-count MediaDevice instances","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","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>","X-List-Received-Date":"Fri, 25 Jan 2019 08:50:11 -0000"}}]