[{"id":13608,"web_url":"https://patchwork.libcamera.org/comment/13608/","msgid":"","date":"2020-11-05T11:24:56","subject":"Re: [libcamera-devel] [PATCH 01/11] libcamera: pipeline_handler:\n\tRemove Camera argument from request handling","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Niklas,\n\nOn 05/11/2020 00:15, Niklas Söderlund wrote:\n> There is no need to pass the Camera pointer to queueRequest(),\n> completeBuffer() and completeRequest() as the Request also passed\n> contains the same information. Remove the Camera argument to avoid\n> situations where the information in the Request and the argument differ.\n> \n> There is no functional change and no public API change as the interface\n> is only used between the Camera and PipelineHandler.\n> \n\nRequests are required to stay with the Camera they were created for, so\nI'm fine with this.\n\nOtherwise, having an API that lets you queue a request to an arbitrary\nCamera doesn't make sense.\n\nReviewed-by: Kieran Bingham \n\n> Signed-off-by: Niklas Söderlund \n> ---\n> include/libcamera/internal/pipeline_handler.h | 7 +++----\n> src/libcamera/camera.cpp | 2 +-\n> src/libcamera/pipeline/ipu3/ipu3.cpp | 8 ++++----\n> src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 8 ++++----\n> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 5 ++---\n> src/libcamera/pipeline/simple/simple.cpp | 12 ++++++------\n> src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 4 ++--\n> src/libcamera/pipeline/vimc/vimc.cpp | 4 ++--\n> src/libcamera/pipeline_handler.cpp | 14 +++++++-------\n> 9 files changed, 31 insertions(+), 33 deletions(-)\n> \n> diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h\n> index c12c8904858e7536..ca1997bc0771b139 100644\n> --- a/include/libcamera/internal/pipeline_handler.h\n> +++ b/include/libcamera/internal/pipeline_handler.h\n> @@ -81,11 +81,10 @@ public:\n> \tvirtual int start(Camera *camera) = 0;\n> \tvirtual void stop(Camera *camera) = 0;\n> \n> -\tint queueRequest(Camera *camera, Request *request);\n> +\tint queueRequest(Request *request);\n> \n> -\tbool completeBuffer(Camera *camera, Request *request,\n> -\t\t\t FrameBuffer *buffer);\n> -\tvoid completeRequest(Camera *camera, Request *request);\n> +\tbool completeBuffer(Request *request, FrameBuffer *buffer);\n> +\tvoid completeRequest(Request *request);\n> \n> \tconst char *name() const { return name_; }\n> \n> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> index 9590ab7249d39e2f..f868161f2a620264 100644\n> --- a/src/libcamera/camera.cpp\n> +++ b/src/libcamera/camera.cpp\n> @@ -914,7 +914,7 @@ int Camera::queueRequest(Request *request)\n> \t}\n> \n> \treturn p_->pipe_->invokeMethod(&PipelineHandler::queueRequest,\n> -\t\t\t\t ConnectionTypeQueued, this, request);\n> +\t\t\t\t ConnectionTypeQueued, request);\n> }\n> \n> /**\n> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> index 5a6ee1a83e45ff75..3f0232bc1eaad048 100644\n> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> @@ -836,13 +836,13 @@ void IPU3CameraData::imguOutputBufferReady(FrameBuffer *buffer)\n> {\n> \tRequest *request = buffer->request();\n> \n> -\tif (!pipe_->completeBuffer(camera_, request, buffer))\n> +\tif (!pipe_->completeBuffer(request, buffer))\n> \t\t/* Request not completed yet, return here. */\n> \t\treturn;\n> \n> \t/* Mark the request as complete. */\n> \trequest->metadata().set(controls::draft::PipelineDepth, 3);\n> -\tpipe_->completeRequest(camera_, request);\n> +\tpipe_->completeRequest(request);\n> }\n> \n> /**\n> @@ -865,10 +865,10 @@ void IPU3CameraData::cio2BufferReady(FrameBuffer *buffer)\n> \t * now as there's no need for ImgU processing.\n> \t */\n> \tif (request->findBuffer(&rawStream_)) {\n> -\t\tbool isComplete = pipe_->completeBuffer(camera_, request, buffer);\n> +\t\tbool isComplete = pipe_->completeBuffer(request, buffer);\n> \t\tif (isComplete) {\n> \t\t\trequest->metadata().set(controls::draft::PipelineDepth, 2);\n> -\t\t\tpipe_->completeRequest(camera_, request);\n> +\t\t\tpipe_->completeRequest(request);\n> \t\t\treturn;\n> \t\t}\n> \t}\n> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> index 220b9c51849662f3..630b876985459d2f 100644\n> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> @@ -1463,10 +1463,10 @@ void RPiCameraData::clearIncompleteRequests()\n> \t\t\t * request? If not, do so now.\n> \t\t\t */\n> \t\t\tif (buffer && buffer->request())\n> -\t\t\t\tpipe_->completeBuffer(camera_, request, buffer);\n> +\t\t\t\tpipe_->completeBuffer(request, buffer);\n> \t\t}\n> \n> -\t\tpipe_->completeRequest(camera_, request);\n> +\t\tpipe_->completeRequest(request);\n> \t\trequestQueue_.pop_front();\n> \t}\n> }\n> @@ -1490,7 +1490,7 @@ void RPiCameraData::handleStreamBuffer(FrameBuffer *buffer, RPi::Stream *stream)\n> \t\t\t * Tag the buffer as completed, returning it to the\n> \t\t\t * application.\n> \t\t\t */\n> -\t\t\tpipe_->completeBuffer(camera_, request, buffer);\n> +\t\t\tpipe_->completeBuffer(request, buffer);\n> \t\t} else {\n> \t\t\t/*\n> \t\t\t * This buffer was not part of the Request, or there is no\n> @@ -1554,7 +1554,7 @@ void RPiCameraData::checkRequestCompleted()\n> \t\tif (state_ != State::IpaComplete)\n> \t\t\treturn;\n> \n> -\t\tpipe_->completeRequest(camera_, request);\n> +\t\tpipe_->completeRequest(request);\n> \t\trequestQueue_.pop_front();\n> \t\trequestCompleted = true;\n> \t}\n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> index 424099d087528fb1..70a10853bf83b6a5 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> @@ -1042,15 +1042,14 @@ void PipelineHandlerRkISP1::tryCompleteRequest(Request *request)\n> \n> \tdata->frameInfo_.destroy(info->frame);\n> \n> -\tcompleteRequest(activeCamera_, request);\n> +\tcompleteRequest(request);\n> }\n> \n> void PipelineHandlerRkISP1::bufferReady(FrameBuffer *buffer)\n> {\n> -\tASSERT(activeCamera_);\n> \tRequest *request = buffer->request();\n> \n> -\tcompleteBuffer(activeCamera_, request, buffer);\n> +\tcompleteBuffer(request, buffer);\n> \ttryCompleteRequest(request);\n> }\n> \n> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp\n> index 3d2039f3f26951bf..dc55304fc6ebb202 100644\n> --- a/src/libcamera/pipeline/simple/simple.cpp\n> +++ b/src/libcamera/pipeline/simple/simple.cpp\n> @@ -903,8 +903,8 @@ void SimplePipelineHandler::bufferReady(FrameBuffer *buffer)\n> \t\t}\n> \n> \t\tRequest *request = buffer->request();\n> -\t\tcompleteBuffer(activeCamera_, request, buffer);\n> -\t\tcompleteRequest(activeCamera_, request);\n> +\t\tcompleteBuffer(request, buffer);\n> +\t\tcompleteRequest(request);\n> \t\treturn;\n> \t}\n> \n> @@ -928,8 +928,8 @@ void SimplePipelineHandler::bufferReady(FrameBuffer *buffer)\n> \n> \t/* Otherwise simply complete the request. */\n> \tRequest *request = buffer->request();\n> -\tcompleteBuffer(activeCamera_, request, buffer);\n> -\tcompleteRequest(activeCamera_, request);\n> +\tcompleteBuffer(request, buffer);\n> +\tcompleteRequest(request);\n> }\n> \n> void SimplePipelineHandler::converterDone(FrameBuffer *input,\n> @@ -940,8 +940,8 @@ void SimplePipelineHandler::converterDone(FrameBuffer *input,\n> \n> \t/* Complete the request. */\n> \tRequest *request = output->request();\n> -\tcompleteBuffer(activeCamera_, request, output);\n> -\tcompleteRequest(activeCamera_, request);\n> +\tcompleteBuffer(request, output);\n> +\tcompleteRequest(request);\n> \n> \t/* Queue the input buffer back for capture. */\n> \tdata->video_->queueBuffer(input);\n> diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> index 3862631b7bedf604..272e4c6bf50e82e4 100644\n> --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> @@ -650,8 +650,8 @@ void UVCCameraData::bufferReady(FrameBuffer *buffer)\n> {\n> \tRequest *request = buffer->request();\n> \n> -\tpipe_->completeBuffer(camera_, request, buffer);\n> -\tpipe_->completeRequest(camera_, request);\n> +\tpipe_->completeBuffer(request, buffer);\n> +\tpipe_->completeRequest(request);\n> }\n> \n> REGISTER_PIPELINE_HANDLER(PipelineHandlerUVC)\n> diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp\n> index 7416c37c6f5a243c..99b0fa2742269147 100644\n> --- a/src/libcamera/pipeline/vimc/vimc.cpp\n> +++ b/src/libcamera/pipeline/vimc/vimc.cpp\n> @@ -529,8 +529,8 @@ void VimcCameraData::bufferReady(FrameBuffer *buffer)\n> {\n> \tRequest *request = buffer->request();\n> \n> -\tpipe_->completeBuffer(camera_, request, buffer);\n> -\tpipe_->completeRequest(camera_, request);\n> +\tpipe_->completeBuffer(request, buffer);\n> +\tpipe_->completeRequest(request);\n> }\n> \n> REGISTER_PIPELINE_HANDLER(PipelineHandlerVimc)\n> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n> index 894200ee4e936735..e60b962d66183e19 100644\n> --- a/src/libcamera/pipeline_handler.cpp\n> +++ b/src/libcamera/pipeline_handler.cpp\n> @@ -376,7 +376,6 @@ const ControlList &PipelineHandler::properties(const Camera *camera) const\n> /**\n> * \\fn PipelineHandler::queueRequest()\n> * \\brief Queue a request to the camera\n> - * \\param[in] camera The camera to queue the request to\n> * \\param[in] request The request to queue\n> *\n> * This method queues a capture request to the pipeline handler for processing.\n> @@ -391,8 +390,9 @@ const ControlList &PipelineHandler::properties(const Camera *camera) const\n> *\n> * \\return 0 on success or a negative error code otherwise\n> */\n> -int PipelineHandler::queueRequest(Camera *camera, Request *request)\n> +int PipelineHandler::queueRequest(Request *request)\n> {\n> +\tCamera *camera = request->camera_;\n> \tCameraData *data = cameraData(camera);\n> \tdata->queuedRequests_.push_back(request);\n> \n> @@ -422,7 +422,6 @@ int PipelineHandler::queueRequest(Camera *camera, Request *request)\n> \n> /**\n> * \\brief Complete a buffer for a request\n> - * \\param[in] camera The camera the request belongs to\n> * \\param[in] request The request the buffer belongs to\n> * \\param[in] buffer The buffer that has completed\n> *\n> @@ -438,16 +437,15 @@ int PipelineHandler::queueRequest(Camera *camera, Request *request)\n> * \\return True if all buffers contained in the request have completed, false\n> * otherwise\n> */\n> -bool PipelineHandler::completeBuffer(Camera *camera, Request *request,\n> -\t\t\t\t FrameBuffer *buffer)\n> +bool PipelineHandler::completeBuffer(Request *request, FrameBuffer *buffer)\n> {\n> +\tCamera *camera = request->camera_;\n> \tcamera->bufferCompleted.emit(request, buffer);\n> \treturn request->completeBuffer(buffer);\n> }\n> \n> /**\n> * \\brief Signal request completion\n> - * \\param[in] camera The camera that the request belongs to\n> * \\param[in] request The request that has completed\n> *\n> * The pipeline handler shall call this method to notify the \\a camera that the\n> @@ -460,8 +458,10 @@ bool PipelineHandler::completeBuffer(Camera *camera, Request *request,\n> *\n> * \\context This function shall be called from the CameraManager thread.\n> */\n> -void PipelineHandler::completeRequest(Camera *camera, Request *request)\n> +void PipelineHandler::completeRequest(Request *request)\n> {\n> +\tCamera *camera = request->camera_;\n> +\n> \trequest->complete();\n> \n> \tCameraData *data = cameraData(camera);\n>","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 8B4FFBE080\n\tfor ;\n\tThu, 5 Nov 2020 11:25:01 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5235F62C7D;\n\tThu, 5 Nov 2020 12:25:01 +0100 (CET)","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 49FCC60353\n\tfor ;\n\tThu, 5 Nov 2020 12:24:59 +0100 (CET)","from [192.168.0.20]\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id BC0F851D;\n\tThu, 5 Nov 2020 12:24:58 +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=\"njqejO3T\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1604575499;\n\tbh=X9tzYfaTqtAeQ97ycMZ5FVP2voudowZCZ2Jq+PPgomM=;\n\th=Reply-To:Subject:To:References:From:Date:In-Reply-To:From;\n\tb=njqejO3TD/S5WmiLqBS0vt5+cMopuCG7t7CP/IBXkIEBLT1JevisYdnjBlx96Jaz5\n\tsZRGoNgfA2iRErzDQKCJVb/d3G/GvjngkR2xbdRqjBR3vIWus81VWuakhpO1FzxDSh\n\ts/R6wcNtwuu4r3NQL/mpS9NcDHuOu/vmqqzkeozU=","To":"=?utf-8?q?Niklas_S=C3=B6derlund?= ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20201105001546.1690179-1-niklas.soderlund@ragnatech.se>\n\t<20201105001546.1690179-2-niklas.soderlund@ragnatech.se>","From":"Kieran Bingham ","Autocrypt":"addr=kieran.bingham@ideasonboard.com; keydata=\n\tmQINBFYE/WYBEACs1PwjMD9rgCu1hlIiUA1AXR4rv2v+BCLUq//vrX5S5bjzxKAryRf0uHat\n\tV/zwz6hiDrZuHUACDB7X8OaQcwhLaVlq6byfoBr25+hbZG7G3+5EUl9cQ7dQEdvNj6V6y/SC\n\trRanWfelwQThCHckbobWiQJfK9n7rYNcPMq9B8e9F020LFH7Kj6YmO95ewJGgLm+idg1Kb3C\n\tpotzWkXc1xmPzcQ1fvQMOfMwdS+4SNw4rY9f07Xb2K99rjMwZVDgESKIzhsDB5GY465sCsiQ\n\tcSAZRxqE49RTBq2+EQsbrQpIc8XiffAB8qexh5/QPzCmR4kJgCGeHIXBtgRj+nIkCJPZvZtf\n\tKr2EAbc6tgg6DkAEHJb+1okosV09+0+TXywYvtEop/WUOWQ+zo+Y/OBd+8Ptgt1pDRyOBzL8\n\tRXa8ZqRf0Mwg75D+dKntZeJHzPRJyrlfQokngAAs4PaFt6UfS+ypMAF37T6CeDArQC41V3ko\n\tlPn1yMsVD0p+6i3DPvA/GPIksDC4owjnzVX9kM8Zc5Cx+XoAN0w5Eqo4t6qEVbuettxx55gq\n\t8K8FieAjgjMSxngo/HST8TpFeqI5nVeq0/lqtBRQKumuIqDg+Bkr4L1V/PSB6XgQcOdhtd36\n\tOe9X9dXB8YSNt7VjOcO7BTmFn/Z8r92mSAfHXpb07YJWJosQOQARAQABtDBLaWVyYW4gQmlu\n\tZ2hhbSA8a2llcmFuLmJpbmdoYW1AaWRlYXNvbmJvYXJkLmNvbT6JAlcEEwEKAEECGwMFCwkI\n\tBwIGFQgJCgsCBBYCAwECHgECF4ACGQEWIQSQLdeYP70o/eNy1HqhHkZyEKRh/QUCXWTtygUJ\n\tCyJXZAAKCRChHkZyEKRh/f8dEACTDsbLN2nioNZMwyLuQRUAFcXNolDX48xcUXsWS2QjxaPm\n\tVsJx8Uy8aYkS85mdPBh0C83OovQR/OVbr8AxhGvYqBs3nQvbWuTl/+4od7DfK2VZOoKBAu5S\n\tQK2FYuUcikDqYcFWJ8DQnubxfE8dvzojHEkXw0sA4igINHDDFX3HJGZtLio+WpEFQtCbfTAG\n\tYZslasz1YZRbwEdSsmO3/kqy5eMnczlm8a21A3fKUo3g8oAZEFM+f4DUNzqIltg31OAB/kZS\n\tenKZQ/SWC8PmLg/ZXBrReYakxXtkP6w3FwMlzOlhGxqhIRNiAJfXJBaRhuUWzPOpEDE9q5YJ\n\tBmqQL2WJm1VSNNVxbXJHpaWMH1sA2R00vmvRrPXGwyIO0IPYeUYQa3gsy6k+En/aMQJd27dp\n\taScf9am9PFICPY5T4ppneeJLif2lyLojo0mcHOV+uyrds9XkLpp14GfTkeKPdPMrLLTsHRfH\n\tfA4I4OBpRrEPiGIZB/0im98MkGY/Mu6qxeZmYLCcgD6qz4idOvfgVOrNh+aA8HzIVR+RMW8H\n\tQGBN9f0E3kfwxuhl3omo6V7lDw8XOdmuWZNC9zPq1UfryVHANYbLGz9KJ4Aw6M+OgBC2JpkD\n\thXMdHUkC+d20dwXrwHTlrJi1YNp6rBc+xald3wsUPOZ5z8moTHUX/uPA/qhGsbkCDQRWBP1m\n\tARAAzijkb+Sau4hAncr1JjOY+KyFEdUNxRy+hqTJdJfaYihxyaj0Ee0P0zEi35CbE6lgU0Uz\n\ttih9fiUbSV3wfsWqg1Ut3/5rTKu7kLFp15kF7eqvV4uezXRD3Qu4yjv/rMmEJbbD4cTvGCYI\n\td6MDC417f7vK3hCbCVIZSp3GXxyC1LU+UQr3fFcOyCwmP9vDUR9JV0BSqHHxRDdpUXE26Dk6\n\tmhf0V1YkspE5St814ETXpEus2urZE5yJIUROlWPIL+hm3NEWfAP06vsQUyLvr/GtbOT79vXl\n\tEn1aulcYyu20dRRxhkQ6iILaURcxIAVJJKPi8dsoMnS8pB0QW12AHWuirPF0g6DiuUfPmrA5\n\tPKe56IGlpkjc8cO51lIxHkWTpCMWigRdPDexKX+Sb+W9QWK/0JjIc4t3KBaiG8O4yRX8ml2R\n\t+rxfAVKM6V769P/hWoRGdgUMgYHFpHGSgEt80OKK5HeUPy2cngDUXzwrqiM5Sz6Od0qw5pCk\n\tNlXqI0W/who0iSVM+8+RmyY0OEkxEcci7rRLsGnM15B5PjLJjh1f2ULYkv8s4SnDwMZ/kE04\n\t/UqCMK/KnX8pwXEMCjz0h6qWNpGwJ0/tYIgQJZh6bqkvBrDogAvuhf60Sogw+mH8b+PBlx1L\n\toeTK396wc+4c3BfiC6pNtUS5GpsPMMjYMk7kVvEAEQEAAYkCPAQYAQoAJgIbDBYhBJAt15g/\n\tvSj943LUeqEeRnIQpGH9BQJdizzIBQkLSKZiAAoJEKEeRnIQpGH9eYgQAJpjaWNgqNOnMTmD\n\tMJggbwjIotypzIXfhHNCeTkG7+qCDlSaBPclcPGYrTwCt0YWPU2TgGgJrVhYT20ierN8LUvj\n\t6qOPTd+Uk7NFzL65qkh80ZKNBFddx1AabQpSVQKbdcLb8OFs85kuSvFdgqZwgxA1vl4TFhNz\n\tPZ79NAmXLackAx3sOVFhk4WQaKRshCB7cSl+RIng5S/ThOBlwNlcKG7j7W2MC06BlTbdEkUp\n\tECzuuRBv8wX4OQl+hbWbB/VKIx5HKlLu1eypen/5lNVzSqMMIYkkZcjV2SWQyUGxSwq0O/sx\n\tS0A8/atCHUXOboUsn54qdxrVDaK+6jIAuo8JiRWctP16KjzUM7MO0/+4zllM8EY57rXrj48j\n\tsbEYX0YQnzaj+jO6kJtoZsIaYR7rMMq9aUAjyiaEZpmP1qF/2sYenDx0Fg2BSlLvLvXM0vU8\n\tpQk3kgDu7kb/7PRYrZvBsr21EIQoIjXbZxDz/o7z95frkP71EaICttZ6k9q5oxxA5WC6sTXc\n\tMW8zs8avFNuA9VpXt0YupJd2ijtZy2mpZNG02fFVXhIn4G807G7+9mhuC4XG5rKlBBUXTvPU\n\tAfYnB4JBDLmLzBFavQfvonSfbitgXwCG3vS+9HEwAjU30Bar1PEOmIbiAoMzuKeRm2LVpmq4\n\tWZw01QYHU/GUV/zHJSFk","Organization":"Ideas on Board","Message-ID":"","Date":"Thu, 5 Nov 2020 11:24:56 +0000","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101\n\tThunderbird/68.10.0","MIME-Version":"1.0","In-Reply-To":"<20201105001546.1690179-2-niklas.soderlund@ragnatech.se>","Content-Language":"en-GB","Subject":"Re: [libcamera-devel] [PATCH 01/11] libcamera: pipeline_handler:\n\tRemove Camera argument from request handling","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","Reply-To":"kieran.bingham@ideasonboard.com","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":13634,"web_url":"https://patchwork.libcamera.org/comment/13634/","msgid":"<20201109095547.nf2qiicsqspc3g4q@uno.localdomain>","date":"2020-11-09T09:55:47","subject":"Re: [libcamera-devel] [PATCH 01/11] libcamera: pipeline_handler:\n\tRemove Camera argument from request handling","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Niklas,\n\nOn Thu, Nov 05, 2020 at 01:15:36AM +0100, Niklas Söderlund wrote:\n> There is no need to pass the Camera pointer to queueRequest(),\n> completeBuffer() and completeRequest() as the Request also passed\n> contains the same information. Remove the Camera argument to avoid\n> situations where the information in the Request and the argument differ.\n>\n> There is no functional change and no public API change as the interface\n> is only used between the Camera and PipelineHandler.\n>\n> Signed-off-by: Niklas Söderlund \n\nThanks\nReviewed-by: Jacopo Mondi \n\n> ---\n> include/libcamera/internal/pipeline_handler.h | 7 +++----\n> src/libcamera/camera.cpp | 2 +-\n> src/libcamera/pipeline/ipu3/ipu3.cpp | 8 ++++----\n> src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 8 ++++----\n> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 5 ++---\n> src/libcamera/pipeline/simple/simple.cpp | 12 ++++++------\n> src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 4 ++--\n> src/libcamera/pipeline/vimc/vimc.cpp | 4 ++--\n> src/libcamera/pipeline_handler.cpp | 14 +++++++-------\n> 9 files changed, 31 insertions(+), 33 deletions(-)\n>\n> diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h\n> index c12c8904858e7536..ca1997bc0771b139 100644\n> --- a/include/libcamera/internal/pipeline_handler.h\n> +++ b/include/libcamera/internal/pipeline_handler.h\n> @@ -81,11 +81,10 @@ public:\n> \tvirtual int start(Camera *camera) = 0;\n> \tvirtual void stop(Camera *camera) = 0;\n>\n> -\tint queueRequest(Camera *camera, Request *request);\n> +\tint queueRequest(Request *request);\n>\n> -\tbool completeBuffer(Camera *camera, Request *request,\n> -\t\t\t FrameBuffer *buffer);\n> -\tvoid completeRequest(Camera *camera, Request *request);\n> +\tbool completeBuffer(Request *request, FrameBuffer *buffer);\n> +\tvoid completeRequest(Request *request);\n>\n> \tconst char *name() const { return name_; }\n>\n> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> index 9590ab7249d39e2f..f868161f2a620264 100644\n> --- a/src/libcamera/camera.cpp\n> +++ b/src/libcamera/camera.cpp\n> @@ -914,7 +914,7 @@ int Camera::queueRequest(Request *request)\n> \t}\n>\n> \treturn p_->pipe_->invokeMethod(&PipelineHandler::queueRequest,\n> -\t\t\t\t ConnectionTypeQueued, this, request);\n> +\t\t\t\t ConnectionTypeQueued, request);\n> }\n>\n> /**\n> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> index 5a6ee1a83e45ff75..3f0232bc1eaad048 100644\n> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> @@ -836,13 +836,13 @@ void IPU3CameraData::imguOutputBufferReady(FrameBuffer *buffer)\n> {\n> \tRequest *request = buffer->request();\n>\n> -\tif (!pipe_->completeBuffer(camera_, request, buffer))\n> +\tif (!pipe_->completeBuffer(request, buffer))\n> \t\t/* Request not completed yet, return here. */\n> \t\treturn;\n>\n> \t/* Mark the request as complete. */\n> \trequest->metadata().set(controls::draft::PipelineDepth, 3);\n> -\tpipe_->completeRequest(camera_, request);\n> +\tpipe_->completeRequest(request);\n> }\n>\n> /**\n> @@ -865,10 +865,10 @@ void IPU3CameraData::cio2BufferReady(FrameBuffer *buffer)\n> \t * now as there's no need for ImgU processing.\n> \t */\n> \tif (request->findBuffer(&rawStream_)) {\n> -\t\tbool isComplete = pipe_->completeBuffer(camera_, request, buffer);\n> +\t\tbool isComplete = pipe_->completeBuffer(request, buffer);\n> \t\tif (isComplete) {\n> \t\t\trequest->metadata().set(controls::draft::PipelineDepth, 2);\n> -\t\t\tpipe_->completeRequest(camera_, request);\n> +\t\t\tpipe_->completeRequest(request);\n> \t\t\treturn;\n> \t\t}\n> \t}\n> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> index 220b9c51849662f3..630b876985459d2f 100644\n> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> @@ -1463,10 +1463,10 @@ void RPiCameraData::clearIncompleteRequests()\n> \t\t\t * request? If not, do so now.\n> \t\t\t */\n> \t\t\tif (buffer && buffer->request())\n> -\t\t\t\tpipe_->completeBuffer(camera_, request, buffer);\n> +\t\t\t\tpipe_->completeBuffer(request, buffer);\n> \t\t}\n>\n> -\t\tpipe_->completeRequest(camera_, request);\n> +\t\tpipe_->completeRequest(request);\n> \t\trequestQueue_.pop_front();\n> \t}\n> }\n> @@ -1490,7 +1490,7 @@ void RPiCameraData::handleStreamBuffer(FrameBuffer *buffer, RPi::Stream *stream)\n> \t\t\t * Tag the buffer as completed, returning it to the\n> \t\t\t * application.\n> \t\t\t */\n> -\t\t\tpipe_->completeBuffer(camera_, request, buffer);\n> +\t\t\tpipe_->completeBuffer(request, buffer);\n> \t\t} else {\n> \t\t\t/*\n> \t\t\t * This buffer was not part of the Request, or there is no\n> @@ -1554,7 +1554,7 @@ void RPiCameraData::checkRequestCompleted()\n> \t\tif (state_ != State::IpaComplete)\n> \t\t\treturn;\n>\n> -\t\tpipe_->completeRequest(camera_, request);\n> +\t\tpipe_->completeRequest(request);\n> \t\trequestQueue_.pop_front();\n> \t\trequestCompleted = true;\n> \t}\n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> index 424099d087528fb1..70a10853bf83b6a5 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> @@ -1042,15 +1042,14 @@ void PipelineHandlerRkISP1::tryCompleteRequest(Request *request)\n>\n> \tdata->frameInfo_.destroy(info->frame);\n>\n> -\tcompleteRequest(activeCamera_, request);\n> +\tcompleteRequest(request);\n> }\n>\n> void PipelineHandlerRkISP1::bufferReady(FrameBuffer *buffer)\n> {\n> -\tASSERT(activeCamera_);\n> \tRequest *request = buffer->request();\n>\n> -\tcompleteBuffer(activeCamera_, request, buffer);\n> +\tcompleteBuffer(request, buffer);\n> \ttryCompleteRequest(request);\n> }\n>\n> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp\n> index 3d2039f3f26951bf..dc55304fc6ebb202 100644\n> --- a/src/libcamera/pipeline/simple/simple.cpp\n> +++ b/src/libcamera/pipeline/simple/simple.cpp\n> @@ -903,8 +903,8 @@ void SimplePipelineHandler::bufferReady(FrameBuffer *buffer)\n> \t\t}\n>\n> \t\tRequest *request = buffer->request();\n> -\t\tcompleteBuffer(activeCamera_, request, buffer);\n> -\t\tcompleteRequest(activeCamera_, request);\n> +\t\tcompleteBuffer(request, buffer);\n> +\t\tcompleteRequest(request);\n> \t\treturn;\n> \t}\n>\n> @@ -928,8 +928,8 @@ void SimplePipelineHandler::bufferReady(FrameBuffer *buffer)\n>\n> \t/* Otherwise simply complete the request. */\n> \tRequest *request = buffer->request();\n> -\tcompleteBuffer(activeCamera_, request, buffer);\n> -\tcompleteRequest(activeCamera_, request);\n> +\tcompleteBuffer(request, buffer);\n> +\tcompleteRequest(request);\n> }\n>\n> void SimplePipelineHandler::converterDone(FrameBuffer *input,\n> @@ -940,8 +940,8 @@ void SimplePipelineHandler::converterDone(FrameBuffer *input,\n>\n> \t/* Complete the request. */\n> \tRequest *request = output->request();\n> -\tcompleteBuffer(activeCamera_, request, output);\n> -\tcompleteRequest(activeCamera_, request);\n> +\tcompleteBuffer(request, output);\n> +\tcompleteRequest(request);\n>\n> \t/* Queue the input buffer back for capture. */\n> \tdata->video_->queueBuffer(input);\n> diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> index 3862631b7bedf604..272e4c6bf50e82e4 100644\n> --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> @@ -650,8 +650,8 @@ void UVCCameraData::bufferReady(FrameBuffer *buffer)\n> {\n> \tRequest *request = buffer->request();\n>\n> -\tpipe_->completeBuffer(camera_, request, buffer);\n> -\tpipe_->completeRequest(camera_, request);\n> +\tpipe_->completeBuffer(request, buffer);\n> +\tpipe_->completeRequest(request);\n> }\n>\n> REGISTER_PIPELINE_HANDLER(PipelineHandlerUVC)\n> diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp\n> index 7416c37c6f5a243c..99b0fa2742269147 100644\n> --- a/src/libcamera/pipeline/vimc/vimc.cpp\n> +++ b/src/libcamera/pipeline/vimc/vimc.cpp\n> @@ -529,8 +529,8 @@ void VimcCameraData::bufferReady(FrameBuffer *buffer)\n> {\n> \tRequest *request = buffer->request();\n>\n> -\tpipe_->completeBuffer(camera_, request, buffer);\n> -\tpipe_->completeRequest(camera_, request);\n> +\tpipe_->completeBuffer(request, buffer);\n> +\tpipe_->completeRequest(request);\n> }\n>\n> REGISTER_PIPELINE_HANDLER(PipelineHandlerVimc)\n> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n> index 894200ee4e936735..e60b962d66183e19 100644\n> --- a/src/libcamera/pipeline_handler.cpp\n> +++ b/src/libcamera/pipeline_handler.cpp\n> @@ -376,7 +376,6 @@ const ControlList &PipelineHandler::properties(const Camera *camera) const\n> /**\n> * \\fn PipelineHandler::queueRequest()\n> * \\brief Queue a request to the camera\n> - * \\param[in] camera The camera to queue the request to\n> * \\param[in] request The request to queue\n> *\n> * This method queues a capture request to the pipeline handler for processing.\n> @@ -391,8 +390,9 @@ const ControlList &PipelineHandler::properties(const Camera *camera) const\n> *\n> * \\return 0 on success or a negative error code otherwise\n> */\n> -int PipelineHandler::queueRequest(Camera *camera, Request *request)\n> +int PipelineHandler::queueRequest(Request *request)\n> {\n> +\tCamera *camera = request->camera_;\n> \tCameraData *data = cameraData(camera);\n> \tdata->queuedRequests_.push_back(request);\n>\n> @@ -422,7 +422,6 @@ int PipelineHandler::queueRequest(Camera *camera, Request *request)\n>\n> /**\n> * \\brief Complete a buffer for a request\n> - * \\param[in] camera The camera the request belongs to\n> * \\param[in] request The request the buffer belongs to\n> * \\param[in] buffer The buffer that has completed\n> *\n> @@ -438,16 +437,15 @@ int PipelineHandler::queueRequest(Camera *camera, Request *request)\n> * \\return True if all buffers contained in the request have completed, false\n> * otherwise\n> */\n> -bool PipelineHandler::completeBuffer(Camera *camera, Request *request,\n> -\t\t\t\t FrameBuffer *buffer)\n> +bool PipelineHandler::completeBuffer(Request *request, FrameBuffer *buffer)\n> {\n> +\tCamera *camera = request->camera_;\n> \tcamera->bufferCompleted.emit(request, buffer);\n> \treturn request->completeBuffer(buffer);\n> }\n>\n> /**\n> * \\brief Signal request completion\n> - * \\param[in] camera The camera that the request belongs to\n> * \\param[in] request The request that has completed\n> *\n> * The pipeline handler shall call this method to notify the \\a camera that the\n> @@ -460,8 +458,10 @@ bool PipelineHandler::completeBuffer(Camera *camera, Request *request,\n> *\n> * \\context This function shall be called from the CameraManager thread.\n> */\n> -void PipelineHandler::completeRequest(Camera *camera, Request *request)\n> +void PipelineHandler::completeRequest(Request *request)\n> {\n> +\tCamera *camera = request->camera_;\n> +\n> \trequest->complete();\n>\n> \tCameraData *data = cameraData(camera);\n> --\n> 2.29.2\n>\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","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 AE833BE082\n\tfor ;\n\tMon, 9 Nov 2020 09:55:48 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 29D7763069;\n\tMon, 9 Nov 2020 10:55:48 +0100 (CET)","from relay9-d.mail.gandi.net (relay9-d.mail.gandi.net\n\t[217.70.183.199])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 4C67A60341\n\tfor ;\n\tMon, 9 Nov 2020 10:55:46 +0100 (CET)","from uno.localdomain (93-34-118-233.ip49.fastwebnet.it\n\t[93.34.118.233]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay9-d.mail.gandi.net (Postfix) with ESMTPSA id AC6A9FF80C;\n\tMon, 9 Nov 2020 09:55:45 +0000 (UTC)"],"X-Originating-IP":"93.34.118.233","Date":"Mon, 9 Nov 2020 10:55:47 +0100","From":"Jacopo Mondi ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Message-ID":"<20201109095547.nf2qiicsqspc3g4q@uno.localdomain>","References":"<20201105001546.1690179-1-niklas.soderlund@ragnatech.se>\n\t<20201105001546.1690179-2-niklas.soderlund@ragnatech.se>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20201105001546.1690179-2-niklas.soderlund@ragnatech.se>","Subject":"Re: [libcamera-devel] [PATCH 01/11] libcamera: pipeline_handler:\n\tRemove Camera argument from request handling","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","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":14110,"web_url":"https://patchwork.libcamera.org/comment/14110/","msgid":"","date":"2020-12-08T01:04:53","subject":"Re: [libcamera-devel] [PATCH 01/11] libcamera: pipeline_handler:\n\tRemove Camera argument from request handling","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Niklas,\n\nThank you for the patch.\n\nOn Thu, Nov 05, 2020 at 01:15:36AM +0100, Niklas Söderlund wrote:\n> There is no need to pass the Camera pointer to queueRequest(),\n> completeBuffer() and completeRequest() as the Request also passed\n> contains the same information. Remove the Camera argument to avoid\n> situations where the information in the Request and the argument differ.\n\nWe could debate which of the explicit or implicit passing of camera\nproduces the best code, but this removes one potential cause of\ninconsistencies, so I think it's good.\n\n> There is no functional change and no public API change as the interface\n> is only used between the Camera and PipelineHandler.\n> \n> Signed-off-by: Niklas Söderlund \n> ---\n> include/libcamera/internal/pipeline_handler.h | 7 +++----\n> src/libcamera/camera.cpp | 2 +-\n> src/libcamera/pipeline/ipu3/ipu3.cpp | 8 ++++----\n> src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 8 ++++----\n> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 5 ++---\n> src/libcamera/pipeline/simple/simple.cpp | 12 ++++++------\n> src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 4 ++--\n> src/libcamera/pipeline/vimc/vimc.cpp | 4 ++--\n> src/libcamera/pipeline_handler.cpp | 14 +++++++-------\n> 9 files changed, 31 insertions(+), 33 deletions(-)\n\nCould you please also update the pipeline handler guide ? With that,\n\n> diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h\n> index c12c8904858e7536..ca1997bc0771b139 100644\n> --- a/include/libcamera/internal/pipeline_handler.h\n> +++ b/include/libcamera/internal/pipeline_handler.h\n> @@ -81,11 +81,10 @@ public:\n> \tvirtual int start(Camera *camera) = 0;\n> \tvirtual void stop(Camera *camera) = 0;\n> \n> -\tint queueRequest(Camera *camera, Request *request);\n> +\tint queueRequest(Request *request);\n> \n> -\tbool completeBuffer(Camera *camera, Request *request,\n> -\t\t\t FrameBuffer *buffer);\n> -\tvoid completeRequest(Camera *camera, Request *request);\n> +\tbool completeBuffer(Request *request, FrameBuffer *buffer);\n> +\tvoid completeRequest(Request *request);\n> \n> \tconst char *name() const { return name_; }\n> \n> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> index 9590ab7249d39e2f..f868161f2a620264 100644\n> --- a/src/libcamera/camera.cpp\n> +++ b/src/libcamera/camera.cpp\n> @@ -914,7 +914,7 @@ int Camera::queueRequest(Request *request)\n> \t}\n> \n> \treturn p_->pipe_->invokeMethod(&PipelineHandler::queueRequest,\n> -\t\t\t\t ConnectionTypeQueued, this, request);\n> +\t\t\t\t ConnectionTypeQueued, request);\n> }\n> \n> /**\n> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> index 5a6ee1a83e45ff75..3f0232bc1eaad048 100644\n> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> @@ -836,13 +836,13 @@ void IPU3CameraData::imguOutputBufferReady(FrameBuffer *buffer)\n> {\n> \tRequest *request = buffer->request();\n> \n> -\tif (!pipe_->completeBuffer(camera_, request, buffer))\n> +\tif (!pipe_->completeBuffer(request, buffer))\n\nI wonder if some of the member variables, such as camera_ in this case,\nare set be otherwise not used after this patch. Could you make sure that\nany such variable gets removed ?\n\n> \t\t/* Request not completed yet, return here. */\n> \t\treturn;\n> \n> \t/* Mark the request as complete. */\n> \trequest->metadata().set(controls::draft::PipelineDepth, 3);\n> -\tpipe_->completeRequest(camera_, request);\n> +\tpipe_->completeRequest(request);\n> }\n> \n> /**\n> @@ -865,10 +865,10 @@ void IPU3CameraData::cio2BufferReady(FrameBuffer *buffer)\n> \t * now as there's no need for ImgU processing.\n> \t */\n> \tif (request->findBuffer(&rawStream_)) {\n> -\t\tbool isComplete = pipe_->completeBuffer(camera_, request, buffer);\n> +\t\tbool isComplete = pipe_->completeBuffer(request, buffer);\n> \t\tif (isComplete) {\n> \t\t\trequest->metadata().set(controls::draft::PipelineDepth, 2);\n> -\t\t\tpipe_->completeRequest(camera_, request);\n> +\t\t\tpipe_->completeRequest(request);\n> \t\t\treturn;\n> \t\t}\n> \t}\n> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> index 220b9c51849662f3..630b876985459d2f 100644\n> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> @@ -1463,10 +1463,10 @@ void RPiCameraData::clearIncompleteRequests()\n> \t\t\t * request? If not, do so now.\n> \t\t\t */\n> \t\t\tif (buffer && buffer->request())\n> -\t\t\t\tpipe_->completeBuffer(camera_, request, buffer);\n> +\t\t\t\tpipe_->completeBuffer(request, buffer);\n> \t\t}\n> \n> -\t\tpipe_->completeRequest(camera_, request);\n> +\t\tpipe_->completeRequest(request);\n> \t\trequestQueue_.pop_front();\n> \t}\n> }\n> @@ -1490,7 +1490,7 @@ void RPiCameraData::handleStreamBuffer(FrameBuffer *buffer, RPi::Stream *stream)\n> \t\t\t * Tag the buffer as completed, returning it to the\n> \t\t\t * application.\n> \t\t\t */\n> -\t\t\tpipe_->completeBuffer(camera_, request, buffer);\n> +\t\t\tpipe_->completeBuffer(request, buffer);\n> \t\t} else {\n> \t\t\t/*\n> \t\t\t * This buffer was not part of the Request, or there is no\n> @@ -1554,7 +1554,7 @@ void RPiCameraData::checkRequestCompleted()\n> \t\tif (state_ != State::IpaComplete)\n> \t\t\treturn;\n> \n> -\t\tpipe_->completeRequest(camera_, request);\n> +\t\tpipe_->completeRequest(request);\n> \t\trequestQueue_.pop_front();\n> \t\trequestCompleted = true;\n> \t}\n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> index 424099d087528fb1..70a10853bf83b6a5 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> @@ -1042,15 +1042,14 @@ void PipelineHandlerRkISP1::tryCompleteRequest(Request *request)\n> \n> \tdata->frameInfo_.destroy(info->frame);\n> \n> -\tcompleteRequest(activeCamera_, request);\n> +\tcompleteRequest(request);\n> }\n> \n> void PipelineHandlerRkISP1::bufferReady(FrameBuffer *buffer)\n> {\n> -\tASSERT(activeCamera_);\n> \tRequest *request = buffer->request();\n> \n> -\tcompleteBuffer(activeCamera_, request, buffer);\n> +\tcompleteBuffer(request, buffer);\n> \ttryCompleteRequest(request);\n> }\n> \n> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp\n> index 3d2039f3f26951bf..dc55304fc6ebb202 100644\n> --- a/src/libcamera/pipeline/simple/simple.cpp\n> +++ b/src/libcamera/pipeline/simple/simple.cpp\n> @@ -903,8 +903,8 @@ void SimplePipelineHandler::bufferReady(FrameBuffer *buffer)\n> \t\t}\n> \n> \t\tRequest *request = buffer->request();\n> -\t\tcompleteBuffer(activeCamera_, request, buffer);\n> -\t\tcompleteRequest(activeCamera_, request);\n> +\t\tcompleteBuffer(request, buffer);\n> +\t\tcompleteRequest(request);\n> \t\treturn;\n> \t}\n> \n> @@ -928,8 +928,8 @@ void SimplePipelineHandler::bufferReady(FrameBuffer *buffer)\n> \n> \t/* Otherwise simply complete the request. */\n> \tRequest *request = buffer->request();\n> -\tcompleteBuffer(activeCamera_, request, buffer);\n> -\tcompleteRequest(activeCamera_, request);\n> +\tcompleteBuffer(request, buffer);\n> +\tcompleteRequest(request);\n> }\n> \n> void SimplePipelineHandler::converterDone(FrameBuffer *input,\n> @@ -940,8 +940,8 @@ void SimplePipelineHandler::converterDone(FrameBuffer *input,\n> \n> \t/* Complete the request. */\n> \tRequest *request = output->request();\n> -\tcompleteBuffer(activeCamera_, request, output);\n> -\tcompleteRequest(activeCamera_, request);\n> +\tcompleteBuffer(request, output);\n> +\tcompleteRequest(request);\n> \n> \t/* Queue the input buffer back for capture. */\n> \tdata->video_->queueBuffer(input);\n> diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> index 3862631b7bedf604..272e4c6bf50e82e4 100644\n> --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> @@ -650,8 +650,8 @@ void UVCCameraData::bufferReady(FrameBuffer *buffer)\n> {\n> \tRequest *request = buffer->request();\n> \n> -\tpipe_->completeBuffer(camera_, request, buffer);\n> -\tpipe_->completeRequest(camera_, request);\n> +\tpipe_->completeBuffer(request, buffer);\n> +\tpipe_->completeRequest(request);\n> }\n> \n> REGISTER_PIPELINE_HANDLER(PipelineHandlerUVC)\n> diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp\n> index 7416c37c6f5a243c..99b0fa2742269147 100644\n> --- a/src/libcamera/pipeline/vimc/vimc.cpp\n> +++ b/src/libcamera/pipeline/vimc/vimc.cpp\n> @@ -529,8 +529,8 @@ void VimcCameraData::bufferReady(FrameBuffer *buffer)\n> {\n> \tRequest *request = buffer->request();\n> \n> -\tpipe_->completeBuffer(camera_, request, buffer);\n> -\tpipe_->completeRequest(camera_, request);\n> +\tpipe_->completeBuffer(request, buffer);\n> +\tpipe_->completeRequest(request);\n> }\n> \n> REGISTER_PIPELINE_HANDLER(PipelineHandlerVimc)\n> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n> index 894200ee4e936735..e60b962d66183e19 100644\n> --- a/src/libcamera/pipeline_handler.cpp\n> +++ b/src/libcamera/pipeline_handler.cpp\n> @@ -376,7 +376,6 @@ const ControlList &PipelineHandler::properties(const Camera *camera) const\n> /**\n> * \\fn PipelineHandler::queueRequest()\n> * \\brief Queue a request to the camera\n\nMaybe s/ to the camera// ?\n\n> - * \\param[in] camera The camera to queue the request to\n> * \\param[in] request The request to queue\n> *\n> * This method queues a capture request to the pipeline handler for processing.\n> @@ -391,8 +390,9 @@ const ControlList &PipelineHandler::properties(const Camera *camera) const\n> *\n> * \\return 0 on success or a negative error code otherwise\n> */\n> -int PipelineHandler::queueRequest(Camera *camera, Request *request)\n> +int PipelineHandler::queueRequest(Request *request)\n> {\n> +\tCamera *camera = request->camera_;\n\nWe should then revisit whether an accessor for Request::camera_ should\nbe added. The downside of exposing it to applications would be that we\ncould have issue with the lifetime of the borrowed reference if\napplications access the field in an incorrect location.\n\nWith these small issues addressed (in particular the pipeline handler\nguide),\n\nReviewed-by: Laurent Pinchart \n\n> \tCameraData *data = cameraData(camera);\n> \tdata->queuedRequests_.push_back(request);\n> \n> @@ -422,7 +422,6 @@ int PipelineHandler::queueRequest(Camera *camera, Request *request)\n> \n> /**\n> * \\brief Complete a buffer for a request\n> - * \\param[in] camera The camera the request belongs to\n> * \\param[in] request The request the buffer belongs to\n> * \\param[in] buffer The buffer that has completed\n> *\n> @@ -438,16 +437,15 @@ int PipelineHandler::queueRequest(Camera *camera, Request *request)\n> * \\return True if all buffers contained in the request have completed, false\n> * otherwise\n> */\n> -bool PipelineHandler::completeBuffer(Camera *camera, Request *request,\n> -\t\t\t\t FrameBuffer *buffer)\n> +bool PipelineHandler::completeBuffer(Request *request, FrameBuffer *buffer)\n> {\n> +\tCamera *camera = request->camera_;\n> \tcamera->bufferCompleted.emit(request, buffer);\n> \treturn request->completeBuffer(buffer);\n> }\n> \n> /**\n> * \\brief Signal request completion\n> - * \\param[in] camera The camera that the request belongs to\n> * \\param[in] request The request that has completed\n> *\n> * The pipeline handler shall call this method to notify the \\a camera that the\n> @@ -460,8 +458,10 @@ bool PipelineHandler::completeBuffer(Camera *camera, Request *request,\n> *\n> * \\context This function shall be called from the CameraManager thread.\n> */\n> -void PipelineHandler::completeRequest(Camera *camera, Request *request)\n> +void PipelineHandler::completeRequest(Request *request)\n> {\n> +\tCamera *camera = request->camera_;\n> +\n> \trequest->complete();\n> \n> \tCameraData *data = cameraData(camera);","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 51992BDB1F\n\tfor ;\n\tTue, 8 Dec 2020 01:04:59 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7E11167E6E;\n\tTue, 8 Dec 2020 02:04:58 +0100 (CET)","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 1582B67E6B\n\tfor ;\n\tTue, 8 Dec 2020 02:04:57 +0100 (CET)","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 6887BDD;\n\tTue, 8 Dec 2020 02:04:56 +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=\"O3qxSeUX\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1607389496;\n\tbh=KTFka9z2oYQDwRrGBEbNVqqgDpmPgVlU20FoS/y0eY4=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=O3qxSeUXqgtBtq4yGAFamjeZWem4YtP4MekRjMB8wjhDhnPBdLMdlJnXxOtrqXHyK\n\tewUYcUoFtD6Mt1+6M3bPO+ggJrc9h5KTX0z32guB0Mh7nm7AlwwL7fXsXj8kt7kY1i\n\tlogUUnSNzAGu2dJpWcLRn/5KWqmxwzO+ybiT9ObI=","Date":"Tue, 8 Dec 2020 03:04:53 +0200","From":"Laurent Pinchart ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Message-ID":"","References":"<20201105001546.1690179-1-niklas.soderlund@ragnatech.se>\n\t<20201105001546.1690179-2-niklas.soderlund@ragnatech.se>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20201105001546.1690179-2-niklas.soderlund@ragnatech.se>","Subject":"Re: [libcamera-devel] [PATCH 01/11] libcamera: pipeline_handler:\n\tRemove Camera argument from request handling","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","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]