[{"id":22221,"web_url":"https://patchwork.libcamera.org/comment/22221/","msgid":"<164631722548.3492470.10928906773156401452@Monstersaurus>","date":"2022-03-03T14:20:25","subject":"Re: [libcamera-devel] [PATCH v2] libcamera: pipeline: raspberrypi:\n\tFix scaler crop when sensor is configured","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi David,\n\nQuoting David Plowman (2022-03-03 12:11:13)\n> We must calculate the initial scaler crop when the camera is\n> configured, otherwise the metadata will report this rectangle as being\n> all zeroes.\n> \n> Because the calculation is identical to that performed later in handling\n> the scaler crop control, we factor it into a small helper function,\n> RPiCameraData::scaleIspCrop.\n> \n> Signed-off-by: David Plowman \n> ---\n> .../pipeline/raspberrypi/raspberrypi.cpp | 20 ++++++++++++++++---\n> 1 file changed, 17 insertions(+), 3 deletions(-)\n> \n> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> index 29bff9d6..eede78e6 100644\n> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> @@ -212,6 +212,7 @@ public:\n> void handleStreamBuffer(FrameBuffer *buffer, RPi::Stream *stream);\n> void handleExternalBuffer(FrameBuffer *buffer, RPi::Stream *stream);\n> void handleState();\n> + Rectangle scaleIspCrop(const Rectangle &ispCrop) const;\n> void applyScalerCrop(const ControlList &controls);\n> \n> std::unique_ptr ipa_;\n> @@ -887,6 +888,9 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)\n> if (ret)\n> LOG(RPI, Error) << \"Failed to configure the IPA: \" << ret;\n> \n> + /* Set the scaler crop to the value we are using (scaled to native sensor coordinates). */\n> + data->scalerCrop_ = data->scaleIspCrop(data->ispCrop_);\n> +\n> /*\n> * Configure the Unicam embedded data output format only if the sensor\n> * supports it.\n> @@ -1974,6 +1978,18 @@ void RPiCameraData::checkRequestCompleted()\n> }\n> }\n> \n> +Rectangle RPiCameraData::scaleIspCrop(const Rectangle &ispCrop) const\n> +{\n> + /*\n> + * Scale a crop rectangle defined in the ISP's coordinates into native sensor\n> + * coordinates.\n> + */\n> + Rectangle nativeCrop = ispCrop.scaledBy(sensorInfo_.analogCrop.size(),\n> + sensorInfo_.outputSize);\n> + nativeCrop.translateBy(sensorInfo_.analogCrop.topLeft());\n\nI'm struggling to be sure, but this is what the code was before, so I'm\nsure it's fine... should the amount we translate by also be scaled?\n\nOr perhaps that's just to maintain the original cropping point.\n\n> + return nativeCrop;\n\nI know it was only two lines, but I certainly prefer this, as it's now\nit's more clear (and more documented, and self-documenting) for what is\nbeing returned.\n\nReviewed-by: Kieran Bingham \n\n\n> +}\n> +\n> void RPiCameraData::applyScalerCrop(const ControlList &controls)\n> {\n> if (controls.contains(controls::ScalerCrop)) {\n> @@ -2006,9 +2022,7 @@ void RPiCameraData::applyScalerCrop(const ControlList &controls)\n> * used. But we must first rescale that from ISP (camera mode) pixels\n> * back into sensor native pixels.\n> */\n> - scalerCrop_ = ispCrop_.scaledBy(sensorInfo_.analogCrop.size(),\n> - sensorInfo_.outputSize);\n> - scalerCrop_.translateBy(sensorInfo_.analogCrop.topLeft());\n> + scalerCrop_ = scaleIspCrop(ispCrop_);\n> }\n> }\n> }\n> -- \n> 2.30.2\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 295AABF415\n\tfor ;\n\tThu, 3 Mar 2022 14:20:31 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7FA98601FF;\n\tThu, 3 Mar 2022 15:20:30 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id EA1B8601FF\n\tfor ;\n\tThu, 3 Mar 2022 15:20:28 +0100 (CET)","from pendragon.ideasonboard.com\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 9193C885;\n\tThu, 3 Mar 2022 15:20:28 +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=\"IDtEwhvW\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1646317228;\n\tbh=32dmouwgjIdoqRG7sq/jkRyxzKiT/Du7oSusIlPP9jw=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=IDtEwhvWQ2p6TDUejbtxifdaQKXJhhfxDbrz73SlRE8tgIab8d08iICUB+vth5OyN\n\tubk0O7kU6Etlk8VPIU/Y2GNhn8sbJR4RNYZUtu1Sn3EU5B3QKNdfdmIrEzjJisbAW6\n\tiGDVsW66zJOTf1Z7IWVFKN5RtWMrOsScFFTj3d4A=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20220303121113.16839-1-david.plowman@raspberrypi.com>","References":"<20220303121113.16839-1-david.plowman@raspberrypi.com>","From":"Kieran Bingham ","To":"David Plowman ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Thu, 03 Mar 2022 14:20:25 +0000","Message-ID":"<164631722548.3492470.10928906773156401452@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH v2] libcamera: pipeline: raspberrypi:\n\tFix scaler crop when sensor is configured","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":22223,"web_url":"https://patchwork.libcamera.org/comment/22223/","msgid":"","date":"2022-03-03T14:32:01","subject":"Re: [libcamera-devel] [PATCH v2] libcamera: pipeline: raspberrypi:\n\tFix scaler crop when sensor is configured","submitter":{"id":42,"url":"https://patchwork.libcamera.org/api/people/42/","name":"David Plowman","email":"david.plowman@raspberrypi.com"},"content":"Hi Kieran\n\nThanks for review number 2!\n\nOn Thu, 3 Mar 2022 at 14:20, Kieran Bingham\n wrote:\n>\n> Hi David,\n>\n> Quoting David Plowman (2022-03-03 12:11:13)\n> > We must calculate the initial scaler crop when the camera is\n> > configured, otherwise the metadata will report this rectangle as being\n> > all zeroes.\n> >\n> > Because the calculation is identical to that performed later in handling\n> > the scaler crop control, we factor it into a small helper function,\n> > RPiCameraData::scaleIspCrop.\n> >\n> > Signed-off-by: David Plowman \n> > ---\n> > .../pipeline/raspberrypi/raspberrypi.cpp | 20 ++++++++++++++++---\n> > 1 file changed, 17 insertions(+), 3 deletions(-)\n> >\n> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > index 29bff9d6..eede78e6 100644\n> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > @@ -212,6 +212,7 @@ public:\n> > void handleStreamBuffer(FrameBuffer *buffer, RPi::Stream *stream);\n> > void handleExternalBuffer(FrameBuffer *buffer, RPi::Stream *stream);\n> > void handleState();\n> > + Rectangle scaleIspCrop(const Rectangle &ispCrop) const;\n> > void applyScalerCrop(const ControlList &controls);\n> >\n> > std::unique_ptr ipa_;\n> > @@ -887,6 +888,9 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)\n> > if (ret)\n> > LOG(RPI, Error) << \"Failed to configure the IPA: \" << ret;\n> >\n> > + /* Set the scaler crop to the value we are using (scaled to native sensor coordinates). */\n> > + data->scalerCrop_ = data->scaleIspCrop(data->ispCrop_);\n> > +\n> > /*\n> > * Configure the Unicam embedded data output format only if the sensor\n> > * supports it.\n> > @@ -1974,6 +1978,18 @@ void RPiCameraData::checkRequestCompleted()\n> > }\n> > }\n> >\n> > +Rectangle RPiCameraData::scaleIspCrop(const Rectangle &ispCrop) const\n> > +{\n> > + /*\n> > + * Scale a crop rectangle defined in the ISP's coordinates into native sensor\n> > + * coordinates.\n> > + */\n> > + Rectangle nativeCrop = ispCrop.scaledBy(sensorInfo_.analogCrop.size(),\n> > + sensorInfo_.outputSize);\n> > + nativeCrop.translateBy(sensorInfo_.analogCrop.topLeft());\n>\n> I'm struggling to be sure, but this is what the code was before, so I'm\n> sure it's fine... should the amount we translate by also be scaled?\n\nI think it's right as it is. sensorInfo_.analogCrop is necessarily in\nthe \"sensor's native coordinates\", which nativeCrop also is by dint of\nthe line above, so it looks plausible to me...\n\nDavid\n\n>\n> Or perhaps that's just to maintain the original cropping point.\n>\n> > + return nativeCrop;\n>\n> I know it was only two lines, but I certainly prefer this, as it's now\n> it's more clear (and more documented, and self-documenting) for what is\n> being returned.\n>\n> Reviewed-by: Kieran Bingham \n>\n>\n> > +}\n> > +\n> > void RPiCameraData::applyScalerCrop(const ControlList &controls)\n> > {\n> > if (controls.contains(controls::ScalerCrop)) {\n> > @@ -2006,9 +2022,7 @@ void RPiCameraData::applyScalerCrop(const ControlList &controls)\n> > * used. But we must first rescale that from ISP (camera mode) pixels\n> > * back into sensor native pixels.\n> > */\n> > - scalerCrop_ = ispCrop_.scaledBy(sensorInfo_.analogCrop.size(),\n> > - sensorInfo_.outputSize);\n> > - scalerCrop_.translateBy(sensorInfo_.analogCrop.topLeft());\n> > + scalerCrop_ = scaleIspCrop(ispCrop_);\n> > }\n> > }\n> > }\n> > --\n> > 2.30.2\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 42D30BE08A\n\tfor ;\n\tThu, 3 Mar 2022 14:32:15 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9130C6115E;\n\tThu, 3 Mar 2022 15:32:14 +0100 (CET)","from mail-wr1-x42e.google.com (mail-wr1-x42e.google.com\n\t[IPv6:2a00:1450:4864:20::42e])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 637CF601FF\n\tfor ;\n\tThu, 3 Mar 2022 15:32:13 +0100 (CET)","by mail-wr1-x42e.google.com with SMTP id bk29so8156011wrb.4\n\tfor ;\n\tThu, 03 Mar 2022 06:32:13 -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=\"cuHLh5mv\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google;\n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=OVt2oCi6fxW+83ujM5IpeW6BCj8V8OLXxUYwEwDmxgY=;\n\tb=cuHLh5mvcbApOPH6IY8KNeKkn43zesn2Hp2w1+eN10g91Q66fGLMUpFFx3o4D4O7zZ\n\tSh/SVykZqU6/VWVCDgkpFE2PrixaF5dyQvKTXQVbDDp4owvL7V+2ButpIGacaK/lUdek\n\tSHl6cm/z1e0rdaqvdgCXKxbNHT4CFDyG5FKrrNM0HQpVc6YQ2EzScnEYN5CH/zA1winX\n\tRGrP53RSm5X90lB2CQbmI8bB36s/33nM2GvHp9Dae43Y47+TFP1GKwmIBfD6AqG5a76M\n\t9LLSwDX94eJKkCUxPsr/bASV4hcjqP7DhcZEFC/k/Hr0CnVnOL2tU5HnoyP/OszAsYCl\n\tuKqg==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=OVt2oCi6fxW+83ujM5IpeW6BCj8V8OLXxUYwEwDmxgY=;\n\tb=fS8ZWIt7JmYhj8KDLEfxR3qoSGonIQOteK9lM+GUwM+PvGz7NqXUHPhUgQkP38hX0n\n\tF6uvFv0iSOWXlbdJyyjcap7mzIaF2EUi6gUlCWyCCBByF+qnbTtiABc7xNGFMCNKXgwt\n\tYHZhehFn7/uqviwK+a6q+J/S6wVuVW2oVhpInqx666JA/X3pxw6turHWcEk0ONr5RBxQ\n\taJ9C1fMJrDSGMpZbCprKV1f0IX8K/58l3ZQdIZWU/dtdv91VOzUQZeVhcnSP60J+hdN3\n\tEyXcTU+JxE2jenlKwPyec/FvkgxuuWO1ef2QBJ/OZIV7+0iVfVGnAEwCaxbF45k9ED5x\n\t4FRg==","X-Gm-Message-State":"AOAM530nEgHDPViDp+BBuU+//I3kznWvXF6xrNHeZOHAhD9fLvDtySw6\n\txLg1dsh8eisRZJkETWMnVf/pvt4YFHlbO35+YxI5HKxntkI=","X-Google-Smtp-Source":"ABdhPJzS0W/KWofj6rlvoBEy5g1gdN78+LHQd5gf9rJE39WzxouQxnmwHzkbX9ka0N+nyhOH0JCcFbrne6Xa/kEY/bA=","X-Received":"by 2002:adf:a198:0:b0:1f0:2477:3b79 with SMTP id\n\tu24-20020adfa198000000b001f024773b79mr6954902wru.24.1646317932062;\n\tThu, 03 Mar 2022 06:32:12 -0800 (PST)","MIME-Version":"1.0","References":"<20220303121113.16839-1-david.plowman@raspberrypi.com>\n\t<164631722548.3492470.10928906773156401452@Monstersaurus>","In-Reply-To":"<164631722548.3492470.10928906773156401452@Monstersaurus>","From":"David Plowman ","Date":"Thu, 3 Mar 2022 14:32:01 +0000","Message-ID":"","To":"Kieran Bingham ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH v2] libcamera: pipeline: raspberrypi:\n\tFix scaler crop when sensor is configured","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 ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":22227,"web_url":"https://patchwork.libcamera.org/comment/22227/","msgid":"","date":"2022-03-04T16:11:49","subject":"Re: [libcamera-devel] [PATCH v2] libcamera: pipeline: raspberrypi:\n\tFix scaler crop when sensor is configured","submitter":{"id":97,"url":"https://patchwork.libcamera.org/api/people/97/","name":"Nicolas Dufresne via libcamera-devel","email":"libcamera-devel@lists.libcamera.org"},"content":"Hi David,\n\nThank you for your patch.\n\nOn Thu, 3 Mar 2022 at 12:11, David Plowman \nwrote:\n\n> We must calculate the initial scaler crop when the camera is\n> configured, otherwise the metadata will report this rectangle as being\n> all zeroes.\n>\n> Because the calculation is identical to that performed later in handling\n> the scaler crop control, we factor it into a small helper function,\n> RPiCameraData::scaleIspCrop.\n>\n> Signed-off-by: David Plowman \n>\n\nReviewed-by: Naushir Patuck \n\n\n> ---\n> .../pipeline/raspberrypi/raspberrypi.cpp | 20 ++++++++++++++++---\n> 1 file changed, 17 insertions(+), 3 deletions(-)\n>\n> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> index 29bff9d6..eede78e6 100644\n> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> @@ -212,6 +212,7 @@ public:\n> void handleStreamBuffer(FrameBuffer *buffer, RPi::Stream *stream);\n> void handleExternalBuffer(FrameBuffer *buffer, RPi::Stream\n> *stream);\n> void handleState();\n> + Rectangle scaleIspCrop(const Rectangle &ispCrop) const;\n> void applyScalerCrop(const ControlList &controls);\n>\n> std::unique_ptr ipa_;\n> @@ -887,6 +888,9 @@ int PipelineHandlerRPi::configure(Camera *camera,\n> CameraConfiguration *config)\n> if (ret)\n> LOG(RPI, Error) << \"Failed to configure the IPA: \" << ret;\n>\n> + /* Set the scaler crop to the value we are using (scaled to native\n> sensor coordinates). */\n> + data->scalerCrop_ = data->scaleIspCrop(data->ispCrop_);\n> +\n> /*\n> * Configure the Unicam embedded data output format only if the\n> sensor\n> * supports it.\n> @@ -1974,6 +1978,18 @@ void RPiCameraData::checkRequestCompleted()\n> }\n> }\n>\n> +Rectangle RPiCameraData::scaleIspCrop(const Rectangle &ispCrop) const\n> +{\n> + /*\n> + * Scale a crop rectangle defined in the ISP's coordinates into\n> native sensor\n> + * coordinates.\n> + */\n> + Rectangle nativeCrop =\n> ispCrop.scaledBy(sensorInfo_.analogCrop.size(),\n> + sensorInfo_.outputSize);\n> + nativeCrop.translateBy(sensorInfo_.analogCrop.topLeft());\n> + return nativeCrop;\n> +}\n> +\n> void RPiCameraData::applyScalerCrop(const ControlList &controls)\n> {\n> if (controls.contains(controls::ScalerCrop)) {\n> @@ -2006,9 +2022,7 @@ void RPiCameraData::applyScalerCrop(const\n> ControlList &controls)\n> * used. But we must first rescale that from ISP\n> (camera mode) pixels\n> * back into sensor native pixels.\n> */\n> - scalerCrop_ =\n> ispCrop_.scaledBy(sensorInfo_.analogCrop.size(),\n> -\n> sensorInfo_.outputSize);\n> -\n> scalerCrop_.translateBy(sensorInfo_.analogCrop.topLeft());\n> + scalerCrop_ = scaleIspCrop(ispCrop_);\n> }\n> }\n> }\n> --\n> 2.30.2\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 63A68BF415\n\tfor ;\n\tFri, 4 Mar 2022 16:12:09 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 89F6561101;\n\tFri, 4 Mar 2022 17:12:08 +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 8F5A3601F7\n\tfor ;\n\tFri, 4 Mar 2022 17:12:06 +0100 (CET)","by mail-lj1-x22e.google.com with SMTP id s25so11635344lji.5\n\tfor ;\n\tFri, 04 Mar 2022 08:12:06 -0800 (PST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1646410328;\n\tbh=i2ZXjTAgr+RyMaG6H9WHbYXoik0jn5u+deWKh2ZZjgQ=;\n\th=References:In-Reply-To:Date:To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=ecLR1m4kPLJ/s7IfuG8Pc/2LgTQBshn1sGoPnhykHlmkMXmP9M03pBy31msI35ewR\n\tLv+a6HwLHa6qymx2wH6BAH4mppGhaXu89p75oT4pbT67leaoHGrNNKPIxuYGJlwatY\n\tcy9MJn8074Ej5AKfQrRv0kbmf3dp+ZOJqFBA1sAkhPiKi6WtiUYW7I3BGAz6T+bcLi\n\tN5byL4s0S8R8/CrQJDMAGq4ICOP470iiFLzHUFvf1mdUeKQxSj7gxeGdhrs07RrUNc\n\tF/RtfKemMRwtT39HZ4lxpc8pt+M5ZuPmaiDpjIWX1F+Qdc3C7yqMullwthPtCkO/Oc\n\ti8xqdOCL5zaCQ==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google;\n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=TgHfES7Rc531XHx887TjXUbZYUO4mWPKtmk7nqZxIkg=;\n\tb=nEw3EYFtZ+PbpWdyB5ZXcKE7Um5wfmy5CXYA+0/7icC0w8j3WTOAjcpvbm88nWyHn+\n\tsW2krl2D0fvxuTaYdtHlhLw2myJ1Co8maPV7xbuILXWz8acXfg5hA5jsIYiHQoAW5L+b\n\tZ6Ew0A0qNAgoFVusuxzrkTB6iabUlyWyaihD8iH5Mls2Rztq2CpHMzZT1M7we1k5/koG\n\tP2drK+lxUtnq+8aYH5Si5otJwbCoWXRC21rV/ElHt//b6snMlGYf1LpGHg1ffbwsHLLu\n\tTlLX/PAbkxf9fhWkmS4aZ2BdaajAcBK3jUEEDQ8g7mWAVicW+0HcJW+0MTHNahumqGgf\n\tacHw=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"nEw3EYFt\"; dkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=TgHfES7Rc531XHx887TjXUbZYUO4mWPKtmk7nqZxIkg=;\n\tb=XmIZLMwQXF9LXGMypeCw5Q3MfD5dXgRaqjrtdaN8701ZPys2OpvvnLoWPYKGFa1rtG\n\tdxgXFx+Dg6tzXu3lqHGp9RNuy9quNqML88rlocP7sWZ9MAecf1+XBnLmMznvjkue0e4g\n\tWMxliF+MJg66YrKgYJde1ALCAswQZMzyQGBvTXRJM6dOqvESeLuAce1ld71ukN1tLH+a\n\tiYiV8zPgtkZi2BRq1cDACEpkyhgDFK6sar76QUtPEfE7p10mzEa19XymkSoI93xKRjv+\n\tPdQdiH7JqBpE6IdWI7jcVwtGXHFg6Qwxmp1JfMz8AWk6VKqI2t+Ck63Jkw8vpwDVYkqs\n\tefhA==","X-Gm-Message-State":"AOAM531RFqnL3zJ3B8dM/RJ/5DQFKUAGu12XR1R98soG8JzEOGD/kwyX\n\tM5L1q62ETPb+gvjINg8BI4xgHWEZ4Hqx3XkcFQAAHg==","X-Google-Smtp-Source":"ABdhPJyO0qzYRrZq3JpITi87s/Q4iGjlMjHqyJR0+OBFrzu0pP1nsPzYexcGs1UKPPfuPKQ1g0BztcBrdgKQf7Ven2E=","X-Received":"by 2002:a2e:b0d5:0:b0:244:6a73:1550 with SMTP id\n\tg21-20020a2eb0d5000000b002446a731550mr27736547ljl.372.1646410325657;\n\tFri, 04 Mar 2022 08:12:05 -0800 (PST)","MIME-Version":"1.0","References":"<20220303121113.16839-1-david.plowman@raspberrypi.com>","In-Reply-To":"<20220303121113.16839-1-david.plowman@raspberrypi.com>","Date":"Fri, 4 Mar 2022 16:11:49 +0000","Message-ID":"","To":"David Plowman ","Content-Type":"multipart/alternative; boundary=\"0000000000009da2c505d966c625\"","Subject":"Re: [libcamera-devel] [PATCH v2] libcamera: pipeline: raspberrypi:\n\tFix scaler crop when sensor is configured","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","From":"Naushir Patuck via libcamera-devel\n\t","Reply-To":"Naushir Patuck ","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":22228,"web_url":"https://patchwork.libcamera.org/comment/22228/","msgid":"","date":"2022-03-04T16:14:19","subject":"Re: [libcamera-devel] [PATCH v2] libcamera: pipeline: raspberrypi:\n\tFix scaler crop when sensor is configured","submitter":{"id":97,"url":"https://patchwork.libcamera.org/api/people/97/","name":"Nicolas Dufresne via libcamera-devel","email":"libcamera-devel@lists.libcamera.org"},"content":"Hi David,\n\nThank you for the patch.\n\nOn Thu, Mar 03, 2022 at 02:32:01PM +0000, David Plowman wrote:\n> On Thu, 3 Mar 2022 at 14:20, Kieran Bingham wrote:\n> > Quoting David Plowman (2022-03-03 12:11:13)\n> > > We must calculate the initial scaler crop when the camera is\n> > > configured, otherwise the metadata will report this rectangle as being\n> > > all zeroes.\n> > >\n> > > Because the calculation is identical to that performed later in handling\n> > > the scaler crop control, we factor it into a small helper function,\n> > > RPiCameraData::scaleIspCrop.\n> > >\n> > > Signed-off-by: David Plowman \n> > > ---\n> > > .../pipeline/raspberrypi/raspberrypi.cpp | 20 ++++++++++++++++---\n> > > 1 file changed, 17 insertions(+), 3 deletions(-)\n> > >\n> > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > index 29bff9d6..eede78e6 100644\n> > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > @@ -212,6 +212,7 @@ public:\n> > > void handleStreamBuffer(FrameBuffer *buffer, RPi::Stream *stream);\n> > > void handleExternalBuffer(FrameBuffer *buffer, RPi::Stream *stream);\n> > > void handleState();\n> > > + Rectangle scaleIspCrop(const Rectangle &ispCrop) const;\n> > > void applyScalerCrop(const ControlList &controls);\n> > >\n> > > std::unique_ptr ipa_;\n> > > @@ -887,6 +888,9 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)\n> > > if (ret)\n> > > LOG(RPI, Error) << \"Failed to configure the IPA: \" << ret;\n> > >\n> > > + /* Set the scaler crop to the value we are using (scaled to native sensor coordinates). */\n\nThis line could be wrapped.\n\n> > > + data->scalerCrop_ = data->scaleIspCrop(data->ispCrop_);\n\nThe function could also use ispCrop_ internally instead of receiving it\nas a parameter.\n\nBoth of these can be done when applying (if desired).\n\nReviewed-by: Laurent Pinchart \n\n> > > +\n> > > /*\n> > > * Configure the Unicam embedded data output format only if the sensor\n> > > * supports it.\n> > > @@ -1974,6 +1978,18 @@ void RPiCameraData::checkRequestCompleted()\n> > > }\n> > > }\n> > >\n> > > +Rectangle RPiCameraData::scaleIspCrop(const Rectangle &ispCrop) const\n> > > +{\n> > > + /*\n> > > + * Scale a crop rectangle defined in the ISP's coordinates into native sensor\n> > > + * coordinates.\n> > > + */\n> > > + Rectangle nativeCrop = ispCrop.scaledBy(sensorInfo_.analogCrop.size(),\n> > > + sensorInfo_.outputSize);\n> > > + nativeCrop.translateBy(sensorInfo_.analogCrop.topLeft());\n> >\n> > I'm struggling to be sure, but this is what the code was before, so I'm\n> > sure it's fine... should the amount we translate by also be scaled?\n> \n> I think it's right as it is. sensorInfo_.analogCrop is necessarily in\n> the \"sensor's native coordinates\", which nativeCrop also is by dint of\n> the line above, so it looks plausible to me...\n> \n> > Or perhaps that's just to maintain the original cropping point.\n> >\n> > > + return nativeCrop;\n> >\n> > I know it was only two lines, but I certainly prefer this, as it's now\n> > it's more clear (and more documented, and self-documenting) for what is\n> > being returned.\n> >\n> > Reviewed-by: Kieran Bingham \n> >\n> > > +}\n> > > +\n> > > void RPiCameraData::applyScalerCrop(const ControlList &controls)\n> > > {\n> > > if (controls.contains(controls::ScalerCrop)) {\n> > > @@ -2006,9 +2022,7 @@ void RPiCameraData::applyScalerCrop(const ControlList &controls)\n> > > * used. But we must first rescale that from ISP (camera mode) pixels\n> > > * back into sensor native pixels.\n> > > */\n> > > - scalerCrop_ = ispCrop_.scaledBy(sensorInfo_.analogCrop.size(),\n> > > - sensorInfo_.outputSize);\n> > > - scalerCrop_.translateBy(sensorInfo_.analogCrop.topLeft());\n> > > + scalerCrop_ = scaleIspCrop(ispCrop_);\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 0268FBE08A\n\tfor ;\n\tFri, 4 Mar 2022 16:14:34 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 59673604EA;\n\tFri, 4 Mar 2022 17:14:34 +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 B3ECB601F7\n\tfor ;\n\tFri, 4 Mar 2022 17:14:32 +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 347EE51C;\n\tFri, 4 Mar 2022 17:14:32 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1646410474;\n\tbh=jqiGyIsfEH+4nKquqyal2oVbCd2poPXyDQXOhax9pIc=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=mODnQgJWS1hwoxkKcZU1JxrOBkmcCwuGc1zN//35CWuiDtC5T3DQgu4z3DeKuk9Mt\n\tknTqxh7mHdKqcTLfdZpLNrA41Qkk6haRuwBoAQ242YAojdDZwU24Go6VKyZ03pnwyt\n\tnz/oJUoGEug6TEpA5YXoud+6B1fIiHXJIvZn5WDXUwwu+HOsqRlkSOwgIKMPI7K7f+\n\t3Gp4ieYDkwTndyWLc7bTzBY4VuHONV7W0HUMc8SJd9zESMjoPAANOv23Zt4Bi7YDfW\n\tC2YjX7fGZkbgqfFE2vcqt000X410ESNfptZu96zXqMzmsi/I6+YE4NE90vsSiE9jer\n\tuwWd4dnBx+1aA==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1646410472;\n\tbh=jqiGyIsfEH+4nKquqyal2oVbCd2poPXyDQXOhax9pIc=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=WSUmtj8+sTYvhiZr+tqKzQ5wUOtGkuNYVZFkNQa/0n/J6Mx5vk8fN1I36+1cSv7cE\n\tfH+RfK9OBjkaqAhR9KiwClK5cN2aNYzfLbECVzIAjEP1k3AqDck10RoHoshIm+RWgL\n\tKp/EJJN3KpQfkni7k4Gwc62bTOVbpqPE+YCkEHI8="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"WSUmtj8+\"; dkim-atps=neutral","Date":"Fri, 4 Mar 2022 18:14:19 +0200","To":"David Plowman ","Message-ID":"","References":"<20220303121113.16839-1-david.plowman@raspberrypi.com>\n\t<164631722548.3492470.10928906773156401452@Monstersaurus>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v2] libcamera: pipeline: raspberrypi:\n\tFix scaler crop when sensor is configured","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","From":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":22229,"web_url":"https://patchwork.libcamera.org/comment/22229/","msgid":"","date":"2022-03-04T16:29:16","subject":"Re: [libcamera-devel] [PATCH v2] libcamera: pipeline: raspberrypi:\n\tFix scaler crop when sensor is configured","submitter":{"id":97,"url":"https://patchwork.libcamera.org/api/people/97/","name":"Nicolas Dufresne via libcamera-devel","email":"libcamera-devel@lists.libcamera.org"},"content":"Hi Laurent\n\nThanks for the review.\n\nOn Fri, 4 Mar 2022 at 16:14, Laurent Pinchart\n wrote:\n>\n> Hi David,\n>\n> Thank you for the patch.\n>\n> On Thu, Mar 03, 2022 at 02:32:01PM +0000, David Plowman wrote:\n> > On Thu, 3 Mar 2022 at 14:20, Kieran Bingham wrote:\n> > > Quoting David Plowman (2022-03-03 12:11:13)\n> > > > We must calculate the initial scaler crop when the camera is\n> > > > configured, otherwise the metadata will report this rectangle as being\n> > > > all zeroes.\n> > > >\n> > > > Because the calculation is identical to that performed later in handling\n> > > > the scaler crop control, we factor it into a small helper function,\n> > > > RPiCameraData::scaleIspCrop.\n> > > >\n> > > > Signed-off-by: David Plowman \n> > > > ---\n> > > > .../pipeline/raspberrypi/raspberrypi.cpp | 20 ++++++++++++++++---\n> > > > 1 file changed, 17 insertions(+), 3 deletions(-)\n> > > >\n> > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > index 29bff9d6..eede78e6 100644\n> > > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > @@ -212,6 +212,7 @@ public:\n> > > > void handleStreamBuffer(FrameBuffer *buffer, RPi::Stream *stream);\n> > > > void handleExternalBuffer(FrameBuffer *buffer, RPi::Stream *stream);\n> > > > void handleState();\n> > > > + Rectangle scaleIspCrop(const Rectangle &ispCrop) const;\n> > > > void applyScalerCrop(const ControlList &controls);\n> > > >\n> > > > std::unique_ptr ipa_;\n> > > > @@ -887,6 +888,9 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)\n> > > > if (ret)\n> > > > LOG(RPI, Error) << \"Failed to configure the IPA: \" << ret;\n> > > >\n> > > > + /* Set the scaler crop to the value we are using (scaled to native sensor coordinates). */\n>\n> This line could be wrapped.\n\nYes, agree.\n\n>\n> > > > + data->scalerCrop_ = data->scaleIspCrop(data->ispCrop_);\n>\n> The function could also use ispCrop_ internally instead of receiving it\n> as a parameter.\n\nI did consider this, actually. I vaguely wondered whether one might in\nfuture have some code where you might want to see what the native crop\nwould be without actually setting it. But possibly I'm committing the\ncommon mistake of over-generalising something without actually having\na reason, so I'm good to make that change too.\n\n>\n> Both of these can be done when applying (if desired).\n\nSo yes please, and thanks again!\nDavid\n\n>\n> Reviewed-by: Laurent Pinchart \n>\n> > > > +\n> > > > /*\n> > > > * Configure the Unicam embedded data output format only if the sensor\n> > > > * supports it.\n> > > > @@ -1974,6 +1978,18 @@ void RPiCameraData::checkRequestCompleted()\n> > > > }\n> > > > }\n> > > >\n> > > > +Rectangle RPiCameraData::scaleIspCrop(const Rectangle &ispCrop) const\n> > > > +{\n> > > > + /*\n> > > > + * Scale a crop rectangle defined in the ISP's coordinates into native sensor\n> > > > + * coordinates.\n> > > > + */\n> > > > + Rectangle nativeCrop = ispCrop.scaledBy(sensorInfo_.analogCrop.size(),\n> > > > + sensorInfo_.outputSize);\n> > > > + nativeCrop.translateBy(sensorInfo_.analogCrop.topLeft());\n> > >\n> > > I'm struggling to be sure, but this is what the code was before, so I'm\n> > > sure it's fine... should the amount we translate by also be scaled?\n> >\n> > I think it's right as it is. sensorInfo_.analogCrop is necessarily in\n> > the \"sensor's native coordinates\", which nativeCrop also is by dint of\n> > the line above, so it looks plausible to me...\n> >\n> > > Or perhaps that's just to maintain the original cropping point.\n> > >\n> > > > + return nativeCrop;\n> > >\n> > > I know it was only two lines, but I certainly prefer this, as it's now\n> > > it's more clear (and more documented, and self-documenting) for what is\n> > > being returned.\n> > >\n> > > Reviewed-by: Kieran Bingham \n> > >\n> > > > +}\n> > > > +\n> > > > void RPiCameraData::applyScalerCrop(const ControlList &controls)\n> > > > {\n> > > > if (controls.contains(controls::ScalerCrop)) {\n> > > > @@ -2006,9 +2022,7 @@ void RPiCameraData::applyScalerCrop(const ControlList &controls)\n> > > > * used. But we must first rescale that from ISP (camera mode) pixels\n> > > > * back into sensor native pixels.\n> > > > */\n> > > > - scalerCrop_ = ispCrop_.scaledBy(sensorInfo_.analogCrop.size(),\n> > > > - sensorInfo_.outputSize);\n> > > > - scalerCrop_.translateBy(sensorInfo_.analogCrop.topLeft());\n> > > > + scalerCrop_ = scaleIspCrop(ispCrop_);\n> > > > }\n> > > > }\n> > > > }\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","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 75ECABE08A\n\tfor ;\n\tFri, 4 Mar 2022 16:29:29 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D6AEF604EA;\n\tFri, 4 Mar 2022 17:29:28 +0100 (CET)","from mail-wr1-x42c.google.com (mail-wr1-x42c.google.com\n\t[IPv6:2a00:1450:4864:20::42c])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 66AD2601F7\n\tfor ;\n\tFri, 4 Mar 2022 17:29:27 +0100 (CET)","by mail-wr1-x42c.google.com with SMTP id j26so3113302wrb.1\n\tfor ;\n\tFri, 04 Mar 2022 08:29:27 -0800 (PST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1646411368;\n\tbh=hpVvKsALcUsOVQhiA7khO6OzoTwCvFgvyyFJ2jRSqWo=;\n\th=References:In-Reply-To:Date:To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=J4g/FXggC/BW//0CxZwMCTq0zYbqTW55Ky4OEBIfLLFxgS+0HW9ZkIP7hfkzDWrEF\n\tM6pNJdiiy6I2VXIz/UUFDg3znSygzInzauKlXO0kDfXF4Eidv1dJEhkwVdiNYhJXev\n\tisAUJfhMeEt1f4ygWOSQjami75ViiGv6K1us4aFYmox3s/+2TK07CYzCVi+Jd4cVUS\n\tKZ4OJahjs4zuU1HmaGC83dRi8k4KQzvvYm74rsfQuHZv3WNVTq1iuVYX8SfKJhhfXX\n\tD4/xTjPiRZPaH7PQ+2gpBjipgq5OLx4BFLVOA5N6SbJMo7m+QaEzcUCZ9/oAffMzfw\n\tX/yyVXwf3prkA==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google;\n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=FCIIFgqtXpGkf35At7UgLBTEiPHX1cTiLPSPpr3dk8Y=;\n\tb=d9n286oXJWL5ZDKHrjQ9FoyhKq5IXiB69pQb7zJaDeP3Jaxif/QNZLQzeLDayJNzhq\n\tk5Ejwhk0atSj4N1gxoVKJByvuDXMrW9dEGI92GDZfhY7tht/qMoe5VwgygYnrpSYdh8g\n\tC+sqdg/eSK0ErwPysa2RoV7xxE/C8RpH9T1m1XLJOcpwn00h0QqjLLIMJ6SEG30K6st/\n\tmbMtP951kBXxIk4eLiEmHTApxxJXMRUdAb9DQCW8SWonPrfTpeOQhy6H/7gdOU2isvUp\n\tSdob5pOyti0RZwPQ/lu/DfrptgHat3Kr13ZpN/gbYrEC+v1H+PEvQMu0WxVzFtbv3jJK\n\tTnvw=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"d9n286oX\"; dkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=FCIIFgqtXpGkf35At7UgLBTEiPHX1cTiLPSPpr3dk8Y=;\n\tb=0kcOLj+wU077N/5T+z7WBPDKnJDZ6vPAL0yJHftjR5PkPH1ZtOmyloAPjP1dn4+vhp\n\t8HuiQ6W/MorhGdRcncVWmnmDZRXbHuAoyq6gFOGspE5hym9WfVPwmEiGqMRsq877uPEb\n\tIZtBt/SahEDbZpTfI/UJH+1M/xOjudGbKh1Nqm/L7oaNq52V0cX2oNy18BNPhp5CsntH\n\tW8QiETZQC92jAuUgPb6rB07mgLt1W3r7I6ZaRp6fVmk/2ZZVSiNVwmfahQ+PVE1xwW0V\n\tPyHS5Z7Qp79CrZ6tuMm4tIObdFM2o71ru0SxiTYeBqsML/iUh6ZQsowyKAZbOYrHgIBk\n\tQi+w==","X-Gm-Message-State":"AOAM531rgxHGPU+5Smhp8z/9aBCtaObMD2dIak0IC5jtG4lNDWz45N3+\n\t7cAqmpGn3dYpP1XOeIiLyXNP+qQehX5GK6FLTNRI45qFRmc=","X-Google-Smtp-Source":"ABdhPJwo3z6SYmORtdozlk8ZAN3hY0ouozzZWP22cn/q8Up+OsM5ZhT7SEXBimQXl1UnVzQKjYmmog+gPmSm2x6XmUE=","X-Received":"by 2002:adf:fd8b:0:b0:1f0:2347:e618 with SMTP id\n\td11-20020adffd8b000000b001f02347e618mr11611894wrr.334.1646411366935;\n\tFri, 04 Mar 2022 08:29:26 -0800 (PST)","MIME-Version":"1.0","References":"<20220303121113.16839-1-david.plowman@raspberrypi.com>\n\t<164631722548.3492470.10928906773156401452@Monstersaurus>\n\t\n\t","In-Reply-To":"","Date":"Fri, 4 Mar 2022 16:29:16 +0000","Message-ID":"","To":"Laurent Pinchart ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH v2] libcamera: pipeline: raspberrypi:\n\tFix scaler crop when sensor is configured","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","From":"David Plowman via libcamera-devel ","Reply-To":"David Plowman ","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":22230,"web_url":"https://patchwork.libcamera.org/comment/22230/","msgid":"<164641370635.4170640.13583261869523485230@Monstersaurus>","date":"2022-03-04T17:08:26","subject":"Re: [libcamera-devel] [PATCH v2] libcamera: pipeline: raspberrypi:\n\tFix scaler crop when sensor is configured","submitter":{"id":97,"url":"https://patchwork.libcamera.org/api/people/97/","name":"Nicolas Dufresne via libcamera-devel","email":"libcamera-devel@lists.libcamera.org"},"content":"Hi David,\n\nQuoting David Plowman (2022-03-04 16:29:16)\n> Hi Laurent\n> \n> Thanks for the review.\n> \n> On Fri, 4 Mar 2022 at 16:14, Laurent Pinchart\n> wrote:\n> >\n> > Hi David,\n> >\n> > Thank you for the patch.\n> >\n> > On Thu, Mar 03, 2022 at 02:32:01PM +0000, David Plowman wrote:\n> > > On Thu, 3 Mar 2022 at 14:20, Kieran Bingham wrote:\n> > > > Quoting David Plowman (2022-03-03 12:11:13)\n> > > > > We must calculate the initial scaler crop when the camera is\n> > > > > configured, otherwise the metadata will report this rectangle as being\n> > > > > all zeroes.\n> > > > >\n> > > > > Because the calculation is identical to that performed later in handling\n> > > > > the scaler crop control, we factor it into a small helper function,\n> > > > > RPiCameraData::scaleIspCrop.\n> > > > >\n> > > > > Signed-off-by: David Plowman \n> > > > > ---\n> > > > > .../pipeline/raspberrypi/raspberrypi.cpp | 20 ++++++++++++++++---\n> > > > > 1 file changed, 17 insertions(+), 3 deletions(-)\n> > > > >\n> > > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > > index 29bff9d6..eede78e6 100644\n> > > > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > > @@ -212,6 +212,7 @@ public:\n> > > > > void handleStreamBuffer(FrameBuffer *buffer, RPi::Stream *stream);\n> > > > > void handleExternalBuffer(FrameBuffer *buffer, RPi::Stream *stream);\n> > > > > void handleState();\n> > > > > + Rectangle scaleIspCrop(const Rectangle &ispCrop) const;\n> > > > > void applyScalerCrop(const ControlList &controls);\n> > > > >\n> > > > > std::unique_ptr ipa_;\n> > > > > @@ -887,6 +888,9 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)\n> > > > > if (ret)\n> > > > > LOG(RPI, Error) << \"Failed to configure the IPA: \" << ret;\n> > > > >\n> > > > > + /* Set the scaler crop to the value we are using (scaled to native sensor coordinates). */\n> >\n> > This line could be wrapped.\n> \n> Yes, agree.\n> \n> >\n> > > > > + data->scalerCrop_ = data->scaleIspCrop(data->ispCrop_);\n> >\n> > The function could also use ispCrop_ internally instead of receiving it\n> > as a parameter.\n> \n> I did consider this, actually. I vaguely wondered whether one might in\n> future have some code where you might want to see what the native crop\n> would be without actually setting it. But possibly I'm committing the\n> common mistake of over-generalising something without actually having\n> a reason, so I'm good to make that change too.\n\nI've merged with the comment wrapped, but kept the variables here as\nthey are.\n\n--\nKieran\n\n\n> \n> >\n> > Both of these can be done when applying (if desired).\n> \n> So yes please, and thanks again!\n> David\n> \n> >\n> > Reviewed-by: Laurent Pinchart \n> >\n> > > > > +\n> > > > > /*\n> > > > > * Configure the Unicam embedded data output format only if the sensor\n> > > > > * supports it.\n> > > > > @@ -1974,6 +1978,18 @@ void RPiCameraData::checkRequestCompleted()\n> > > > > }\n> > > > > }\n> > > > >\n> > > > > +Rectangle RPiCameraData::scaleIspCrop(const Rectangle &ispCrop) const\n> > > > > +{\n> > > > > + /*\n> > > > > + * Scale a crop rectangle defined in the ISP's coordinates into native sensor\n> > > > > + * coordinates.\n> > > > > + */\n> > > > > + Rectangle nativeCrop = ispCrop.scaledBy(sensorInfo_.analogCrop.size(),\n> > > > > + sensorInfo_.outputSize);\n> > > > > + nativeCrop.translateBy(sensorInfo_.analogCrop.topLeft());\n> > > >\n> > > > I'm struggling to be sure, but this is what the code was before, so I'm\n> > > > sure it's fine... should the amount we translate by also be scaled?\n> > >\n> > > I think it's right as it is. sensorInfo_.analogCrop is necessarily in\n> > > the \"sensor's native coordinates\", which nativeCrop also is by dint of\n> > > the line above, so it looks plausible to me...\n> > >\n> > > > Or perhaps that's just to maintain the original cropping point.\n> > > >\n> > > > > + return nativeCrop;\n> > > >\n> > > > I know it was only two lines, but I certainly prefer this, as it's now\n> > > > it's more clear (and more documented, and self-documenting) for what is\n> > > > being returned.\n> > > >\n> > > > Reviewed-by: Kieran Bingham \n> > > >\n> > > > > +}\n> > > > > +\n> > > > > void RPiCameraData::applyScalerCrop(const ControlList &controls)\n> > > > > {\n> > > > > if (controls.contains(controls::ScalerCrop)) {\n> > > > > @@ -2006,9 +2022,7 @@ void RPiCameraData::applyScalerCrop(const ControlList &controls)\n> > > > > * used. But we must first rescale that from ISP (camera mode) pixels\n> > > > > * back into sensor native pixels.\n> > > > > */\n> > > > > - scalerCrop_ = ispCrop_.scaledBy(sensorInfo_.analogCrop.size(),\n> > > > > - sensorInfo_.outputSize);\n> > > > > - scalerCrop_.translateBy(sensorInfo_.analogCrop.topLeft());\n> > > > > + scalerCrop_ = scaleIspCrop(ispCrop_);\n> > > > > }\n> > > > > }\n> > > > > }\n> >\n> > --\n> > Regards,\n> >\n> > Laurent Pinchart","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 B290EBF415\n\tfor ;\n\tFri, 4 Mar 2022 17:08:31 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 125E96115E;\n\tFri, 4 Mar 2022 18:08:31 +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 EE5ED601F7\n\tfor ;\n\tFri, 4 Mar 2022 18:08:28 +0100 (CET)","from pendragon.ideasonboard.com\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 7ED8051C;\n\tFri, 4 Mar 2022 18:08:28 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1646413711;\n\tbh=mfXDmashGZGWzaYYqI3BEnpHpWheW9HanoT6iyTMnuI=;\n\th=In-Reply-To:References:To:Date:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=GQspQAd35nsQR7d94ensJf8d5reVRt1xhjr+RQG/jVxh4Zxhk7ZnIEzFlBsqj+HPq\n\tElEZ9pSYV92/puhWuP9WohnZ+UqRFjQ8270711iTw9gp/9ctzIvgeENzO3TYbVv5li\n\tEjTgzfNoqLCKF3JtnyfOFQRZsSqLFb4zH+t8mqejJQ00heHpuzXp0WTExeW01hX8Ik\n\tf751BZ80OK0+XStQGqPOWOIDmMnxnZfP3AHm98jeG4siif265Jxn4GTFl5aYcmYWdM\n\tNKaiCyGFTF+s3OobKS1oGDrbL1LfyaYt7PP8cucjJQc9c8lCk0KdK7yy+6iAcWZUuB\n\tCAuf561QuMgFA==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1646413708;\n\tbh=mfXDmashGZGWzaYYqI3BEnpHpWheW9HanoT6iyTMnuI=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=Iinyp7LzkgTQJOSfgWUkgk5KkVkUCsycA+ZEE2UDkHKDMCZHZnURkxsPuslhcXQFJ\n\tbdWiGbctUdaGW2ozRIwFEQe1BqjnBB27tXgOAE1dAy6reQz8lrHIEceg79m+2k3QpR\n\tRcP3d4ABYOxfYVvW6Zof2/WsQ1+W1UF9Tr+wgRhE="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"Iinyp7Lz\"; dkim-atps=neutral","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"","References":"<20220303121113.16839-1-david.plowman@raspberrypi.com>\n\t<164631722548.3492470.10928906773156401452@Monstersaurus>\n\t\n\t\n\t","To":"David Plowman ,\n\tLaurent Pinchart ","Date":"Fri, 04 Mar 2022 17:08:26 +0000","Message-ID":"<164641370635.4170640.13583261869523485230@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH v2] libcamera: pipeline: raspberrypi:\n\tFix scaler crop when sensor is configured","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","From":"Kieran Bingham via libcamera-devel\n\t","Reply-To":"Kieran Bingham ","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]