[{"id":17112,"web_url":"https://patchwork.libcamera.org/comment/17112/","msgid":"","date":"2021-05-22T09:30:23","subject":"Re: [libcamera-devel] [PATCH v3 3/8] libcamera: pipeline_handler:\n\tCancel Request on queueing failure","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Jacopo,\n\nThanks for your work.\n\nOn 2021-05-21 17:42:22 +0200, Jacopo Mondi wrote:\n> Capture requests are queued by the PipelineHandler base class to each\n> pipeline handler implementation using the virtual queueRequestDevice()\n> function.\n> \n> However, if the pipeline handler fails to queue the request to the\n> hardware, the request gets silently deleted from the list of queued\n> ones, without notifying application of the error.\n> \n> Reporting to applications that a Request has failed to queue by\n> cancelling and then completing it allows applications to maintain their\n> request-tracking mechanism consistent with the one internal to the library.\n\nI think this touches on a larger issue, PipelineHandler::queueRequest() \nshould really return something to the caller about if it succeeded or \nnot. We have had similar needs in the past. But that is above the intent \nof this change so,\n\nReviewed-by: Niklas Söderlund \n\n> \n> Signed-off-by: Jacopo Mondi \n> ---\n> src/libcamera/pipeline_handler.cpp | 8 ++++++--\n> 1 file changed, 6 insertions(+), 2 deletions(-)\n> \n> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n> index f41b7a7b3308..e507a8bba8a6 100644\n> --- a/src/libcamera/pipeline_handler.cpp\n> +++ b/src/libcamera/pipeline_handler.cpp\n> @@ -391,6 +391,8 @@ bool PipelineHandler::hasPendingRequests(const Camera *camera) const\n> * This method queues a capture request to the pipeline handler for processing.\n> * The request is first added to the internal list of queued requests, and\n> * then passed to the pipeline handler with a call to queueRequestDevice().\n> + * If the pipeline handler fails in queuing the request to the hardware the\n> + * request is cancelled.\n> *\n> * Keeping track of queued requests ensures automatic completion of all requests\n> * when the pipeline handler is stopped with stop(). Request completion shall be\n> @@ -409,8 +411,10 @@ void PipelineHandler::queueRequest(Request *request)\n> \trequest->sequence_ = data->requestSequence_++;\n> \n> \tint ret = queueRequestDevice(camera, request);\n> -\tif (ret)\n> -\t\tdata->queuedRequests_.remove(request);\n> +\tif (ret) {\n> +\t\trequest->cancel();\n> +\t\tcompleteRequest(request);\n> +\t}\n> }\n> \n> /**\n> -- \n> 2.31.1\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 6A3C4C3200\n\tfor ;\n\tSat, 22 May 2021 09:30:27 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id DDA7D602B1;\n\tSat, 22 May 2021 11:30:26 +0200 (CEST)","from mail-lj1-x233.google.com (mail-lj1-x233.google.com\n\t[IPv6:2a00:1450:4864:20::233])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 402AE602B0\n\tfor ;\n\tSat, 22 May 2021 11:30:25 +0200 (CEST)","by mail-lj1-x233.google.com with SMTP id b12so19632226ljp.1\n\tfor ;\n\tSat, 22 May 2021 02:30:25 -0700 (PDT)","from localhost (h-155-4-209-203.A463.priv.bahnhof.se.\n\t[155.4.209.203]) by smtp.gmail.com with ESMTPSA id\n\tz13sm919509lji.115.2021.05.22.02.30.24\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tSat, 22 May 2021 02:30:24 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key;\n\tunprotected) header.d=ragnatech-se.20150623.gappssmtp.com\n\theader.i=@ragnatech-se.20150623.gappssmtp.com\n\theader.b=\"TBLtJ7qc\"; dkim-atps=neutral","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\tbh=pg4D0s/GE3lM6JhVOvc3FXMcvLApcJguW1fUldMdUrY=;\n\tb=TBLtJ7qcNZrH7vARYpJZ5zWbJvWcnHMIo2xqw7zWXuLSE+/Sp7GDykG+9bVTy8NTvo\n\tQhDWWtOV4tvl1odr2Z10f063dOkqVxgqgg/RYmoZdPQwErAGWgVrI7mTXI/MyPyxBnhY\n\tNRwH6omPcn0eAeeevZK9u5iSGjiCFoad5kHEXJGYJEuMQGEZp5oVb7iRAppQrSU3F6Fx\n\t1FGh8LN9YaiGXFHC/JihkzbvU6b0g4ruunfhlL05eM55CW2dgb1ECV9CB8sovKTwPo+k\n\tOjRSzx3iZGbPa5rUijsWb1ntXkjfPnJrwhWVeAsDNA96ygI0xvheLkv/FoQoTzMVF2Vi\n\tdJww==","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;\n\tbh=pg4D0s/GE3lM6JhVOvc3FXMcvLApcJguW1fUldMdUrY=;\n\tb=hZoDXufzpbClcaR8oGZoy4ZzGFf63Zx1sfbl1gE+KmiyLuacBdRiXnzrT8Lxh4+ZkA\n\tRWJx7B9QfamVe+DNKiU0V4HBPR2yqyQjEgbcIxqQJ9oVI11v3o5Op0zHP+Nvc0wV0RnZ\n\t0yR1i0eipuPBEe4apAV95LUEPfb341MaEXRwmNYXdVx8mpKLkc6r4LypvQZMlInDiDbb\n\tZH8BuTkIWBN/2jboQpHccef052uBw3bAufb4Gj++CDS4DOnEPMxWXizOinQohknhTZAv\n\tJPcTebrHmczLSWFA1VINxBVd7Mt0ivIRlrDPnjEOH845Yel9jiGX4u+iH2gghzuEgzox\n\t2gyQ==","X-Gm-Message-State":"AOAM532P3UMF4fB+7dJzrkFzWR63JVxxG8V10Y9e9OsZSyxdoK5tHn3y\n\tZGsQQRYI8gMk2UmMZ5Vz6OlnDXC2CCBHVg==","X-Google-Smtp-Source":"ABdhPJzf7XppMgBwoM7RLODq6WW21g4Gkr6ndsdVxh2xMoeCfn+8Tkv0wUClQlYKsihCNWCKWSIptA==","X-Received":"by 2002:a05:651c:292:: with SMTP id\n\tb18mr9433500ljo.456.1621675824674; \n\tSat, 22 May 2021 02:30:24 -0700 (PDT)","Date":"Sat, 22 May 2021 11:30:23 +0200","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Jacopo Mondi ","Message-ID":"","References":"<20210521154227.60186-1-jacopo@jmondi.org>\n\t<20210521154227.60186-4-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=iso-8859-1","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20210521154227.60186-4-jacopo@jmondi.org>","Subject":"Re: [libcamera-devel] [PATCH v3 3/8] libcamera: pipeline_handler:\n\tCancel Request on queueing failure","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":17136,"web_url":"https://patchwork.libcamera.org/comment/17136/","msgid":"","date":"2021-05-23T17:59:44","subject":"Re: [libcamera-devel] [PATCH v3 3/8] libcamera: pipeline_handler:\n\tCancel Request on queueing failure","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Niklas and Jacopo,\n\nOn Sat, May 22, 2021 at 11:30:23AM +0200, Niklas Söderlund wrote:\n> On 2021-05-21 17:42:22 +0200, Jacopo Mondi wrote:\n> > Capture requests are queued by the PipelineHandler base class to each\n> > pipeline handler implementation using the virtual queueRequestDevice()\n> > function.\n> > \n> > However, if the pipeline handler fails to queue the request to the\n> > hardware, the request gets silently deleted from the list of queued\n> > ones, without notifying application of the error.\n> > \n> > Reporting to applications that a Request has failed to queue by\n> > cancelling and then completing it allows applications to maintain their\n> > request-tracking mechanism consistent with the one internal to the library.\n> \n> I think this touches on a larger issue, PipelineHandler::queueRequest() \n> should really return something to the caller about if it succeeded or \n> not. We have had similar needs in the past. But that is above the intent \n> of this change so,\n\nPipelineHandler::queueRequest() is called asynchronously, so it can't\nreturn an error to Camera::queueRequest(). That's why errors have to be\nhandled internally.\n\n> Reviewed-by: Niklas Söderlund \n> \n> > Signed-off-by: Jacopo Mondi \n> > ---\n> > src/libcamera/pipeline_handler.cpp | 8 ++++++--\n> > 1 file changed, 6 insertions(+), 2 deletions(-)\n> > \n> > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n> > index f41b7a7b3308..e507a8bba8a6 100644\n> > --- a/src/libcamera/pipeline_handler.cpp\n> > +++ b/src/libcamera/pipeline_handler.cpp\n> > @@ -391,6 +391,8 @@ bool PipelineHandler::hasPendingRequests(const Camera *camera) const\n> > * This method queues a capture request to the pipeline handler for processing.\n> > * The request is first added to the internal list of queued requests, and\n> > * then passed to the pipeline handler with a call to queueRequestDevice().\n> > + * If the pipeline handler fails in queuing the request to the hardware the\n> > + * request is cancelled.\n\nThe Cancelled state was meant for requests cancelled when the camera\ngets stopped. We may need a different state for errors at runtime. That\nwill be for another patch series though.\n\nReviewed-by: Laurent Pinchart \n\n> > *\n> > * Keeping track of queued requests ensures automatic completion of all requests\n> > * when the pipeline handler is stopped with stop(). Request completion shall be\n> > @@ -409,8 +411,10 @@ void PipelineHandler::queueRequest(Request *request)\n> > \trequest->sequence_ = data->requestSequence_++;\n> > \n> > \tint ret = queueRequestDevice(camera, request);\n> > -\tif (ret)\n> > -\t\tdata->queuedRequests_.remove(request);\n> > +\tif (ret) {\n> > +\t\trequest->cancel();\n> > +\t\tcompleteRequest(request);\n> > +\t}\n> > }\n> > \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 8A41CC3200\n\tfor ;\n\tSun, 23 May 2021 17:59:50 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1345F602B1;\n\tSun, 23 May 2021 19:59:50 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C15F6601A9\n\tfor ;\n\tSun, 23 May 2021 19:59:48 +0200 (CEST)","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 1B7132A8;\n\tSun, 23 May 2021 19:59:47 +0200 (CEST)"],"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=\"oohrLIi9\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1621792788;\n\tbh=RatVQhUeCx1U8kKiSS5z0M+81U9C6fjkr+PU++O8jzI=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=oohrLIi9e3Qi6TKCxo8ihJZu6jU8ph+BC+Pfhk9uiFuU6H8XnO2RMSPWO4HBhcUOI\n\t/Ywbfxq2rAfPqpoe4opw3KUzFdXRNzgo4nXmz40mN7AzS3+x6XPs2csn1Al8cMB8/f\n\tLjoThNxrcpr1TG1gRousIKqaNNZ70EP8jHhqGYk8=","Date":"Sun, 23 May 2021 20:59:44 +0300","From":"Laurent Pinchart ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Message-ID":"","References":"<20210521154227.60186-1-jacopo@jmondi.org>\n\t<20210521154227.60186-4-jacopo@jmondi.org>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v3 3/8] libcamera: pipeline_handler:\n\tCancel Request on queueing failure","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]