[{"id":32638,"web_url":"https://patchwork.libcamera.org/comment/32638/","msgid":"<173375651075.1401494.7449740860466036842@ping.linuxembedded.co.uk>","date":"2024-12-09T15:01:50","subject":"Re: [PATCH] V4L2VideoDevice: Call FrameBuffer::Private::cancel() in\n\tstreamOff()","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Harvey Yang (2024-12-03 09:52:13)\n> At the moment `V4L2VideoDevice::streamOff()` sets\n> `FrameBuffer::Private`'s metadata directly, while that's equivalent to\n> calling `FrameBuffer::Private::cancel()`. To ease code tracing, this\n> patch replace the manual modification with the function call.\n> \n> Signed-off-by: Harvey Yang \n> ---\n> src/libcamera/v4l2_videodevice.cpp | 3 +--\n> 1 file changed, 1 insertion(+), 2 deletions(-)\n> \n> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp\n> index a5cf67845..0558434cb 100644\n> --- a/src/libcamera/v4l2_videodevice.cpp\n> +++ b/src/libcamera/v4l2_videodevice.cpp\n> @@ -2007,10 +2007,9 @@ int V4L2VideoDevice::streamOff()\n> /* Send back all queued buffers. */\n> for (auto it : queuedBuffers_) {\n> FrameBuffer *buffer = it.second;\n> - FrameMetadata &metadata = buffer->_d()->metadata();\n> + buffer->_d()->cancel();\n\n\nbuffer->_d()->cancel() indeed implements:\n\n\tvoid cancel() { metadata_.status = FrameMetadata::FrameCancelled; }\n\nso this looks good to me.\n\nReviewed-by: Kieran Bingham \n\n> \n> cache_->put(it.first);\n> - metadata.status = FrameMetadata::FrameCancelled;\n\nBut you have changed \"/where/\" this is being done. Now the\nmetadata.status is being set /before/ cache_->put(it.first); which isn't\nmentioned in the commit message. Is it an intentional change?\n\nI believe it's fine though, but would have been note worthy to make sure\nit was clear you had considered this and it's ok.\n\n--\nKieran\n\n\n> bufferReady.emit(buffer);\n> }\n> \n> -- \n> 2.47.0.338.g60cca15819-goog\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 E487DBE080\n\tfor ;\n\tMon, 9 Dec 2024 15:01:56 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A513267E66;\n\tMon, 9 Dec 2024 16:01:55 +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 5555866132\n\tfor ;\n\tMon, 9 Dec 2024 16:01:54 +0100 (CET)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust6594.18-1.cable.virginm.net [86.31.185.195])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 8A34A89A;\n\tMon, 9 Dec 2024 16:01:22 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"nS5RHHwg\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1733756482;\n\tbh=N/7bQ0LJRvQ3/B9cdJERyX++k3bHbRB3VHOpABFOsMU=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=nS5RHHwgPSWRjDz+bV160RHmcg47GSxpcuquuhB5VFB4SFnG0GNTLneOb/GDeJuWu\n\ty36f/aqk9v0vGbpMBRPoIHY4FNKvNdzvnkEgDSgx68JsV/lXGIgpF5GC0D/x8G2TSo\n\tb6NjUYymHCcSxz9/zLN8n0ivzNCkZoHdT2ajs8A4=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20241203095217.2155153-1-chenghaoyang@chromium.org>","References":"<20241203095217.2155153-1-chenghaoyang@chromium.org>","Subject":"Re: [PATCH] V4L2VideoDevice: Call FrameBuffer::Private::cancel() in\n\tstreamOff()","From":"Kieran Bingham ","Cc":"Harvey Yang ","To":"Harvey Yang ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Mon, 09 Dec 2024 15:01:50 +0000","Message-ID":"<173375651075.1401494.7449740860466036842@ping.linuxembedded.co.uk>","User-Agent":"alot/0.10","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":32639,"web_url":"https://patchwork.libcamera.org/comment/32639/","msgid":"","date":"2024-12-09T16:12:38","subject":"Re: [PATCH] V4L2VideoDevice: Call FrameBuffer::Private::cancel() in\n\tstreamOff()","submitter":{"id":117,"url":"https://patchwork.libcamera.org/api/people/117/","name":"Cheng-Hao Yang","email":"chenghaoyang@chromium.org"},"content":"Hi Kieran,\n\nOn Mon, Dec 9, 2024 at 11:01 PM Kieran Bingham\n wrote:\n>\n> Quoting Harvey Yang (2024-12-03 09:52:13)\n> > At the moment `V4L2VideoDevice::streamOff()` sets\n> > `FrameBuffer::Private`'s metadata directly, while that's equivalent to\n> > calling `FrameBuffer::Private::cancel()`. To ease code tracing, this\n> > patch replace the manual modification with the function call.\n> >\n> > Signed-off-by: Harvey Yang \n> > ---\n> > src/libcamera/v4l2_videodevice.cpp | 3 +--\n> > 1 file changed, 1 insertion(+), 2 deletions(-)\n> >\n> > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp\n> > index a5cf67845..0558434cb 100644\n> > --- a/src/libcamera/v4l2_videodevice.cpp\n> > +++ b/src/libcamera/v4l2_videodevice.cpp\n> > @@ -2007,10 +2007,9 @@ int V4L2VideoDevice::streamOff()\n> > /* Send back all queued buffers. */\n> > for (auto it : queuedBuffers_) {\n> > FrameBuffer *buffer = it.second;\n> > - FrameMetadata &metadata = buffer->_d()->metadata();\n> > + buffer->_d()->cancel();\n>\n>\n> buffer->_d()->cancel() indeed implements:\n>\n> void cancel() { metadata_.status = FrameMetadata::FrameCancelled; }\n>\n> so this looks good to me.\n>\n> Reviewed-by: Kieran Bingham \n>\n> >\n> > cache_->put(it.first);\n> > - metadata.status = FrameMetadata::FrameCancelled;\n>\n> But you have changed \"/where/\" this is being done. Now the\n> metadata.status is being set /before/ cache_->put(it.first); which isn't\n> mentioned in the commit message. Is it an intentional change?\n>\n> I believe it's fine though, but would have been note worthy to make sure\n> it was clear you had considered this and it's ok.\n\nRight, I should set status after `cache_->put(it.first);` to avoid\nunnecessary changes. Let me upload another version.\n\nBR,\nHarvey\n\n>\n> --\n> Kieran\n>\n>\n> > bufferReady.emit(buffer);\n> > }\n> >\n> > --\n> > 2.47.0.338.g60cca15819-goog\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 55400BD80A\n\tfor ;\n\tMon, 9 Dec 2024 16:12:52 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 257DC67E68;\n\tMon, 9 Dec 2024 17:12:51 +0100 (CET)","from mail-lj1-x22e.google.com (mail-lj1-x22e.google.com\n\t[IPv6:2a00:1450:4864:20::22e])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2039266132\n\tfor ;\n\tMon, 9 Dec 2024 17:12:50 +0100 (CET)","by mail-lj1-x22e.google.com with SMTP id\n\t38308e7fff4ca-2f7657f9f62so40665561fa.3\n\tfor ;\n\tMon, 09 Dec 2024 08:12:50 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"lOqlPefR\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=chromium.org; s=google; t=1733760769; x=1734365569;\n\tdarn=lists.libcamera.org; \n\th=content-transfer-encoding:cc:to:subject:message-id:date:from\n\t:in-reply-to:references:mime-version:from:to:cc:subject:date\n\t:message-id:reply-to;\n\tbh=4u4owADXjV65wJaSTYPIFm0RHhgF/vB23StysufgHW0=;\n\tb=lOqlPefR1eN5gjhy57yB5p/gLmtZ+9d6MKKOxq8F086Uk8C+h1Oga85OfEpvf6ZOXk\n\tjE7NAra+sqsFuIBNLOirh1nhAK7DyIHIOr7MEK2umBglcEi8B06RtAym/uXzV7zPtmfk\n\tdm/2uUL4OPGCV5LCRY8bDLIeBDqV2f4FixnV0=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1733760769; x=1734365569;\n\th=content-transfer-encoding:cc:to:subject:message-id:date:from\n\t:in-reply-to:references:mime-version:x-gm-message-state:from:to:cc\n\t:subject:date:message-id:reply-to;\n\tbh=4u4owADXjV65wJaSTYPIFm0RHhgF/vB23StysufgHW0=;\n\tb=fY8YRtX1U43oMyzTs8tIoezUU8LeRk5c9fDPbE+gZlP7KHpcPourYh2eqGsqfDAWT+\n\trzfuV9+jeHGHXU93HF/4Qxa+GH9aKYgyi3QsXoh17zRuGyGTbVcZJAS7JAyikc8G0sOR\n\txnL57GHRx7gWBDvWxApCGmgH3RokFrnjLGoaw/3GqTvHpn3Dj+tQ7FxK+bQnMYjK21MP\n\tQZxw8iKV+31A2AFT981vuaTQFVxGZp5eDhz0MMht9vqyUdWtl6eC0eZb7n3fl5YymR3n\n\tDAPNJa0tQp/prRKazR2bMX7ZiWFT9nvdG7AnNsp3IoHgraG8ZzmmJVNbK8lAXX9ghOni\n\t4yIA==","X-Gm-Message-State":"AOJu0Ywr7o/NsGxGDRse+7XPvXpaonkZR1QJyqicR+rptATW1lWhMK35\n\tQD6uPsqMa7NW9mWp3iwNsuK1ZX6WMTME0DR6nSC7eM73NIk+T5QRoCtVBWZPC6ztpuUFH+UC073\n\tM5W8SlcEZRSVW7RdQ3AQ2ZP1l4SpVHob2jlZgPbhvCM9Ox6s7Xg==","X-Gm-Gg":"ASbGnctJu78WHxp0qfnwhkjt5ch63LhhI8Mvgd+l6oowDB0QGZRx1EdCmOCF2Vy44Bh\n\tSs4FNP6EzTQG42/d8lNzqjETZma/Rqyhj","X-Google-Smtp-Source":"AGHT+IGTY8jdBSGQ0tUobisa+fDnyNpLl+2eaZ3Jj+I5uEoQfioljFRUedii3nhqXUXNaOsGGBGShtnDNsPosr6Ir04=","X-Received":"by 2002:a05:651c:1507:b0:2ff:d32f:f881 with SMTP id\n\t38308e7fff4ca-3002fcc639cmr44879261fa.38.1733760769258;\n\tMon, 09 Dec 2024 08:12:49 -0800 (PST)","MIME-Version":"1.0","References":"<20241203095217.2155153-1-chenghaoyang@chromium.org>\n\t<173375651075.1401494.7449740860466036842@ping.linuxembedded.co.uk>","In-Reply-To":"<173375651075.1401494.7449740860466036842@ping.linuxembedded.co.uk>","From":"Cheng-Hao Yang ","Date":"Tue, 10 Dec 2024 00:12:38 +0800","Message-ID":"","Subject":"Re: [PATCH] V4L2VideoDevice: Call FrameBuffer::Private::cancel() in\n\tstreamOff()","To":"Kieran Bingham ","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"UTF-8\"","Content-Transfer-Encoding":"quoted-printable","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]