[{"id":32395,"web_url":"https://patchwork.libcamera.org/comment/32395/","msgid":"<20241126180857.GM5461@pendragon.ideasonboard.com>","date":"2024-11-26T18:08:57","subject":"Re: [PATCH v1] libcamera: Don't copy `StreamConfiguration` when\n\titerating","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Barnabás,\n\nThank you for the patch.\n\nOn Tue, Nov 26, 2024 at 06:03:05PM +0000, Barnabás Pőcze wrote:\n> A copy is made in the range-based for loop, and thus all modifications\n> done in the for loop body are lost, and not actually applied to\n> the object in the container.\n> \n> Fix that by taking a reference in the range-based for loop.\n> \n> Fixes: 4217c9f1aa863c (\"libcamera: camera: Zero streams before validate()\")\n> Fixes: 613d5402673eb9 (\"pipeline: raspberrypi: Fix handling of colour spaces\")\n> Signed-off-by: Barnabás Pőcze \n\nReviewed-by: Laurent Pinchart \n\nNaush, it looks like you may have been missing some tests related to\ncolour space handling. Could you double-check that this change doesn't\nbreak anything ? It could be that fixing a bug would uncover another\none.\n\n> ---\n> src/libcamera/camera.cpp | 4 ++--\n> src/libcamera/pipeline/rpi/common/pipeline_base.cpp | 4 ++--\n> 2 files changed, 4 insertions(+), 4 deletions(-)\n> \n> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> index 7507e9dd..4c865a46 100644\n> --- a/src/libcamera/camera.cpp\n> +++ b/src/libcamera/camera.cpp\n> @@ -1178,8 +1178,8 @@ int Camera::configure(CameraConfiguration *config)\n> \tif (ret < 0)\n> \t\treturn ret;\n> \n> -\tfor (auto it : *config)\n> -\t\tit.setStream(nullptr);\n> +\tfor (auto &cfg : *config)\n> +\t\tcfg.setStream(nullptr);\n> \n> \tif (config->validate() != CameraConfiguration::Valid) {\n> \t\tLOG(Camera, Error)\n> diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp\n> index 9e2d9d23..6f278b29 100644\n> --- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp\n> +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp\n> @@ -105,7 +105,7 @@ CameraConfiguration::Status RPiCameraConfiguration::validateColorSpaces([[maybe_\n> \tStatus status = Valid;\n> \tyuvColorSpace_.reset();\n> \n> -\tfor (auto cfg : config_) {\n> +\tfor (auto &cfg : config_) {\n> \t\t/* First fix up raw streams to have the \"raw\" colour space. */\n> \t\tif (PipelineHandlerBase::isRaw(cfg.pixelFormat)) {\n> \t\t\t/* If there was no value here, that doesn't count as \"adjusted\". */\n> @@ -130,7 +130,7 @@ CameraConfiguration::Status RPiCameraConfiguration::validateColorSpaces([[maybe_\n> \trgbColorSpace_->range = ColorSpace::Range::Full;\n> \n> \t/* Go through the streams again and force everyone to the same colour space. */\n> -\tfor (auto cfg : config_) {\n> +\tfor (auto &cfg : config_) {\n> \t\tif (cfg.colorSpace == ColorSpace::Raw)\n> \t\t\tcontinue;\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 54202BD16B\n\tfor ;\n\tTue, 26 Nov 2024 18:09:09 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 90CD166088;\n\tTue, 26 Nov 2024 19:09:08 +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 4557A6607E\n\tfor ;\n\tTue, 26 Nov 2024 19:09:07 +0100 (CET)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 882BB6BE;\n\tTue, 26 Nov 2024 19:08:44 +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=\"FYoYPJAd\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1732644524;\n\tbh=W0Miha019UaUUncj88enDky44MERCwAj2CqZPXocuaY=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=FYoYPJAdgIj7gwYkzSPqCCyuOJzaABX+UXK6kNNDMVf5atD7p9HZ3ywTZDqd30bmR\n\tb/BsLehZb0PSOJt0lSkD7xyxwZwNE6ML3FNcAiARlvE1oILu81XFLOqRQsd8DelS3r\n\tFKeLX+AjtFAjNg/HZ4jV86WPfBQs1m0XHl39N+CE=","Date":"Tue, 26 Nov 2024 20:08:57 +0200","From":"Laurent Pinchart ","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v1] libcamera: Don't copy `StreamConfiguration` when\n\titerating","Message-ID":"<20241126180857.GM5461@pendragon.ideasonboard.com>","References":"<20241126180302.685265-1-pobrn@protonmail.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20241126180302.685265-1-pobrn@protonmail.com>","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":32396,"web_url":"https://patchwork.libcamera.org/comment/32396/","msgid":"<20241126180916.GN5461@pendragon.ideasonboard.com>","date":"2024-11-26T18:09:16","subject":"Re: [PATCH v1] libcamera: Don't copy `StreamConfiguration` when\n\titerating","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"CC'ing Naush\n\nOn Tue, Nov 26, 2024 at 08:08:59PM +0200, Laurent Pinchart wrote:\n> Hi Barnabás,\n> \n> Thank you for the patch.\n> \n> On Tue, Nov 26, 2024 at 06:03:05PM +0000, Barnabás Pőcze wrote:\n> > A copy is made in the range-based for loop, and thus all modifications\n> > done in the for loop body are lost, and not actually applied to\n> > the object in the container.\n> > \n> > Fix that by taking a reference in the range-based for loop.\n> > \n> > Fixes: 4217c9f1aa863c (\"libcamera: camera: Zero streams before validate()\")\n> > Fixes: 613d5402673eb9 (\"pipeline: raspberrypi: Fix handling of colour spaces\")\n> > Signed-off-by: Barnabás Pőcze \n> \n> Reviewed-by: Laurent Pinchart \n> \n> Naush, it looks like you may have been missing some tests related to\n> colour space handling. Could you double-check that this change doesn't\n> break anything ? It could be that fixing a bug would uncover another\n> one.\n> \n> > ---\n> > src/libcamera/camera.cpp | 4 ++--\n> > src/libcamera/pipeline/rpi/common/pipeline_base.cpp | 4 ++--\n> > 2 files changed, 4 insertions(+), 4 deletions(-)\n> > \n> > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> > index 7507e9dd..4c865a46 100644\n> > --- a/src/libcamera/camera.cpp\n> > +++ b/src/libcamera/camera.cpp\n> > @@ -1178,8 +1178,8 @@ int Camera::configure(CameraConfiguration *config)\n> > \tif (ret < 0)\n> > \t\treturn ret;\n> > \n> > -\tfor (auto it : *config)\n> > -\t\tit.setStream(nullptr);\n> > +\tfor (auto &cfg : *config)\n> > +\t\tcfg.setStream(nullptr);\n> > \n> > \tif (config->validate() != CameraConfiguration::Valid) {\n> > \t\tLOG(Camera, Error)\n> > diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp\n> > index 9e2d9d23..6f278b29 100644\n> > --- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp\n> > +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp\n> > @@ -105,7 +105,7 @@ CameraConfiguration::Status RPiCameraConfiguration::validateColorSpaces([[maybe_\n> > \tStatus status = Valid;\n> > \tyuvColorSpace_.reset();\n> > \n> > -\tfor (auto cfg : config_) {\n> > +\tfor (auto &cfg : config_) {\n> > \t\t/* First fix up raw streams to have the \"raw\" colour space. */\n> > \t\tif (PipelineHandlerBase::isRaw(cfg.pixelFormat)) {\n> > \t\t\t/* If there was no value here, that doesn't count as \"adjusted\". */\n> > @@ -130,7 +130,7 @@ CameraConfiguration::Status RPiCameraConfiguration::validateColorSpaces([[maybe_\n> > \trgbColorSpace_->range = ColorSpace::Range::Full;\n> > \n> > \t/* Go through the streams again and force everyone to the same colour space. */\n> > -\tfor (auto cfg : config_) {\n> > +\tfor (auto &cfg : config_) {\n> > \t\tif (cfg.colorSpace == ColorSpace::Raw)\n> > \t\t\tcontinue;\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 00FCFBD16B\n\tfor ;\n\tTue, 26 Nov 2024 18:09:28 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A69A66608B;\n\tTue, 26 Nov 2024 19:09:27 +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 C796966081\n\tfor ;\n\tTue, 26 Nov 2024 19:09:25 +0100 (CET)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 35C55B63;\n\tTue, 26 Nov 2024 19:09:03 +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=\"MIupxD3b\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1732644543;\n\tbh=Gs9dWm3IVjsKn6WBUMwKIiFeYNXbQYxaQB7JhNF4dL8=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=MIupxD3b2BC1IT1kGcg29R7VO95Ztk2F1/ccCvScYs61tZPjPh8uz+ZKnq5VulCmG\n\trZAsXgfE4CZnTXEPMWMlMwrKRZw4ptwYMV2TumO8KYRhcg9iK+qjv66RPZbkxVVvs8\n\tQ30dRIC6b0bMXnDC76eGgiHF1p4wm8OdekT34rgg=","Date":"Tue, 26 Nov 2024 20:09:16 +0200","From":"Laurent Pinchart ","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Cc":"libcamera-devel@lists.libcamera.org,\n\tNaushir Patuck ","Subject":"Re: [PATCH v1] libcamera: Don't copy `StreamConfiguration` when\n\titerating","Message-ID":"<20241126180916.GN5461@pendragon.ideasonboard.com>","References":"<20241126180302.685265-1-pobrn@protonmail.com>\n\t<20241126180857.GM5461@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20241126180857.GM5461@pendragon.ideasonboard.com>","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":32397,"web_url":"https://patchwork.libcamera.org/comment/32397/","msgid":"","date":"2024-11-26T18:32:13","subject":"Re: [PATCH v1] libcamera: Don't copy `StreamConfiguration` when\n\titerating","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"+David Plowman \n\nOn Tue, 26 Nov 2024, 6:09 pm Laurent Pinchart, <\nlaurent.pinchart@ideasonboard.com> wrote:\n\n> CC'ing Naush\n>\n> On Tue, Nov 26, 2024 at 08:08:59PM +0200, Laurent Pinchart wrote:\n> > Hi Barnabás,\n> >\n> > Thank you for the patch.\n> >\n> > On Tue, Nov 26, 2024 at 06:03:05PM +0000, Barnabás Pőcze wrote:\n> > > A copy is made in the range-based for loop, and thus all modifications\n> > > done in the for loop body are lost, and not actually applied to\n> > > the object in the container.\n> > >\n> > > Fix that by taking a reference in the range-based for loop.\n> > >\n> > > Fixes: 4217c9f1aa863c (\"libcamera: camera: Zero streams before\n> validate()\")\n> > > Fixes: 613d5402673eb9 (\"pipeline: raspberrypi: Fix handling of colour\n> spaces\")\n> > > Signed-off-by: Barnabás Pőcze \n> >\n> > Reviewed-by: Laurent Pinchart \n> >\n> > Naush, it looks like you may have been missing some tests related to\n> > colour space handling. Could you double-check that this change doesn't\n> > break anything ? It could be that fixing a bug would uncover another\n> > one.\n> >\n> > > ---\n> > > src/libcamera/camera.cpp | 4 ++--\n> > > src/libcamera/pipeline/rpi/common/pipeline_base.cpp | 4 ++--\n> > > 2 files changed, 4 insertions(+), 4 deletions(-)\n> > >\n> > > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> > > index 7507e9dd..4c865a46 100644\n> > > --- a/src/libcamera/camera.cpp\n> > > +++ b/src/libcamera/camera.cpp\n> > > @@ -1178,8 +1178,8 @@ int Camera::configure(CameraConfiguration\n> *config)\n> > > if (ret < 0)\n> > > return ret;\n> > >\n> > > - for (auto it : *config)\n> > > - it.setStream(nullptr);\n> > > + for (auto &cfg : *config)\n> > > + cfg.setStream(nullptr);\n> > >\n> > > if (config->validate() != CameraConfiguration::Valid) {\n> > > LOG(Camera, Error)\n> > > diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp\n> b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp\n> > > index 9e2d9d23..6f278b29 100644\n> > > --- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp\n> > > +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp\n> > > @@ -105,7 +105,7 @@ CameraConfiguration::Status\n> RPiCameraConfiguration::validateColorSpaces([[maybe_\n> > > Status status = Valid;\n> > > yuvColorSpace_.reset();\n> > >\n> > > - for (auto cfg : config_) {\n> > > + for (auto &cfg : config_) {\n> > > /* First fix up raw streams to have the \"raw\" colour\n> space. */\n> > > if (PipelineHandlerBase::isRaw(cfg.pixelFormat)) {\n> > > /* If there was no value here, that doesn't count\n> as \"adjusted\". */\n> > > @@ -130,7 +130,7 @@ CameraConfiguration::Status\n> RPiCameraConfiguration::validateColorSpaces([[maybe_\n> > > rgbColorSpace_->range = ColorSpace::Range::Full;\n> > >\n> > > /* Go through the streams again and force everyone to the same\n> colour space. */\n> > > - for (auto cfg : config_) {\n> > > + for (auto &cfg : config_) {\n> > > if (cfg.colorSpace == ColorSpace::Raw)\n> > > continue;\n> > >\n>\n> --\n> Regards,\n>\n> Laurent Pinchart\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 3B3FAC3200\n\tfor ;\n\tTue, 26 Nov 2024 18:32:29 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3D5C76608D;\n\tTue, 26 Nov 2024 19:32:28 +0100 (CET)","from mail-yw1-x1135.google.com (mail-yw1-x1135.google.com\n\t[IPv6:2607:f8b0:4864:20::1135])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D58A066081\n\tfor ;\n\tTue, 26 Nov 2024 19:32:25 +0100 (CET)","by mail-yw1-x1135.google.com with SMTP id\n\t00721157ae682-6ee724cb04eso4144397b3.1\n\tfor ;\n\tTue, 26 Nov 2024 10:32:25 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"SPePSExK\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google; t=1732645945; x=1733250745;\n\tdarn=lists.libcamera.org; \n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:from:to:cc:subject:date:message-id:reply-to;\n\tbh=5Bx9yVVjczuWBxunErgua3cSLEcCXUZ74axwanhlsws=;\n\tb=SPePSExKfCD6nGzbYjXF6qqDqYF9nEI5Bq1S/0t13qJohsp8hAnYIABrXcD7mu2X+U\n\tq9fLJRzWnzlrOYSqw00MEG14YW8ibr8gdf83YCevyL5Wri6sRFjBpzxMGiLiWFLrYgrn\n\tMN0eVqHgR7WMYOfK+xtoKFISw5O+3lnaD8GZVVcFm1Sp7tfMO1XkYVmMuf+0FlJPYn6E\n\tSg0AuDBuPZB7780cglHKCeqefkBwzMB9nMSNoc+gMO068jCBVRyuVEXyImnFRdng7kQt\n\t0oDp8PYEhAF7k0U/YS1+zShL0s2c5h4NnEFMdFdvBWdyyI3ik9PohF3jdm12rvcnBY2I\n\tIPLQ==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1732645945; x=1733250745;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:x-gm-message-state:from:to:cc:subject:date:message-id\n\t:reply-to;\n\tbh=5Bx9yVVjczuWBxunErgua3cSLEcCXUZ74axwanhlsws=;\n\tb=fL4gRlMHZ9iVbj2C2oYKblx5eVlA+PQQ6yjz0TasNlytwSGeVt92Txz/KSVJ8av7ro\n\tp1tqTTbwo4yQezcesWFmU9lmxykipiCOUUdYL/+qWeUImRH2zxd5v2l2hwptRvgp3+uM\n\tOR1rSD9kF6Fg7Vdz67EEySNb+9JFFF+BN/tNkmOIQsNcxVE3Q8VsbIC0Da/wcTMtLxKz\n\t5RNhuo40pJue8WOqwAGdn0TynGXwbhAg7Z5g3TgsgwtfH7bf+tGPsAhwQyrF3NXHPFpe\n\taTxH7IrO7TacxWovpLIADL5bdtuKTzqXwRWPnplx0E4E/OX7Ij7MO9geacT3l4aRZnis\n\tuQ9A==","X-Forwarded-Encrypted":"i=1;\n\tAJvYcCWF7rqZB6i3mBAN+qPgEW5YW62wZIgqaX5TxANOxzqwkzF6FhstFqe2v9RiXILHbVXjKrR5t/hBuvIg0lTe5QE=@lists.libcamera.org","X-Gm-Message-State":"AOJu0YyAm3JlGa4kelxRX2oIPQCbweZTd9TFMdpeiRjUbI4/To2vzTV8\n\tyVtY4RLghH4n1TUUQ5d5FbdGC+/YxNMe+j7ki7dnuipkw93P7raC58euH9XVvW9Tju/KmSpG6u3\n\tIAIwmAmamneF85v7DI0uh86ZLyCHwD/hTsmgMww==","X-Gm-Gg":"ASbGncuuyWXexRVdU9GLYZgJzbiGB8bXK70CRQEeIf7CPLbbBm2x0+H2wI8/P7I27u9\n\tXSlPu9eOOkROYZ+5UL/sVyJV9x7Qo","X-Google-Smtp-Source":"AGHT+IG3yPas8mEZg5kVC6Wd30szsw1YV6ARg9+mz87c2AfQ6tT5nGDj3s3BFZY6Th55xMm+iucZ64VmxZ1o/8RPJEk=","X-Received":"by 2002:a05:690c:6a06:b0:6ee:b726:62cd with SMTP id\n\t00721157ae682-6ef372c5b97mr985087b3.9.1732645944802; Tue, 26 Nov 2024\n\t10:32:24 -0800 (PST)","MIME-Version":"1.0","References":"<20241126180302.685265-1-pobrn@protonmail.com>\n\t<20241126180857.GM5461@pendragon.ideasonboard.com>\n\t<20241126180916.GN5461@pendragon.ideasonboard.com>","In-Reply-To":"<20241126180916.GN5461@pendragon.ideasonboard.com>","From":"Naushir Patuck ","Date":"Tue, 26 Nov 2024 18:32:13 +0000","Message-ID":"","Subject":"Re: [PATCH v1] libcamera: Don't copy `StreamConfiguration` when\n\titerating","To":"Laurent Pinchart , \n\tDavid Plowman ","Cc":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ,\n\tlibcamera devel ","Content-Type":"multipart/alternative; boundary=\"0000000000000fd8870627d51241\"","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":32411,"web_url":"https://patchwork.libcamera.org/comment/32411/","msgid":"","date":"2024-11-27T09:22:24","subject":"Re: [PATCH v1] libcamera: Don't copy `StreamConfiguration` when\n\titerating","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi Barnabás,\n\nThank you for this fix.\n\nOn Tue, 26 Nov 2024 at 18:03, Barnabás Pőcze wrote:\n>\n> A copy is made in the range-based for loop, and thus all modifications\n> done in the for loop body are lost, and not actually applied to\n> the object in the container.\n>\n> Fix that by taking a reference in the range-based for loop.\n>\n> Fixes: 4217c9f1aa863c (\"libcamera: camera: Zero streams before validate()\")\n> Fixes: 613d5402673eb9 (\"pipeline: raspberrypi: Fix handling of colour spaces\")\n> Signed-off-by: Barnabás Pőcze \n\nI've run this change through our regression testing and it passes. I\nwas expecting a failure with this change, but it turns out we have CS\nvalidation code in Picamera2 that avoids us hitting the below\nadjustment code.\n\nReviewed-by: Naushir Patuck \n\n> ---\n> src/libcamera/camera.cpp | 4 ++--\n> src/libcamera/pipeline/rpi/common/pipeline_base.cpp | 4 ++--\n> 2 files changed, 4 insertions(+), 4 deletions(-)\n>\n> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> index 7507e9dd..4c865a46 100644\n> --- a/src/libcamera/camera.cpp\n> +++ b/src/libcamera/camera.cpp\n> @@ -1178,8 +1178,8 @@ int Camera::configure(CameraConfiguration *config)\n> if (ret < 0)\n> return ret;\n>\n> - for (auto it : *config)\n> - it.setStream(nullptr);\n> + for (auto &cfg : *config)\n> + cfg.setStream(nullptr);\n>\n> if (config->validate() != CameraConfiguration::Valid) {\n> LOG(Camera, Error)\n> diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp\n> index 9e2d9d23..6f278b29 100644\n> --- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp\n> +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp\n> @@ -105,7 +105,7 @@ CameraConfiguration::Status RPiCameraConfiguration::validateColorSpaces([[maybe_\n> Status status = Valid;\n> yuvColorSpace_.reset();\n>\n> - for (auto cfg : config_) {\n> + for (auto &cfg : config_) {\n> /* First fix up raw streams to have the \"raw\" colour space. */\n> if (PipelineHandlerBase::isRaw(cfg.pixelFormat)) {\n> /* If there was no value here, that doesn't count as \"adjusted\". */\n> @@ -130,7 +130,7 @@ CameraConfiguration::Status RPiCameraConfiguration::validateColorSpaces([[maybe_\n> rgbColorSpace_->range = ColorSpace::Range::Full;\n>\n> /* Go through the streams again and force everyone to the same colour space. */\n> - for (auto cfg : config_) {\n> + for (auto &cfg : config_) {\n> if (cfg.colorSpace == ColorSpace::Raw)\n> continue;\n>\n> --\n> 2.47.1\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 9BFBBC3200\n\tfor ;\n\tWed, 27 Nov 2024 09:23:03 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 934B7660B5;\n\tWed, 27 Nov 2024 10:23:02 +0100 (CET)","from mail-yw1-x1131.google.com (mail-yw1-x1131.google.com\n\t[IPv6:2607:f8b0:4864:20::1131])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 19BAB660A7\n\tfor ;\n\tWed, 27 Nov 2024 10:23:01 +0100 (CET)","by mail-yw1-x1131.google.com with SMTP id\n\t00721157ae682-6ee91fe8550so1747667b3.2\n\tfor ;\n\tWed, 27 Nov 2024 01:23:01 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"ne+WaCO7\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google; t=1732699380; x=1733304180;\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=8n6/BScfWWh10L5dKNOAiwR3sy1XmIZkiaedkOtEG6Q=;\n\tb=ne+WaCO7HU8UKD7ulnEqlN4T/ekca/qoSogKJQa7aILmrJxgFDGOVuLiOU5gZRvGrB\n\tU+MzcEyKzPEgAsuBkQBUl+grJRmZ0BU8g+4FOLfcmN4/ucyChRzVpbI2ZzirmxaaOe57\n\twI9VyjSrwL+b62T8/JacAjUtdMAD2hj7ctCiUWxGmX7IJbGpE7cXID6s+xGVlwU061UA\n\tjTsAPmtwyzai1K1BMkigpHjQAgdEkv3l4iqYPLAWTW8wDhKgE0/3TaxBsi0nqY7FPV5/\n\tZFZ+OFvAuFyjHpoluf7PtuAbRWbzIAe+TFSFZq28XfhXA+/hV/O0D8NglR4wEcwFyeWi\n\tvGUw==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1732699380; x=1733304180;\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=8n6/BScfWWh10L5dKNOAiwR3sy1XmIZkiaedkOtEG6Q=;\n\tb=i9WFrCT3XqMKUYtgWW/EW+vIi41zjVKQ3mUUFu8sDFVqYe40ryP0WPFvzzFgBmOeD7\n\t0DMRV0REFRnjr5nZu0N5oop9fAEN5vLryZ/3DvkwFwu/3MOYMOe3Vo5/+3oNwYU7IFGN\n\tEK7fvJEWB7pLcrO07gm8b5gakeikDO4Tbe5MArpOrbYO+cay3lhxmSJtlt3hKcDKHoRk\n\tJ/s6nPfaCsYleuZNlpaBLfTd9ZOVtpZqPs/0MFSrvGRmgZl/Ksm6Js4eOOFeA8FSly0X\n\tWUSNpUCTqdL4eNe77f9ml6yQhrIqqoHMrjzLlOSbrvcA70dvJihBMkjsoHx1c22C5kQU\n\teX2g==","X-Gm-Message-State":"AOJu0Yyl/C+z2CsEJ+07lhbLpt2IrF/+rB5TP4YdgJewDDOFNmEG5NIN\n\tIgmNzhhCTBditUQxeV0h8++LA3lwxbb8sljbWIxPAnrwi+w2k7Yjr+Zt8HcAG79AXNpmd2mugzG\n\tBS8KBGAZrG5v43dG4xEQNypsmkOHG6czH5qOzoQ==","X-Gm-Gg":"ASbGncufxjUYSNHAvMt/nm0VmAjjITWMLL/8QsSubniBt0PBjtkHdApJVpenfBSpARI\n\tgOqnEG6ID/TiexoLpMZ80TGc2cnvq6/cfAnrrJlnrNU6tsRkQY0BwDXNNtacwew==","X-Google-Smtp-Source":"AGHT+IEOB00kE+iIPcMT24PhrOlmV5UECGyW5p8m9j8KOXfFNusp4Sf3I8Ff4wEkbSEzSzxF2kxLUwqUAsluBe6THp8=","X-Received":"by 2002:a05:690c:6c90:b0:6ea:85ee:b5bb with SMTP id\n\t00721157ae682-6ef3720365bmr11107997b3.3.1732699379803;\n\tWed, 27 Nov 2024 01:22:59 -0800 (PST)","MIME-Version":"1.0","References":"<20241126180302.685265-1-pobrn@protonmail.com>","In-Reply-To":"<20241126180302.685265-1-pobrn@protonmail.com>","From":"Naushir Patuck ","Date":"Wed, 27 Nov 2024 09:22:24 +0000","Message-ID":"","Subject":"Re: [PATCH v1] libcamera: Don't copy `StreamConfiguration` when\n\titerating","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Cc":"libcamera-devel@lists.libcamera.org, \n\tDavid Plowman ","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\" "}}]