[{"id":960,"web_url":"https://patchwork.libcamera.org/comment/960/","msgid":"<20190228172925.GI899@bigcity.dyn.berto.se>","date":"2019-02-28T17:29:25","subject":"Re: [libcamera-devel] [PATCH 06/10] libcamera: pipeline_handler:\n\tMake pipeline-specific data mandatory","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 patch.\n\nOn 2019-02-28 18:29:09 +0200, Laurent Pinchart wrote:\n> Mandate creationg of pipeline-specific data by pipeline handlers. This\n> allows simplifying the API by passing the pipeline-specific data to the\n> registerCamera() method and removing the separate setCameraData()\n> method.\n> \n> Signed-off-by: Laurent Pinchart \n\nI like the diffstat :-)\n\nReviewed-by: Niklas Söderlund \n\n> ---\n> src/libcamera/include/pipeline_handler.h | 4 +-\n> src/libcamera/pipeline/ipu3/ipu3.cpp | 3 +-\n> src/libcamera/pipeline/uvcvideo.cpp | 4 +-\n> src/libcamera/pipeline/vimc.cpp | 4 +-\n> src/libcamera/pipeline_handler.cpp | 56 +++++-------------------\n> 5 files changed, 15 insertions(+), 56 deletions(-)\n> \n> diff --git a/src/libcamera/include/pipeline_handler.h b/src/libcamera/include/pipeline_handler.h\n> index bc4da5820ac4..b2b9c94cebdc 100644\n> --- a/src/libcamera/include/pipeline_handler.h\n> +++ b/src/libcamera/include/pipeline_handler.h\n> @@ -63,11 +63,11 @@ public:\n> \tvirtual int queueRequest(Camera *camera, Request *request) = 0;\n> \n> protected:\n> -\tvoid registerCamera(std::shared_ptr camera);\n> +\tvoid registerCamera(std::shared_ptr camera,\n> +\t\t\t std::unique_ptr data);\n> \tvoid hotplugMediaDevice(MediaDevice *media);\n> \n> \tCameraData *cameraData(const Camera *camera);\n> -\tvoid setCameraData(const Camera *camera, std::unique_ptr data);\n> \n> \tCameraManager *manager_;\n> \n> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> index 347ee657fddf..776b7f07f78d 100644\n> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> @@ -406,8 +406,7 @@ void PipelineHandlerIPU3::registerCameras()\n> \t\tif (ret)\n> \t\t\tcontinue;\n> \n> -\t\tsetCameraData(camera.get(), std::move(data));\n> -\t\tregisterCamera(std::move(camera));\n> +\t\tregisterCamera(std::move(camera), std::move(data));\n> \n> \t\tLOG(IPU3, Info)\n> \t\t\t<< \"Registered Camera[\" << numCameras << \"] \\\"\"\n> diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp\n> index 9af4838891f4..f121d3c5633e 100644\n> --- a/src/libcamera/pipeline/uvcvideo.cpp\n> +++ b/src/libcamera/pipeline/uvcvideo.cpp\n> @@ -201,9 +201,7 @@ bool PipelineHandlerUVC::match(DeviceEnumerator *enumerator)\n> \t/* Create and register the camera. */\n> \tstd::vector streams{ &data->stream_ };\n> \tstd::shared_ptr camera = Camera::create(this, media_->model(), streams);\n> -\n> -\tsetCameraData(camera.get(), std::move(data));\n> -\tregisterCamera(std::move(camera));\n> +\tregisterCamera(std::move(camera), std::move(data));\n> \n> \t/* Enable hot-unplug notifications. */\n> \thotplugMediaDevice(media_.get());\n> diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp\n> index c4c1eb0dc19f..6d022c37eb9f 100644\n> --- a/src/libcamera/pipeline/vimc.cpp\n> +++ b/src/libcamera/pipeline/vimc.cpp\n> @@ -209,9 +209,7 @@ bool PipelineHandlerVimc::match(DeviceEnumerator *enumerator)\n> \tstd::vector streams{ &data->stream_ };\n> \tstd::shared_ptr camera = Camera::create(this, \"VIMC Sensor B\",\n> \t\t\t\t\t\t\tstreams);\n> -\n> -\tsetCameraData(camera.get(), std::move(data));\n> -\tregisterCamera(std::move(camera));\n> +\tregisterCamera(std::move(camera), std::move(data));\n> \n> \treturn true;\n> }\n> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n> index ac1acea5a318..54f237942a48 100644\n> --- a/src/libcamera/pipeline_handler.cpp\n> +++ b/src/libcamera/pipeline_handler.cpp\n> @@ -247,20 +247,18 @@ PipelineHandler::~PipelineHandler()\n> /**\n> * \\brief Register a camera to the camera manager and pipeline handler\n> * \\param[in] camera The camera to be added\n> + * \\param[in] data Pipeline-specific data for the camera\n> *\n> * This method is called by pipeline handlers to register the cameras they\n> - * handle with the camera manager. If no CameraData has been associated with\n> - * the camera with setCameraData() by the pipeline handler, the method creates\n> - * a default CameraData instance for the \\a camera.\n> + * handle with the camera manager. It associates the pipeline-specific \\a data\n> + * with the camera, for later retrieval with cameraData(). Ownership of \\a data\n> + * is transferred to the PipelineHandler.\n> */\n> -void PipelineHandler::registerCamera(std::shared_ptr camera)\n> +void PipelineHandler::registerCamera(std::shared_ptr camera,\n> +\t\t\t\t std::unique_ptr data)\n> {\n> -\tif (!cameraData_.count(camera.get())) {\n> -\t\tstd::unique_ptr data = utils::make_unique(this);\n> -\t\tsetCameraData(camera.get(), std::move(data));\n> -\t}\n> -\n> -\tcameraData(camera.get())->camera_ = camera.get();\n> +\tdata->camera_ = camera.get();\n> +\tcameraData_[camera.get()] = std::move(data);\n> \tcameras_.push_back(camera);\n> \tmanager_->addCamera(std::move(camera));\n> }\n> @@ -325,51 +323,17 @@ void PipelineHandler::disconnect()\n> * \\brief Retrieve the pipeline-specific data associated with a Camera\n> * \\param camera The camera whose data to retrieve\n> *\n> - * \\return A pointer to the pipeline-specific data set with setCameraData().\n> + * \\return A pointer to the pipeline-specific data passed to registerCamera().\n> * The returned pointer is a borrowed reference and is guaranteed to remain\n> * valid until the pipeline handler is destroyed. It shall not be deleted\n> * manually by the caller.\n> */\n> CameraData *PipelineHandler::cameraData(const Camera *camera)\n> {\n> -\tif (!cameraData_.count(camera)) {\n> -\t\tLOG(Pipeline, Error)\n> -\t\t\t<< \"Cannot get data associated with camera \"\n> -\t\t\t<< camera->name();\n> -\t\treturn nullptr;\n> -\t}\n> -\n> +\tASSERT(cameraData_.count(camera));\n> \treturn cameraData_[camera].get();\n> }\n> \n> -/**\n> - * \\brief Set pipeline-specific data for the camera\n> - * \\param camera The camera to associate data to\n> - * \\param data The pipeline-specific data\n> - *\n> - * This method allows pipeline handlers to associate pipeline-specific\n> - * information with \\a camera. Ownership of \\a data is transferred to\n> - * the PipelineHandler.\n> - *\n> - * Pipeline-specific data can only be set once, and shall be set before\n> - * registering the camera with registerCamera(). Any attempt to call this\n> - * method more than once for the same camera, or to call it after registering\n> - * the camera, will not change the pipeline-specific data.\n> - *\n> - * The data can be retrieved by pipeline handlers using the cameraData() method.\n> - */\n> -void PipelineHandler::setCameraData(const Camera *camera,\n> -\t\t\t\t std::unique_ptr data)\n> -{\n> -\tif (cameraData_.find(camera) != cameraData_.end()) {\n> -\t\tLOG(Pipeline, Error)\n> -\t\t\t<< \"Replacing data associated with a camera is forbidden\";\n> -\t\treturn;\n> -\t}\n> -\n> -\tcameraData_[camera] = std::move(data);\n> -}\n> -\n> /**\n> * \\var PipelineHandler::manager_\n> * \\brief The Camera manager associated with the pipeline handler\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":"","Received":["from mail-lf1-x12e.google.com (mail-lf1-x12e.google.com\n\t[IPv6:2a00:1450:4864:20::12e])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id ABC85610B6\n\tfor ;\n\tThu, 28 Feb 2019 18:29:26 +0100 (CET)","by mail-lf1-x12e.google.com with SMTP id m73so10315367lfa.2\n\tfor ;\n\tThu, 28 Feb 2019 09:29:26 -0800 (PST)","from localhost (89-233-230-99.cust.bredband2.com. [89.233.230.99])\n\tby smtp.gmail.com with ESMTPSA id\n\tw30sm4123479ljd.65.2019.02.28.09.29.25\n\t(version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256);\n\tThu, 28 Feb 2019 09:29:25 -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=RPRJQb3t9wYyiKYjQWVOdqIfIWrBEyeg7q9mkyYoklA=;\n\tb=X+kXvtT+NXdUBtVd4vMQiRIxp/eBY9Ihb2kf3bdBeQ9ikYPx21jduq0Hhmaukx5pUZ\n\tTKGtdcMVPsKPuuGhVmNWJIIfmyzc3LyD979ATF0be/uAmzBo5smntLuY7JY6ekwzPfiB\n\tngOG7z/nSi2gmE6iSNtwuRg9YrxppXDUyYKfL6I36bNhu3trC72bP1AXJpXnKdzCN8Zu\n\t3/qOMDW8mZSk8QVLT+iUIcXqaoW+4A3jRrML0JAZBytjM5fBHp8j7YjDK/SxXL4MfPK/\n\tDGY4WBUDnnFxiIYIaMjW8kgoEWQCXGLjOrzE6MIhS8QVW+ScCS2V8oYNrcADFpafcsb4\n\tXYxw==","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=RPRJQb3t9wYyiKYjQWVOdqIfIWrBEyeg7q9mkyYoklA=;\n\tb=O6wSYQy/KKzHIyTGZ1sKEcoATR58FtToMf/mxX8P7D6LSjGlBgtlINsM1xDjP1lTPP\n\tQ4LMl9mFMgIWM2moHbAsXFbWGapGqFMM6f+BZt8tfj7kXJcYsQTgL6r3EbpkbZkQs9y4\n\tq+lC6Xf0/zAlG9nPH0JcG9fK47gup+dHcns7ANY7okwsn60VlJKsMb8LdaT53w3LCAr2\n\tEjC6MAkL0ahtWMsXhExwLBhJi4EYJfN0GGYVAiFuCCXXyQSqwP09amLnA6S60/VuaJUr\n\t14w7oZHY6eQZh/RHJhX8EJAA6wMKwWqIVLUuUPKd2vAM8J1AWoQhodVWDONcb+KmZnpW\n\twOKA==","X-Gm-Message-State":"APjAAAV6nG8EcE9z8CjreU1zt/2hNPuOZHp2pNMJwUde6HrXJE8RCbsH\n\tU4AK+uryMuoRroYYJ58gxdWON9TM0os=","X-Google-Smtp-Source":"APXvYqxUOIAEfTVhd1rd7crhY9zPc8zfpf+OyGTmqgm6pPpRYLdON3tbJBi1n5KFWxsd9FveScxglQ==","X-Received":"by 2002:ac2:4192:: with SMTP id z18mr349307lfh.39.1551374965912; \n\tThu, 28 Feb 2019 09:29:25 -0800 (PST)","Date":"Thu, 28 Feb 2019 18:29:25 +0100","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190228172925.GI899@bigcity.dyn.berto.se>","References":"<20190228162913.6508-1-laurent.pinchart@ideasonboard.com>\n\t<20190228162913.6508-7-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":"<20190228162913.6508-7-laurent.pinchart@ideasonboard.com>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH 06/10] libcamera: pipeline_handler:\n\tMake pipeline-specific data mandatory","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Thu, 28 Feb 2019 17:29:27 -0000"}}]