[{"id":30939,"web_url":"https://patchwork.libcamera.org/comment/30939/","msgid":"<adws66lzfgyd4jqc32ar5fobokudsvgopwzhqqyzpgl67xoblz@r2v4vkxtcvdm>","date":"2024-08-28T09:31:05","subject":"Re: [PATCH v1 3/7] pipeline: rpi: Pass crop rectangle as a parameter\n\tto platformSetIspCrop()","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Naush\n\nOn Thu, Aug 08, 2024 at 11:23:42AM GMT, Naushir Patuck wrote:\n> This will be required when we program separate crop values to each ISP\n> output in a future commit.\n>\n> Signed-off-by: Naushir Patuck <naush@raspberrypi.com>\n> ---\n>  src/libcamera/pipeline/rpi/common/pipeline_base.cpp | 3 +--\n>  src/libcamera/pipeline/rpi/common/pipeline_base.h   | 2 +-\n>  src/libcamera/pipeline/rpi/vc4/vc4.cpp              | 7 ++++---\n>  3 files changed, 6 insertions(+), 6 deletions(-)\n>\n> diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp\n> index 5322fd798a36..2de6111bacfd 100644\n> --- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp\n> +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp\n> @@ -1314,8 +1314,7 @@ void CameraData::applyScalerCrop(const ControlList &controls)\n>\n>  \t\tif (ispCrop != ispCrop_) {\n>  \t\t\tispCrop_ = ispCrop;\n> -\t\t\tplatformSetIspCrop();\n> -\n> +\t\t\tplatformSetIspCrop(ispCrop);\n\nI see this introducing a potential issue ? Before this change\nplatformSetIspCrop() operated on ispCrop_\n\n   isp_[Isp::Input].dev()->setSelection(V4L2_SEL_TGT_CROP, &ispCrop_);\n\nWith the side effect that ispCrop_ was adjusted to whatever the v4l2\nvideo device has effectively applied.\n\nNow you're going through a temporary variable\n\n+               Rectangle crop = ispCrop;\n+               isp_[Isp::Input].dev()->setSelection(V4L2_SEL_TGT_CROP, &crop);\n\nMeaning ispCrop_ will not be updated.\n\nIs this an issue in your opinion ? Will it change in the next patches ?\n\n>  \t\t}\n>  \t}\n>  }\n> diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.h b/src/libcamera/pipeline/rpi/common/pipeline_base.h\n> index 5161c16e518f..d65b695c30b5 100644\n> --- a/src/libcamera/pipeline/rpi/common/pipeline_base.h\n> +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.h\n> @@ -83,7 +83,7 @@ public:\n>\n>  \tRectangle scaleIspCrop(const Rectangle &ispCrop) const;\n>  \tvoid applyScalerCrop(const ControlList &controls);\n> -\tvirtual void platformSetIspCrop() = 0;\n> +\tvirtual void platformSetIspCrop(const Rectangle &ispCrop) = 0;\n>\n>  \tvoid cameraTimeout();\n>  \tvoid frameStarted(uint32_t sequence);\n> diff --git a/src/libcamera/pipeline/rpi/vc4/vc4.cpp b/src/libcamera/pipeline/rpi/vc4/vc4.cpp\n> index e5b6ef2b37cd..0ea032293bc9 100644\n> --- a/src/libcamera/pipeline/rpi/vc4/vc4.cpp\n> +++ b/src/libcamera/pipeline/rpi/vc4/vc4.cpp\n> @@ -109,9 +109,10 @@ public:\n>  \tConfig config_;\n>\n>  private:\n> -\tvoid platformSetIspCrop() override\n> +\tvoid platformSetIspCrop(const Rectangle &ispCrop) override\n>  \t{\n> -\t\tisp_[Isp::Input].dev()->setSelection(V4L2_SEL_TGT_CROP, &ispCrop_);\n> +\t\tRectangle crop = ispCrop;\n> +\t\tisp_[Isp::Input].dev()->setSelection(V4L2_SEL_TGT_CROP, &crop);\n>  \t}\n>\n>  \tint platformConfigure(const RPi::RPiCameraConfiguration *rpiConfig) override;\n> @@ -707,7 +708,7 @@ int Vc4CameraData::platformConfigure(const RPi::RPiCameraConfiguration *rpiConfi\n>  \tSize size = unicamFormat.size.boundedToAspectRatio(maxSize);\n>  \tispCrop_ = size.centeredTo(Rectangle(unicamFormat.size).center());\n>\n> -\tplatformSetIspCrop();\n> +\tplatformSetIspCrop(ispCrop_);\n>\n>  \treturn 0;\n>  }\n> --\n> 2.34.1\n>","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 CFE9CC323E\n\tfor <parsemail@patchwork.libcamera.org>;\n\tWed, 28 Aug 2024 09:31:12 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7592663460;\n\tWed, 28 Aug 2024 11:31:11 +0200 (CEST)","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 21D9861903\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tWed, 28 Aug 2024 11:31:10 +0200 (CEST)","from ideasonboard.com (mob-5-90-141-165.net.vodafone.it\n\t[5.90.141.165])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 4E64A9FF;\n\tWed, 28 Aug 2024 11:30:02 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"RDaFMxob\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1724837402;\n\tbh=OkkQhSbsm3wvH5u7/gX3t7vEIEibt9naTQeOinx34f4=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=RDaFMxobvHjdbPbYJ5FnQ4kIEjEyYMTiABtK5k4MgFezmkQLbENCCZvvhusfIwkne\n\ta5luyB4wZX12FUJjZoX+oQIYIwrDzSXIlULk1EP3WwotRbi2usbytCq2+ZhNUszETj\n\trRnBRrsfi6hXYGsARAzDgfFW5SN6udhCnFHO2lz0=","Date":"Wed, 28 Aug 2024 11:31:05 +0200","From":"Jacopo Mondi <jacopo.mondi@ideasonboard.com>","To":"Naushir Patuck <naush@raspberrypi.com>","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v1 3/7] pipeline: rpi: Pass crop rectangle as a parameter\n\tto platformSetIspCrop()","Message-ID":"<adws66lzfgyd4jqc32ar5fobokudsvgopwzhqqyzpgl67xoblz@r2v4vkxtcvdm>","References":"<20240808102346.13065-1-naush@raspberrypi.com>\n\t<20240808102346.13065-4-naush@raspberrypi.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20240808102346.13065-4-naush@raspberrypi.com>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":30942,"web_url":"https://patchwork.libcamera.org/comment/30942/","msgid":"<CAEmqJPrhWhCAxj34KW+Hjmwk5WPqxnEFhXmXCRp=-Uirc+_Ljw@mail.gmail.com>","date":"2024-08-28T10:00:15","subject":"Re: [PATCH v1 3/7] pipeline: rpi: Pass crop rectangle as a parameter\n\tto platformSetIspCrop()","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi Jacopo,\n\nThanks for the feedback.\n\nOn Wed, 28 Aug 2024 at 10:31, Jacopo Mondi\n<jacopo.mondi@ideasonboard.com> wrote:\n>\n> Hi Naush\n>\n> On Thu, Aug 08, 2024 at 11:23:42AM GMT, Naushir Patuck wrote:\n> > This will be required when we program separate crop values to each ISP\n> > output in a future commit.\n> >\n> > Signed-off-by: Naushir Patuck <naush@raspberrypi.com>\n> > ---\n> >  src/libcamera/pipeline/rpi/common/pipeline_base.cpp | 3 +--\n> >  src/libcamera/pipeline/rpi/common/pipeline_base.h   | 2 +-\n> >  src/libcamera/pipeline/rpi/vc4/vc4.cpp              | 7 ++++---\n> >  3 files changed, 6 insertions(+), 6 deletions(-)\n> >\n> > diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp\n> > index 5322fd798a36..2de6111bacfd 100644\n> > --- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp\n> > +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp\n> > @@ -1314,8 +1314,7 @@ void CameraData::applyScalerCrop(const ControlList &controls)\n> >\n> >               if (ispCrop != ispCrop_) {\n> >                       ispCrop_ = ispCrop;\n> > -                     platformSetIspCrop();\n> > -\n> > +                     platformSetIspCrop(ispCrop);\n>\n> I see this introducing a potential issue ? Before this change\n> platformSetIspCrop() operated on ispCrop_\n>\n>    isp_[Isp::Input].dev()->setSelection(V4L2_SEL_TGT_CROP, &ispCrop_);\n>\n> With the side effect that ispCrop_ was adjusted to whatever the v4l2\n> video device has effectively applied.\n>\n> Now you're going through a temporary variable\n>\n> +               Rectangle crop = ispCrop;\n> +               isp_[Isp::Input].dev()->setSelection(V4L2_SEL_TGT_CROP, &crop);\n>\n> Meaning ispCrop_ will not be updated.\n>\n> Is this an issue in your opinion ? Will it change in the next patches ?\n\nThis is not a problem.  Practically the vc4 ISP driver never changes\nthe crop rectangle, so there is no need to store it back into\nispCrop_.\n\nRegards,\nNaush\n\n>\n>\n> >               }\n> >       }\n> >  }\n> > diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.h b/src/libcamera/pipeline/rpi/common/pipeline_base.h\n> > index 5161c16e518f..d65b695c30b5 100644\n> > --- a/src/libcamera/pipeline/rpi/common/pipeline_base.h\n> > +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.h\n> > @@ -83,7 +83,7 @@ public:\n> >\n> >       Rectangle scaleIspCrop(const Rectangle &ispCrop) const;\n> >       void applyScalerCrop(const ControlList &controls);\n> > -     virtual void platformSetIspCrop() = 0;\n> > +     virtual void platformSetIspCrop(const Rectangle &ispCrop) = 0;\n> >\n> >       void cameraTimeout();\n> >       void frameStarted(uint32_t sequence);\n> > diff --git a/src/libcamera/pipeline/rpi/vc4/vc4.cpp b/src/libcamera/pipeline/rpi/vc4/vc4.cpp\n> > index e5b6ef2b37cd..0ea032293bc9 100644\n> > --- a/src/libcamera/pipeline/rpi/vc4/vc4.cpp\n> > +++ b/src/libcamera/pipeline/rpi/vc4/vc4.cpp\n> > @@ -109,9 +109,10 @@ public:\n> >       Config config_;\n> >\n> >  private:\n> > -     void platformSetIspCrop() override\n> > +     void platformSetIspCrop(const Rectangle &ispCrop) override\n> >       {\n> > -             isp_[Isp::Input].dev()->setSelection(V4L2_SEL_TGT_CROP, &ispCrop_);\n> > +             Rectangle crop = ispCrop;\n> > +             isp_[Isp::Input].dev()->setSelection(V4L2_SEL_TGT_CROP, &crop);\n> >       }\n> >\n> >       int platformConfigure(const RPi::RPiCameraConfiguration *rpiConfig) override;\n> > @@ -707,7 +708,7 @@ int Vc4CameraData::platformConfigure(const RPi::RPiCameraConfiguration *rpiConfi\n> >       Size size = unicamFormat.size.boundedToAspectRatio(maxSize);\n> >       ispCrop_ = size.centeredTo(Rectangle(unicamFormat.size).center());\n> >\n> > -     platformSetIspCrop();\n> > +     platformSetIspCrop(ispCrop_);\n> >\n> >       return 0;\n> >  }\n> > --\n> > 2.34.1\n> >","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 0803FC323E\n\tfor <parsemail@patchwork.libcamera.org>;\n\tWed, 28 Aug 2024 10:00:44 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9482D63458;\n\tWed, 28 Aug 2024 12:00:43 +0200 (CEST)","from mail-yb1-xb33.google.com (mail-yb1-xb33.google.com\n\t[IPv6:2607:f8b0:4864:20::b33])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 546BF61E4F\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tWed, 28 Aug 2024 12:00:42 +0200 (CEST)","by mail-yb1-xb33.google.com with SMTP id\n\t3f1490d57ef6-e11628fa133so605862276.3\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tWed, 28 Aug 2024 03:00:42 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"tr3VXUwx\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google; t=1724839241; x=1725444041;\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=7EiRpofeHieTUQlL5luCk1WC496chSKSTKISxW9Fra0=;\n\tb=tr3VXUwxkEVQyy2s8lLxHCAhxe0u98zVOO0XkHh28CaXNpIIRW/+lijjAJ9kY2DGxF\n\tRTVrvhomQ8V+elIjoaXDaHI8eKRmnPaCRXTJba3pQZF9RRdfIz1Aym1dU99wahFxaaHx\n\tbO05CYeicA/LKOdXjMg2r+Y8eOfKmNtueKpfejfcynBtOH09Uez0YURj6dZ7di4uC04+\n\tZD6a0YmrjIfqb80zkH/6/g/ekAPkibFQoLZcCuBlL4C2jNxFV2DlECcqmk+NGe1nE7aa\n\tzEZxd4Hb0LuSaPgCYiq7O6beNFiogPudI7z3j6VE0dL18I0ifGiFHAvRL1FUXJYBDkuk\n\tmSMQ==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1724839241; x=1725444041;\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=7EiRpofeHieTUQlL5luCk1WC496chSKSTKISxW9Fra0=;\n\tb=S1Ms45XbgcGfZLCwpd/5TZAIrdBAQJRbr/yphrsnhLU4KPvcPMjMb1Vb+ZT5/xjgkI\n\tg8smWxkVfEor2G9GOPzJCHCeYYBok1IQnVIndFT8ZGOdXF2YjRsE3VX1GAh6FaDcjzW9\n\taXmUmBZkLVUrmQgY2LpMzZjiIicesIMNtbYH2My+pB1l0Jw1aLBrUiaZ7q8YDAP3xXq7\n\tPd6cV0Zy5HbmkXSqWrMnLtWYBakxgEXWT7D/NonBcRKdoDKfySL3qLokcqwgEGEmFDmL\n\tLgXTILS69ig7aUhCceih1/STQEbJ+yV4ksC8yIrz+a62tznSQlr1w8/4DE9edqggPWB9\n\tAY6Q==","X-Gm-Message-State":"AOJu0YwZ42ocqucMPNmYZcqJFSmH1LyGvMLstIc4o7A6lI2Uorw865dH\n\tV5uFmNkwUEr3Cmy8n5x7+Uo7SSNyoes3Vgy2Y2nywX7eBAPNUYUfdlj3mZP1AwYUciIghVufbGc\n\tFM+7yVXTOktG7teWvXvwnN/t9SgafNjJqMdDLdW+o+xm2yOb6","X-Google-Smtp-Source":"AGHT+IH5U0sQtOKomQ794Ywv2HF6fhzotwCls/jjth741mk86UzsF4ck3mgDnfE2aYI6OCeeIEbyf7gU3XcAHM7Vst0=","X-Received":"by 2002:a05:690c:39f:b0:6c8:7827:f28c with SMTP id\n\t00721157ae682-6c87827f65amr91349737b3.6.1724839241194;\n\tWed, 28 Aug 2024 03:00:41 -0700 (PDT)","MIME-Version":"1.0","References":"<20240808102346.13065-1-naush@raspberrypi.com>\n\t<20240808102346.13065-4-naush@raspberrypi.com>\n\t<adws66lzfgyd4jqc32ar5fobokudsvgopwzhqqyzpgl67xoblz@r2v4vkxtcvdm>","In-Reply-To":"<adws66lzfgyd4jqc32ar5fobokudsvgopwzhqqyzpgl67xoblz@r2v4vkxtcvdm>","From":"Naushir Patuck <naush@raspberrypi.com>","Date":"Wed, 28 Aug 2024 11:00:15 +0100","Message-ID":"<CAEmqJPrhWhCAxj34KW+Hjmwk5WPqxnEFhXmXCRp=-Uirc+_Ljw@mail.gmail.com>","Subject":"Re: [PATCH v1 3/7] pipeline: rpi: Pass crop rectangle as a parameter\n\tto platformSetIspCrop()","To":"Jacopo Mondi <jacopo.mondi@ideasonboard.com>","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"UTF-8\"","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":30951,"web_url":"https://patchwork.libcamera.org/comment/30951/","msgid":"<20240828162824.GH27131@pendragon.ideasonboard.com>","date":"2024-08-28T16:28:24","subject":"Re: [PATCH v1 3/7] pipeline: rpi: Pass crop rectangle as a parameter\n\tto platformSetIspCrop()","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hello,\n\nOn Wed, Aug 28, 2024 at 11:00:15AM +0100, Naushir Patuck wrote:\n> On Wed, 28 Aug 2024 at 10:31, Jacopo Mondi wrote:\n> > On Thu, Aug 08, 2024 at 11:23:42AM GMT, Naushir Patuck wrote:\n> > > This will be required when we program separate crop values to each ISP\n> > > output in a future commit.\n> > >\n> > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com>\n> > > ---\n> > >  src/libcamera/pipeline/rpi/common/pipeline_base.cpp | 3 +--\n> > >  src/libcamera/pipeline/rpi/common/pipeline_base.h   | 2 +-\n> > >  src/libcamera/pipeline/rpi/vc4/vc4.cpp              | 7 ++++---\n> > >  3 files changed, 6 insertions(+), 6 deletions(-)\n> > >\n> > > diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp\n> > > index 5322fd798a36..2de6111bacfd 100644\n> > > --- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp\n> > > +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp\n> > > @@ -1314,8 +1314,7 @@ void CameraData::applyScalerCrop(const ControlList &controls)\n> > >\n> > >               if (ispCrop != ispCrop_) {\n> > >                       ispCrop_ = ispCrop;\n> > > -                     platformSetIspCrop();\n> > > -\n> > > +                     platformSetIspCrop(ispCrop);\n> >\n> > I see this introducing a potential issue ? Before this change\n> > platformSetIspCrop() operated on ispCrop_\n> >\n> >    isp_[Isp::Input].dev()->setSelection(V4L2_SEL_TGT_CROP, &ispCrop_);\n> >\n> > With the side effect that ispCrop_ was adjusted to whatever the v4l2\n> > video device has effectively applied.\n> >\n> > Now you're going through a temporary variable\n> >\n> > +               Rectangle crop = ispCrop;\n> > +               isp_[Isp::Input].dev()->setSelection(V4L2_SEL_TGT_CROP, &crop);\n> >\n> > Meaning ispCrop_ will not be updated.\n> >\n> > Is this an issue in your opinion ? Will it change in the next patches ?\n> \n> This is not a problem.  Practically the vc4 ISP driver never changes\n> the crop rectangle, so there is no need to store it back into\n> ispCrop_.\n\nReviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n\n> > >               }\n> > >       }\n> > >  }\n> > > diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.h b/src/libcamera/pipeline/rpi/common/pipeline_base.h\n> > > index 5161c16e518f..d65b695c30b5 100644\n> > > --- a/src/libcamera/pipeline/rpi/common/pipeline_base.h\n> > > +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.h\n> > > @@ -83,7 +83,7 @@ public:\n> > >\n> > >       Rectangle scaleIspCrop(const Rectangle &ispCrop) const;\n> > >       void applyScalerCrop(const ControlList &controls);\n> > > -     virtual void platformSetIspCrop() = 0;\n> > > +     virtual void platformSetIspCrop(const Rectangle &ispCrop) = 0;\n> > >\n> > >       void cameraTimeout();\n> > >       void frameStarted(uint32_t sequence);\n> > > diff --git a/src/libcamera/pipeline/rpi/vc4/vc4.cpp b/src/libcamera/pipeline/rpi/vc4/vc4.cpp\n> > > index e5b6ef2b37cd..0ea032293bc9 100644\n> > > --- a/src/libcamera/pipeline/rpi/vc4/vc4.cpp\n> > > +++ b/src/libcamera/pipeline/rpi/vc4/vc4.cpp\n> > > @@ -109,9 +109,10 @@ public:\n> > >       Config config_;\n> > >\n> > >  private:\n> > > -     void platformSetIspCrop() override\n> > > +     void platformSetIspCrop(const Rectangle &ispCrop) override\n> > >       {\n> > > -             isp_[Isp::Input].dev()->setSelection(V4L2_SEL_TGT_CROP, &ispCrop_);\n> > > +             Rectangle crop = ispCrop;\n> > > +             isp_[Isp::Input].dev()->setSelection(V4L2_SEL_TGT_CROP, &crop);\n> > >       }\n> > >\n> > >       int platformConfigure(const RPi::RPiCameraConfiguration *rpiConfig) override;\n> > > @@ -707,7 +708,7 @@ int Vc4CameraData::platformConfigure(const RPi::RPiCameraConfiguration *rpiConfi\n> > >       Size size = unicamFormat.size.boundedToAspectRatio(maxSize);\n> > >       ispCrop_ = size.centeredTo(Rectangle(unicamFormat.size).center());\n> > >\n> > > -     platformSetIspCrop();\n> > > +     platformSetIspCrop(ispCrop_);\n> > >\n> > >       return 0;\n> > >  }","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 24B79C324C\n\tfor <parsemail@patchwork.libcamera.org>;\n\tWed, 28 Aug 2024 16:28:31 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 07D6163458;\n\tWed, 28 Aug 2024 18:28:30 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A0FEB61E4F\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tWed, 28 Aug 2024 18:28:28 +0200 (CEST)","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 8FDC1220;\n\tWed, 28 Aug 2024 18:27:20 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"dqivZMao\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1724862440;\n\tbh=NIQ6F+eZGINOwZBVtNeZ1QIFyuac1AFKg3sPg9ldWN0=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=dqivZMaosgYE8spRx4l/3mFCU0pLzB5HF5wuzxR6Z9+Vvj6HFV1oYYnMrYVykapT6\n\t6YyzXR4jrepOHxD6IzghmGy3StewywzkKmoha9iaaAc9M0PRS/X1C969Z/i881WE1y\n\twKdLwb3meHJ1NkwUkqulQwMj2ubNPsWbsMLf+fY8=","Date":"Wed, 28 Aug 2024 19:28:24 +0300","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Naushir Patuck <naush@raspberrypi.com>","Cc":"Jacopo Mondi <jacopo.mondi@ideasonboard.com>,\n\tlibcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v1 3/7] pipeline: rpi: Pass crop rectangle as a parameter\n\tto platformSetIspCrop()","Message-ID":"<20240828162824.GH27131@pendragon.ideasonboard.com>","References":"<20240808102346.13065-1-naush@raspberrypi.com>\n\t<20240808102346.13065-4-naush@raspberrypi.com>\n\t<adws66lzfgyd4jqc32ar5fobokudsvgopwzhqqyzpgl67xoblz@r2v4vkxtcvdm>\n\t<CAEmqJPrhWhCAxj34KW+Hjmwk5WPqxnEFhXmXCRp=-Uirc+_Ljw@mail.gmail.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<CAEmqJPrhWhCAxj34KW+Hjmwk5WPqxnEFhXmXCRp=-Uirc+_Ljw@mail.gmail.com>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}}]