[{"id":13162,"web_url":"https://patchwork.libcamera.org/comment/13162/","msgid":"<20201011214708.GA3944@pendragon.ideasonboard.com>","date":"2020-10-11T21:47:08","subject":"Re: [libcamera-devel] [PATCH v3 0/6] Digital zoom","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi David,\n\nThank you for the patches. And more than sorry for reviewing the series\nso late.\n\nOn Tue, Sep 29, 2020 at 05:39:54PM +0100, David Plowman wrote:\n> Hi everyone\n> \n> Despite not originally intending to, I've actually made a version 3 of\n> the digital zoom patches, just to take care of a few things that might\n> be a bit annoying otherwise.\n> \n> 1. I've improved the description of the IspCrop control as was\n> suggested.\n> \n> 2. I've improved the description of the zoom option in cam (if we\n> decide to use this patch!), also as was proposed.\n> \n> 3. There was actually a problem with the \"{}\" syntax to denote zero\n> Rectangles in include/libcamera/ipa/raspberrypi.h. They were failing a\n> range type test in ControlInfoMap::generateIdmap() and so the control\n> wasn't working. I've replaced \"{}\" by \"Rectangle{}\" which seems OK.\n> \n> 4. There's been a bit of churn in the RPi pipeline handler lately so\n> rebasing gave some conflicts. I've fixed those up.\n> \n> Otherwise everything else remains the same.\n\nI've gone through the different versions of this series, and the first\nthing I want to see is that it has improved very nicely over time. Nice\njob !\n\nConfiguring zoom through an absolute crop rectangle is I believe the way\nto go, so the overall approach is good in my opinion. There are however\na few points I'd like to discuss, related to the SensorOutputSize\nproperty and IspCrop control.\n\nReading through the discussion, I become increasingly aware of a topic\nthat was present in the background without ever being named, and that's\nthe camera pipeline model. As mentioned yesterday in a few other replies\nto selected patches in this series and its previous versions, the job of\nlibcamera is to expose to applications a camera model that abstract\nhardware differences. This is not an easy job, as we have to strike the\nright balance between a higher-level inflexible but simple to use model\nthat can't support many of the less common use cases, and a lower-level\npowerful but complex to use model that exposes a very large range of\nhardware features. We have implicitly defined the skeleton of such a\nmodel through the API of the Camera and PipelineHandler classes, but\nhave never made it explicit.\n\nThis needs to change. I see no reason to block the digital zoom feature\nuntil we finish documenting the pipeline model, but I would like to\ndesign the feature while thinking about the bigger picture. Here are the\nassumptions I think the pipeline model should make for devices that\nsupport digital zoom.\n\n- The camera pipeline starts with a camera sensor, that may implement\n binning, skipping and/or cropping.\n\n- The subsequent blocks in the pipeline may perform additional cropping,\n either at the direct command of the pipeline handler (e.g. cropping at\n the input of the scaler), or automatically to support image processing\n steps (e.g. colour interpoloation often drops a few lines and columns\n on all edges of the image).\n\n- The pipeline ends with a scaler that can implement digital zoom\n through a combination of cropping followed by scaling.\n\nThe exact order of the processing steps at the hardware level doesn't\nneed to match the pipeline model. For instance, cropping at the input\nand output of the scaler are interchangeable (not taking into account\nsub-pixel differences). It doesn't matter if the ISP scales before\ncropping the output, the hardware scaler parameters and output crop\nrectangle can be computed from an abstract input crop rectangle and\noutput size. This is crucial to consider for the definition of the\npipeline model: we need to design it in a way that ensures all features\ncan be mapped to how they are implemented in the different types of\nhardware we want to support, but we're otherwise free to not map\ncontrols and properties 1:1 with the hardware parameters. When multiple\noptions are possible, we should be guided by API design criteria such as\ncoherency, simplicity and flexibility.\n\nComing back to digital zoom, this series exposes a new SensorOutputSize\nproperty and a new IspCrop control. The SensorOutpuSize property is\nintroduced to support the IspCrop control, as the base rectangle in\nwhich the scaler can crop. There are two issues here that bother me:\n\n- The property, despite being named SensorOutputSize, potentially takes\n into account the cropping added by the CSI-2 receiver and by the ISP\n for operations that consume lines and columns on the edges of the\n image. The naming can create some confusion, which can possibly be\n addressed by a combination of documentation (you're covering that\n already) and possibly a more explicit name for the property. However,\n as the property bundles crop operations perfomed in different stages\n of the pipeline, I'm worried that it will turn out to be a bit\n ill-defined regardless of how well we document it, with slightly\n different behaviours in different implementations.\n\n- Ignoring the additional cropping performed in the CSI-2 receiver and\n ISP (if any), the property exposes the sensor binning, skipping and\n cropping. It bundles those three operations together, and thus doesn't\n convey how the cropping affects the field of view as a given output\n size can be achieved with different combinations of skipping/binning\n and cropping.\n\nFor these reasons, I'm concerned that the SensorOutputCrop property is a\nad-hoc solution to provide a reference for the IspCrop property, and\nwill not fit clearly in a pipeline model that should be based on simple,\nbase building blocks. I would thus like to propose an alternative\noption.\n\nInstead of expressing the IspCrop controls (which I think we should\nrename to ScalerCrop) relatively to the SensorOutputSize property, could\nwe express it relatively to the existing PixelArrayActiveAreas property\n? This would have the advantage, in my opinion, of abstracting binning\nand skipping from applications. The pipeline handlers would need to\nperform a bit more work to compute the crop rectangle actually applied\nto the scaler, in order to take sensor binning/skipping and all\nadditional cropping in the pipeline into account. The upside is that the\nScalerCrop will directly define how the field of view is affected. It\nwould also simplify the API, as no intermediate property between\nPixelArrayActiveAreas and ScalerCrop would need to be defined, and the\nScalerCrop coordinates wouldn't depend on the active camera\nconfiguration. I think this would be easier to clearly document as part\nof a camera pipeline model.\n\nTwo additional points I'd like to consider (and which are orthogonal to\nthe previous one) are:\n\n- Should we automatically adjust the ScalerCrop rectangle to always\n output square pixels, or should we allow modifying the aspect ratio\n when scaling ? Most use cases call for square pixels, but I don't\n think we necessarily want to create an artificial limitation here (as\n long as we make it easy for applications to compute the scaling\n parameters that will give them square pixels)/\n\n- Should we allow a ScalerCrop rectangle smaller than the stream output\n size, or should we restrict scaling to down-scaling only ?\n\n> David Plowman (6):\n> libcamera: Add SensorOutputSize property\n> libcamera: Initialise the SensorOutputSize property\n> libcamera: Add IspCrop control\n> libcamera: Add geometry helper functions\n> libcamera: pipeline: raspberrypi: Implementation of digital zoom\n> cam: Add command line option to test IspCrop control\n> \n> include/libcamera/geometry.h | 20 +++\n> include/libcamera/ipa/raspberrypi.h | 1 +\n> src/cam/capture.cpp | 25 +++-\n> src/cam/capture.h | 2 +-\n> src/cam/main.cpp | 3 +\n> src/cam/main.h | 1 +\n> src/ipa/raspberrypi/raspberrypi.cpp | 7 +\n> src/libcamera/camera_sensor.cpp | 6 +\n> src/libcamera/control_ids.yaml | 12 ++\n> src/libcamera/geometry.cpp | 129 ++++++++++++++++++\n> .../pipeline/raspberrypi/raspberrypi.cpp | 47 +++++++\n> src/libcamera/property_ids.yaml | 19 +++\n> 12 files changed, 269 insertions(+), 3 deletions(-)","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 A9464BEEE0\n\tfor ;\n\tSun, 11 Oct 2020 21:47:55 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3885960730;\n\tSun, 11 Oct 2020 23:47:55 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A1108605BE\n\tfor ;\n\tSun, 11 Oct 2020 23:47:54 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 18A42308;\n\tSun, 11 Oct 2020 23:47:54 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"DpZCZUcT\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1602452874;\n\tbh=/HEFsoQs+0A13O0AHvMGUbBONNP/hz2xvDKz0fQuBQk=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=DpZCZUcT9oxU8SSLJvmXX3jQbVYDXMSBjwvdng/x+dr6o5y9TfM/50xm4mfQzNg2Q\n\tXqnf4FBiHSp37V540WWBrpCAaidxa0Kzk6y+5VeXkVRtLBDl4oyAGYr0BF1cgnExzY\n\tkmK2rUXwj+YJhLAtiZcZOpFiDeRcTyK164sEcVFs=","Date":"Mon, 12 Oct 2020 00:47:08 +0300","From":"Laurent Pinchart ","To":"David Plowman ","Message-ID":"<20201011214708.GA3944@pendragon.ideasonboard.com>","References":"<20200929164000.15429-1-david.plowman@raspberrypi.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200929164000.15429-1-david.plowman@raspberrypi.com>","Subject":"Re: [libcamera-devel] [PATCH v3 0/6] Digital zoom","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":13180,"web_url":"https://patchwork.libcamera.org/comment/13180/","msgid":"","date":"2020-10-12T18:49:31","subject":"Re: [libcamera-devel] [PATCH v3 0/6] Digital zoom","submitter":{"id":42,"url":"https://patchwork.libcamera.org/api/people/42/","name":"David Plowman","email":"david.plowman@raspberrypi.com"},"content":"Hi Laurent\n\nThanks for the comments. Some interesting points.\n\nOn Sun, 11 Oct 2020 at 22:47, Laurent Pinchart <\nlaurent.pinchart@ideasonboard.com> wrote:\n\n> Hi David,\n>\n> Thank you for the patches. And more than sorry for reviewing the series\n> so late.\n>\n> On Tue, Sep 29, 2020 at 05:39:54PM +0100, David Plowman wrote:\n> > Hi everyone\n> >\n> > Despite not originally intending to, I've actually made a version 3 of\n> > the digital zoom patches, just to take care of a few things that might\n> > be a bit annoying otherwise.\n> >\n> > 1. I've improved the description of the IspCrop control as was\n> > suggested.\n> >\n> > 2. I've improved the description of the zoom option in cam (if we\n> > decide to use this patch!), also as was proposed.\n> >\n> > 3. There was actually a problem with the \"{}\" syntax to denote zero\n> > Rectangles in include/libcamera/ipa/raspberrypi.h. They were failing a\n> > range type test in ControlInfoMap::generateIdmap() and so the control\n> > wasn't working. I've replaced \"{}\" by \"Rectangle{}\" which seems OK.\n> >\n> > 4. There's been a bit of churn in the RPi pipeline handler lately so\n> > rebasing gave some conflicts. I've fixed those up.\n> >\n> > Otherwise everything else remains the same.\n>\n> I've gone through the different versions of this series, and the first\n> thing I want to see is that it has improved very nicely over time. Nice\n> job !\n>\n> Configuring zoom through an absolute crop rectangle is I believe the way\n> to go, so the overall approach is good in my opinion. There are however\n> a few points I'd like to discuss, related to the SensorOutputSize\n> property and IspCrop control.\n>\n> Reading through the discussion, I become increasingly aware of a topic\n> that was present in the background without ever being named, and that's\n> the camera pipeline model. As mentioned yesterday in a few other replies\n> to selected patches in this series and its previous versions, the job of\n> libcamera is to expose to applications a camera model that abstract\n> hardware differences. This is not an easy job, as we have to strike the\n> right balance between a higher-level inflexible but simple to use model\n> that can't support many of the less common use cases, and a lower-level\n> powerful but complex to use model that exposes a very large range of\n> hardware features. We have implicitly defined the skeleton of such a\n> model through the API of the Camera and PipelineHandler classes, but\n> have never made it explicit.\n>\n> This needs to change. I see no reason to block the digital zoom feature\n> until we finish documenting the pipeline model, but I would like to\n> design the feature while thinking about the bigger picture. Here are the\n> assumptions I think the pipeline model should make for devices that\n> support digital zoom.\n>\n> - The camera pipeline starts with a camera sensor, that may implement\n> binning, skipping and/or cropping.\n>\n> - The subsequent blocks in the pipeline may perform additional cropping,\n> either at the direct command of the pipeline handler (e.g. cropping at\n> the input of the scaler), or automatically to support image processing\n> steps (e.g. colour interpoloation often drops a few lines and columns\n> on all edges of the image).\n>\n> - The pipeline ends with a scaler that can implement digital zoom\n> through a combination of cropping followed by scaling.\n>\n> The exact order of the processing steps at the hardware level doesn't\n> need to match the pipeline model. For instance, cropping at the input\n> and output of the scaler are interchangeable (not taking into account\n> sub-pixel differences). It doesn't matter if the ISP scales before\n> cropping the output, the hardware scaler parameters and output crop\n> rectangle can be computed from an abstract input crop rectangle and\n> output size. This is crucial to consider for the definition of the\n> pipeline model: we need to design it in a way that ensures all features\n> can be mapped to how they are implemented in the different types of\n> hardware we want to support, but we're otherwise free to not map\n> controls and properties 1:1 with the hardware parameters. When multiple\n> options are possible, we should be guided by API design criteria such as\n> coherency, simplicity and flexibility.\n>\n> Coming back to digital zoom, this series exposes a new SensorOutputSize\n> property and a new IspCrop control. The SensorOutpuSize property is\n> introduced to support the IspCrop control, as the base rectangle in\n> which the scaler can crop. There are two issues here that bother me:\n>\n> - The property, despite being named SensorOutputSize, potentially takes\n> into account the cropping added by the CSI-2 receiver and by the ISP\n> for operations that consume lines and columns on the edges of the\n> image. The naming can create some confusion, which can possibly be\n> addressed by a combination of documentation (you're covering that\n> already) and possibly a more explicit name for the property. However,\n> as the property bundles crop operations perfomed in different stages\n> of the pipeline, I'm worried that it will turn out to be a bit\n> ill-defined regardless of how well we document it, with slightly\n> different behaviours in different implementations.\n>\n> - Ignoring the additional cropping performed in the CSI-2 receiver and\n> ISP (if any), the property exposes the sensor binning, skipping and\n> cropping. It bundles those three operations together, and thus doesn't\n> convey how the cropping affects the field of view as a given output\n> size can be achieved with different combinations of skipping/binning\n> and cropping.\n>\n> For these reasons, I'm concerned that the SensorOutputCrop property is a\n> ad-hoc solution to provide a reference for the IspCrop property, and\n> will not fit clearly in a pipeline model that should be based on simple,\n> base building blocks. I would thus like to propose an alternative\n> option.\n>\n> Instead of expressing the IspCrop controls (which I think we should\n> rename to ScalerCrop) relatively to the SensorOutputSize property, could\n> we express it relatively to the existing PixelArrayActiveAreas property\n> ? This would have the advantage, in my opinion, of abstracting binning\n> and skipping from applications. The pipeline handlers would need to\n> perform a bit more work to compute the crop rectangle actually applied\n> to the scaler, in order to take sensor binning/skipping and all\n> additional cropping in the pipeline into account. The upside is that the\n> ScalerCrop will directly define how the field of view is affected. It\n> would also simplify the API, as no intermediate property between\n> PixelArrayActiveAreas and ScalerCrop would need to be defined, and the\n> ScalerCrop coordinates wouldn't depend on the active camera\n> configuration. I think this would be easier to clearly document as part\n> of a camera pipeline model.\n>\n\nRenaming IspCrop to ScalerCrop sounds fine to me. It has had so many\ndifferent names!\n\nI'm less sure about trying to derive the SensorOutputSize (or\nScalerInputSize or whatever else we want to call it!) from the\nPixelArrayActiveAreas property. Let me try and take a step back.\n\nSo first, I think knowing the PixelArrayActiveArea isn't enough. How would\nyou know if the pipeline handler was doing some extra cropping that wasn't\n\"strictly necessary\", perhaps to reduce memory traffic, or for a faster\nframerate. How would the application know not to try and zoom there? It\nseems to me that this really is a decision for the pipeline handler based\non the sensor driver, it isn't available from the properties of the sensor\nitself.\n\nActually I'd quite like to leave the discussion there for the moment and\nsee if that much is controversial or not. Of course we then have to move on\nbut maybe let's see what we think about that first...\n\nThoughts??\n\nTwo additional points I'd like to consider (and which are orthogonal to\n> the previous one) are:\n>\n> - Should we automatically adjust the ScalerCrop rectangle to always\n> output square pixels, or should we allow modifying the aspect ratio\n> when scaling ? Most use cases call for square pixels, but I don't\n> think we necessarily want to create an artificial limitation here (as\n> long as we make it easy for applications to compute the scaling\n> parameters that will give them square pixels)/\n>\n\nPersonally I see no reason to restrict what an application can request. We\nneed to make it easy to request the right aspect ratio (hence those\ngeometry helpers), but if people actually have a use-case for something\nstrange...\n\n\n>\n> - Should we allow a ScalerCrop rectangle smaller than the stream output\n> size, or should we restrict scaling to down-scaling only ?\n>\n\nI think up-scaling is probably the most common use-case for us (though\ndownscaling will happen too). Think of all those (rubbish) 30x zoom\npictures that some phones like to produce...!\n\nThanks and best regards\nDavid\n\n\n>\n> > David Plowman (6):\n> > libcamera: Add SensorOutputSize property\n> > libcamera: Initialise the SensorOutputSize property\n> > libcamera: Add IspCrop control\n> > libcamera: Add geometry helper functions\n> > libcamera: pipeline: raspberrypi: Implementation of digital zoom\n> > cam: Add command line option to test IspCrop control\n> >\n> > include/libcamera/geometry.h | 20 +++\n> > include/libcamera/ipa/raspberrypi.h | 1 +\n> > src/cam/capture.cpp | 25 +++-\n> > src/cam/capture.h | 2 +-\n> > src/cam/main.cpp | 3 +\n> > src/cam/main.h | 1 +\n> > src/ipa/raspberrypi/raspberrypi.cpp | 7 +\n> > src/libcamera/camera_sensor.cpp | 6 +\n> > src/libcamera/control_ids.yaml | 12 ++\n> > src/libcamera/geometry.cpp | 129 ++++++++++++++++++\n> > .../pipeline/raspberrypi/raspberrypi.cpp | 47 +++++++\n> > src/libcamera/property_ids.yaml | 19 +++\n> > 12 files changed, 269 insertions(+), 3 deletions(-)\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 6782EBEEE0\n\tfor ;\n\tMon, 12 Oct 2020 18:49:49 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C930D60D5C;\n\tMon, 12 Oct 2020 20:49:48 +0200 (CEST)","from mail-oi1-x231.google.com (mail-oi1-x231.google.com\n\t[IPv6:2607:f8b0:4864:20::231])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B0BAD60580\n\tfor ;\n\tMon, 12 Oct 2020 20:49:46 +0200 (CEST)","by mail-oi1-x231.google.com with SMTP id c13so19731438oiy.6\n\tfor ;\n\tMon, 12 Oct 2020 11:49:46 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"pFqRpruh\"; 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=xgI6DSsuIRzpNI0jHzd45HRRi0DV1l/ZRgx0370G7Yw=;\n\tb=pFqRpruhIgZBtDAsz1iA1AdbnDZwFEBjhw9g3Rs2pMnIktnSlkmsx/ruXfV+3A+/Ey\n\t0L1RVkW92Rm2J/E+rgkO+fwOFlF2y5pebK15t1lTRGQdv2U0NrwY6YALswQwdK4iHvwL\n\tHXw7JE88qk5gyACTAGvqd9LRNMyDb8WshI6s61lVLWi4bjWA9PN0M2hZQMnTYhsxBFnb\n\tHAb2e+G44F9D3HTb76FI87VlU9+wRAJx+kZu0ogCdcsyIQJrhh/SxlawqBwWa3t+JXmp\n\tmzkyBpbhVRhkjJrrxLMWa9dkCyb8H9xMerL0xrx0CXdCfSwYcQEenmWpjJ3cHfsn51qW\n\tVsBA==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=xgI6DSsuIRzpNI0jHzd45HRRi0DV1l/ZRgx0370G7Yw=;\n\tb=gDPDFMiIXFqIVZ66OgK8VeJzS3GsPwycfj+GdI3fGczac3XkETUKUtZ21zLa3rYq5H\n\th8iS12b4lFJVCJ/aCJcRqquUfRCcxGw2ifQ7J6HtI1yW1jSrpr6HXTpX23rduemMGTSy\n\tToK9f8lTEji2yWxJ/sZh5xGx1CpKswwn+zRiXRgnWNp2vtSrU1D2kPpKJqZ0rZUx4fdc\n\tWYM86CKwgwK1Tywnbfvdwvjp27aOb0+eVJ4vbP98feZWlJApIQZ1nsepkb0tNMYQnhpT\n\t4t4euSg44VcfclGgVfFeWE2v1rjPdWBx9leG9ST4AThuzwMbp24BYX3DkCrYl1zBBVBg\n\tk8UA==","X-Gm-Message-State":"AOAM531ptzsHqeHpKN/7nKbNp/X84pW6UT2B1vufSfjyX2lFVG3RMO6k\n\tEpY1cXFRXCzi0b9Be1lgiZSvJtokUNkV6NGGh0eVng==","X-Google-Smtp-Source":"ABdhPJyJGICx7v4nwphyME8AeNF58Hw5Z798afyTsA7MKZe5uBCSoi6YuYtweQW+0gxBaEWuVFXFlJIM+CQPp94yVwY=","X-Received":"by 2002:aca:c354:: with SMTP id\n\tt81mr11013495oif.55.1602528583497; \n\tMon, 12 Oct 2020 11:49:43 -0700 (PDT)","MIME-Version":"1.0","References":"<20200929164000.15429-1-david.plowman@raspberrypi.com>\n\t<20201011214708.GA3944@pendragon.ideasonboard.com>","In-Reply-To":"<20201011214708.GA3944@pendragon.ideasonboard.com>","From":"David Plowman ","Date":"Mon, 12 Oct 2020 19:49:31 +0100","Message-ID":"","To":"Laurent Pinchart ","Subject":"Re: [libcamera-devel] [PATCH v3 0/6] Digital zoom","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"multipart/mixed;\n\tboundary=\"===============7063782129645898462==\"","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":13182,"web_url":"https://patchwork.libcamera.org/comment/13182/","msgid":"<20201013014422.GD3942@pendragon.ideasonboard.com>","date":"2020-10-13T01:44:22","subject":"Re: [libcamera-devel] [PATCH v3 0/6] Digital zoom","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi David,\n\nOn Mon, Oct 12, 2020 at 07:49:31PM +0100, David Plowman wrote:\n> Hi Laurent\n> \n> Thanks for the comments. Some interesting points.\n\nSorry about chiming in so late and challenging some of the base\nassumptions though. I'll now try not to introduce any further delay.\n\n> On Sun, 11 Oct 2020 at 22:47, Laurent Pinchart wrote:\n> \n> > Hi David,\n> >\n> > Thank you for the patches. And more than sorry for reviewing the series\n> > so late.\n> >\n> > On Tue, Sep 29, 2020 at 05:39:54PM +0100, David Plowman wrote:\n> > > Hi everyone\n> > >\n> > > Despite not originally intending to, I've actually made a version 3 of\n> > > the digital zoom patches, just to take care of a few things that might\n> > > be a bit annoying otherwise.\n> > >\n> > > 1. I've improved the description of the IspCrop control as was\n> > > suggested.\n> > >\n> > > 2. I've improved the description of the zoom option in cam (if we\n> > > decide to use this patch!), also as was proposed.\n> > >\n> > > 3. There was actually a problem with the \"{}\" syntax to denote zero\n> > > Rectangles in include/libcamera/ipa/raspberrypi.h. They were failing a\n> > > range type test in ControlInfoMap::generateIdmap() and so the control\n> > > wasn't working. I've replaced \"{}\" by \"Rectangle{}\" which seems OK.\n> > >\n> > > 4. There's been a bit of churn in the RPi pipeline handler lately so\n> > > rebasing gave some conflicts. I've fixed those up.\n> > >\n> > > Otherwise everything else remains the same.\n> >\n> > I've gone through the different versions of this series, and the first\n> > thing I want to see is that it has improved very nicely over time. Nice\n> > job !\n> >\n> > Configuring zoom through an absolute crop rectangle is I believe the way\n> > to go, so the overall approach is good in my opinion. There are however\n> > a few points I'd like to discuss, related to the SensorOutputSize\n> > property and IspCrop control.\n> >\n> > Reading through the discussion, I become increasingly aware of a topic\n> > that was present in the background without ever being named, and that's\n> > the camera pipeline model. As mentioned yesterday in a few other replies\n> > to selected patches in this series and its previous versions, the job of\n> > libcamera is to expose to applications a camera model that abstract\n> > hardware differences. This is not an easy job, as we have to strike the\n> > right balance between a higher-level inflexible but simple to use model\n> > that can't support many of the less common use cases, and a lower-level\n> > powerful but complex to use model that exposes a very large range of\n> > hardware features. We have implicitly defined the skeleton of such a\n> > model through the API of the Camera and PipelineHandler classes, but\n> > have never made it explicit.\n> >\n> > This needs to change. I see no reason to block the digital zoom feature\n> > until we finish documenting the pipeline model, but I would like to\n> > design the feature while thinking about the bigger picture. Here are the\n> > assumptions I think the pipeline model should make for devices that\n> > support digital zoom.\n> >\n> > - The camera pipeline starts with a camera sensor, that may implement\n> > binning, skipping and/or cropping.\n> >\n> > - The subsequent blocks in the pipeline may perform additional cropping,\n> > either at the direct command of the pipeline handler (e.g. cropping at\n> > the input of the scaler), or automatically to support image processing\n> > steps (e.g. colour interpoloation often drops a few lines and columns\n> > on all edges of the image).\n> >\n> > - The pipeline ends with a scaler that can implement digital zoom\n> > through a combination of cropping followed by scaling.\n> >\n> > The exact order of the processing steps at the hardware level doesn't\n> > need to match the pipeline model. For instance, cropping at the input\n> > and output of the scaler are interchangeable (not taking into account\n> > sub-pixel differences). It doesn't matter if the ISP scales before\n> > cropping the output, the hardware scaler parameters and output crop\n> > rectangle can be computed from an abstract input crop rectangle and\n> > output size. This is crucial to consider for the definition of the\n> > pipeline model: we need to design it in a way that ensures all features\n> > can be mapped to how they are implemented in the different types of\n> > hardware we want to support, but we're otherwise free to not map\n> > controls and properties 1:1 with the hardware parameters. When multiple\n> > options are possible, we should be guided by API design criteria such as\n> > coherency, simplicity and flexibility.\n> >\n> > Coming back to digital zoom, this series exposes a new SensorOutputSize\n> > property and a new IspCrop control. The SensorOutpuSize property is\n> > introduced to support the IspCrop control, as the base rectangle in\n> > which the scaler can crop. There are two issues here that bother me:\n> >\n> > - The property, despite being named SensorOutputSize, potentially takes\n> > into account the cropping added by the CSI-2 receiver and by the ISP\n> > for operations that consume lines and columns on the edges of the\n> > image. The naming can create some confusion, which can possibly be\n> > addressed by a combination of documentation (you're covering that\n> > already) and possibly a more explicit name for the property. However,\n> > as the property bundles crop operations perfomed in different stages\n> > of the pipeline, I'm worried that it will turn out to be a bit\n> > ill-defined regardless of how well we document it, with slightly\n> > different behaviours in different implementations.\n> >\n> > - Ignoring the additional cropping performed in the CSI-2 receiver and\n> > ISP (if any), the property exposes the sensor binning, skipping and\n> > cropping. It bundles those three operations together, and thus doesn't\n> > convey how the cropping affects the field of view as a given output\n> > size can be achieved with different combinations of skipping/binning\n> > and cropping.\n> >\n> > For these reasons, I'm concerned that the SensorOutputCrop property is a\n> > ad-hoc solution to provide a reference for the IspCrop property, and\n> > will not fit clearly in a pipeline model that should be based on simple,\n> > base building blocks. I would thus like to propose an alternative\n> > option.\n> >\n> > Instead of expressing the IspCrop controls (which I think we should\n> > rename to ScalerCrop) relatively to the SensorOutputSize property, could\n> > we express it relatively to the existing PixelArrayActiveAreas property\n> > ? This would have the advantage, in my opinion, of abstracting binning\n> > and skipping from applications. The pipeline handlers would need to\n> > perform a bit more work to compute the crop rectangle actually applied\n> > to the scaler, in order to take sensor binning/skipping and all\n> > additional cropping in the pipeline into account. The upside is that the\n> > ScalerCrop will directly define how the field of view is affected. It\n> > would also simplify the API, as no intermediate property between\n> > PixelArrayActiveAreas and ScalerCrop would need to be defined, and the\n> > ScalerCrop coordinates wouldn't depend on the active camera\n> > configuration. I think this would be easier to clearly document as part\n> > of a camera pipeline model.\n> \n> Renaming IspCrop to ScalerCrop sounds fine to me. It has had so many\n> different names!\n> \n> I'm less sure about trying to derive the SensorOutputSize (or\n> ScalerInputSize or whatever else we want to call it!) from the\n> PixelArrayActiveAreas property. Let me try and take a step back.\n> \n> So first, I think knowing the PixelArrayActiveArea isn't enough. How would\n> you know if the pipeline handler was doing some extra cropping that wasn't\n> \"strictly necessary\", perhaps to reduce memory traffic, or for a faster\n> framerate. How would the application know not to try and zoom there? It\n> seems to me that this really is a decision for the pipeline handler based\n> on the sensor driver, it isn't available from the properties of the sensor\n> itself.\n> \n> Actually I'd quite like to leave the discussion there for the moment and\n> see if that much is controversial or not. Of course we then have to move on\n> but maybe let's see what we think about that first...\n> \n> Thoughts??\n\nI fully agree with you that the pipeline handler has the last word when\nit comes to the (combined) internal cropping. It may not have any choice\n(if the CFA interpolation eats two or four lines and columns on each\nside of the image due to the hardware implementation, there's nothing\nthat can be done against it), or may just want to decide on policies for\nwhatever reason it sees fit (while trying not to over-police though, to\nleave options to applications).\n\nMy proposal doesn't deny this from the pipeline handler, but tries to\nexpress the crop rectangle in a way that maps directly to the field of\nview. To do so, binning/skipping would be hidden from applications by\nusing a reference for the ScalerCrop property that has no\nbinning/skipping applied.\n\nThe pipeline handler would restrict the requested ScalerCrop control\nbased on the internal cropping that is always applied (combining sensor,\nCSI-2 receiver and ISP), and report the result through the request\nmetadata. The application would thus know the exact crop rectangle that\nhas been used, in unscaled (binning/skipping) sensor coordinates.\n\nWhile in some use cases the actual crop rectangle reported through\nmetadata would be enough (for instance, in a GUI that starts with an\nunzoomed view, the application would get the maximum possible crop\nrectangle in the metadata of the first frame), I can imagine we would\nhave use cases that need this information before capturing the first\nframe. I initially thought we could report it through the max value of\nthe ControlInfo instance for the ScalerCrop control, but this is\nsupposed to be static information. As this would be (in my opinion) a\nneat solution, I'd like to investigate the possibility of making\nControlInfo dynamic, but maybe we will need a different solution. For\nforesee the addition of an API that will create request templates,\nfilling them with default control values based on a requested use case,\nand the maximum ScalerCrop could possibly be reported through that\nmechanism. I'm also fine reporting it through a temporary mechanism (for\ninstance adding it to CameraConfiguration, or creating a dynamic\nproperty) for the time being, if you already have pressing use cases for\nknowing the maximum value before capturing the first frame.\n\nThoughts ? :-)\n\n> > Two additional points I'd like to consider (and which are orthogonal to\n> > the previous one) are:\n> >\n> > - Should we automatically adjust the ScalerCrop rectangle to always\n> > output square pixels, or should we allow modifying the aspect ratio\n> > when scaling ? Most use cases call for square pixels, but I don't\n> > think we necessarily want to create an artificial limitation here (as\n> > long as we make it easy for applications to compute the scaling\n> > parameters that will give them square pixels)/\n> \n> Personally I see no reason to restrict what an application can request. We\n> need to make it easy to request the right aspect ratio (hence those\n> geometry helpers), but if people actually have a use-case for something\n> strange...\n\nFine with me.\n\n> > - Should we allow a ScalerCrop rectangle smaller than the stream output\n> > size, or should we restrict scaling to down-scaling only ?\n> \n> I think up-scaling is probably the most common use-case for us (though\n> downscaling will happen too). Think of all those (rubbish) 30x zoom\n> pictures that some phones like to produce...!\n\nRubbish is exactly the work I would have used :-) I'm tempted to try and\nteach users that they shouldn't do this by disabling this feature, but\nthat would be pretentious. I suppose there's no good reason to forbid\nthis. Should we put a limit on the upscaling factor though ?\n\n> > > David Plowman (6):\n> > > libcamera: Add SensorOutputSize property\n> > > libcamera: Initialise the SensorOutputSize property\n> > > libcamera: Add IspCrop control\n> > > libcamera: Add geometry helper functions\n> > > libcamera: pipeline: raspberrypi: Implementation of digital zoom\n> > > cam: Add command line option to test IspCrop control\n> > >\n> > > include/libcamera/geometry.h | 20 +++\n> > > include/libcamera/ipa/raspberrypi.h | 1 +\n> > > src/cam/capture.cpp | 25 +++-\n> > > src/cam/capture.h | 2 +-\n> > > src/cam/main.cpp | 3 +\n> > > src/cam/main.h | 1 +\n> > > src/ipa/raspberrypi/raspberrypi.cpp | 7 +\n> > > src/libcamera/camera_sensor.cpp | 6 +\n> > > src/libcamera/control_ids.yaml | 12 ++\n> > > src/libcamera/geometry.cpp | 129 ++++++++++++++++++\n> > > .../pipeline/raspberrypi/raspberrypi.cpp | 47 +++++++\n> > > src/libcamera/property_ids.yaml | 19 +++\n> > > 12 files changed, 269 insertions(+), 3 deletions(-)","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 8C951BEEE0\n\tfor ;\n\tTue, 13 Oct 2020 01:45:11 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0BD48605BE;\n\tTue, 13 Oct 2020 03:45: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 47455605BE\n\tfor ;\n\tTue, 13 Oct 2020 03:45:09 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id AFDB4528;\n\tTue, 13 Oct 2020 03:45:08 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"vWjdWWqh\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1602553508;\n\tbh=QuX5KxZEBXLS8EFRwGKIezLXjFSUKO5Yisg5dzYkijk=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=vWjdWWqhvA0Cm5TLCWVmxcJix6ckM1MdVQ6YsXEqbRz69FEEKzNI48/O5kWT82P8v\n\tXE8xYo3BA5Hs8xYyuhb5esfwoHz8UgbQ1o7BqA/zHO9MqrLn51zkJUacJGfIGwpV5p\n\tr/q19wASlWZW3AtJeF9k7HAm6VQ1kz1SnL6qijyc=","Date":"Tue, 13 Oct 2020 04:44:22 +0300","From":"Laurent Pinchart ","To":"David Plowman ","Message-ID":"<20201013014422.GD3942@pendragon.ideasonboard.com>","References":"<20200929164000.15429-1-david.plowman@raspberrypi.com>\n\t<20201011214708.GA3944@pendragon.ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v3 0/6] Digital zoom","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":13183,"web_url":"https://patchwork.libcamera.org/comment/13183/","msgid":"","date":"2020-10-13T08:36:50","subject":"Re: [libcamera-devel] [PATCH v3 0/6] Digital zoom","submitter":{"id":42,"url":"https://patchwork.libcamera.org/api/people/42/","name":"David Plowman","email":"david.plowman@raspberrypi.com"},"content":"Hi Laurent\n\nOn Tue, 13 Oct 2020 at 02:45, Laurent Pinchart\n wrote:\n>\n> Hi David,\n>\n> On Mon, Oct 12, 2020 at 07:49:31PM +0100, David Plowman wrote:\n> > Hi Laurent\n> >\n> > Thanks for the comments. Some interesting points.\n>\n> Sorry about chiming in so late and challenging some of the base\n> assumptions though. I'll now try not to introduce any further delay.\n>\n> > On Sun, 11 Oct 2020 at 22:47, Laurent Pinchart wrote:\n> >\n> > > Hi David,\n> > >\n> > > Thank you for the patches. And more than sorry for reviewing the series\n> > > so late.\n> > >\n> > > On Tue, Sep 29, 2020 at 05:39:54PM +0100, David Plowman wrote:\n> > > > Hi everyone\n> > > >\n> > > > Despite not originally intending to, I've actually made a version 3 of\n> > > > the digital zoom patches, just to take care of a few things that might\n> > > > be a bit annoying otherwise.\n> > > >\n> > > > 1. I've improved the description of the IspCrop control as was\n> > > > suggested.\n> > > >\n> > > > 2. I've improved the description of the zoom option in cam (if we\n> > > > decide to use this patch!), also as was proposed.\n> > > >\n> > > > 3. There was actually a problem with the \"{}\" syntax to denote zero\n> > > > Rectangles in include/libcamera/ipa/raspberrypi.h. They were failing a\n> > > > range type test in ControlInfoMap::generateIdmap() and so the control\n> > > > wasn't working. I've replaced \"{}\" by \"Rectangle{}\" which seems OK.\n> > > >\n> > > > 4. There's been a bit of churn in the RPi pipeline handler lately so\n> > > > rebasing gave some conflicts. I've fixed those up.\n> > > >\n> > > > Otherwise everything else remains the same.\n> > >\n> > > I've gone through the different versions of this series, and the first\n> > > thing I want to see is that it has improved very nicely over time. Nice\n> > > job !\n> > >\n> > > Configuring zoom through an absolute crop rectangle is I believe the way\n> > > to go, so the overall approach is good in my opinion. There are however\n> > > a few points I'd like to discuss, related to the SensorOutputSize\n> > > property and IspCrop control.\n> > >\n> > > Reading through the discussion, I become increasingly aware of a topic\n> > > that was present in the background without ever being named, and that's\n> > > the camera pipeline model. As mentioned yesterday in a few other replies\n> > > to selected patches in this series and its previous versions, the job of\n> > > libcamera is to expose to applications a camera model that abstract\n> > > hardware differences. This is not an easy job, as we have to strike the\n> > > right balance between a higher-level inflexible but simple to use model\n> > > that can't support many of the less common use cases, and a lower-level\n> > > powerful but complex to use model that exposes a very large range of\n> > > hardware features. We have implicitly defined the skeleton of such a\n> > > model through the API of the Camera and PipelineHandler classes, but\n> > > have never made it explicit.\n> > >\n> > > This needs to change. I see no reason to block the digital zoom feature\n> > > until we finish documenting the pipeline model, but I would like to\n> > > design the feature while thinking about the bigger picture. Here are the\n> > > assumptions I think the pipeline model should make for devices that\n> > > support digital zoom.\n> > >\n> > > - The camera pipeline starts with a camera sensor, that may implement\n> > > binning, skipping and/or cropping.\n> > >\n> > > - The subsequent blocks in the pipeline may perform additional cropping,\n> > > either at the direct command of the pipeline handler (e.g. cropping at\n> > > the input of the scaler), or automatically to support image processing\n> > > steps (e.g. colour interpoloation often drops a few lines and columns\n> > > on all edges of the image).\n> > >\n> > > - The pipeline ends with a scaler that can implement digital zoom\n> > > through a combination of cropping followed by scaling.\n> > >\n> > > The exact order of the processing steps at the hardware level doesn't\n> > > need to match the pipeline model. For instance, cropping at the input\n> > > and output of the scaler are interchangeable (not taking into account\n> > > sub-pixel differences). It doesn't matter if the ISP scales before\n> > > cropping the output, the hardware scaler parameters and output crop\n> > > rectangle can be computed from an abstract input crop rectangle and\n> > > output size. This is crucial to consider for the definition of the\n> > > pipeline model: we need to design it in a way that ensures all features\n> > > can be mapped to how they are implemented in the different types of\n> > > hardware we want to support, but we're otherwise free to not map\n> > > controls and properties 1:1 with the hardware parameters. When multiple\n> > > options are possible, we should be guided by API design criteria such as\n> > > coherency, simplicity and flexibility.\n> > >\n> > > Coming back to digital zoom, this series exposes a new SensorOutputSize\n> > > property and a new IspCrop control. The SensorOutpuSize property is\n> > > introduced to support the IspCrop control, as the base rectangle in\n> > > which the scaler can crop. There are two issues here that bother me:\n> > >\n> > > - The property, despite being named SensorOutputSize, potentially takes\n> > > into account the cropping added by the CSI-2 receiver and by the ISP\n> > > for operations that consume lines and columns on the edges of the\n> > > image. The naming can create some confusion, which can possibly be\n> > > addressed by a combination of documentation (you're covering that\n> > > already) and possibly a more explicit name for the property. However,\n> > > as the property bundles crop operations perfomed in different stages\n> > > of the pipeline, I'm worried that it will turn out to be a bit\n> > > ill-defined regardless of how well we document it, with slightly\n> > > different behaviours in different implementations.\n> > >\n> > > - Ignoring the additional cropping performed in the CSI-2 receiver and\n> > > ISP (if any), the property exposes the sensor binning, skipping and\n> > > cropping. It bundles those three operations together, and thus doesn't\n> > > convey how the cropping affects the field of view as a given output\n> > > size can be achieved with different combinations of skipping/binning\n> > > and cropping.\n> > >\n> > > For these reasons, I'm concerned that the SensorOutputCrop property is a\n> > > ad-hoc solution to provide a reference for the IspCrop property, and\n> > > will not fit clearly in a pipeline model that should be based on simple,\n> > > base building blocks. I would thus like to propose an alternative\n> > > option.\n> > >\n> > > Instead of expressing the IspCrop controls (which I think we should\n> > > rename to ScalerCrop) relatively to the SensorOutputSize property, could\n> > > we express it relatively to the existing PixelArrayActiveAreas property\n> > > ? This would have the advantage, in my opinion, of abstracting binning\n> > > and skipping from applications. The pipeline handlers would need to\n> > > perform a bit more work to compute the crop rectangle actually applied\n> > > to the scaler, in order to take sensor binning/skipping and all\n> > > additional cropping in the pipeline into account. The upside is that the\n> > > ScalerCrop will directly define how the field of view is affected. It\n> > > would also simplify the API, as no intermediate property between\n> > > PixelArrayActiveAreas and ScalerCrop would need to be defined, and the\n> > > ScalerCrop coordinates wouldn't depend on the active camera\n> > > configuration. I think this would be easier to clearly document as part\n> > > of a camera pipeline model.\n> >\n> > Renaming IspCrop to ScalerCrop sounds fine to me. It has had so many\n> > different names!\n> >\n> > I'm less sure about trying to derive the SensorOutputSize (or\n> > ScalerInputSize or whatever else we want to call it!) from the\n> > PixelArrayActiveAreas property. Let me try and take a step back.\n> >\n> > So first, I think knowing the PixelArrayActiveArea isn't enough. How would\n> > you know if the pipeline handler was doing some extra cropping that wasn't\n> > \"strictly necessary\", perhaps to reduce memory traffic, or for a faster\n> > framerate. How would the application know not to try and zoom there? It\n> > seems to me that this really is a decision for the pipeline handler based\n> > on the sensor driver, it isn't available from the properties of the sensor\n> > itself.\n> >\n> > Actually I'd quite like to leave the discussion there for the moment and\n> > see if that much is controversial or not. Of course we then have to move on\n> > but maybe let's see what we think about that first...\n> >\n> > Thoughts??\n>\n> I fully agree with you that the pipeline handler has the last word when\n> it comes to the (combined) internal cropping. It may not have any choice\n> (if the CFA interpolation eats two or four lines and columns on each\n> side of the image due to the hardware implementation, there's nothing\n> that can be done against it), or may just want to decide on policies for\n> whatever reason it sees fit (while trying not to over-police though, to\n> leave options to applications).\n>\n> My proposal doesn't deny this from the pipeline handler, but tries to\n> express the crop rectangle in a way that maps directly to the field of\n> view. To do so, binning/skipping would be hidden from applications by\n> using a reference for the ScalerCrop property that has no\n> binning/skipping applied.\n\nSo we certainly agree on the necessity for \"something\", that's good,\nand then it's a case of figuring out what \"something\" is. I have\nseveral conflicting thoughts here (I frequently disagree with myself):\n\n* Actually, as a basic principle, I think applications should be able\nto find out \"the truth\" if they want it. This would mean the sensor\ncrop, binning/scaling, and any subsequent cropping.\n\n* I even wonder sometimes whether there shouldn't really be a class\nthat describes how the camera mode has been derived from the sensor,\nwith all this information made explicit. But I think that leads us\neven deeper into the woods, and I suspect we don't really want to go\nthere! (Also, I'm not sure if the V4L2 API would even let you discover\nall of it, which might be a problem...)\n\n* But I also think digital zoom needs to be easy to use for\napplications that don't care about all these details (\"the truth\", as\nI called it!). Actually I feel the overwhelming majority of\napplications will fall into this category.\n\n>\n> The pipeline handler would restrict the requested ScalerCrop control\n> based on the internal cropping that is always applied (combining sensor,\n> CSI-2 receiver and ISP), and report the result through the request\n> metadata. The application would thus know the exact crop rectangle that\n> has been used, in unscaled (binning/skipping) sensor coordinates.\n\nLet me try and paraphrase a bit and you can tell me if I've got it wrong.\n\nYou're proposing a property (details in a moment) that can be used to\nchoose zoom regions for the ScalerCrop control.\n\nThe units for this property are the native sensor pixels, ignoring any\nbinning/scaling that might be going on.\n\nThe property would give you a Rectangle that expresses exactly the\narea in which you are permitted to zoom? The offset of the rectangle\ntells you where in the sensor array this rectangle actually lies.\n\nThe term \"sensor array\" there needs some clarification. I think it\nprobably means the \"PixelArraySize\" less any invalid or optical black\npixels. (Do we have a way to get hold of that easily?)\n\nThe ScalerCrop requested, and reported back, gives the offset and size\nof the zoom rectangle relative to the rectangle given by the property\nunder discussion here (I think that's the most convenient choice). So\nif it reports back offsets of (0,0) then you know you're in the top\nleft hand corner of what you could possibly ever see in this camera\nmode (but not necessarily of the sensor as a whole).\n\nI think this could work and is a fair compromise between the amount of\ninformation and the ease of use. Most digital zoom applications could\nsimply request its Size, and go from there. Only a few minor\nmis-givings:\n\n* Does V4L2 actually give you a way of finding out where a particular\n\"camera mode\" lies in the pixel array? Do you even know if a mode is\nbinned or heavily cropped... someone who knows rather more about V4L2\nneeds to answer that one!\n\n* Use of native sensor coords is OK, though a few sensors might\nsupport non-integer scaling at which point the property would be\nslightly \"approximate\". That seems fairly minor to me, though. (Some\nISPs might even do non-integer cropping, so the problem can exist\nregardless.)\n\n* It doesn't really support my assertion that you should be able to\nknow exactly what's going on, if you want to. (But if we decide this\ndoesn't matter, then that's fine too.)\n\n* In one of our early discussions we suggested that having the exact\npixel level information might be useful, and you could do things like\npan one pixel at a time. We lose that by going to native sensor\ncoordinates. I think I said at the time that this ability feels more\nuseful than I believe it really is, so I wouldn't be too upset.\n\n>\n> While in some use cases the actual crop rectangle reported through\n> metadata would be enough (for instance, in a GUI that starts with an\n> unzoomed view, the application would get the maximum possible crop\n> rectangle in the metadata of the first frame), I can imagine we would\n> have use cases that need this information before capturing the first\n\nIndeed. And I also think you should be able to set digital zoom to\ntake effect *on* the very first frame itself (this spills over into\nNaush's most recent patch set...)\n\n> frame. I initially thought we could report it through the max value of\n> the ControlInfo instance for the ScalerCrop control, but this is\n> supposed to be static information. As this would be (in my opinion) a\n> neat solution, I'd like to investigate the possibility of making\n> ControlInfo dynamic, but maybe we will need a different solution. For\n> foresee the addition of an API that will create request templates,\n> filling them with default control values based on a requested use case,\n> and the maximum ScalerCrop could possibly be reported through that\n> mechanism. I'm also fine reporting it through a temporary mechanism (for\n> instance adding it to CameraConfiguration, or creating a dynamic\n> property) for the time being, if you already have pressing use cases for\n> knowing the maximum value before capturing the first frame.\n>\n> Thoughts ? :-)\n\nI'm glad we're talking about all this stuff!!\n\n>\n> > > Two additional points I'd like to consider (and which are orthogonal to\n> > > the previous one) are:\n> > >\n> > > - Should we automatically adjust the ScalerCrop rectangle to always\n> > > output square pixels, or should we allow modifying the aspect ratio\n> > > when scaling ? Most use cases call for square pixels, but I don't\n> > > think we necessarily want to create an artificial limitation here (as\n> > > long as we make it easy for applications to compute the scaling\n> > > parameters that will give them square pixels)/\n> >\n> > Personally I see no reason to restrict what an application can request. We\n> > need to make it easy to request the right aspect ratio (hence those\n> > geometry helpers), but if people actually have a use-case for something\n> > strange...\n>\n> Fine with me.\n>\n> > > - Should we allow a ScalerCrop rectangle smaller than the stream output\n> > > size, or should we restrict scaling to down-scaling only ?\n> >\n> > I think up-scaling is probably the most common use-case for us (though\n> > downscaling will happen too). Think of all those (rubbish) 30x zoom\n> > pictures that some phones like to produce...!\n>\n> Rubbish is exactly the work I would have used :-) I'm tempted to try and\n> teach users that they shouldn't do this by disabling this feature, but\n> that would be pretentious. I suppose there's no good reason to forbid\n> this. Should we put a limit on the upscaling factor though ?\n\nAlways tricky. To what extent should we save people from themselves?\nI'm pretty sure the Pi imposes some kind of a limit, but don't really\nknow. In the end, so long as it doesn't blow up, that's kind of OK...\n\nBest regards\nDavid\n\n>\n> > > > David Plowman (6):\n> > > > libcamera: Add SensorOutputSize property\n> > > > libcamera: Initialise the SensorOutputSize property\n> > > > libcamera: Add IspCrop control\n> > > > libcamera: Add geometry helper functions\n> > > > libcamera: pipeline: raspberrypi: Implementation of digital zoom\n> > > > cam: Add command line option to test IspCrop control\n> > > >\n> > > > include/libcamera/geometry.h | 20 +++\n> > > > include/libcamera/ipa/raspberrypi.h | 1 +\n> > > > src/cam/capture.cpp | 25 +++-\n> > > > src/cam/capture.h | 2 +-\n> > > > src/cam/main.cpp | 3 +\n> > > > src/cam/main.h | 1 +\n> > > > src/ipa/raspberrypi/raspberrypi.cpp | 7 +\n> > > > src/libcamera/camera_sensor.cpp | 6 +\n> > > > src/libcamera/control_ids.yaml | 12 ++\n> > > > src/libcamera/geometry.cpp | 129 ++++++++++++++++++\n> > > > .../pipeline/raspberrypi/raspberrypi.cpp | 47 +++++++\n> > > > src/libcamera/property_ids.yaml | 19 +++\n> > > > 12 files changed, 269 insertions(+), 3 deletions(-)\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 7867ABEEE0\n\tfor ;\n\tTue, 13 Oct 2020 08:37:05 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id DAE1E60DFD;\n\tTue, 13 Oct 2020 10:37:04 +0200 (CEST)","from mail-ot1-x32b.google.com (mail-ot1-x32b.google.com\n\t[IPv6:2607:f8b0:4864:20::32b])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id BCCF260354\n\tfor ;\n\tTue, 13 Oct 2020 10:37:03 +0200 (CEST)","by mail-ot1-x32b.google.com with SMTP id t15so18360113otk.0\n\tfor ;\n\tTue, 13 Oct 2020 01:37:03 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"kv7c6IJb\"; 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=Ql5Y+LNxDJszhH/dgHBnbmskeCYHUFW7betRmtACUCM=;\n\tb=kv7c6IJb8XTR0aWqMVOdwV91cjWMjsz51/TNXbzN45+gal2Rl1bxCrNasWbOfhc09v\n\tozs3To7+Xb/5Xtkhr7Y2zQhFrvXZolbntMZPczh+dMeZk8xvuFUnuIliGT17HlHHlkLC\n\tIq9VPsSAwvL1gJGeBnx4LBCJroYmUl3xbTlSdspMakAzULEzNQlIbOhjSbdGwX7zlFdP\n\th6EmGWRg4U2/ZIOTm7RLt29L5bBHEPbkRRmvGDkzCfVnH+foiWCCrOvCpTMLbTTB2PVN\n\taZUno9h6lGG8rLYuJEvk5Z4sIdqe2kTO7gOFdj1dk58jgaNaB1KtddGqVcU1sBC9HRCr\n\tDUMA==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=Ql5Y+LNxDJszhH/dgHBnbmskeCYHUFW7betRmtACUCM=;\n\tb=fKCZpL4xg50sIfwjqn6osPe9xwmEoCfkwdhWBCXzrQDyEyvTsffpAepy8evPP2bLqx\n\t99XySJxp3teEba/80EL6VFgknNaHH5TzaFNAMeZ4AZ1vw5PoSNZZEZLQvFKAtNKtbfIb\n\tDjMeAC2CTLKdfJXWoiSG77uAr8vwSmjDz46tXInvkpv2bX/nqb01671N/ppDfLYoLyJ1\n\t59VkN6KNrELr9L+m1xKrndpVDVaep6QXke9EksX+PqfAROHBB5yO08l0pvr+xBcyTA8s\n\tM5/G0W3Uj2dbyvmxr86nycVIlxhKPHgDg3L++nrbCIO95yus7OHBAvd/pSkYK9SLwW7b\n\tTUig==","X-Gm-Message-State":"AOAM532BwEAwJkEptQNyvYDB5V4iQSDnRkN7bXd+ppKVCGVh2PktsbMQ\n\tGr7UyTzt0gcXphW/vWr+vm7EPFMPEod90u28wk3A7A==","X-Google-Smtp-Source":"ABdhPJzUIrIYs3vYBdliTdSrOtBy6wcmA0ibU8ZftU9DsUzfHl5Y1HnEpjcWNeN9SAiWO8TbPfnPlEzUrJT01mbtzlY=","X-Received":"by 2002:a05:6830:1091:: with SMTP id\n\ty17mr21933046oto.160.1602578221946; \n\tTue, 13 Oct 2020 01:37:01 -0700 (PDT)","MIME-Version":"1.0","References":"<20200929164000.15429-1-david.plowman@raspberrypi.com>\n\t<20201011214708.GA3944@pendragon.ideasonboard.com>\n\t\n\t<20201013014422.GD3942@pendragon.ideasonboard.com>","In-Reply-To":"<20201013014422.GD3942@pendragon.ideasonboard.com>","From":"David Plowman ","Date":"Tue, 13 Oct 2020 09:36:50 +0100","Message-ID":"","To":"Laurent Pinchart ","Subject":"Re: [libcamera-devel] [PATCH v3 0/6] Digital zoom","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":13208,"web_url":"https://patchwork.libcamera.org/comment/13208/","msgid":"<20201015021650.GH3858@pendragon.ideasonboard.com>","date":"2020-10-15T02:16:50","subject":"Re: [libcamera-devel] [PATCH v3 0/6] Digital zoom","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi David,\n\nOn Tue, Oct 13, 2020 at 09:36:50AM +0100, David Plowman wrote:\n> On Tue, 13 Oct 2020 at 02:45, Laurent Pinchart wrote:\n> > On Mon, Oct 12, 2020 at 07:49:31PM +0100, David Plowman wrote:\n> > > Hi Laurent\n> > >\n> > > Thanks for the comments. Some interesting points.\n> >\n> > Sorry about chiming in so late and challenging some of the base\n> > assumptions though. I'll now try not to introduce any further delay.\n> >\n> > > On Sun, 11 Oct 2020 at 22:47, Laurent Pinchart wrote:\n> > >\n> > > > Hi David,\n> > > >\n> > > > Thank you for the patches. And more than sorry for reviewing the series\n> > > > so late.\n> > > >\n> > > > On Tue, Sep 29, 2020 at 05:39:54PM +0100, David Plowman wrote:\n> > > > > Hi everyone\n> > > > >\n> > > > > Despite not originally intending to, I've actually made a version 3 of\n> > > > > the digital zoom patches, just to take care of a few things that might\n> > > > > be a bit annoying otherwise.\n> > > > >\n> > > > > 1. I've improved the description of the IspCrop control as was\n> > > > > suggested.\n> > > > >\n> > > > > 2. I've improved the description of the zoom option in cam (if we\n> > > > > decide to use this patch!), also as was proposed.\n> > > > >\n> > > > > 3. There was actually a problem with the \"{}\" syntax to denote zero\n> > > > > Rectangles in include/libcamera/ipa/raspberrypi.h. They were failing a\n> > > > > range type test in ControlInfoMap::generateIdmap() and so the control\n> > > > > wasn't working. I've replaced \"{}\" by \"Rectangle{}\" which seems OK.\n> > > > >\n> > > > > 4. There's been a bit of churn in the RPi pipeline handler lately so\n> > > > > rebasing gave some conflicts. I've fixed those up.\n> > > > >\n> > > > > Otherwise everything else remains the same.\n> > > >\n> > > > I've gone through the different versions of this series, and the first\n> > > > thing I want to see is that it has improved very nicely over time. Nice\n> > > > job !\n> > > >\n> > > > Configuring zoom through an absolute crop rectangle is I believe the way\n> > > > to go, so the overall approach is good in my opinion. There are however\n> > > > a few points I'd like to discuss, related to the SensorOutputSize\n> > > > property and IspCrop control.\n> > > >\n> > > > Reading through the discussion, I become increasingly aware of a topic\n> > > > that was present in the background without ever being named, and that's\n> > > > the camera pipeline model. As mentioned yesterday in a few other replies\n> > > > to selected patches in this series and its previous versions, the job of\n> > > > libcamera is to expose to applications a camera model that abstract\n> > > > hardware differences. This is not an easy job, as we have to strike the\n> > > > right balance between a higher-level inflexible but simple to use model\n> > > > that can't support many of the less common use cases, and a lower-level\n> > > > powerful but complex to use model that exposes a very large range of\n> > > > hardware features. We have implicitly defined the skeleton of such a\n> > > > model through the API of the Camera and PipelineHandler classes, but\n> > > > have never made it explicit.\n> > > >\n> > > > This needs to change. I see no reason to block the digital zoom feature\n> > > > until we finish documenting the pipeline model, but I would like to\n> > > > design the feature while thinking about the bigger picture. Here are the\n> > > > assumptions I think the pipeline model should make for devices that\n> > > > support digital zoom.\n> > > >\n> > > > - The camera pipeline starts with a camera sensor, that may implement\n> > > > binning, skipping and/or cropping.\n> > > >\n> > > > - The subsequent blocks in the pipeline may perform additional cropping,\n> > > > either at the direct command of the pipeline handler (e.g. cropping at\n> > > > the input of the scaler), or automatically to support image processing\n> > > > steps (e.g. colour interpoloation often drops a few lines and columns\n> > > > on all edges of the image).\n> > > >\n> > > > - The pipeline ends with a scaler that can implement digital zoom\n> > > > through a combination of cropping followed by scaling.\n> > > >\n> > > > The exact order of the processing steps at the hardware level doesn't\n> > > > need to match the pipeline model. For instance, cropping at the input\n> > > > and output of the scaler are interchangeable (not taking into account\n> > > > sub-pixel differences). It doesn't matter if the ISP scales before\n> > > > cropping the output, the hardware scaler parameters and output crop\n> > > > rectangle can be computed from an abstract input crop rectangle and\n> > > > output size. This is crucial to consider for the definition of the\n> > > > pipeline model: we need to design it in a way that ensures all features\n> > > > can be mapped to how they are implemented in the different types of\n> > > > hardware we want to support, but we're otherwise free to not map\n> > > > controls and properties 1:1 with the hardware parameters. When multiple\n> > > > options are possible, we should be guided by API design criteria such as\n> > > > coherency, simplicity and flexibility.\n> > > >\n> > > > Coming back to digital zoom, this series exposes a new SensorOutputSize\n> > > > property and a new IspCrop control. The SensorOutpuSize property is\n> > > > introduced to support the IspCrop control, as the base rectangle in\n> > > > which the scaler can crop. There are two issues here that bother me:\n> > > >\n> > > > - The property, despite being named SensorOutputSize, potentially takes\n> > > > into account the cropping added by the CSI-2 receiver and by the ISP\n> > > > for operations that consume lines and columns on the edges of the\n> > > > image. The naming can create some confusion, which can possibly be\n> > > > addressed by a combination of documentation (you're covering that\n> > > > already) and possibly a more explicit name for the property. However,\n> > > > as the property bundles crop operations perfomed in different stages\n> > > > of the pipeline, I'm worried that it will turn out to be a bit\n> > > > ill-defined regardless of how well we document it, with slightly\n> > > > different behaviours in different implementations.\n> > > >\n> > > > - Ignoring the additional cropping performed in the CSI-2 receiver and\n> > > > ISP (if any), the property exposes the sensor binning, skipping and\n> > > > cropping. It bundles those three operations together, and thus doesn't\n> > > > convey how the cropping affects the field of view as a given output\n> > > > size can be achieved with different combinations of skipping/binning\n> > > > and cropping.\n> > > >\n> > > > For these reasons, I'm concerned that the SensorOutputCrop property is a\n> > > > ad-hoc solution to provide a reference for the IspCrop property, and\n> > > > will not fit clearly in a pipeline model that should be based on simple,\n> > > > base building blocks. I would thus like to propose an alternative\n> > > > option.\n> > > >\n> > > > Instead of expressing the IspCrop controls (which I think we should\n> > > > rename to ScalerCrop) relatively to the SensorOutputSize property, could\n> > > > we express it relatively to the existing PixelArrayActiveAreas property\n> > > > ? This would have the advantage, in my opinion, of abstracting binning\n> > > > and skipping from applications. The pipeline handlers would need to\n> > > > perform a bit more work to compute the crop rectangle actually applied\n> > > > to the scaler, in order to take sensor binning/skipping and all\n> > > > additional cropping in the pipeline into account. The upside is that the\n> > > > ScalerCrop will directly define how the field of view is affected. It\n> > > > would also simplify the API, as no intermediate property between\n> > > > PixelArrayActiveAreas and ScalerCrop would need to be defined, and the\n> > > > ScalerCrop coordinates wouldn't depend on the active camera\n> > > > configuration. I think this would be easier to clearly document as part\n> > > > of a camera pipeline model.\n> > >\n> > > Renaming IspCrop to ScalerCrop sounds fine to me. It has had so many\n> > > different names!\n> > >\n> > > I'm less sure about trying to derive the SensorOutputSize (or\n> > > ScalerInputSize or whatever else we want to call it!) from the\n> > > PixelArrayActiveAreas property. Let me try and take a step back.\n> > >\n> > > So first, I think knowing the PixelArrayActiveArea isn't enough. How would\n> > > you know if the pipeline handler was doing some extra cropping that wasn't\n> > > \"strictly necessary\", perhaps to reduce memory traffic, or for a faster\n> > > framerate. How would the application know not to try and zoom there? It\n> > > seems to me that this really is a decision for the pipeline handler based\n> > > on the sensor driver, it isn't available from the properties of the sensor\n> > > itself.\n> > >\n> > > Actually I'd quite like to leave the discussion there for the moment and\n> > > see if that much is controversial or not. Of course we then have to move on\n> > > but maybe let's see what we think about that first...\n> > >\n> > > Thoughts??\n> >\n> > I fully agree with you that the pipeline handler has the last word when\n> > it comes to the (combined) internal cropping. It may not have any choice\n> > (if the CFA interpolation eats two or four lines and columns on each\n> > side of the image due to the hardware implementation, there's nothing\n> > that can be done against it), or may just want to decide on policies for\n> > whatever reason it sees fit (while trying not to over-police though, to\n> > leave options to applications).\n> >\n> > My proposal doesn't deny this from the pipeline handler, but tries to\n> > express the crop rectangle in a way that maps directly to the field of\n> > view. To do so, binning/skipping would be hidden from applications by\n> > using a reference for the ScalerCrop property that has no\n> > binning/skipping applied.\n> \n> So we certainly agree on the necessity for \"something\", that's good,\n> and then it's a case of figuring out what \"something\" is. I have\n> several conflicting thoughts here (I frequently disagree with myself):\n> \n> * Actually, as a basic principle, I think applications should be able\n> to find out \"the truth\" if they want it. This would mean the sensor\n> crop, binning/scaling, and any subsequent cropping.\n\nI agree. I can even foresee that a low-level API to control the sensor\nparameters explicitly could be useful in some advanced cases. That\nshould be an add-on though, it shouldn't cause the main API to be more\nconvoluted.\n\n> * I even wonder sometimes whether there shouldn't really be a class\n> that describes how the camera mode has been derived from the sensor,\n> with all this information made explicit. But I think that leads us\n> even deeper into the woods, and I suspect we don't really want to go\n> there! (Also, I'm not sure if the V4L2 API would even let you discover\n> all of it, which might be a problem...)\n\nI think we will go there :-) I expect sensors to require more complex\nconfigurations in the future, as the MIPI CCS spec gets adopted. The\nkernel driver will expose 2 or 3 subdevs, and we will have a class to\nabstract that in libcamera to make it easier for pipeline handlers.\n\n> * But I also think digital zoom needs to be easy to use for\n> applications that don't care about all these details (\"the truth\", as\n> I called it!). Actually I feel the overwhelming majority of\n> applications will fall into this category.\n\nWe're on the same page, good :-)\n\n> > The pipeline handler would restrict the requested ScalerCrop control\n> > based on the internal cropping that is always applied (combining sensor,\n> > CSI-2 receiver and ISP), and report the result through the request\n> > metadata. The application would thus know the exact crop rectangle that\n> > has been used, in unscaled (binning/skipping) sensor coordinates.\n> \n> Let me try and paraphrase a bit and you can tell me if I've got it wrong.\n> \n> You're proposing a property (details in a moment) that can be used to\n> choose zoom regions for the ScalerCrop control.\n> \n> The units for this property are the native sensor pixels, ignoring any\n> binning/scaling that might be going on.\n> \n> The property would give you a Rectangle that expresses exactly the\n> area in which you are permitted to zoom? The offset of the rectangle\n> tells you where in the sensor array this rectangle actually lies.\n> \n> The term \"sensor array\" there needs some clarification. I think it\n> probably means the \"PixelArraySize\" less any invalid or optical black\n> pixels. (Do we have a way to get hold of that easily?)\n\nI think we should use PixelArrayActiveAreas, while we may be able to\ncapture dark pixels with some sensors, I think they're out of scope for\ndigital zoom.\n\nMy proposal initially didn't include a new property to express the area\nin which we can zoom. I was considering reporting that through the\nScalerCrop maximum value. Every control exposed by a pipeline handler\nhas a ControlInfo instance associated with it, to report the minimum,\ndefault and maximum values. I was thinking about reporting the area in\nwhich we can zoom as the maximum value of the ControlInfo for the\nScalerCrop control. This would give applications all the information\nthey need, without the need to introduce a new property.\n\nThe issue with this is that ControlInfo is currently static, so we can't\nchange the ScalerCrop reported maximum value when the camera is\nconfigured. On the other hand, properties are supposed to be static too.\nAs we need to report the maximum dynamically, we will need to either\nallow ControlInfo to be dynamic, or properties to be dynamic. A change\nin behaviour is thus required, which lead me to think we should\ninvestigate the ControlInfo path.\n\nThis being said, I don't want to delay this feature much longer, so I'm\nfine adding a new property to report the ScalerCrop maximum value (we\ncould name it ScalerCropMaximum or ScalerCropBound for instance) for\nnow, with the property value being updated when the camera is\nconfigured. We can then explore making ControlInfo dynamic, and if it\nturns out to be a good idea, drop the ScalerCropBound in the future.\n\n> The ScalerCrop requested, and reported back, gives the offset and size\n> of the zoom rectangle relative to the rectangle given by the property\n> under discussion here (I think that's the most convenient choice). So\n> if it reports back offsets of (0,0) then you know you're in the top\n> left hand corner of what you could possibly ever see in this camera\n> mode (but not necessarily of the sensor as a whole).\n> \n> I think this could work and is a fair compromise between the amount of\n> information and the ease of use. Most digital zoom applications could\n> simply request its Size, and go from there. Only a few minor\n> mis-givings:\n>\n> * Does V4L2 actually give you a way of finding out where a particular\n> \"camera mode\" lies in the pixel array? Do you even know if a mode is\n> binned or heavily cropped... someone who knows rather more about V4L2\n> needs to answer that one!\n\nThis is exactly why we have implemented the read-only subdev API, to\nallow userspace to query detailed information about the current mode.\nThe amount of information that can be extracted from the sensor driver\ndepends on how the driver models the sensor. In order to expose analog\ncrop, binning, skipping and digital crop separately, we would need to\nexpose as least two subdevs to userspace. That's the direction I want to\ntake, and libcamera will be able to handle that, but it means some\nextra work on the kernel side to implement this in sensor drivers (it's\nno rocket science though).\n\n> * Use of native sensor coords is OK, though a few sensors might\n> support non-integer scaling at which point the property would be\n> slightly \"approximate\". That seems fairly minor to me, though. (Some\n> ISPs might even do non-integer cropping, so the problem can exist\n> regardless.)\n\nIf a sensor implements scaling in addition to cropping, binning and\nskipping, three subdevs will be required :-) That's how the smiapp\ndriver is implemented today, and the new ccs driver (\"[PATCH 000/100]\nCCS driver\" on the linux-media mailing list) follows the same path (it\nactually replaces the smiapp driver, implementing support for both\nSMIA++ and CCS).\n\n> * It doesn't really support my assertion that you should be able to\n> know exactly what's going on, if you want to. (But if we decide this\n> doesn't matter, then that's fine too.)\n\nIt doesn't by itself, but we could then add additional properties, or\nextra information in the CameraConfiguration, to report the detailed\nconfiguration of the sensor. What I like with this approach is that not\nonly the two will not conflict (we will be able to add the properties or\nextend CameraConfiguration without touching the digital zoom API), but\nthe will be no overlap between the two features either, they will each\nhave one dedicated purpose.\n\n> * In one of our early discussions we suggested that having the exact\n> pixel level information might be useful, and you could do things like\n> pan one pixel at a time. We lose that by going to native sensor\n> coordinates. I think I said at the time that this ability feels more\n> useful than I believe it really is, so I wouldn't be too upset.\n\nI'm not sure to follow you here, why wouldn't we be able to pan by one\npixel ? If the sensor is configured with binning/skipping an application\nwould have to pan by 2 pixels to achieve an effective cropping of 1\npixel, so maybe we will need to report a step, or just report\nbinning/skipping factors, but with the proposed API the crop rectangle\ncan be set to a pixel-perfect position. Or am I missing something ?\n\n> > While in some use cases the actual crop rectangle reported through\n> > metadata would be enough (for instance, in a GUI that starts with an\n> > unzoomed view, the application would get the maximum possible crop\n> > rectangle in the metadata of the first frame), I can imagine we would\n> > have use cases that need this information before capturing the first\n> \n> Indeed. And I also think you should be able to set digital zoom to\n> take effect *on* the very first frame itself (this spills over into\n> Naush's most recent patch set...)\n\nYes, I think we should allow that, even if most use cases will likely\nnot need it.\n\n> > frame. I initially thought we could report it through the max value of\n> > the ControlInfo instance for the ScalerCrop control, but this is\n> > supposed to be static information. As this would be (in my opinion) a\n> > neat solution, I'd like to investigate the possibility of making\n> > ControlInfo dynamic, but maybe we will need a different solution. For\n> > foresee the addition of an API that will create request templates,\n> > filling them with default control values based on a requested use case,\n> > and the maximum ScalerCrop could possibly be reported through that\n> > mechanism. I'm also fine reporting it through a temporary mechanism (for\n> > instance adding it to CameraConfiguration, or creating a dynamic\n> > property) for the time being, if you already have pressing use cases for\n> > knowing the maximum value before capturing the first frame.\n> >\n> > Thoughts ? :-)\n> \n> I'm glad we're talking about all this stuff!!\n> \n> > > > Two additional points I'd like to consider (and which are orthogonal to\n> > > > the previous one) are:\n> > > >\n> > > > - Should we automatically adjust the ScalerCrop rectangle to always\n> > > > output square pixels, or should we allow modifying the aspect ratio\n> > > > when scaling ? Most use cases call for square pixels, but I don't\n> > > > think we necessarily want to create an artificial limitation here (as\n> > > > long as we make it easy for applications to compute the scaling\n> > > > parameters that will give them square pixels)/\n> > >\n> > > Personally I see no reason to restrict what an application can request. We\n> > > need to make it easy to request the right aspect ratio (hence those\n> > > geometry helpers), but if people actually have a use-case for something\n> > > strange...\n> >\n> > Fine with me.\n> >\n> > > > - Should we allow a ScalerCrop rectangle smaller than the stream output\n> > > > size, or should we restrict scaling to down-scaling only ?\n> > >\n> > > I think up-scaling is probably the most common use-case for us (though\n> > > downscaling will happen too). Think of all those (rubbish) 30x zoom\n> > > pictures that some phones like to produce...!\n> >\n> > Rubbish is exactly the work I would have used :-) I'm tempted to try and\n> > teach users that they shouldn't do this by disabling this feature, but\n> > that would be pretentious. I suppose there's no good reason to forbid\n> > this. Should we put a limit on the upscaling factor though ?\n> \n> Always tricky. To what extent should we save people from themselves?\n> I'm pretty sure the Pi imposes some kind of a limit, but don't really\n> know. In the end, so long as it doesn't blow up, that's kind of OK...\n\nMaybe we'll introduce the concept of soft and hard limits in the future,\nwith the soft limit being what the pipeline handler recommends not to\nexceed to guarantee \"reasonable\" quality, and the hard limit being what\nyou can get out of the hardware. If someone finds a good use case for\nx100 digital zoom, I'll be curious to hear about it. We unfortunately\ndon't have the same ISPs as in those movies where a 4 pixels detail in\nan image can be zoomed in full screen :-)\n\n> > > > > David Plowman (6):\n> > > > > libcamera: Add SensorOutputSize property\n> > > > > libcamera: Initialise the SensorOutputSize property\n> > > > > libcamera: Add IspCrop control\n> > > > > libcamera: Add geometry helper functions\n> > > > > libcamera: pipeline: raspberrypi: Implementation of digital zoom\n> > > > > cam: Add command line option to test IspCrop control\n> > > > >\n> > > > > include/libcamera/geometry.h | 20 +++\n> > > > > include/libcamera/ipa/raspberrypi.h | 1 +\n> > > > > src/cam/capture.cpp | 25 +++-\n> > > > > src/cam/capture.h | 2 +-\n> > > > > src/cam/main.cpp | 3 +\n> > > > > src/cam/main.h | 1 +\n> > > > > src/ipa/raspberrypi/raspberrypi.cpp | 7 +\n> > > > > src/libcamera/camera_sensor.cpp | 6 +\n> > > > > src/libcamera/control_ids.yaml | 12 ++\n> > > > > src/libcamera/geometry.cpp | 129 ++++++++++++++++++\n> > > > > .../pipeline/raspberrypi/raspberrypi.cpp | 47 +++++++\n> > > > > src/libcamera/property_ids.yaml | 19 +++\n> > > > > 12 files changed, 269 insertions(+), 3 deletions(-)","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 CD6DABEEE0\n\tfor ;\n\tThu, 15 Oct 2020 02:17:39 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2BFB160E97;\n\tThu, 15 Oct 2020 04:17:39 +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 D836260355\n\tfor ;\n\tThu, 15 Oct 2020 04:17:37 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 5703EA42;\n\tThu, 15 Oct 2020 04:17:37 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"fKikJ1MB\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1602728257;\n\tbh=dGSWpqka6CA5O81o/ZEW6EBAQBaADoiOfcwaJvA93+4=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=fKikJ1MBCXndXU2lnTk3/YRLiR2SdiytRRzw6D7iTO8Sa+GJkilEEetqoxDs4EmJM\n\tLIoUffmk/05mzWVyI/MMLoLMNOsNtRIUg1POQCHwkk0TD5PPQOab78IZBlWEdH9BYQ\n\tPh/tlzZB64sQinPf2lKqTSIgSEdGJ0lozU0O1srw=","Date":"Thu, 15 Oct 2020 05:16:50 +0300","From":"Laurent Pinchart ","To":"David Plowman ","Message-ID":"<20201015021650.GH3858@pendragon.ideasonboard.com>","References":"<20200929164000.15429-1-david.plowman@raspberrypi.com>\n\t<20201011214708.GA3944@pendragon.ideasonboard.com>\n\t\n\t<20201013014422.GD3942@pendragon.ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v3 0/6] Digital zoom","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":13217,"web_url":"https://patchwork.libcamera.org/comment/13217/","msgid":"","date":"2020-10-15T14:30:56","subject":"Re: [libcamera-devel] [PATCH v3 0/6] Digital zoom","submitter":{"id":42,"url":"https://patchwork.libcamera.org/api/people/42/","name":"David Plowman","email":"david.plowman@raspberrypi.com"},"content":"Hi Laurent\n\nThanks for all that! The discussion is getting rather long so perhaps\nI can try to wrap up all the points in the thread and sum it up at the\ntop here with an outline of the plan as I understand it!\n\n1. ScalerCrop control\n\nThis will be a Rectangle indicating the crop for digital zoom. The\nunits will be native pixel coordinates. The (x,y) offsets should, I\nthink, start at (0,0) because the \"top-left-most\" thing you can have\nshould probably be signalled with (0,0) - but see below.\n\n2. ScalerCropMaximum property\n\nThis indicates the maximum image area from which we can crop, again in\nnative pixel coordinates. It could be a Rectangle and we could use the\n(x,y) offsets perhaps to indicate the top left coordinates in the\nsensor - i.e. this would (mostly) be the analogCrop then. (Or one\ncould leave them as zero, in which case you might as well leave this\nas a Size.)\n\nWe've talked about making this a \"dynamic\" ControlInfo maximum, though\nit's not quite the same thing. For example, it would typically have\nthe \"wrong\" aspect ratio so taking and setting the \"maximum\" would\ngive you a weird image - would a user expect that? Also, the meaning\nof the (x,y) offsets as described for the control does not align as\nthings stand.\n\n3. Other stuff...\n\n* With this scheme there's no way for an application to know what the\ntrue pixel scale for the ScalerCrop is. Maybe that kind of information\ngets exposed later. (I seem to remember wanting to expose the whole\nCameraSensorInfo several months ago...)\n\n* There's no real allowance for any cropping after binning/scaling.\nMaybe you have to adjust the ScalerCropMaximum to make it look like\nall the cropping is happening up front? TBD, I think.\n\n* Pipeline handlers will all have to go around rescaling the\nScalerCrop rectangles from native to actual pixel coordinates. Maybe\nthere's another geometry helper function there: Rectangle\nRectangle::rescaledTo(const Size &from, const Size &to)\ne.g. actualCrop = nativeCrop.rescaledTo(sensorInfo.analogCrop.size(),\nsensorInfo.outputSize)\n\nPlease correct me if I've got any of that wrong. Overall I can't quite\nescape the nagging feeling that there should be better ways of getting\nall that information about the camera mode to an application, but\nwhatever mechanism we use now can be improved upon later. The basic\nScalerCrop control would not be affected, I think.\n\nBest regards\nDavid\n\n\nOn Thu, 15 Oct 2020 at 03:17, Laurent Pinchart\n wrote:\n>\n> Hi David,\n>\n> On Tue, Oct 13, 2020 at 09:36:50AM +0100, David Plowman wrote:\n> > On Tue, 13 Oct 2020 at 02:45, Laurent Pinchart wrote:\n> > > On Mon, Oct 12, 2020 at 07:49:31PM +0100, David Plowman wrote:\n> > > > Hi Laurent\n> > > >\n> > > > Thanks for the comments. Some interesting points.\n> > >\n> > > Sorry about chiming in so late and challenging some of the base\n> > > assumptions though. I'll now try not to introduce any further delay.\n> > >\n> > > > On Sun, 11 Oct 2020 at 22:47, Laurent Pinchart wrote:\n> > > >\n> > > > > Hi David,\n> > > > >\n> > > > > Thank you for the patches. And more than sorry for reviewing the series\n> > > > > so late.\n> > > > >\n> > > > > On Tue, Sep 29, 2020 at 05:39:54PM +0100, David Plowman wrote:\n> > > > > > Hi everyone\n> > > > > >\n> > > > > > Despite not originally intending to, I've actually made a version 3 of\n> > > > > > the digital zoom patches, just to take care of a few things that might\n> > > > > > be a bit annoying otherwise.\n> > > > > >\n> > > > > > 1. I've improved the description of the IspCrop control as was\n> > > > > > suggested.\n> > > > > >\n> > > > > > 2. I've improved the description of the zoom option in cam (if we\n> > > > > > decide to use this patch!), also as was proposed.\n> > > > > >\n> > > > > > 3. There was actually a problem with the \"{}\" syntax to denote zero\n> > > > > > Rectangles in include/libcamera/ipa/raspberrypi.h. They were failing a\n> > > > > > range type test in ControlInfoMap::generateIdmap() and so the control\n> > > > > > wasn't working. I've replaced \"{}\" by \"Rectangle{}\" which seems OK.\n> > > > > >\n> > > > > > 4. There's been a bit of churn in the RPi pipeline handler lately so\n> > > > > > rebasing gave some conflicts. I've fixed those up.\n> > > > > >\n> > > > > > Otherwise everything else remains the same.\n> > > > >\n> > > > > I've gone through the different versions of this series, and the first\n> > > > > thing I want to see is that it has improved very nicely over time. Nice\n> > > > > job !\n> > > > >\n> > > > > Configuring zoom through an absolute crop rectangle is I believe the way\n> > > > > to go, so the overall approach is good in my opinion. There are however\n> > > > > a few points I'd like to discuss, related to the SensorOutputSize\n> > > > > property and IspCrop control.\n> > > > >\n> > > > > Reading through the discussion, I become increasingly aware of a topic\n> > > > > that was present in the background without ever being named, and that's\n> > > > > the camera pipeline model. As mentioned yesterday in a few other replies\n> > > > > to selected patches in this series and its previous versions, the job of\n> > > > > libcamera is to expose to applications a camera model that abstract\n> > > > > hardware differences. This is not an easy job, as we have to strike the\n> > > > > right balance between a higher-level inflexible but simple to use model\n> > > > > that can't support many of the less common use cases, and a lower-level\n> > > > > powerful but complex to use model that exposes a very large range of\n> > > > > hardware features. We have implicitly defined the skeleton of such a\n> > > > > model through the API of the Camera and PipelineHandler classes, but\n> > > > > have never made it explicit.\n> > > > >\n> > > > > This needs to change. I see no reason to block the digital zoom feature\n> > > > > until we finish documenting the pipeline model, but I would like to\n> > > > > design the feature while thinking about the bigger picture. Here are the\n> > > > > assumptions I think the pipeline model should make for devices that\n> > > > > support digital zoom.\n> > > > >\n> > > > > - The camera pipeline starts with a camera sensor, that may implement\n> > > > > binning, skipping and/or cropping.\n> > > > >\n> > > > > - The subsequent blocks in the pipeline may perform additional cropping,\n> > > > > either at the direct command of the pipeline handler (e.g. cropping at\n> > > > > the input of the scaler), or automatically to support image processing\n> > > > > steps (e.g. colour interpoloation often drops a few lines and columns\n> > > > > on all edges of the image).\n> > > > >\n> > > > > - The pipeline ends with a scaler that can implement digital zoom\n> > > > > through a combination of cropping followed by scaling.\n> > > > >\n> > > > > The exact order of the processing steps at the hardware level doesn't\n> > > > > need to match the pipeline model. For instance, cropping at the input\n> > > > > and output of the scaler are interchangeable (not taking into account\n> > > > > sub-pixel differences). It doesn't matter if the ISP scales before\n> > > > > cropping the output, the hardware scaler parameters and output crop\n> > > > > rectangle can be computed from an abstract input crop rectangle and\n> > > > > output size. This is crucial to consider for the definition of the\n> > > > > pipeline model: we need to design it in a way that ensures all features\n> > > > > can be mapped to how they are implemented in the different types of\n> > > > > hardware we want to support, but we're otherwise free to not map\n> > > > > controls and properties 1:1 with the hardware parameters. When multiple\n> > > > > options are possible, we should be guided by API design criteria such as\n> > > > > coherency, simplicity and flexibility.\n> > > > >\n> > > > > Coming back to digital zoom, this series exposes a new SensorOutputSize\n> > > > > property and a new IspCrop control. The SensorOutpuSize property is\n> > > > > introduced to support the IspCrop control, as the base rectangle in\n> > > > > which the scaler can crop. There are two issues here that bother me:\n> > > > >\n> > > > > - The property, despite being named SensorOutputSize, potentially takes\n> > > > > into account the cropping added by the CSI-2 receiver and by the ISP\n> > > > > for operations that consume lines and columns on the edges of the\n> > > > > image. The naming can create some confusion, which can possibly be\n> > > > > addressed by a combination of documentation (you're covering that\n> > > > > already) and possibly a more explicit name for the property. However,\n> > > > > as the property bundles crop operations perfomed in different stages\n> > > > > of the pipeline, I'm worried that it will turn out to be a bit\n> > > > > ill-defined regardless of how well we document it, with slightly\n> > > > > different behaviours in different implementations.\n> > > > >\n> > > > > - Ignoring the additional cropping performed in the CSI-2 receiver and\n> > > > > ISP (if any), the property exposes the sensor binning, skipping and\n> > > > > cropping. It bundles those three operations together, and thus doesn't\n> > > > > convey how the cropping affects the field of view as a given output\n> > > > > size can be achieved with different combinations of skipping/binning\n> > > > > and cropping.\n> > > > >\n> > > > > For these reasons, I'm concerned that the SensorOutputCrop property is a\n> > > > > ad-hoc solution to provide a reference for the IspCrop property, and\n> > > > > will not fit clearly in a pipeline model that should be based on simple,\n> > > > > base building blocks. I would thus like to propose an alternative\n> > > > > option.\n> > > > >\n> > > > > Instead of expressing the IspCrop controls (which I think we should\n> > > > > rename to ScalerCrop) relatively to the SensorOutputSize property, could\n> > > > > we express it relatively to the existing PixelArrayActiveAreas property\n> > > > > ? This would have the advantage, in my opinion, of abstracting binning\n> > > > > and skipping from applications. The pipeline handlers would need to\n> > > > > perform a bit more work to compute the crop rectangle actually applied\n> > > > > to the scaler, in order to take sensor binning/skipping and all\n> > > > > additional cropping in the pipeline into account. The upside is that the\n> > > > > ScalerCrop will directly define how the field of view is affected. It\n> > > > > would also simplify the API, as no intermediate property between\n> > > > > PixelArrayActiveAreas and ScalerCrop would need to be defined, and the\n> > > > > ScalerCrop coordinates wouldn't depend on the active camera\n> > > > > configuration. I think this would be easier to clearly document as part\n> > > > > of a camera pipeline model.\n> > > >\n> > > > Renaming IspCrop to ScalerCrop sounds fine to me. It has had so many\n> > > > different names!\n> > > >\n> > > > I'm less sure about trying to derive the SensorOutputSize (or\n> > > > ScalerInputSize or whatever else we want to call it!) from the\n> > > > PixelArrayActiveAreas property. Let me try and take a step back.\n> > > >\n> > > > So first, I think knowing the PixelArrayActiveArea isn't enough. How would\n> > > > you know if the pipeline handler was doing some extra cropping that wasn't\n> > > > \"strictly necessary\", perhaps to reduce memory traffic, or for a faster\n> > > > framerate. How would the application know not to try and zoom there? It\n> > > > seems to me that this really is a decision for the pipeline handler based\n> > > > on the sensor driver, it isn't available from the properties of the sensor\n> > > > itself.\n> > > >\n> > > > Actually I'd quite like to leave the discussion there for the moment and\n> > > > see if that much is controversial or not. Of course we then have to move on\n> > > > but maybe let's see what we think about that first...\n> > > >\n> > > > Thoughts??\n> > >\n> > > I fully agree with you that the pipeline handler has the last word when\n> > > it comes to the (combined) internal cropping. It may not have any choice\n> > > (if the CFA interpolation eats two or four lines and columns on each\n> > > side of the image due to the hardware implementation, there's nothing\n> > > that can be done against it), or may just want to decide on policies for\n> > > whatever reason it sees fit (while trying not to over-police though, to\n> > > leave options to applications).\n> > >\n> > > My proposal doesn't deny this from the pipeline handler, but tries to\n> > > express the crop rectangle in a way that maps directly to the field of\n> > > view. To do so, binning/skipping would be hidden from applications by\n> > > using a reference for the ScalerCrop property that has no\n> > > binning/skipping applied.\n> >\n> > So we certainly agree on the necessity for \"something\", that's good,\n> > and then it's a case of figuring out what \"something\" is. I have\n> > several conflicting thoughts here (I frequently disagree with myself):\n> >\n> > * Actually, as a basic principle, I think applications should be able\n> > to find out \"the truth\" if they want it. This would mean the sensor\n> > crop, binning/scaling, and any subsequent cropping.\n>\n> I agree. I can even foresee that a low-level API to control the sensor\n> parameters explicitly could be useful in some advanced cases. That\n> should be an add-on though, it shouldn't cause the main API to be more\n> convoluted.\n>\n> > * I even wonder sometimes whether there shouldn't really be a class\n> > that describes how the camera mode has been derived from the sensor,\n> > with all this information made explicit. But I think that leads us\n> > even deeper into the woods, and I suspect we don't really want to go\n> > there! (Also, I'm not sure if the V4L2 API would even let you discover\n> > all of it, which might be a problem...)\n>\n> I think we will go there :-) I expect sensors to require more complex\n> configurations in the future, as the MIPI CCS spec gets adopted. The\n> kernel driver will expose 2 or 3 subdevs, and we will have a class to\n> abstract that in libcamera to make it easier for pipeline handlers.\n>\n> > * But I also think digital zoom needs to be easy to use for\n> > applications that don't care about all these details (\"the truth\", as\n> > I called it!). Actually I feel the overwhelming majority of\n> > applications will fall into this category.\n>\n> We're on the same page, good :-)\n>\n> > > The pipeline handler would restrict the requested ScalerCrop control\n> > > based on the internal cropping that is always applied (combining sensor,\n> > > CSI-2 receiver and ISP), and report the result through the request\n> > > metadata. The application would thus know the exact crop rectangle that\n> > > has been used, in unscaled (binning/skipping) sensor coordinates.\n> >\n> > Let me try and paraphrase a bit and you can tell me if I've got it wrong.\n> >\n> > You're proposing a property (details in a moment) that can be used to\n> > choose zoom regions for the ScalerCrop control.\n> >\n> > The units for this property are the native sensor pixels, ignoring any\n> > binning/scaling that might be going on.\n> >\n> > The property would give you a Rectangle that expresses exactly the\n> > area in which you are permitted to zoom? The offset of the rectangle\n> > tells you where in the sensor array this rectangle actually lies.\n> >\n> > The term \"sensor array\" there needs some clarification. I think it\n> > probably means the \"PixelArraySize\" less any invalid or optical black\n> > pixels. (Do we have a way to get hold of that easily?)\n>\n> I think we should use PixelArrayActiveAreas, while we may be able to\n> capture dark pixels with some sensors, I think they're out of scope for\n> digital zoom.\n>\n> My proposal initially didn't include a new property to express the area\n> in which we can zoom. I was considering reporting that through the\n> ScalerCrop maximum value. Every control exposed by a pipeline handler\n> has a ControlInfo instance associated with it, to report the minimum,\n> default and maximum values. I was thinking about reporting the area in\n> which we can zoom as the maximum value of the ControlInfo for the\n> ScalerCrop control. This would give applications all the information\n> they need, without the need to introduce a new property.\n>\n> The issue with this is that ControlInfo is currently static, so we can't\n> change the ScalerCrop reported maximum value when the camera is\n> configured. On the other hand, properties are supposed to be static too.\n> As we need to report the maximum dynamically, we will need to either\n> allow ControlInfo to be dynamic, or properties to be dynamic. A change\n> in behaviour is thus required, which lead me to think we should\n> investigate the ControlInfo path.\n>\n> This being said, I don't want to delay this feature much longer, so I'm\n> fine adding a new property to report the ScalerCrop maximum value (we\n> could name it ScalerCropMaximum or ScalerCropBound for instance) for\n> now, with the property value being updated when the camera is\n> configured. We can then explore making ControlInfo dynamic, and if it\n> turns out to be a good idea, drop the ScalerCropBound in the future.\n>\n> > The ScalerCrop requested, and reported back, gives the offset and size\n> > of the zoom rectangle relative to the rectangle given by the property\n> > under discussion here (I think that's the most convenient choice). So\n> > if it reports back offsets of (0,0) then you know you're in the top\n> > left hand corner of what you could possibly ever see in this camera\n> > mode (but not necessarily of the sensor as a whole).\n> >\n> > I think this could work and is a fair compromise between the amount of\n> > information and the ease of use. Most digital zoom applications could\n> > simply request its Size, and go from there. Only a few minor\n> > mis-givings:\n> >\n> > * Does V4L2 actually give you a way of finding out where a particular\n> > \"camera mode\" lies in the pixel array? Do you even know if a mode is\n> > binned or heavily cropped... someone who knows rather more about V4L2\n> > needs to answer that one!\n>\n> This is exactly why we have implemented the read-only subdev API, to\n> allow userspace to query detailed information about the current mode.\n> The amount of information that can be extracted from the sensor driver\n> depends on how the driver models the sensor. In order to expose analog\n> crop, binning, skipping and digital crop separately, we would need to\n> expose as least two subdevs to userspace. That's the direction I want to\n> take, and libcamera will be able to handle that, but it means some\n> extra work on the kernel side to implement this in sensor drivers (it's\n> no rocket science though).\n>\n> > * Use of native sensor coords is OK, though a few sensors might\n> > support non-integer scaling at which point the property would be\n> > slightly \"approximate\". That seems fairly minor to me, though. (Some\n> > ISPs might even do non-integer cropping, so the problem can exist\n> > regardless.)\n>\n> If a sensor implements scaling in addition to cropping, binning and\n> skipping, three subdevs will be required :-) That's how the smiapp\n> driver is implemented today, and the new ccs driver (\"[PATCH 000/100]\n> CCS driver\" on the linux-media mailing list) follows the same path (it\n> actually replaces the smiapp driver, implementing support for both\n> SMIA++ and CCS).\n>\n> > * It doesn't really support my assertion that you should be able to\n> > know exactly what's going on, if you want to. (But if we decide this\n> > doesn't matter, then that's fine too.)\n>\n> It doesn't by itself, but we could then add additional properties, or\n> extra information in the CameraConfiguration, to report the detailed\n> configuration of the sensor. What I like with this approach is that not\n> only the two will not conflict (we will be able to add the properties or\n> extend CameraConfiguration without touching the digital zoom API), but\n> the will be no overlap between the two features either, they will each\n> have one dedicated purpose.\n>\n> > * In one of our early discussions we suggested that having the exact\n> > pixel level information might be useful, and you could do things like\n> > pan one pixel at a time. We lose that by going to native sensor\n> > coordinates. I think I said at the time that this ability feels more\n> > useful than I believe it really is, so I wouldn't be too upset.\n>\n> I'm not sure to follow you here, why wouldn't we be able to pan by one\n> pixel ? If the sensor is configured with binning/skipping an application\n> would have to pan by 2 pixels to achieve an effective cropping of 1\n> pixel, so maybe we will need to report a step, or just report\n> binning/skipping factors, but with the proposed API the crop rectangle\n> can be set to a pixel-perfect position. Or am I missing something ?\n>\n> > > While in some use cases the actual crop rectangle reported through\n> > > metadata would be enough (for instance, in a GUI that starts with an\n> > > unzoomed view, the application would get the maximum possible crop\n> > > rectangle in the metadata of the first frame), I can imagine we would\n> > > have use cases that need this information before capturing the first\n> >\n> > Indeed. And I also think you should be able to set digital zoom to\n> > take effect *on* the very first frame itself (this spills over into\n> > Naush's most recent patch set...)\n>\n> Yes, I think we should allow that, even if most use cases will likely\n> not need it.\n>\n> > > frame. I initially thought we could report it through the max value of\n> > > the ControlInfo instance for the ScalerCrop control, but this is\n> > > supposed to be static information. As this would be (in my opinion) a\n> > > neat solution, I'd like to investigate the possibility of making\n> > > ControlInfo dynamic, but maybe we will need a different solution. For\n> > > foresee the addition of an API that will create request templates,\n> > > filling them with default control values based on a requested use case,\n> > > and the maximum ScalerCrop could possibly be reported through that\n> > > mechanism. I'm also fine reporting it through a temporary mechanism (for\n> > > instance adding it to CameraConfiguration, or creating a dynamic\n> > > property) for the time being, if you already have pressing use cases for\n> > > knowing the maximum value before capturing the first frame.\n> > >\n> > > Thoughts ? :-)\n> >\n> > I'm glad we're talking about all this stuff!!\n> >\n> > > > > Two additional points I'd like to consider (and which are orthogonal to\n> > > > > the previous one) are:\n> > > > >\n> > > > > - Should we automatically adjust the ScalerCrop rectangle to always\n> > > > > output square pixels, or should we allow modifying the aspect ratio\n> > > > > when scaling ? Most use cases call for square pixels, but I don't\n> > > > > think we necessarily want to create an artificial limitation here (as\n> > > > > long as we make it easy for applications to compute the scaling\n> > > > > parameters that will give them square pixels)/\n> > > >\n> > > > Personally I see no reason to restrict what an application can request. We\n> > > > need to make it easy to request the right aspect ratio (hence those\n> > > > geometry helpers), but if people actually have a use-case for something\n> > > > strange...\n> > >\n> > > Fine with me.\n> > >\n> > > > > - Should we allow a ScalerCrop rectangle smaller than the stream output\n> > > > > size, or should we restrict scaling to down-scaling only ?\n> > > >\n> > > > I think up-scaling is probably the most common use-case for us (though\n> > > > downscaling will happen too). Think of all those (rubbish) 30x zoom\n> > > > pictures that some phones like to produce...!\n> > >\n> > > Rubbish is exactly the work I would have used :-) I'm tempted to try and\n> > > teach users that they shouldn't do this by disabling this feature, but\n> > > that would be pretentious. I suppose there's no good reason to forbid\n> > > this. Should we put a limit on the upscaling factor though ?\n> >\n> > Always tricky. To what extent should we save people from themselves?\n> > I'm pretty sure the Pi imposes some kind of a limit, but don't really\n> > know. In the end, so long as it doesn't blow up, that's kind of OK...\n>\n> Maybe we'll introduce the concept of soft and hard limits in the future,\n> with the soft limit being what the pipeline handler recommends not to\n> exceed to guarantee \"reasonable\" quality, and the hard limit being what\n> you can get out of the hardware. If someone finds a good use case for\n> x100 digital zoom, I'll be curious to hear about it. We unfortunately\n> don't have the same ISPs as in those movies where a 4 pixels detail in\n> an image can be zoomed in full screen :-)\n>\n> > > > > > David Plowman (6):\n> > > > > > libcamera: Add SensorOutputSize property\n> > > > > > libcamera: Initialise the SensorOutputSize property\n> > > > > > libcamera: Add IspCrop control\n> > > > > > libcamera: Add geometry helper functions\n> > > > > > libcamera: pipeline: raspberrypi: Implementation of digital zoom\n> > > > > > cam: Add command line option to test IspCrop control\n> > > > > >\n> > > > > > include/libcamera/geometry.h | 20 +++\n> > > > > > include/libcamera/ipa/raspberrypi.h | 1 +\n> > > > > > src/cam/capture.cpp | 25 +++-\n> > > > > > src/cam/capture.h | 2 +-\n> > > > > > src/cam/main.cpp | 3 +\n> > > > > > src/cam/main.h | 1 +\n> > > > > > src/ipa/raspberrypi/raspberrypi.cpp | 7 +\n> > > > > > src/libcamera/camera_sensor.cpp | 6 +\n> > > > > > src/libcamera/control_ids.yaml | 12 ++\n> > > > > > src/libcamera/geometry.cpp | 129 ++++++++++++++++++\n> > > > > > .../pipeline/raspberrypi/raspberrypi.cpp | 47 +++++++\n> > > > > > src/libcamera/property_ids.yaml | 19 +++\n> > > > > > 12 files changed, 269 insertions(+), 3 deletions(-)\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 E330BBE905\n\tfor ;\n\tThu, 15 Oct 2020 14:31:10 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 69AE161027;\n\tThu, 15 Oct 2020 16:31:10 +0200 (CEST)","from mail-oo1-xc2d.google.com (mail-oo1-xc2d.google.com\n\t[IPv6:2607:f8b0:4864:20::c2d])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5FDED605BF\n\tfor ;\n\tThu, 15 Oct 2020 16:31:09 +0200 (CEST)","by mail-oo1-xc2d.google.com with SMTP id z1so764308ooj.3\n\tfor ;\n\tThu, 15 Oct 2020 07:31:09 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"hjxtZVS/\"; 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=j7Yf1rbXXcsJSae9VaKcIhdXqz5ol86AnbHTcKHigd4=;\n\tb=hjxtZVS/FgNpHDPlZKKSIV8Bp/IwFhoZQZcvIz04lm7vpfY38hpLJ7poi4SAigCK0x\n\t8ebPu5//dgC6U/h7FRiLdcHZSBwEjnB0fauJrz03tGxvqSy9TdkHFzElEzeJNm54fh/c\n\tand1FvPI3OIizVsjsnrsxzt4yBTNAdcizol/MGxZl2pJzSjoLMn70BMSmTVRQz4MEDk1\n\tLVykQqOq03VOoRMIBR9Y943vViQTEvpfsDW4DN6FWH+YKS4wPdfXCoCu0P77PP/OnEUo\n\tKNjVqBCcIm7+0v8yOtqEBfyrFB78wmiTwla25l2KQGoO3awlMv05+uZMcptu8CCo+gfW\n\tGvOQ==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=j7Yf1rbXXcsJSae9VaKcIhdXqz5ol86AnbHTcKHigd4=;\n\tb=YBYilsIwZyueLg5aMSCXi/6rbX/weczMM0TI5xw1BrlL7PdA6sc8K4rsRkBBkqgsWg\n\tNqNWTRYF7SN6M+/ab8eW2UutZxOF09Gj6EiHTZOxE79eJmPhBPR/3VisQRosbs0r/QrF\n\tFXrfBlmgMU/huEtnyTDDgegV0A4aP60nzMvsGvMmWe34tHVa05V4ippw+IHewOKj7lEb\n\tzWcO+uPpiyIPsMwglqAJ4iKVJp2PRKmal13ZgFrLmHwW/e6WFHUT8GhoIB8aVFso3A6P\n\tq95LOw8b04mgNP1k/jE/ZVKvATjs+zwAo1z/SwQl1Ff9kffhHomik05P1Dqiq19VVbZr\n\twz9w==","X-Gm-Message-State":"AOAM53315y95rJUxBIFn82WsFrB7NkjdJ6swf2/IRbKnWqsCZL5FSeV6\n\tkubRVOlLQ+p904BmP5zxVoV9hSJUcQ+awqAFyZ7wnN3zZAc=","X-Google-Smtp-Source":"ABdhPJwawb3AK95GAzuslZZCddAMEf5Bz5VDYOXwFIW5kWqgDOObSZm4WeDK9x8MPrGRZSYMlM0Sqqz4/NFgUC33fAY=","X-Received":"by 2002:a4a:7656:: with SMTP id w22mr2562585ooe.72.1602772267222;\n\tThu, 15 Oct 2020 07:31:07 -0700 (PDT)","MIME-Version":"1.0","References":"<20200929164000.15429-1-david.plowman@raspberrypi.com>\n\t<20201011214708.GA3944@pendragon.ideasonboard.com>\n\t\n\t<20201013014422.GD3942@pendragon.ideasonboard.com>\n\t\n\t<20201015021650.GH3858@pendragon.ideasonboard.com>","In-Reply-To":"<20201015021650.GH3858@pendragon.ideasonboard.com>","From":"David Plowman ","Date":"Thu, 15 Oct 2020 15:30:56 +0100","Message-ID":"","To":"Laurent Pinchart ","Subject":"Re: [libcamera-devel] [PATCH v3 0/6] Digital zoom","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 ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":13234,"web_url":"https://patchwork.libcamera.org/comment/13234/","msgid":"<20201016063048.GJ3829@pendragon.ideasonboard.com>","date":"2020-10-16T06:30:48","subject":"Re: [libcamera-devel] [PATCH v3 0/6] Digital zoom","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi David,\n\nOn Thu, Oct 15, 2020 at 03:30:56PM +0100, David Plowman wrote:\n> Hi Laurent\n> \n> Thanks for all that! The discussion is getting rather long so perhaps\n> I can try to wrap up all the points in the thread and sum it up at the\n> top here with an outline of the plan as I understand it!\n\nThank you for summarizing this.\n\n> 1. ScalerCrop control\n> \n> This will be a Rectangle indicating the crop for digital zoom. The\n> units will be native pixel coordinates. The (x,y) offsets should, I\n> think, start at (0,0) because the \"top-left-most\" thing you can have\n> should probably be signalled with (0,0) - but see below.\n\nI think the ScalerCrop control should have the same unit and the same\nreference as the ScalerCropMaximum property. ScalerCropMaximum is meant\nto convey the largest possible crop rectangle, so it should be valid to\nset ScalerCrop = ScalerCropMaximum. As ScalerCropMaximum will typically\nhave (x,y) > (0,0) (unless the ISP doesn't need to crop at all for\ninternal reasons), I don't think we should consider that ScalerCrop\nstarts at (0,0).\n\nThe alternative is to express ScalerCrop relatively to\nScalerCropMaximum, but in that case ScalerCropMaximum wouldn't be the\nmaximum value for the ScalerCrop control anymore. In order to later\nreplace\n\n\tcamera->properties().at(properties::ScalerCropMaximum)\n\nwith\n\n\tcamera->controls().at(controls::ScalerCrop).max()\n\nwe need to express the two with the same reference.\n\n> 2. ScalerCropMaximum property\n> \n> This indicates the maximum image area from which we can crop, again in\n> native pixel coordinates. It could be a Rectangle and we could use the\n> (x,y) offsets perhaps to indicate the top left coordinates in the\n> sensor - i.e. this would (mostly) be the analogCrop then. (Or one\n> could leave them as zero, in which case you might as well leave this\n> as a Size.)\n\nThe idea is that ScalerCropMaximum is indeed expressed similarly to an\nanalog crop rectangle, but its meaning is different. The coordinate\nsystem is the sensor's pixel array active area, but the value takes into\naccount mandatory cropping performed in all stages of the pipeline,\nincluding the ISP.\n\n> We've talked about making this a \"dynamic\" ControlInfo maximum, though\n> it's not quite the same thing. For example, it would typically have\n> the \"wrong\" aspect ratio so taking and setting the \"maximum\" would\n> give you a weird image - would a user expect that? Also, the meaning\n> of the (x,y) offsets as described for the control does not align as\n> things stand.\n\nSetting ScalerCrop = ScalerCropMaximum would indeed give the wrong\naspect ratio. We will however have nice geometry helpers (thank you for\nyour work on that) that should make it easy to get the right behaviour.\n\n> 3. Other stuff...\n> \n> * With this scheme there's no way for an application to know what the\n> true pixel scale for the ScalerCrop is. Maybe that kind of information\n> gets exposed later. (I seem to remember wanting to expose the whole\n> CameraSensorInfo several months ago...)\n\nI'm not sure to follow you here. With coordinates for ScalerCrop and\nScalerCropMaximum being sensor pixels, the true pixel scale will simply\nbe the ratio between the stream output size and ScalerCrop.size(), won't\nit ?\n\n> * There's no real allowance for any cropping after binning/scaling.\n> Maybe you have to adjust the ScalerCropMaximum to make it look like\n> all the cropping is happening up front? TBD, I think.\n\nMaybe this is where we haven't understood each other properly.\nScalerCropMaximum (and thus ScalerCrop) are expressed in sensor pixel\ncoordinates, as if all cropping was applied before binning and skipping,\nbut they include all cropping operations through the whole pipeline.\n\n> * Pipeline handlers will all have to go around rescaling the\n> ScalerCrop rectangles from native to actual pixel coordinates.\n\nCorrect, in order to compute the real scaler input crop rectangle,\npipeline handlers will need to divide ScalerCrop by the binning/skipping\nfactors, and subtract the internal crop that is performed in other parts\nof the pipeline. Let's consider a few examples.\n\nLet's assume a sensor native resolution of 4056x3040 active pixels\n(non-active and optical dark pixels are ignored in this example). Let's\nfurther assume that the ISP, for internal reasons, consumes 4 lines and\ncolumns on each side of the image.\n\nFor the first example, let's assume that the user wants to capture the\nfull image. No binning, skipping or cropping is applied in the sensor.\nDue to ISP cropping, the pipeline handler should restrict the output\nsize to 4048x3032 in order to keep pixels square, as upscaling 4048x3032\nto 4056x3040 will modify the aspect ratio slightly. The\nScalerCropMaximum property will be reported as (4,4)-4048x3032, and the\ndefault ScalerCrop value will be (4,4)-4048x3032. Applications can\nselect a smaller ScalerCrop rectangle, and they will be responsible for\nensuring that its size matches the aspect ratio of the output image\n(4048x3032) if they want to keep pixels square.\n\nIn a second example, let's assume the user sets the stream output size\nto 1920x1080. The pipeline handler decides to enable binning on the\nsensor side, but without applying any cropping on the sensor. The sensor\noutput is thus 2028x1520. Due to the ISP internal cropping of 4 pixels\non each side, the image at the scaler input will be at most 2020x1512.\nScaling this back to pixel array coordinates, the ScalerCropMaximum will\nbe reported as (8,8)-4040x3024. Note how it is slightly smaller than in\nthe previous example. The pipeline handler will furthermore set the\nactual scaler crop rectangle, expressed in scaler input coordinates\n(thus relative to a 2020x1512 size) to (2,189)-2016x1134 to get a 16:9\naspect ratio (it could also set it to (0,188)-2020x1136 to use the full\nwidth, but the aspect ratio would then be slightly off as 2020 isn't a\nmultiple of 16). Scaling this back to the pixel array coordinates, the\ndefault ScalerCrop rectangle reported to the application is\n(12,386)-4032x2268. Applications will be able to pan within the\n(8,8)-4040x3024 maximum crop rectangle, and possibly scaling a ~4:3\nimage to 16:9 if they set ScalerCrop = ScalerCropMaximum.\n\nIn a third example, with the same 1920x1080 output resolution, the\npipeline handler may decide to crop on the sensor, for instance cropping\nthe binned 2028x1520 output to (50,216)-1928x1088. The scaler input\nsize, after the ISP internal cropping, will be 1920x1080, matching the\nstream output size. The image doesn't need to be scaled, but the field\nof view is reduced. In this case, the ScalerCropMaximum will be reported\nas (108,440)-3840x2160, and the default ScalerCrop value will be set to\n(108,440)-3840x2160 as well. Applications will be able to pan within the\n(108,440)-3840x2160 maximum crop rectangle, but unlike in the previous\nexample, they won't be able to pan outside of the default, as panning is\nonly possible in the scaler, the sensor crop rectangle is fixed.\n\nDo these examples help clarifying the proposal ?\n\n> Maybe there's another geometry helper function there: Rectangle\n> Rectangle::rescaledTo(const Size &from, const Size &to)\n> e.g. actualCrop = nativeCrop.rescaledTo(sensorInfo.analogCrop.size(),\n> sensorInfo.outputSize)\n> \n> Please correct me if I've got any of that wrong. Overall I can't quite\n> escape the nagging feeling that there should be better ways of getting\n> all that information about the camera mode to an application, but\n> whatever mechanism we use now can be improved upon later. The basic\n> ScalerCrop control would not be affected, I think.\n> \n> On Thu, 15 Oct 2020 at 03:17, Laurent Pinchart wrote:\n> > On Tue, Oct 13, 2020 at 09:36:50AM +0100, David Plowman wrote:\n> > > On Tue, 13 Oct 2020 at 02:45, Laurent Pinchart wrote:\n> > > > On Mon, Oct 12, 2020 at 07:49:31PM +0100, David Plowman wrote:\n> > > > > Hi Laurent\n> > > > >\n> > > > > Thanks for the comments. Some interesting points.\n> > > >\n> > > > Sorry about chiming in so late and challenging some of the base\n> > > > assumptions though. I'll now try not to introduce any further delay.\n> > > >\n> > > > > On Sun, 11 Oct 2020 at 22:47, Laurent Pinchart wrote:\n> > > > >\n> > > > > > Hi David,\n> > > > > >\n> > > > > > Thank you for the patches. And more than sorry for reviewing the series\n> > > > > > so late.\n> > > > > >\n> > > > > > On Tue, Sep 29, 2020 at 05:39:54PM +0100, David Plowman wrote:\n> > > > > > > Hi everyone\n> > > > > > >\n> > > > > > > Despite not originally intending to, I've actually made a version 3 of\n> > > > > > > the digital zoom patches, just to take care of a few things that might\n> > > > > > > be a bit annoying otherwise.\n> > > > > > >\n> > > > > > > 1. I've improved the description of the IspCrop control as was\n> > > > > > > suggested.\n> > > > > > >\n> > > > > > > 2. I've improved the description of the zoom option in cam (if we\n> > > > > > > decide to use this patch!), also as was proposed.\n> > > > > > >\n> > > > > > > 3. There was actually a problem with the \"{}\" syntax to denote zero\n> > > > > > > Rectangles in include/libcamera/ipa/raspberrypi.h. They were failing a\n> > > > > > > range type test in ControlInfoMap::generateIdmap() and so the control\n> > > > > > > wasn't working. I've replaced \"{}\" by \"Rectangle{}\" which seems OK.\n> > > > > > >\n> > > > > > > 4. There's been a bit of churn in the RPi pipeline handler lately so\n> > > > > > > rebasing gave some conflicts. I've fixed those up.\n> > > > > > >\n> > > > > > > Otherwise everything else remains the same.\n> > > > > >\n> > > > > > I've gone through the different versions of this series, and the first\n> > > > > > thing I want to see is that it has improved very nicely over time. Nice\n> > > > > > job !\n> > > > > >\n> > > > > > Configuring zoom through an absolute crop rectangle is I believe the way\n> > > > > > to go, so the overall approach is good in my opinion. There are however\n> > > > > > a few points I'd like to discuss, related to the SensorOutputSize\n> > > > > > property and IspCrop control.\n> > > > > >\n> > > > > > Reading through the discussion, I become increasingly aware of a topic\n> > > > > > that was present in the background without ever being named, and that's\n> > > > > > the camera pipeline model. As mentioned yesterday in a few other replies\n> > > > > > to selected patches in this series and its previous versions, the job of\n> > > > > > libcamera is to expose to applications a camera model that abstract\n> > > > > > hardware differences. This is not an easy job, as we have to strike the\n> > > > > > right balance between a higher-level inflexible but simple to use model\n> > > > > > that can't support many of the less common use cases, and a lower-level\n> > > > > > powerful but complex to use model that exposes a very large range of\n> > > > > > hardware features. We have implicitly defined the skeleton of such a\n> > > > > > model through the API of the Camera and PipelineHandler classes, but\n> > > > > > have never made it explicit.\n> > > > > >\n> > > > > > This needs to change. I see no reason to block the digital zoom feature\n> > > > > > until we finish documenting the pipeline model, but I would like to\n> > > > > > design the feature while thinking about the bigger picture. Here are the\n> > > > > > assumptions I think the pipeline model should make for devices that\n> > > > > > support digital zoom.\n> > > > > >\n> > > > > > - The camera pipeline starts with a camera sensor, that may implement\n> > > > > > binning, skipping and/or cropping.\n> > > > > >\n> > > > > > - The subsequent blocks in the pipeline may perform additional cropping,\n> > > > > > either at the direct command of the pipeline handler (e.g. cropping at\n> > > > > > the input of the scaler), or automatically to support image processing\n> > > > > > steps (e.g. colour interpoloation often drops a few lines and columns\n> > > > > > on all edges of the image).\n> > > > > >\n> > > > > > - The pipeline ends with a scaler that can implement digital zoom\n> > > > > > through a combination of cropping followed by scaling.\n> > > > > >\n> > > > > > The exact order of the processing steps at the hardware level doesn't\n> > > > > > need to match the pipeline model. For instance, cropping at the input\n> > > > > > and output of the scaler are interchangeable (not taking into account\n> > > > > > sub-pixel differences). It doesn't matter if the ISP scales before\n> > > > > > cropping the output, the hardware scaler parameters and output crop\n> > > > > > rectangle can be computed from an abstract input crop rectangle and\n> > > > > > output size. This is crucial to consider for the definition of the\n> > > > > > pipeline model: we need to design it in a way that ensures all features\n> > > > > > can be mapped to how they are implemented in the different types of\n> > > > > > hardware we want to support, but we're otherwise free to not map\n> > > > > > controls and properties 1:1 with the hardware parameters. When multiple\n> > > > > > options are possible, we should be guided by API design criteria such as\n> > > > > > coherency, simplicity and flexibility.\n> > > > > >\n> > > > > > Coming back to digital zoom, this series exposes a new SensorOutputSize\n> > > > > > property and a new IspCrop control. The SensorOutpuSize property is\n> > > > > > introduced to support the IspCrop control, as the base rectangle in\n> > > > > > which the scaler can crop. There are two issues here that bother me:\n> > > > > >\n> > > > > > - The property, despite being named SensorOutputSize, potentially takes\n> > > > > > into account the cropping added by the CSI-2 receiver and by the ISP\n> > > > > > for operations that consume lines and columns on the edges of the\n> > > > > > image. The naming can create some confusion, which can possibly be\n> > > > > > addressed by a combination of documentation (you're covering that\n> > > > > > already) and possibly a more explicit name for the property. However,\n> > > > > > as the property bundles crop operations perfomed in different stages\n> > > > > > of the pipeline, I'm worried that it will turn out to be a bit\n> > > > > > ill-defined regardless of how well we document it, with slightly\n> > > > > > different behaviours in different implementations.\n> > > > > >\n> > > > > > - Ignoring the additional cropping performed in the CSI-2 receiver and\n> > > > > > ISP (if any), the property exposes the sensor binning, skipping and\n> > > > > > cropping. It bundles those three operations together, and thus doesn't\n> > > > > > convey how the cropping affects the field of view as a given output\n> > > > > > size can be achieved with different combinations of skipping/binning\n> > > > > > and cropping.\n> > > > > >\n> > > > > > For these reasons, I'm concerned that the SensorOutputCrop property is a\n> > > > > > ad-hoc solution to provide a reference for the IspCrop property, and\n> > > > > > will not fit clearly in a pipeline model that should be based on simple,\n> > > > > > base building blocks. I would thus like to propose an alternative\n> > > > > > option.\n> > > > > >\n> > > > > > Instead of expressing the IspCrop controls (which I think we should\n> > > > > > rename to ScalerCrop) relatively to the SensorOutputSize property, could\n> > > > > > we express it relatively to the existing PixelArrayActiveAreas property\n> > > > > > ? This would have the advantage, in my opinion, of abstracting binning\n> > > > > > and skipping from applications. The pipeline handlers would need to\n> > > > > > perform a bit more work to compute the crop rectangle actually applied\n> > > > > > to the scaler, in order to take sensor binning/skipping and all\n> > > > > > additional cropping in the pipeline into account. The upside is that the\n> > > > > > ScalerCrop will directly define how the field of view is affected. It\n> > > > > > would also simplify the API, as no intermediate property between\n> > > > > > PixelArrayActiveAreas and ScalerCrop would need to be defined, and the\n> > > > > > ScalerCrop coordinates wouldn't depend on the active camera\n> > > > > > configuration. I think this would be easier to clearly document as part\n> > > > > > of a camera pipeline model.\n> > > > >\n> > > > > Renaming IspCrop to ScalerCrop sounds fine to me. It has had so many\n> > > > > different names!\n> > > > >\n> > > > > I'm less sure about trying to derive the SensorOutputSize (or\n> > > > > ScalerInputSize or whatever else we want to call it!) from the\n> > > > > PixelArrayActiveAreas property. Let me try and take a step back.\n> > > > >\n> > > > > So first, I think knowing the PixelArrayActiveArea isn't enough. How would\n> > > > > you know if the pipeline handler was doing some extra cropping that wasn't\n> > > > > \"strictly necessary\", perhaps to reduce memory traffic, or for a faster\n> > > > > framerate. How would the application know not to try and zoom there? It\n> > > > > seems to me that this really is a decision for the pipeline handler based\n> > > > > on the sensor driver, it isn't available from the properties of the sensor\n> > > > > itself.\n> > > > >\n> > > > > Actually I'd quite like to leave the discussion there for the moment and\n> > > > > see if that much is controversial or not. Of course we then have to move on\n> > > > > but maybe let's see what we think about that first...\n> > > > >\n> > > > > Thoughts??\n> > > >\n> > > > I fully agree with you that the pipeline handler has the last word when\n> > > > it comes to the (combined) internal cropping. It may not have any choice\n> > > > (if the CFA interpolation eats two or four lines and columns on each\n> > > > side of the image due to the hardware implementation, there's nothing\n> > > > that can be done against it), or may just want to decide on policies for\n> > > > whatever reason it sees fit (while trying not to over-police though, to\n> > > > leave options to applications).\n> > > >\n> > > > My proposal doesn't deny this from the pipeline handler, but tries to\n> > > > express the crop rectangle in a way that maps directly to the field of\n> > > > view. To do so, binning/skipping would be hidden from applications by\n> > > > using a reference for the ScalerCrop property that has no\n> > > > binning/skipping applied.\n> > >\n> > > So we certainly agree on the necessity for \"something\", that's good,\n> > > and then it's a case of figuring out what \"something\" is. I have\n> > > several conflicting thoughts here (I frequently disagree with myself):\n> > >\n> > > * Actually, as a basic principle, I think applications should be able\n> > > to find out \"the truth\" if they want it. This would mean the sensor\n> > > crop, binning/scaling, and any subsequent cropping.\n> >\n> > I agree. I can even foresee that a low-level API to control the sensor\n> > parameters explicitly could be useful in some advanced cases. That\n> > should be an add-on though, it shouldn't cause the main API to be more\n> > convoluted.\n> >\n> > > * I even wonder sometimes whether there shouldn't really be a class\n> > > that describes how the camera mode has been derived from the sensor,\n> > > with all this information made explicit. But I think that leads us\n> > > even deeper into the woods, and I suspect we don't really want to go\n> > > there! (Also, I'm not sure if the V4L2 API would even let you discover\n> > > all of it, which might be a problem...)\n> >\n> > I think we will go there :-) I expect sensors to require more complex\n> > configurations in the future, as the MIPI CCS spec gets adopted. The\n> > kernel driver will expose 2 or 3 subdevs, and we will have a class to\n> > abstract that in libcamera to make it easier for pipeline handlers.\n> >\n> > > * But I also think digital zoom needs to be easy to use for\n> > > applications that don't care about all these details (\"the truth\", as\n> > > I called it!). Actually I feel the overwhelming majority of\n> > > applications will fall into this category.\n> >\n> > We're on the same page, good :-)\n> >\n> > > > The pipeline handler would restrict the requested ScalerCrop control\n> > > > based on the internal cropping that is always applied (combining sensor,\n> > > > CSI-2 receiver and ISP), and report the result through the request\n> > > > metadata. The application would thus know the exact crop rectangle that\n> > > > has been used, in unscaled (binning/skipping) sensor coordinates.\n> > >\n> > > Let me try and paraphrase a bit and you can tell me if I've got it wrong.\n> > >\n> > > You're proposing a property (details in a moment) that can be used to\n> > > choose zoom regions for the ScalerCrop control.\n> > >\n> > > The units for this property are the native sensor pixels, ignoring any\n> > > binning/scaling that might be going on.\n> > >\n> > > The property would give you a Rectangle that expresses exactly the\n> > > area in which you are permitted to zoom? The offset of the rectangle\n> > > tells you where in the sensor array this rectangle actually lies.\n> > >\n> > > The term \"sensor array\" there needs some clarification. I think it\n> > > probably means the \"PixelArraySize\" less any invalid or optical black\n> > > pixels. (Do we have a way to get hold of that easily?)\n> >\n> > I think we should use PixelArrayActiveAreas, while we may be able to\n> > capture dark pixels with some sensors, I think they're out of scope for\n> > digital zoom.\n> >\n> > My proposal initially didn't include a new property to express the area\n> > in which we can zoom. I was considering reporting that through the\n> > ScalerCrop maximum value. Every control exposed by a pipeline handler\n> > has a ControlInfo instance associated with it, to report the minimum,\n> > default and maximum values. I was thinking about reporting the area in\n> > which we can zoom as the maximum value of the ControlInfo for the\n> > ScalerCrop control. This would give applications all the information\n> > they need, without the need to introduce a new property.\n> >\n> > The issue with this is that ControlInfo is currently static, so we can't\n> > change the ScalerCrop reported maximum value when the camera is\n> > configured. On the other hand, properties are supposed to be static too.\n> > As we need to report the maximum dynamically, we will need to either\n> > allow ControlInfo to be dynamic, or properties to be dynamic. A change\n> > in behaviour is thus required, which lead me to think we should\n> > investigate the ControlInfo path.\n> >\n> > This being said, I don't want to delay this feature much longer, so I'm\n> > fine adding a new property to report the ScalerCrop maximum value (we\n> > could name it ScalerCropMaximum or ScalerCropBound for instance) for\n> > now, with the property value being updated when the camera is\n> > configured. We can then explore making ControlInfo dynamic, and if it\n> > turns out to be a good idea, drop the ScalerCropBound in the future.\n> >\n> > > The ScalerCrop requested, and reported back, gives the offset and size\n> > > of the zoom rectangle relative to the rectangle given by the property\n> > > under discussion here (I think that's the most convenient choice). So\n> > > if it reports back offsets of (0,0) then you know you're in the top\n> > > left hand corner of what you could possibly ever see in this camera\n> > > mode (but not necessarily of the sensor as a whole).\n> > >\n> > > I think this could work and is a fair compromise between the amount of\n> > > information and the ease of use. Most digital zoom applications could\n> > > simply request its Size, and go from there. Only a few minor\n> > > mis-givings:\n> > >\n> > > * Does V4L2 actually give you a way of finding out where a particular\n> > > \"camera mode\" lies in the pixel array? Do you even know if a mode is\n> > > binned or heavily cropped... someone who knows rather more about V4L2\n> > > needs to answer that one!\n> >\n> > This is exactly why we have implemented the read-only subdev API, to\n> > allow userspace to query detailed information about the current mode.\n> > The amount of information that can be extracted from the sensor driver\n> > depends on how the driver models the sensor. In order to expose analog\n> > crop, binning, skipping and digital crop separately, we would need to\n> > expose as least two subdevs to userspace. That's the direction I want to\n> > take, and libcamera will be able to handle that, but it means some\n> > extra work on the kernel side to implement this in sensor drivers (it's\n> > no rocket science though).\n> >\n> > > * Use of native sensor coords is OK, though a few sensors might\n> > > support non-integer scaling at which point the property would be\n> > > slightly \"approximate\". That seems fairly minor to me, though. (Some\n> > > ISPs might even do non-integer cropping, so the problem can exist\n> > > regardless.)\n> >\n> > If a sensor implements scaling in addition to cropping, binning and\n> > skipping, three subdevs will be required :-) That's how the smiapp\n> > driver is implemented today, and the new ccs driver (\"[PATCH 000/100]\n> > CCS driver\" on the linux-media mailing list) follows the same path (it\n> > actually replaces the smiapp driver, implementing support for both\n> > SMIA++ and CCS).\n> >\n> > > * It doesn't really support my assertion that you should be able to\n> > > know exactly what's going on, if you want to. (But if we decide this\n> > > doesn't matter, then that's fine too.)\n> >\n> > It doesn't by itself, but we could then add additional properties, or\n> > extra information in the CameraConfiguration, to report the detailed\n> > configuration of the sensor. What I like with this approach is that not\n> > only the two will not conflict (we will be able to add the properties or\n> > extend CameraConfiguration without touching the digital zoom API), but\n> > the will be no overlap between the two features either, they will each\n> > have one dedicated purpose.\n> >\n> > > * In one of our early discussions we suggested that having the exact\n> > > pixel level information might be useful, and you could do things like\n> > > pan one pixel at a time. We lose that by going to native sensor\n> > > coordinates. I think I said at the time that this ability feels more\n> > > useful than I believe it really is, so I wouldn't be too upset.\n> >\n> > I'm not sure to follow you here, why wouldn't we be able to pan by one\n> > pixel ? If the sensor is configured with binning/skipping an application\n> > would have to pan by 2 pixels to achieve an effective cropping of 1\n> > pixel, so maybe we will need to report a step, or just report\n> > binning/skipping factors, but with the proposed API the crop rectangle\n> > can be set to a pixel-perfect position. Or am I missing something ?\n> >\n> > > > While in some use cases the actual crop rectangle reported through\n> > > > metadata would be enough (for instance, in a GUI that starts with an\n> > > > unzoomed view, the application would get the maximum possible crop\n> > > > rectangle in the metadata of the first frame), I can imagine we would\n> > > > have use cases that need this information before capturing the first\n> > >\n> > > Indeed. And I also think you should be able to set digital zoom to\n> > > take effect *on* the very first frame itself (this spills over into\n> > > Naush's most recent patch set...)\n> >\n> > Yes, I think we should allow that, even if most use cases will likely\n> > not need it.\n> >\n> > > > frame. I initially thought we could report it through the max value of\n> > > > the ControlInfo instance for the ScalerCrop control, but this is\n> > > > supposed to be static information. As this would be (in my opinion) a\n> > > > neat solution, I'd like to investigate the possibility of making\n> > > > ControlInfo dynamic, but maybe we will need a different solution. For\n> > > > foresee the addition of an API that will create request templates,\n> > > > filling them with default control values based on a requested use case,\n> > > > and the maximum ScalerCrop could possibly be reported through that\n> > > > mechanism. I'm also fine reporting it through a temporary mechanism (for\n> > > > instance adding it to CameraConfiguration, or creating a dynamic\n> > > > property) for the time being, if you already have pressing use cases for\n> > > > knowing the maximum value before capturing the first frame.\n> > > >\n> > > > Thoughts ? :-)\n> > >\n> > > I'm glad we're talking about all this stuff!!\n> > >\n> > > > > > Two additional points I'd like to consider (and which are orthogonal to\n> > > > > > the previous one) are:\n> > > > > >\n> > > > > > - Should we automatically adjust the ScalerCrop rectangle to always\n> > > > > > output square pixels, or should we allow modifying the aspect ratio\n> > > > > > when scaling ? Most use cases call for square pixels, but I don't\n> > > > > > think we necessarily want to create an artificial limitation here (as\n> > > > > > long as we make it easy for applications to compute the scaling\n> > > > > > parameters that will give them square pixels)/\n> > > > >\n> > > > > Personally I see no reason to restrict what an application can request. We\n> > > > > need to make it easy to request the right aspect ratio (hence those\n> > > > > geometry helpers), but if people actually have a use-case for something\n> > > > > strange...\n> > > >\n> > > > Fine with me.\n> > > >\n> > > > > > - Should we allow a ScalerCrop rectangle smaller than the stream output\n> > > > > > size, or should we restrict scaling to down-scaling only ?\n> > > > >\n> > > > > I think up-scaling is probably the most common use-case for us (though\n> > > > > downscaling will happen too). Think of all those (rubbish) 30x zoom\n> > > > > pictures that some phones like to produce...!\n> > > >\n> > > > Rubbish is exactly the work I would have used :-) I'm tempted to try and\n> > > > teach users that they shouldn't do this by disabling this feature, but\n> > > > that would be pretentious. I suppose there's no good reason to forbid\n> > > > this. Should we put a limit on the upscaling factor though ?\n> > >\n> > > Always tricky. To what extent should we save people from themselves?\n> > > I'm pretty sure the Pi imposes some kind of a limit, but don't really\n> > > know. In the end, so long as it doesn't blow up, that's kind of OK...\n> >\n> > Maybe we'll introduce the concept of soft and hard limits in the future,\n> > with the soft limit being what the pipeline handler recommends not to\n> > exceed to guarantee \"reasonable\" quality, and the hard limit being what\n> > you can get out of the hardware. If someone finds a good use case for\n> > x100 digital zoom, I'll be curious to hear about it. We unfortunately\n> > don't have the same ISPs as in those movies where a 4 pixels detail in\n> > an image can be zoomed in full screen :-)\n> >\n> > > > > > > David Plowman (6):\n> > > > > > > libcamera: Add SensorOutputSize property\n> > > > > > > libcamera: Initialise the SensorOutputSize property\n> > > > > > > libcamera: Add IspCrop control\n> > > > > > > libcamera: Add geometry helper functions\n> > > > > > > libcamera: pipeline: raspberrypi: Implementation of digital zoom\n> > > > > > > cam: Add command line option to test IspCrop control\n> > > > > > >\n> > > > > > > include/libcamera/geometry.h | 20 +++\n> > > > > > > include/libcamera/ipa/raspberrypi.h | 1 +\n> > > > > > > src/cam/capture.cpp | 25 +++-\n> > > > > > > src/cam/capture.h | 2 +-\n> > > > > > > src/cam/main.cpp | 3 +\n> > > > > > > src/cam/main.h | 1 +\n> > > > > > > src/ipa/raspberrypi/raspberrypi.cpp | 7 +\n> > > > > > > src/libcamera/camera_sensor.cpp | 6 +\n> > > > > > > src/libcamera/control_ids.yaml | 12 ++\n> > > > > > > src/libcamera/geometry.cpp | 129 ++++++++++++++++++\n> > > > > > > .../pipeline/raspberrypi/raspberrypi.cpp | 47 +++++++\n> > > > > > > src/libcamera/property_ids.yaml | 19 +++\n> > > > > > > 12 files changed, 269 insertions(+), 3 deletions(-)","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 7247BBDB13\n\tfor ;\n\tFri, 16 Oct 2020 06:31:37 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id CD276610A9;\n\tFri, 16 Oct 2020 08:31:36 +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 AEDA960CE6\n\tfor ;\n\tFri, 16 Oct 2020 08:31:35 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 13048528;\n\tFri, 16 Oct 2020 08:31:35 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"CYXIanW2\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1602829895;\n\tbh=YxfZS2pghMgQujkPwxaapFmMHdoDoX2zDKnWlXJPBio=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=CYXIanW2kZyZUEWbMoE8CJIsWC+NYDaI+AKrtApIZH5Z4jAPdkahCqnDwi2MhyabB\n\to/81+Z3MWUUcx7FnHacW39nwbWHqNuWia6cP4EOzSSWRO3J/fIWUVs1z6IZQoxRM24\n\tIwjya4CrB6OZ2HnkbofYtGNXHO9uDg/SA2Ctubbg=","Date":"Fri, 16 Oct 2020 09:30:48 +0300","From":"Laurent Pinchart ","To":"David Plowman ","Message-ID":"<20201016063048.GJ3829@pendragon.ideasonboard.com>","References":"<20200929164000.15429-1-david.plowman@raspberrypi.com>\n\t<20201011214708.GA3944@pendragon.ideasonboard.com>\n\t\n\t<20201013014422.GD3942@pendragon.ideasonboard.com>\n\t\n\t<20201015021650.GH3858@pendragon.ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v3 0/6] Digital zoom","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 ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":13237,"web_url":"https://patchwork.libcamera.org/comment/13237/","msgid":"","date":"2020-10-16T07:21:45","subject":"Re: [libcamera-devel] [PATCH v3 0/6] Digital zoom","submitter":{"id":42,"url":"https://patchwork.libcamera.org/api/people/42/","name":"David Plowman","email":"david.plowman@raspberrypi.com"},"content":"Hi Laurent\n\nThanks for the explanation, indeed I think that does make things clearer.\n\nSo the next step is probably now for me to modify my implementation\nand we can take the discussion from there once we have something\nconcrete. I'll try and send it round early next week.\n\nBest regards\nDavid\n\nOn Fri, 16 Oct 2020 at 07:31, Laurent Pinchart\n wrote:\n>\n> Hi David,\n>\n> On Thu, Oct 15, 2020 at 03:30:56PM +0100, David Plowman wrote:\n> > Hi Laurent\n> >\n> > Thanks for all that! The discussion is getting rather long so perhaps\n> > I can try to wrap up all the points in the thread and sum it up at the\n> > top here with an outline of the plan as I understand it!\n>\n> Thank you for summarizing this.\n>\n> > 1. ScalerCrop control\n> >\n> > This will be a Rectangle indicating the crop for digital zoom. The\n> > units will be native pixel coordinates. The (x,y) offsets should, I\n> > think, start at (0,0) because the \"top-left-most\" thing you can have\n> > should probably be signalled with (0,0) - but see below.\n>\n> I think the ScalerCrop control should have the same unit and the same\n> reference as the ScalerCropMaximum property. ScalerCropMaximum is meant\n> to convey the largest possible crop rectangle, so it should be valid to\n> set ScalerCrop = ScalerCropMaximum. As ScalerCropMaximum will typically\n> have (x,y) > (0,0) (unless the ISP doesn't need to crop at all for\n> internal reasons), I don't think we should consider that ScalerCrop\n> starts at (0,0).\n>\n> The alternative is to express ScalerCrop relatively to\n> ScalerCropMaximum, but in that case ScalerCropMaximum wouldn't be the\n> maximum value for the ScalerCrop control anymore. In order to later\n> replace\n>\n> camera->properties().at(properties::ScalerCropMaximum)\n>\n> with\n>\n> camera->controls().at(controls::ScalerCrop).max()\n>\n> we need to express the two with the same reference.\n>\n> > 2. ScalerCropMaximum property\n> >\n> > This indicates the maximum image area from which we can crop, again in\n> > native pixel coordinates. It could be a Rectangle and we could use the\n> > (x,y) offsets perhaps to indicate the top left coordinates in the\n> > sensor - i.e. this would (mostly) be the analogCrop then. (Or one\n> > could leave them as zero, in which case you might as well leave this\n> > as a Size.)\n>\n> The idea is that ScalerCropMaximum is indeed expressed similarly to an\n> analog crop rectangle, but its meaning is different. The coordinate\n> system is the sensor's pixel array active area, but the value takes into\n> account mandatory cropping performed in all stages of the pipeline,\n> including the ISP.\n>\n> > We've talked about making this a \"dynamic\" ControlInfo maximum, though\n> > it's not quite the same thing. For example, it would typically have\n> > the \"wrong\" aspect ratio so taking and setting the \"maximum\" would\n> > give you a weird image - would a user expect that? Also, the meaning\n> > of the (x,y) offsets as described for the control does not align as\n> > things stand.\n>\n> Setting ScalerCrop = ScalerCropMaximum would indeed give the wrong\n> aspect ratio. We will however have nice geometry helpers (thank you for\n> your work on that) that should make it easy to get the right behaviour.\n>\n> > 3. Other stuff...\n> >\n> > * With this scheme there's no way for an application to know what the\n> > true pixel scale for the ScalerCrop is. Maybe that kind of information\n> > gets exposed later. (I seem to remember wanting to expose the whole\n> > CameraSensorInfo several months ago...)\n>\n> I'm not sure to follow you here. With coordinates for ScalerCrop and\n> ScalerCropMaximum being sensor pixels, the true pixel scale will simply\n> be the ratio between the stream output size and ScalerCrop.size(), won't\n> it ?\n>\n> > * There's no real allowance for any cropping after binning/scaling.\n> > Maybe you have to adjust the ScalerCropMaximum to make it look like\n> > all the cropping is happening up front? TBD, I think.\n>\n> Maybe this is where we haven't understood each other properly.\n> ScalerCropMaximum (and thus ScalerCrop) are expressed in sensor pixel\n> coordinates, as if all cropping was applied before binning and skipping,\n> but they include all cropping operations through the whole pipeline.\n>\n> > * Pipeline handlers will all have to go around rescaling the\n> > ScalerCrop rectangles from native to actual pixel coordinates.\n>\n> Correct, in order to compute the real scaler input crop rectangle,\n> pipeline handlers will need to divide ScalerCrop by the binning/skipping\n> factors, and subtract the internal crop that is performed in other parts\n> of the pipeline. Let's consider a few examples.\n>\n> Let's assume a sensor native resolution of 4056x3040 active pixels\n> (non-active and optical dark pixels are ignored in this example). Let's\n> further assume that the ISP, for internal reasons, consumes 4 lines and\n> columns on each side of the image.\n>\n> For the first example, let's assume that the user wants to capture the\n> full image. No binning, skipping or cropping is applied in the sensor.\n> Due to ISP cropping, the pipeline handler should restrict the output\n> size to 4048x3032 in order to keep pixels square, as upscaling 4048x3032\n> to 4056x3040 will modify the aspect ratio slightly. The\n> ScalerCropMaximum property will be reported as (4,4)-4048x3032, and the\n> default ScalerCrop value will be (4,4)-4048x3032. Applications can\n> select a smaller ScalerCrop rectangle, and they will be responsible for\n> ensuring that its size matches the aspect ratio of the output image\n> (4048x3032) if they want to keep pixels square.\n>\n> In a second example, let's assume the user sets the stream output size\n> to 1920x1080. The pipeline handler decides to enable binning on the\n> sensor side, but without applying any cropping on the sensor. The sensor\n> output is thus 2028x1520. Due to the ISP internal cropping of 4 pixels\n> on each side, the image at the scaler input will be at most 2020x1512.\n> Scaling this back to pixel array coordinates, the ScalerCropMaximum will\n> be reported as (8,8)-4040x3024. Note how it is slightly smaller than in\n> the previous example. The pipeline handler will furthermore set the\n> actual scaler crop rectangle, expressed in scaler input coordinates\n> (thus relative to a 2020x1512 size) to (2,189)-2016x1134 to get a 16:9\n> aspect ratio (it could also set it to (0,188)-2020x1136 to use the full\n> width, but the aspect ratio would then be slightly off as 2020 isn't a\n> multiple of 16). Scaling this back to the pixel array coordinates, the\n> default ScalerCrop rectangle reported to the application is\n> (12,386)-4032x2268. Applications will be able to pan within the\n> (8,8)-4040x3024 maximum crop rectangle, and possibly scaling a ~4:3\n> image to 16:9 if they set ScalerCrop = ScalerCropMaximum.\n>\n> In a third example, with the same 1920x1080 output resolution, the\n> pipeline handler may decide to crop on the sensor, for instance cropping\n> the binned 2028x1520 output to (50,216)-1928x1088. The scaler input\n> size, after the ISP internal cropping, will be 1920x1080, matching the\n> stream output size. The image doesn't need to be scaled, but the field\n> of view is reduced. In this case, the ScalerCropMaximum will be reported\n> as (108,440)-3840x2160, and the default ScalerCrop value will be set to\n> (108,440)-3840x2160 as well. Applications will be able to pan within the\n> (108,440)-3840x2160 maximum crop rectangle, but unlike in the previous\n> example, they won't be able to pan outside of the default, as panning is\n> only possible in the scaler, the sensor crop rectangle is fixed.\n>\n> Do these examples help clarifying the proposal ?\n>\n> > Maybe there's another geometry helper function there: Rectangle\n> > Rectangle::rescaledTo(const Size &from, const Size &to)\n> > e.g. actualCrop = nativeCrop.rescaledTo(sensorInfo.analogCrop.size(),\n> > sensorInfo.outputSize)\n> >\n> > Please correct me if I've got any of that wrong. Overall I can't quite\n> > escape the nagging feeling that there should be better ways of getting\n> > all that information about the camera mode to an application, but\n> > whatever mechanism we use now can be improved upon later. The basic\n> > ScalerCrop control would not be affected, I think.\n> >\n> > On Thu, 15 Oct 2020 at 03:17, Laurent Pinchart wrote:\n> > > On Tue, Oct 13, 2020 at 09:36:50AM +0100, David Plowman wrote:\n> > > > On Tue, 13 Oct 2020 at 02:45, Laurent Pinchart wrote:\n> > > > > On Mon, Oct 12, 2020 at 07:49:31PM +0100, David Plowman wrote:\n> > > > > > Hi Laurent\n> > > > > >\n> > > > > > Thanks for the comments. Some interesting points.\n> > > > >\n> > > > > Sorry about chiming in so late and challenging some of the base\n> > > > > assumptions though. I'll now try not to introduce any further delay.\n> > > > >\n> > > > > > On Sun, 11 Oct 2020 at 22:47, Laurent Pinchart wrote:\n> > > > > >\n> > > > > > > Hi David,\n> > > > > > >\n> > > > > > > Thank you for the patches. And more than sorry for reviewing the series\n> > > > > > > so late.\n> > > > > > >\n> > > > > > > On Tue, Sep 29, 2020 at 05:39:54PM +0100, David Plowman wrote:\n> > > > > > > > Hi everyone\n> > > > > > > >\n> > > > > > > > Despite not originally intending to, I've actually made a version 3 of\n> > > > > > > > the digital zoom patches, just to take care of a few things that might\n> > > > > > > > be a bit annoying otherwise.\n> > > > > > > >\n> > > > > > > > 1. I've improved the description of the IspCrop control as was\n> > > > > > > > suggested.\n> > > > > > > >\n> > > > > > > > 2. I've improved the description of the zoom option in cam (if we\n> > > > > > > > decide to use this patch!), also as was proposed.\n> > > > > > > >\n> > > > > > > > 3. There was actually a problem with the \"{}\" syntax to denote zero\n> > > > > > > > Rectangles in include/libcamera/ipa/raspberrypi.h. They were failing a\n> > > > > > > > range type test in ControlInfoMap::generateIdmap() and so the control\n> > > > > > > > wasn't working. I've replaced \"{}\" by \"Rectangle{}\" which seems OK.\n> > > > > > > >\n> > > > > > > > 4. There's been a bit of churn in the RPi pipeline handler lately so\n> > > > > > > > rebasing gave some conflicts. I've fixed those up.\n> > > > > > > >\n> > > > > > > > Otherwise everything else remains the same.\n> > > > > > >\n> > > > > > > I've gone through the different versions of this series, and the first\n> > > > > > > thing I want to see is that it has improved very nicely over time. Nice\n> > > > > > > job !\n> > > > > > >\n> > > > > > > Configuring zoom through an absolute crop rectangle is I believe the way\n> > > > > > > to go, so the overall approach is good in my opinion. There are however\n> > > > > > > a few points I'd like to discuss, related to the SensorOutputSize\n> > > > > > > property and IspCrop control.\n> > > > > > >\n> > > > > > > Reading through the discussion, I become increasingly aware of a topic\n> > > > > > > that was present in the background without ever being named, and that's\n> > > > > > > the camera pipeline model. As mentioned yesterday in a few other replies\n> > > > > > > to selected patches in this series and its previous versions, the job of\n> > > > > > > libcamera is to expose to applications a camera model that abstract\n> > > > > > > hardware differences. This is not an easy job, as we have to strike the\n> > > > > > > right balance between a higher-level inflexible but simple to use model\n> > > > > > > that can't support many of the less common use cases, and a lower-level\n> > > > > > > powerful but complex to use model that exposes a very large range of\n> > > > > > > hardware features. We have implicitly defined the skeleton of such a\n> > > > > > > model through the API of the Camera and PipelineHandler classes, but\n> > > > > > > have never made it explicit.\n> > > > > > >\n> > > > > > > This needs to change. I see no reason to block the digital zoom feature\n> > > > > > > until we finish documenting the pipeline model, but I would like to\n> > > > > > > design the feature while thinking about the bigger picture. Here are the\n> > > > > > > assumptions I think the pipeline model should make for devices that\n> > > > > > > support digital zoom.\n> > > > > > >\n> > > > > > > - The camera pipeline starts with a camera sensor, that may implement\n> > > > > > > binning, skipping and/or cropping.\n> > > > > > >\n> > > > > > > - The subsequent blocks in the pipeline may perform additional cropping,\n> > > > > > > either at the direct command of the pipeline handler (e.g. cropping at\n> > > > > > > the input of the scaler), or automatically to support image processing\n> > > > > > > steps (e.g. colour interpoloation often drops a few lines and columns\n> > > > > > > on all edges of the image).\n> > > > > > >\n> > > > > > > - The pipeline ends with a scaler that can implement digital zoom\n> > > > > > > through a combination of cropping followed by scaling.\n> > > > > > >\n> > > > > > > The exact order of the processing steps at the hardware level doesn't\n> > > > > > > need to match the pipeline model. For instance, cropping at the input\n> > > > > > > and output of the scaler are interchangeable (not taking into account\n> > > > > > > sub-pixel differences). It doesn't matter if the ISP scales before\n> > > > > > > cropping the output, the hardware scaler parameters and output crop\n> > > > > > > rectangle can be computed from an abstract input crop rectangle and\n> > > > > > > output size. This is crucial to consider for the definition of the\n> > > > > > > pipeline model: we need to design it in a way that ensures all features\n> > > > > > > can be mapped to how they are implemented in the different types of\n> > > > > > > hardware we want to support, but we're otherwise free to not map\n> > > > > > > controls and properties 1:1 with the hardware parameters. When multiple\n> > > > > > > options are possible, we should be guided by API design criteria such as\n> > > > > > > coherency, simplicity and flexibility.\n> > > > > > >\n> > > > > > > Coming back to digital zoom, this series exposes a new SensorOutputSize\n> > > > > > > property and a new IspCrop control. The SensorOutpuSize property is\n> > > > > > > introduced to support the IspCrop control, as the base rectangle in\n> > > > > > > which the scaler can crop. There are two issues here that bother me:\n> > > > > > >\n> > > > > > > - The property, despite being named SensorOutputSize, potentially takes\n> > > > > > > into account the cropping added by the CSI-2 receiver and by the ISP\n> > > > > > > for operations that consume lines and columns on the edges of the\n> > > > > > > image. The naming can create some confusion, which can possibly be\n> > > > > > > addressed by a combination of documentation (you're covering that\n> > > > > > > already) and possibly a more explicit name for the property. However,\n> > > > > > > as the property bundles crop operations perfomed in different stages\n> > > > > > > of the pipeline, I'm worried that it will turn out to be a bit\n> > > > > > > ill-defined regardless of how well we document it, with slightly\n> > > > > > > different behaviours in different implementations.\n> > > > > > >\n> > > > > > > - Ignoring the additional cropping performed in the CSI-2 receiver and\n> > > > > > > ISP (if any), the property exposes the sensor binning, skipping and\n> > > > > > > cropping. It bundles those three operations together, and thus doesn't\n> > > > > > > convey how the cropping affects the field of view as a given output\n> > > > > > > size can be achieved with different combinations of skipping/binning\n> > > > > > > and cropping.\n> > > > > > >\n> > > > > > > For these reasons, I'm concerned that the SensorOutputCrop property is a\n> > > > > > > ad-hoc solution to provide a reference for the IspCrop property, and\n> > > > > > > will not fit clearly in a pipeline model that should be based on simple,\n> > > > > > > base building blocks. I would thus like to propose an alternative\n> > > > > > > option.\n> > > > > > >\n> > > > > > > Instead of expressing the IspCrop controls (which I think we should\n> > > > > > > rename to ScalerCrop) relatively to the SensorOutputSize property, could\n> > > > > > > we express it relatively to the existing PixelArrayActiveAreas property\n> > > > > > > ? This would have the advantage, in my opinion, of abstracting binning\n> > > > > > > and skipping from applications. The pipeline handlers would need to\n> > > > > > > perform a bit more work to compute the crop rectangle actually applied\n> > > > > > > to the scaler, in order to take sensor binning/skipping and all\n> > > > > > > additional cropping in the pipeline into account. The upside is that the\n> > > > > > > ScalerCrop will directly define how the field of view is affected. It\n> > > > > > > would also simplify the API, as no intermediate property between\n> > > > > > > PixelArrayActiveAreas and ScalerCrop would need to be defined, and the\n> > > > > > > ScalerCrop coordinates wouldn't depend on the active camera\n> > > > > > > configuration. I think this would be easier to clearly document as part\n> > > > > > > of a camera pipeline model.\n> > > > > >\n> > > > > > Renaming IspCrop to ScalerCrop sounds fine to me. It has had so many\n> > > > > > different names!\n> > > > > >\n> > > > > > I'm less sure about trying to derive the SensorOutputSize (or\n> > > > > > ScalerInputSize or whatever else we want to call it!) from the\n> > > > > > PixelArrayActiveAreas property. Let me try and take a step back.\n> > > > > >\n> > > > > > So first, I think knowing the PixelArrayActiveArea isn't enough. How would\n> > > > > > you know if the pipeline handler was doing some extra cropping that wasn't\n> > > > > > \"strictly necessary\", perhaps to reduce memory traffic, or for a faster\n> > > > > > framerate. How would the application know not to try and zoom there? It\n> > > > > > seems to me that this really is a decision for the pipeline handler based\n> > > > > > on the sensor driver, it isn't available from the properties of the sensor\n> > > > > > itself.\n> > > > > >\n> > > > > > Actually I'd quite like to leave the discussion there for the moment and\n> > > > > > see if that much is controversial or not. Of course we then have to move on\n> > > > > > but maybe let's see what we think about that first...\n> > > > > >\n> > > > > > Thoughts??\n> > > > >\n> > > > > I fully agree with you that the pipeline handler has the last word when\n> > > > > it comes to the (combined) internal cropping. It may not have any choice\n> > > > > (if the CFA interpolation eats two or four lines and columns on each\n> > > > > side of the image due to the hardware implementation, there's nothing\n> > > > > that can be done against it), or may just want to decide on policies for\n> > > > > whatever reason it sees fit (while trying not to over-police though, to\n> > > > > leave options to applications).\n> > > > >\n> > > > > My proposal doesn't deny this from the pipeline handler, but tries to\n> > > > > express the crop rectangle in a way that maps directly to the field of\n> > > > > view. To do so, binning/skipping would be hidden from applications by\n> > > > > using a reference for the ScalerCrop property that has no\n> > > > > binning/skipping applied.\n> > > >\n> > > > So we certainly agree on the necessity for \"something\", that's good,\n> > > > and then it's a case of figuring out what \"something\" is. I have\n> > > > several conflicting thoughts here (I frequently disagree with myself):\n> > > >\n> > > > * Actually, as a basic principle, I think applications should be able\n> > > > to find out \"the truth\" if they want it. This would mean the sensor\n> > > > crop, binning/scaling, and any subsequent cropping.\n> > >\n> > > I agree. I can even foresee that a low-level API to control the sensor\n> > > parameters explicitly could be useful in some advanced cases. That\n> > > should be an add-on though, it shouldn't cause the main API to be more\n> > > convoluted.\n> > >\n> > > > * I even wonder sometimes whether there shouldn't really be a class\n> > > > that describes how the camera mode has been derived from the sensor,\n> > > > with all this information made explicit. But I think that leads us\n> > > > even deeper into the woods, and I suspect we don't really want to go\n> > > > there! (Also, I'm not sure if the V4L2 API would even let you discover\n> > > > all of it, which might be a problem...)\n> > >\n> > > I think we will go there :-) I expect sensors to require more complex\n> > > configurations in the future, as the MIPI CCS spec gets adopted. The\n> > > kernel driver will expose 2 or 3 subdevs, and we will have a class to\n> > > abstract that in libcamera to make it easier for pipeline handlers.\n> > >\n> > > > * But I also think digital zoom needs to be easy to use for\n> > > > applications that don't care about all these details (\"the truth\", as\n> > > > I called it!). Actually I feel the overwhelming majority of\n> > > > applications will fall into this category.\n> > >\n> > > We're on the same page, good :-)\n> > >\n> > > > > The pipeline handler would restrict the requested ScalerCrop control\n> > > > > based on the internal cropping that is always applied (combining sensor,\n> > > > > CSI-2 receiver and ISP), and report the result through the request\n> > > > > metadata. The application would thus know the exact crop rectangle that\n> > > > > has been used, in unscaled (binning/skipping) sensor coordinates.\n> > > >\n> > > > Let me try and paraphrase a bit and you can tell me if I've got it wrong.\n> > > >\n> > > > You're proposing a property (details in a moment) that can be used to\n> > > > choose zoom regions for the ScalerCrop control.\n> > > >\n> > > > The units for this property are the native sensor pixels, ignoring any\n> > > > binning/scaling that might be going on.\n> > > >\n> > > > The property would give you a Rectangle that expresses exactly the\n> > > > area in which you are permitted to zoom? The offset of the rectangle\n> > > > tells you where in the sensor array this rectangle actually lies.\n> > > >\n> > > > The term \"sensor array\" there needs some clarification. I think it\n> > > > probably means the \"PixelArraySize\" less any invalid or optical black\n> > > > pixels. (Do we have a way to get hold of that easily?)\n> > >\n> > > I think we should use PixelArrayActiveAreas, while we may be able to\n> > > capture dark pixels with some sensors, I think they're out of scope for\n> > > digital zoom.\n> > >\n> > > My proposal initially didn't include a new property to express the area\n> > > in which we can zoom. I was considering reporting that through the\n> > > ScalerCrop maximum value. Every control exposed by a pipeline handler\n> > > has a ControlInfo instance associated with it, to report the minimum,\n> > > default and maximum values. I was thinking about reporting the area in\n> > > which we can zoom as the maximum value of the ControlInfo for the\n> > > ScalerCrop control. This would give applications all the information\n> > > they need, without the need to introduce a new property.\n> > >\n> > > The issue with this is that ControlInfo is currently static, so we can't\n> > > change the ScalerCrop reported maximum value when the camera is\n> > > configured. On the other hand, properties are supposed to be static too.\n> > > As we need to report the maximum dynamically, we will need to either\n> > > allow ControlInfo to be dynamic, or properties to be dynamic. A change\n> > > in behaviour is thus required, which lead me to think we should\n> > > investigate the ControlInfo path.\n> > >\n> > > This being said, I don't want to delay this feature much longer, so I'm\n> > > fine adding a new property to report the ScalerCrop maximum value (we\n> > > could name it ScalerCropMaximum or ScalerCropBound for instance) for\n> > > now, with the property value being updated when the camera is\n> > > configured. We can then explore making ControlInfo dynamic, and if it\n> > > turns out to be a good idea, drop the ScalerCropBound in the future.\n> > >\n> > > > The ScalerCrop requested, and reported back, gives the offset and size\n> > > > of the zoom rectangle relative to the rectangle given by the property\n> > > > under discussion here (I think that's the most convenient choice). So\n> > > > if it reports back offsets of (0,0) then you know you're in the top\n> > > > left hand corner of what you could possibly ever see in this camera\n> > > > mode (but not necessarily of the sensor as a whole).\n> > > >\n> > > > I think this could work and is a fair compromise between the amount of\n> > > > information and the ease of use. Most digital zoom applications could\n> > > > simply request its Size, and go from there. Only a few minor\n> > > > mis-givings:\n> > > >\n> > > > * Does V4L2 actually give you a way of finding out where a particular\n> > > > \"camera mode\" lies in the pixel array? Do you even know if a mode is\n> > > > binned or heavily cropped... someone who knows rather more about V4L2\n> > > > needs to answer that one!\n> > >\n> > > This is exactly why we have implemented the read-only subdev API, to\n> > > allow userspace to query detailed information about the current mode.\n> > > The amount of information that can be extracted from the sensor driver\n> > > depends on how the driver models the sensor. In order to expose analog\n> > > crop, binning, skipping and digital crop separately, we would need to\n> > > expose as least two subdevs to userspace. That's the direction I want to\n> > > take, and libcamera will be able to handle that, but it means some\n> > > extra work on the kernel side to implement this in sensor drivers (it's\n> > > no rocket science though).\n> > >\n> > > > * Use of native sensor coords is OK, though a few sensors might\n> > > > support non-integer scaling at which point the property would be\n> > > > slightly \"approximate\". That seems fairly minor to me, though. (Some\n> > > > ISPs might even do non-integer cropping, so the problem can exist\n> > > > regardless.)\n> > >\n> > > If a sensor implements scaling in addition to cropping, binning and\n> > > skipping, three subdevs will be required :-) That's how the smiapp\n> > > driver is implemented today, and the new ccs driver (\"[PATCH 000/100]\n> > > CCS driver\" on the linux-media mailing list) follows the same path (it\n> > > actually replaces the smiapp driver, implementing support for both\n> > > SMIA++ and CCS).\n> > >\n> > > > * It doesn't really support my assertion that you should be able to\n> > > > know exactly what's going on, if you want to. (But if we decide this\n> > > > doesn't matter, then that's fine too.)\n> > >\n> > > It doesn't by itself, but we could then add additional properties, or\n> > > extra information in the CameraConfiguration, to report the detailed\n> > > configuration of the sensor. What I like with this approach is that not\n> > > only the two will not conflict (we will be able to add the properties or\n> > > extend CameraConfiguration without touching the digital zoom API), but\n> > > the will be no overlap between the two features either, they will each\n> > > have one dedicated purpose.\n> > >\n> > > > * In one of our early discussions we suggested that having the exact\n> > > > pixel level information might be useful, and you could do things like\n> > > > pan one pixel at a time. We lose that by going to native sensor\n> > > > coordinates. I think I said at the time that this ability feels more\n> > > > useful than I believe it really is, so I wouldn't be too upset.\n> > >\n> > > I'm not sure to follow you here, why wouldn't we be able to pan by one\n> > > pixel ? If the sensor is configured with binning/skipping an application\n> > > would have to pan by 2 pixels to achieve an effective cropping of 1\n> > > pixel, so maybe we will need to report a step, or just report\n> > > binning/skipping factors, but with the proposed API the crop rectangle\n> > > can be set to a pixel-perfect position. Or am I missing something ?\n> > >\n> > > > > While in some use cases the actual crop rectangle reported through\n> > > > > metadata would be enough (for instance, in a GUI that starts with an\n> > > > > unzoomed view, the application would get the maximum possible crop\n> > > > > rectangle in the metadata of the first frame), I can imagine we would\n> > > > > have use cases that need this information before capturing the first\n> > > >\n> > > > Indeed. And I also think you should be able to set digital zoom to\n> > > > take effect *on* the very first frame itself (this spills over into\n> > > > Naush's most recent patch set...)\n> > >\n> > > Yes, I think we should allow that, even if most use cases will likely\n> > > not need it.\n> > >\n> > > > > frame. I initially thought we could report it through the max value of\n> > > > > the ControlInfo instance for the ScalerCrop control, but this is\n> > > > > supposed to be static information. As this would be (in my opinion) a\n> > > > > neat solution, I'd like to investigate the possibility of making\n> > > > > ControlInfo dynamic, but maybe we will need a different solution. For\n> > > > > foresee the addition of an API that will create request templates,\n> > > > > filling them with default control values based on a requested use case,\n> > > > > and the maximum ScalerCrop could possibly be reported through that\n> > > > > mechanism. I'm also fine reporting it through a temporary mechanism (for\n> > > > > instance adding it to CameraConfiguration, or creating a dynamic\n> > > > > property) for the time being, if you already have pressing use cases for\n> > > > > knowing the maximum value before capturing the first frame.\n> > > > >\n> > > > > Thoughts ? :-)\n> > > >\n> > > > I'm glad we're talking about all this stuff!!\n> > > >\n> > > > > > > Two additional points I'd like to consider (and which are orthogonal to\n> > > > > > > the previous one) are:\n> > > > > > >\n> > > > > > > - Should we automatically adjust the ScalerCrop rectangle to always\n> > > > > > > output square pixels, or should we allow modifying the aspect ratio\n> > > > > > > when scaling ? Most use cases call for square pixels, but I don't\n> > > > > > > think we necessarily want to create an artificial limitation here (as\n> > > > > > > long as we make it easy for applications to compute the scaling\n> > > > > > > parameters that will give them square pixels)/\n> > > > > >\n> > > > > > Personally I see no reason to restrict what an application can request. We\n> > > > > > need to make it easy to request the right aspect ratio (hence those\n> > > > > > geometry helpers), but if people actually have a use-case for something\n> > > > > > strange...\n> > > > >\n> > > > > Fine with me.\n> > > > >\n> > > > > > > - Should we allow a ScalerCrop rectangle smaller than the stream output\n> > > > > > > size, or should we restrict scaling to down-scaling only ?\n> > > > > >\n> > > > > > I think up-scaling is probably the most common use-case for us (though\n> > > > > > downscaling will happen too). Think of all those (rubbish) 30x zoom\n> > > > > > pictures that some phones like to produce...!\n> > > > >\n> > > > > Rubbish is exactly the work I would have used :-) I'm tempted to try and\n> > > > > teach users that they shouldn't do this by disabling this feature, but\n> > > > > that would be pretentious. I suppose there's no good reason to forbid\n> > > > > this. Should we put a limit on the upscaling factor though ?\n> > > >\n> > > > Always tricky. To what extent should we save people from themselves?\n> > > > I'm pretty sure the Pi imposes some kind of a limit, but don't really\n> > > > know. In the end, so long as it doesn't blow up, that's kind of OK...\n> > >\n> > > Maybe we'll introduce the concept of soft and hard limits in the future,\n> > > with the soft limit being what the pipeline handler recommends not to\n> > > exceed to guarantee \"reasonable\" quality, and the hard limit being what\n> > > you can get out of the hardware. If someone finds a good use case for\n> > > x100 digital zoom, I'll be curious to hear about it. We unfortunately\n> > > don't have the same ISPs as in those movies where a 4 pixels detail in\n> > > an image can be zoomed in full screen :-)\n> > >\n> > > > > > > > David Plowman (6):\n> > > > > > > > libcamera: Add SensorOutputSize property\n> > > > > > > > libcamera: Initialise the SensorOutputSize property\n> > > > > > > > libcamera: Add IspCrop control\n> > > > > > > > libcamera: Add geometry helper functions\n> > > > > > > > libcamera: pipeline: raspberrypi: Implementation of digital zoom\n> > > > > > > > cam: Add command line option to test IspCrop control\n> > > > > > > >\n> > > > > > > > include/libcamera/geometry.h | 20 +++\n> > > > > > > > include/libcamera/ipa/raspberrypi.h | 1 +\n> > > > > > > > src/cam/capture.cpp | 25 +++-\n> > > > > > > > src/cam/capture.h | 2 +-\n> > > > > > > > src/cam/main.cpp | 3 +\n> > > > > > > > src/cam/main.h | 1 +\n> > > > > > > > src/ipa/raspberrypi/raspberrypi.cpp | 7 +\n> > > > > > > > src/libcamera/camera_sensor.cpp | 6 +\n> > > > > > > > src/libcamera/control_ids.yaml | 12 ++\n> > > > > > > > src/libcamera/geometry.cpp | 129 ++++++++++++++++++\n> > > > > > > > .../pipeline/raspberrypi/raspberrypi.cpp | 47 +++++++\n> > > > > > > > src/libcamera/property_ids.yaml | 19 +++\n> > > > > > > > 12 files changed, 269 insertions(+), 3 deletions(-)\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 9D83FBDB13\n\tfor ;\n\tFri, 16 Oct 2020 07:22:01 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 254266108B;\n\tFri, 16 Oct 2020 09:22:01 +0200 (CEST)","from mail-oo1-xc31.google.com (mail-oo1-xc31.google.com\n\t[IPv6:2607:f8b0:4864:20::c31])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C965160530\n\tfor ;\n\tFri, 16 Oct 2020 09:21:58 +0200 (CEST)","by mail-oo1-xc31.google.com with SMTP id f19so391954oot.4\n\tfor ;\n\tFri, 16 Oct 2020 00:21:58 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"GRKcPD/F\"; 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=T8KajUb8sJ7BcaX0L0EHeqmRqYodd0EJ1iS6zRRU6rU=;\n\tb=GRKcPD/FVljS4g6/YOI3nw5++cJ4e/1eov29tKhQBYFCLzT0MWtmcwEXKHMnRBShjM\n\tAFsMy6XrWjd5WFgqk145V8oPCQHi18Map4DAq0OVN2tMD0q4C7IewkG3rejR7UHMgTFx\n\t2s4JCyhSs3BP16aViz/Iv9cH+bYqm77Vt7J3ZdpF6YhjA9rQlghuByR+Glc4WpajDoIe\n\tzHqmrfErzcuhjP1QwMzk7x3W8AnBjyOFxPDpgea9RChZtkK6bFIKQzmLIvm6tRu0EDPI\n\tEuQcaq9TDaQDXekV0lc8BIZLM68Zqu57HOL45Mf+cBpsSNb2H5A/vJMxiIkjHH2JlK50\n\tJ5OA==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=T8KajUb8sJ7BcaX0L0EHeqmRqYodd0EJ1iS6zRRU6rU=;\n\tb=c7nJmVoEcIOKWez3ZQBepJ88OUoklAIsnlOe6skAbWt61v/sAbGm1anObVtq2K0WZf\n\to/7jg8PL92GtE9N0MNLB1xQDQVanbBVMql3hS3brHDDULw75waCDlrS85Ip6m70+5NbG\n\tLUALRAhJzq9U7dGCvdbnyCcnlAjG9gD6NsaAxdEqlOqNzrPeSyvIGzvR7da59fa5nnMq\n\t70tXJ0p0w29VzLN9NggkV0CysBbO8SmHeSqV6pyGrOaYWX5uFY2ImZg100SUbk2NFd3Z\n\tzJG3XoE7Oc/nP5415An7vSFAOclgWWStB1UlA4g6vWfTUNqfxhuIOZGP/9EyXhklPzu6\n\tHF5Q==","X-Gm-Message-State":"AOAM533AnQm6CJTRYte9Xq+J18t7ObOqeIqVic5whc6mYsDHapE9LGyc\n\t1uneS/Gubo7XKwd9Ef9Cs4L+Wq5kCKimBiY3kv/qig==","X-Google-Smtp-Source":"ABdhPJw3ddCg+UX8eFgPl+qaGAeQUNmGjZ1Qj0YTHEyc6oGd/2zHUSI5BMWIqoFQsTW4EtZZjSG2BJ9Afr/HYIggrDM=","X-Received":"by 2002:a4a:7656:: with SMTP id w22mr1686914ooe.72.1602832916732;\n\tFri, 16 Oct 2020 00:21:56 -0700 (PDT)","MIME-Version":"1.0","References":"<20200929164000.15429-1-david.plowman@raspberrypi.com>\n\t<20201011214708.GA3944@pendragon.ideasonboard.com>\n\t\n\t<20201013014422.GD3942@pendragon.ideasonboard.com>\n\t\n\t<20201015021650.GH3858@pendragon.ideasonboard.com>\n\t\n\t<20201016063048.GJ3829@pendragon.ideasonboard.com>","In-Reply-To":"<20201016063048.GJ3829@pendragon.ideasonboard.com>","From":"David Plowman ","Date":"Fri, 16 Oct 2020 08:21:45 +0100","Message-ID":"","To":"Laurent Pinchart ","Subject":"Re: [libcamera-devel] [PATCH v3 0/6] Digital zoom","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 ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":13240,"web_url":"https://patchwork.libcamera.org/comment/13240/","msgid":"<20201016134804.GM3829@pendragon.ideasonboard.com>","date":"2020-10-16T13:48:04","subject":"Re: [libcamera-devel] [PATCH v3 0/6] Digital zoom","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi David,\n\nOn Fri, Oct 16, 2020 at 08:21:45AM +0100, David Plowman wrote:\n> Hi Laurent\n> \n> Thanks for the explanation, indeed I think that does make things clearer.\n\nPlease feel free to tell me if anything doesn't make sense, or is just a\nbad idea :-)\n\n> So the next step is probably now for me to modify my implementation\n> and we can take the discussion from there once we have something\n> concrete. I'll try and send it round early next week.\n\nI'll do my best to review it without delay. It's converging, I think\nwe'll be able to merge it soon.\n\n> On Fri, 16 Oct 2020 at 07:31, Laurent Pinchart wrote:\n> > On Thu, Oct 15, 2020 at 03:30:56PM +0100, David Plowman wrote:\n> > > Hi Laurent\n> > >\n> > > Thanks for all that! The discussion is getting rather long so perhaps\n> > > I can try to wrap up all the points in the thread and sum it up at the\n> > > top here with an outline of the plan as I understand it!\n> >\n> > Thank you for summarizing this.\n> >\n> > > 1. ScalerCrop control\n> > >\n> > > This will be a Rectangle indicating the crop for digital zoom. The\n> > > units will be native pixel coordinates. The (x,y) offsets should, I\n> > > think, start at (0,0) because the \"top-left-most\" thing you can have\n> > > should probably be signalled with (0,0) - but see below.\n> >\n> > I think the ScalerCrop control should have the same unit and the same\n> > reference as the ScalerCropMaximum property. ScalerCropMaximum is meant\n> > to convey the largest possible crop rectangle, so it should be valid to\n> > set ScalerCrop = ScalerCropMaximum. As ScalerCropMaximum will typically\n> > have (x,y) > (0,0) (unless the ISP doesn't need to crop at all for\n> > internal reasons), I don't think we should consider that ScalerCrop\n> > starts at (0,0).\n> >\n> > The alternative is to express ScalerCrop relatively to\n> > ScalerCropMaximum, but in that case ScalerCropMaximum wouldn't be the\n> > maximum value for the ScalerCrop control anymore. In order to later\n> > replace\n> >\n> > camera->properties().at(properties::ScalerCropMaximum)\n> >\n> > with\n> >\n> > camera->controls().at(controls::ScalerCrop).max()\n> >\n> > we need to express the two with the same reference.\n> >\n> > > 2. ScalerCropMaximum property\n> > >\n> > > This indicates the maximum image area from which we can crop, again in\n> > > native pixel coordinates. It could be a Rectangle and we could use the\n> > > (x,y) offsets perhaps to indicate the top left coordinates in the\n> > > sensor - i.e. this would (mostly) be the analogCrop then. (Or one\n> > > could leave them as zero, in which case you might as well leave this\n> > > as a Size.)\n> >\n> > The idea is that ScalerCropMaximum is indeed expressed similarly to an\n> > analog crop rectangle, but its meaning is different. The coordinate\n> > system is the sensor's pixel array active area, but the value takes into\n> > account mandatory cropping performed in all stages of the pipeline,\n> > including the ISP.\n> >\n> > > We've talked about making this a \"dynamic\" ControlInfo maximum, though\n> > > it's not quite the same thing. For example, it would typically have\n> > > the \"wrong\" aspect ratio so taking and setting the \"maximum\" would\n> > > give you a weird image - would a user expect that? Also, the meaning\n> > > of the (x,y) offsets as described for the control does not align as\n> > > things stand.\n> >\n> > Setting ScalerCrop = ScalerCropMaximum would indeed give the wrong\n> > aspect ratio. We will however have nice geometry helpers (thank you for\n> > your work on that) that should make it easy to get the right behaviour.\n> >\n> > > 3. Other stuff...\n> > >\n> > > * With this scheme there's no way for an application to know what the\n> > > true pixel scale for the ScalerCrop is. Maybe that kind of information\n> > > gets exposed later. (I seem to remember wanting to expose the whole\n> > > CameraSensorInfo several months ago...)\n> >\n> > I'm not sure to follow you here. With coordinates for ScalerCrop and\n> > ScalerCropMaximum being sensor pixels, the true pixel scale will simply\n> > be the ratio between the stream output size and ScalerCrop.size(), won't\n> > it ?\n> >\n> > > * There's no real allowance for any cropping after binning/scaling.\n> > > Maybe you have to adjust the ScalerCropMaximum to make it look like\n> > > all the cropping is happening up front? TBD, I think.\n> >\n> > Maybe this is where we haven't understood each other properly.\n> > ScalerCropMaximum (and thus ScalerCrop) are expressed in sensor pixel\n> > coordinates, as if all cropping was applied before binning and skipping,\n> > but they include all cropping operations through the whole pipeline.\n> >\n> > > * Pipeline handlers will all have to go around rescaling the\n> > > ScalerCrop rectangles from native to actual pixel coordinates.\n> >\n> > Correct, in order to compute the real scaler input crop rectangle,\n> > pipeline handlers will need to divide ScalerCrop by the binning/skipping\n> > factors, and subtract the internal crop that is performed in other parts\n> > of the pipeline. Let's consider a few examples.\n> >\n> > Let's assume a sensor native resolution of 4056x3040 active pixels\n> > (non-active and optical dark pixels are ignored in this example). Let's\n> > further assume that the ISP, for internal reasons, consumes 4 lines and\n> > columns on each side of the image.\n> >\n> > For the first example, let's assume that the user wants to capture the\n> > full image. No binning, skipping or cropping is applied in the sensor.\n> > Due to ISP cropping, the pipeline handler should restrict the output\n> > size to 4048x3032 in order to keep pixels square, as upscaling 4048x3032\n> > to 4056x3040 will modify the aspect ratio slightly. The\n> > ScalerCropMaximum property will be reported as (4,4)-4048x3032, and the\n> > default ScalerCrop value will be (4,4)-4048x3032. Applications can\n> > select a smaller ScalerCrop rectangle, and they will be responsible for\n> > ensuring that its size matches the aspect ratio of the output image\n> > (4048x3032) if they want to keep pixels square.\n> >\n> > In a second example, let's assume the user sets the stream output size\n> > to 1920x1080. The pipeline handler decides to enable binning on the\n> > sensor side, but without applying any cropping on the sensor. The sensor\n> > output is thus 2028x1520. Due to the ISP internal cropping of 4 pixels\n> > on each side, the image at the scaler input will be at most 2020x1512.\n> > Scaling this back to pixel array coordinates, the ScalerCropMaximum will\n> > be reported as (8,8)-4040x3024. Note how it is slightly smaller than in\n> > the previous example. The pipeline handler will furthermore set the\n> > actual scaler crop rectangle, expressed in scaler input coordinates\n> > (thus relative to a 2020x1512 size) to (2,189)-2016x1134 to get a 16:9\n> > aspect ratio (it could also set it to (0,188)-2020x1136 to use the full\n> > width, but the aspect ratio would then be slightly off as 2020 isn't a\n> > multiple of 16). Scaling this back to the pixel array coordinates, the\n> > default ScalerCrop rectangle reported to the application is\n> > (12,386)-4032x2268. Applications will be able to pan within the\n> > (8,8)-4040x3024 maximum crop rectangle, and possibly scaling a ~4:3\n> > image to 16:9 if they set ScalerCrop = ScalerCropMaximum.\n> >\n> > In a third example, with the same 1920x1080 output resolution, the\n> > pipeline handler may decide to crop on the sensor, for instance cropping\n> > the binned 2028x1520 output to (50,216)-1928x1088. The scaler input\n> > size, after the ISP internal cropping, will be 1920x1080, matching the\n> > stream output size. The image doesn't need to be scaled, but the field\n> > of view is reduced. In this case, the ScalerCropMaximum will be reported\n> > as (108,440)-3840x2160, and the default ScalerCrop value will be set to\n> > (108,440)-3840x2160 as well. Applications will be able to pan within the\n> > (108,440)-3840x2160 maximum crop rectangle, but unlike in the previous\n> > example, they won't be able to pan outside of the default, as panning is\n> > only possible in the scaler, the sensor crop rectangle is fixed.\n> >\n> > Do these examples help clarifying the proposal ?\n> >\n> > > Maybe there's another geometry helper function there: Rectangle\n> > > Rectangle::rescaledTo(const Size &from, const Size &to)\n> > > e.g. actualCrop = nativeCrop.rescaledTo(sensorInfo.analogCrop.size(),\n> > > sensorInfo.outputSize)\n> > >\n> > > Please correct me if I've got any of that wrong. Overall I can't quite\n> > > escape the nagging feeling that there should be better ways of getting\n> > > all that information about the camera mode to an application, but\n> > > whatever mechanism we use now can be improved upon later. The basic\n> > > ScalerCrop control would not be affected, I think.\n> > >\n> > > On Thu, 15 Oct 2020 at 03:17, Laurent Pinchart wrote:\n> > > > On Tue, Oct 13, 2020 at 09:36:50AM +0100, David Plowman wrote:\n> > > > > On Tue, 13 Oct 2020 at 02:45, Laurent Pinchart wrote:\n> > > > > > On Mon, Oct 12, 2020 at 07:49:31PM +0100, David Plowman wrote:\n> > > > > > > Hi Laurent\n> > > > > > >\n> > > > > > > Thanks for the comments. Some interesting points.\n> > > > > >\n> > > > > > Sorry about chiming in so late and challenging some of the base\n> > > > > > assumptions though. I'll now try not to introduce any further delay.\n> > > > > >\n> > > > > > > On Sun, 11 Oct 2020 at 22:47, Laurent Pinchart wrote:\n> > > > > > >\n> > > > > > > > Hi David,\n> > > > > > > >\n> > > > > > > > Thank you for the patches. And more than sorry for reviewing the series\n> > > > > > > > so late.\n> > > > > > > >\n> > > > > > > > On Tue, Sep 29, 2020 at 05:39:54PM +0100, David Plowman wrote:\n> > > > > > > > > Hi everyone\n> > > > > > > > >\n> > > > > > > > > Despite not originally intending to, I've actually made a version 3 of\n> > > > > > > > > the digital zoom patches, just to take care of a few things that might\n> > > > > > > > > be a bit annoying otherwise.\n> > > > > > > > >\n> > > > > > > > > 1. I've improved the description of the IspCrop control as was\n> > > > > > > > > suggested.\n> > > > > > > > >\n> > > > > > > > > 2. I've improved the description of the zoom option in cam (if we\n> > > > > > > > > decide to use this patch!), also as was proposed.\n> > > > > > > > >\n> > > > > > > > > 3. There was actually a problem with the \"{}\" syntax to denote zero\n> > > > > > > > > Rectangles in include/libcamera/ipa/raspberrypi.h. They were failing a\n> > > > > > > > > range type test in ControlInfoMap::generateIdmap() and so the control\n> > > > > > > > > wasn't working. I've replaced \"{}\" by \"Rectangle{}\" which seems OK.\n> > > > > > > > >\n> > > > > > > > > 4. There's been a bit of churn in the RPi pipeline handler lately so\n> > > > > > > > > rebasing gave some conflicts. I've fixed those up.\n> > > > > > > > >\n> > > > > > > > > Otherwise everything else remains the same.\n> > > > > > > >\n> > > > > > > > I've gone through the different versions of this series, and the first\n> > > > > > > > thing I want to see is that it has improved very nicely over time. Nice\n> > > > > > > > job !\n> > > > > > > >\n> > > > > > > > Configuring zoom through an absolute crop rectangle is I believe the way\n> > > > > > > > to go, so the overall approach is good in my opinion. There are however\n> > > > > > > > a few points I'd like to discuss, related to the SensorOutputSize\n> > > > > > > > property and IspCrop control.\n> > > > > > > >\n> > > > > > > > Reading through the discussion, I become increasingly aware of a topic\n> > > > > > > > that was present in the background without ever being named, and that's\n> > > > > > > > the camera pipeline model. As mentioned yesterday in a few other replies\n> > > > > > > > to selected patches in this series and its previous versions, the job of\n> > > > > > > > libcamera is to expose to applications a camera model that abstract\n> > > > > > > > hardware differences. This is not an easy job, as we have to strike the\n> > > > > > > > right balance between a higher-level inflexible but simple to use model\n> > > > > > > > that can't support many of the less common use cases, and a lower-level\n> > > > > > > > powerful but complex to use model that exposes a very large range of\n> > > > > > > > hardware features. We have implicitly defined the skeleton of such a\n> > > > > > > > model through the API of the Camera and PipelineHandler classes, but\n> > > > > > > > have never made it explicit.\n> > > > > > > >\n> > > > > > > > This needs to change. I see no reason to block the digital zoom feature\n> > > > > > > > until we finish documenting the pipeline model, but I would like to\n> > > > > > > > design the feature while thinking about the bigger picture. Here are the\n> > > > > > > > assumptions I think the pipeline model should make for devices that\n> > > > > > > > support digital zoom.\n> > > > > > > >\n> > > > > > > > - The camera pipeline starts with a camera sensor, that may implement\n> > > > > > > > binning, skipping and/or cropping.\n> > > > > > > >\n> > > > > > > > - The subsequent blocks in the pipeline may perform additional cropping,\n> > > > > > > > either at the direct command of the pipeline handler (e.g. cropping at\n> > > > > > > > the input of the scaler), or automatically to support image processing\n> > > > > > > > steps (e.g. colour interpoloation often drops a few lines and columns\n> > > > > > > > on all edges of the image).\n> > > > > > > >\n> > > > > > > > - The pipeline ends with a scaler that can implement digital zoom\n> > > > > > > > through a combination of cropping followed by scaling.\n> > > > > > > >\n> > > > > > > > The exact order of the processing steps at the hardware level doesn't\n> > > > > > > > need to match the pipeline model. For instance, cropping at the input\n> > > > > > > > and output of the scaler are interchangeable (not taking into account\n> > > > > > > > sub-pixel differences). It doesn't matter if the ISP scales before\n> > > > > > > > cropping the output, the hardware scaler parameters and output crop\n> > > > > > > > rectangle can be computed from an abstract input crop rectangle and\n> > > > > > > > output size. This is crucial to consider for the definition of the\n> > > > > > > > pipeline model: we need to design it in a way that ensures all features\n> > > > > > > > can be mapped to how they are implemented in the different types of\n> > > > > > > > hardware we want to support, but we're otherwise free to not map\n> > > > > > > > controls and properties 1:1 with the hardware parameters. When multiple\n> > > > > > > > options are possible, we should be guided by API design criteria such as\n> > > > > > > > coherency, simplicity and flexibility.\n> > > > > > > >\n> > > > > > > > Coming back to digital zoom, this series exposes a new SensorOutputSize\n> > > > > > > > property and a new IspCrop control. The SensorOutpuSize property is\n> > > > > > > > introduced to support the IspCrop control, as the base rectangle in\n> > > > > > > > which the scaler can crop. There are two issues here that bother me:\n> > > > > > > >\n> > > > > > > > - The property, despite being named SensorOutputSize, potentially takes\n> > > > > > > > into account the cropping added by the CSI-2 receiver and by the ISP\n> > > > > > > > for operations that consume lines and columns on the edges of the\n> > > > > > > > image. The naming can create some confusion, which can possibly be\n> > > > > > > > addressed by a combination of documentation (you're covering that\n> > > > > > > > already) and possibly a more explicit name for the property. However,\n> > > > > > > > as the property bundles crop operations perfomed in different stages\n> > > > > > > > of the pipeline, I'm worried that it will turn out to be a bit\n> > > > > > > > ill-defined regardless of how well we document it, with slightly\n> > > > > > > > different behaviours in different implementations.\n> > > > > > > >\n> > > > > > > > - Ignoring the additional cropping performed in the CSI-2 receiver and\n> > > > > > > > ISP (if any), the property exposes the sensor binning, skipping and\n> > > > > > > > cropping. It bundles those three operations together, and thus doesn't\n> > > > > > > > convey how the cropping affects the field of view as a given output\n> > > > > > > > size can be achieved with different combinations of skipping/binning\n> > > > > > > > and cropping.\n> > > > > > > >\n> > > > > > > > For these reasons, I'm concerned that the SensorOutputCrop property is a\n> > > > > > > > ad-hoc solution to provide a reference for the IspCrop property, and\n> > > > > > > > will not fit clearly in a pipeline model that should be based on simple,\n> > > > > > > > base building blocks. I would thus like to propose an alternative\n> > > > > > > > option.\n> > > > > > > >\n> > > > > > > > Instead of expressing the IspCrop controls (which I think we should\n> > > > > > > > rename to ScalerCrop) relatively to the SensorOutputSize property, could\n> > > > > > > > we express it relatively to the existing PixelArrayActiveAreas property\n> > > > > > > > ? This would have the advantage, in my opinion, of abstracting binning\n> > > > > > > > and skipping from applications. The pipeline handlers would need to\n> > > > > > > > perform a bit more work to compute the crop rectangle actually applied\n> > > > > > > > to the scaler, in order to take sensor binning/skipping and all\n> > > > > > > > additional cropping in the pipeline into account. The upside is that the\n> > > > > > > > ScalerCrop will directly define how the field of view is affected. It\n> > > > > > > > would also simplify the API, as no intermediate property between\n> > > > > > > > PixelArrayActiveAreas and ScalerCrop would need to be defined, and the\n> > > > > > > > ScalerCrop coordinates wouldn't depend on the active camera\n> > > > > > > > configuration. I think this would be easier to clearly document as part\n> > > > > > > > of a camera pipeline model.\n> > > > > > >\n> > > > > > > Renaming IspCrop to ScalerCrop sounds fine to me. It has had so many\n> > > > > > > different names!\n> > > > > > >\n> > > > > > > I'm less sure about trying to derive the SensorOutputSize (or\n> > > > > > > ScalerInputSize or whatever else we want to call it!) from the\n> > > > > > > PixelArrayActiveAreas property. Let me try and take a step back.\n> > > > > > >\n> > > > > > > So first, I think knowing the PixelArrayActiveArea isn't enough. How would\n> > > > > > > you know if the pipeline handler was doing some extra cropping that wasn't\n> > > > > > > \"strictly necessary\", perhaps to reduce memory traffic, or for a faster\n> > > > > > > framerate. How would the application know not to try and zoom there? It\n> > > > > > > seems to me that this really is a decision for the pipeline handler based\n> > > > > > > on the sensor driver, it isn't available from the properties of the sensor\n> > > > > > > itself.\n> > > > > > >\n> > > > > > > Actually I'd quite like to leave the discussion there for the moment and\n> > > > > > > see if that much is controversial or not. Of course we then have to move on\n> > > > > > > but maybe let's see what we think about that first...\n> > > > > > >\n> > > > > > > Thoughts??\n> > > > > >\n> > > > > > I fully agree with you that the pipeline handler has the last word when\n> > > > > > it comes to the (combined) internal cropping. It may not have any choice\n> > > > > > (if the CFA interpolation eats two or four lines and columns on each\n> > > > > > side of the image due to the hardware implementation, there's nothing\n> > > > > > that can be done against it), or may just want to decide on policies for\n> > > > > > whatever reason it sees fit (while trying not to over-police though, to\n> > > > > > leave options to applications).\n> > > > > >\n> > > > > > My proposal doesn't deny this from the pipeline handler, but tries to\n> > > > > > express the crop rectangle in a way that maps directly to the field of\n> > > > > > view. To do so, binning/skipping would be hidden from applications by\n> > > > > > using a reference for the ScalerCrop property that has no\n> > > > > > binning/skipping applied.\n> > > > >\n> > > > > So we certainly agree on the necessity for \"something\", that's good,\n> > > > > and then it's a case of figuring out what \"something\" is. I have\n> > > > > several conflicting thoughts here (I frequently disagree with myself):\n> > > > >\n> > > > > * Actually, as a basic principle, I think applications should be able\n> > > > > to find out \"the truth\" if they want it. This would mean the sensor\n> > > > > crop, binning/scaling, and any subsequent cropping.\n> > > >\n> > > > I agree. I can even foresee that a low-level API to control the sensor\n> > > > parameters explicitly could be useful in some advanced cases. That\n> > > > should be an add-on though, it shouldn't cause the main API to be more\n> > > > convoluted.\n> > > >\n> > > > > * I even wonder sometimes whether there shouldn't really be a class\n> > > > > that describes how the camera mode has been derived from the sensor,\n> > > > > with all this information made explicit. But I think that leads us\n> > > > > even deeper into the woods, and I suspect we don't really want to go\n> > > > > there! (Also, I'm not sure if the V4L2 API would even let you discover\n> > > > > all of it, which might be a problem...)\n> > > >\n> > > > I think we will go there :-) I expect sensors to require more complex\n> > > > configurations in the future, as the MIPI CCS spec gets adopted. The\n> > > > kernel driver will expose 2 or 3 subdevs, and we will have a class to\n> > > > abstract that in libcamera to make it easier for pipeline handlers.\n> > > >\n> > > > > * But I also think digital zoom needs to be easy to use for\n> > > > > applications that don't care about all these details (\"the truth\", as\n> > > > > I called it!). Actually I feel the overwhelming majority of\n> > > > > applications will fall into this category.\n> > > >\n> > > > We're on the same page, good :-)\n> > > >\n> > > > > > The pipeline handler would restrict the requested ScalerCrop control\n> > > > > > based on the internal cropping that is always applied (combining sensor,\n> > > > > > CSI-2 receiver and ISP), and report the result through the request\n> > > > > > metadata. The application would thus know the exact crop rectangle that\n> > > > > > has been used, in unscaled (binning/skipping) sensor coordinates.\n> > > > >\n> > > > > Let me try and paraphrase a bit and you can tell me if I've got it wrong.\n> > > > >\n> > > > > You're proposing a property (details in a moment) that can be used to\n> > > > > choose zoom regions for the ScalerCrop control.\n> > > > >\n> > > > > The units for this property are the native sensor pixels, ignoring any\n> > > > > binning/scaling that might be going on.\n> > > > >\n> > > > > The property would give you a Rectangle that expresses exactly the\n> > > > > area in which you are permitted to zoom? The offset of the rectangle\n> > > > > tells you where in the sensor array this rectangle actually lies.\n> > > > >\n> > > > > The term \"sensor array\" there needs some clarification. I think it\n> > > > > probably means the \"PixelArraySize\" less any invalid or optical black\n> > > > > pixels. (Do we have a way to get hold of that easily?)\n> > > >\n> > > > I think we should use PixelArrayActiveAreas, while we may be able to\n> > > > capture dark pixels with some sensors, I think they're out of scope for\n> > > > digital zoom.\n> > > >\n> > > > My proposal initially didn't include a new property to express the area\n> > > > in which we can zoom. I was considering reporting that through the\n> > > > ScalerCrop maximum value. Every control exposed by a pipeline handler\n> > > > has a ControlInfo instance associated with it, to report the minimum,\n> > > > default and maximum values. I was thinking about reporting the area in\n> > > > which we can zoom as the maximum value of the ControlInfo for the\n> > > > ScalerCrop control. This would give applications all the information\n> > > > they need, without the need to introduce a new property.\n> > > >\n> > > > The issue with this is that ControlInfo is currently static, so we can't\n> > > > change the ScalerCrop reported maximum value when the camera is\n> > > > configured. On the other hand, properties are supposed to be static too.\n> > > > As we need to report the maximum dynamically, we will need to either\n> > > > allow ControlInfo to be dynamic, or properties to be dynamic. A change\n> > > > in behaviour is thus required, which lead me to think we should\n> > > > investigate the ControlInfo path.\n> > > >\n> > > > This being said, I don't want to delay this feature much longer, so I'm\n> > > > fine adding a new property to report the ScalerCrop maximum value (we\n> > > > could name it ScalerCropMaximum or ScalerCropBound for instance) for\n> > > > now, with the property value being updated when the camera is\n> > > > configured. We can then explore making ControlInfo dynamic, and if it\n> > > > turns out to be a good idea, drop the ScalerCropBound in the future.\n> > > >\n> > > > > The ScalerCrop requested, and reported back, gives the offset and size\n> > > > > of the zoom rectangle relative to the rectangle given by the property\n> > > > > under discussion here (I think that's the most convenient choice). So\n> > > > > if it reports back offsets of (0,0) then you know you're in the top\n> > > > > left hand corner of what you could possibly ever see in this camera\n> > > > > mode (but not necessarily of the sensor as a whole).\n> > > > >\n> > > > > I think this could work and is a fair compromise between the amount of\n> > > > > information and the ease of use. Most digital zoom applications could\n> > > > > simply request its Size, and go from there. Only a few minor\n> > > > > mis-givings:\n> > > > >\n> > > > > * Does V4L2 actually give you a way of finding out where a particular\n> > > > > \"camera mode\" lies in the pixel array? Do you even know if a mode is\n> > > > > binned or heavily cropped... someone who knows rather more about V4L2\n> > > > > needs to answer that one!\n> > > >\n> > > > This is exactly why we have implemented the read-only subdev API, to\n> > > > allow userspace to query detailed information about the current mode.\n> > > > The amount of information that can be extracted from the sensor driver\n> > > > depends on how the driver models the sensor. In order to expose analog\n> > > > crop, binning, skipping and digital crop separately, we would need to\n> > > > expose as least two subdevs to userspace. That's the direction I want to\n> > > > take, and libcamera will be able to handle that, but it means some\n> > > > extra work on the kernel side to implement this in sensor drivers (it's\n> > > > no rocket science though).\n> > > >\n> > > > > * Use of native sensor coords is OK, though a few sensors might\n> > > > > support non-integer scaling at which point the property would be\n> > > > > slightly \"approximate\". That seems fairly minor to me, though. (Some\n> > > > > ISPs might even do non-integer cropping, so the problem can exist\n> > > > > regardless.)\n> > > >\n> > > > If a sensor implements scaling in addition to cropping, binning and\n> > > > skipping, three subdevs will be required :-) That's how the smiapp\n> > > > driver is implemented today, and the new ccs driver (\"[PATCH 000/100]\n> > > > CCS driver\" on the linux-media mailing list) follows the same path (it\n> > > > actually replaces the smiapp driver, implementing support for both\n> > > > SMIA++ and CCS).\n> > > >\n> > > > > * It doesn't really support my assertion that you should be able to\n> > > > > know exactly what's going on, if you want to. (But if we decide this\n> > > > > doesn't matter, then that's fine too.)\n> > > >\n> > > > It doesn't by itself, but we could then add additional properties, or\n> > > > extra information in the CameraConfiguration, to report the detailed\n> > > > configuration of the sensor. What I like with this approach is that not\n> > > > only the two will not conflict (we will be able to add the properties or\n> > > > extend CameraConfiguration without touching the digital zoom API), but\n> > > > the will be no overlap between the two features either, they will each\n> > > > have one dedicated purpose.\n> > > >\n> > > > > * In one of our early discussions we suggested that having the exact\n> > > > > pixel level information might be useful, and you could do things like\n> > > > > pan one pixel at a time. We lose that by going to native sensor\n> > > > > coordinates. I think I said at the time that this ability feels more\n> > > > > useful than I believe it really is, so I wouldn't be too upset.\n> > > >\n> > > > I'm not sure to follow you here, why wouldn't we be able to pan by one\n> > > > pixel ? If the sensor is configured with binning/skipping an application\n> > > > would have to pan by 2 pixels to achieve an effective cropping of 1\n> > > > pixel, so maybe we will need to report a step, or just report\n> > > > binning/skipping factors, but with the proposed API the crop rectangle\n> > > > can be set to a pixel-perfect position. Or am I missing something ?\n> > > >\n> > > > > > While in some use cases the actual crop rectangle reported through\n> > > > > > metadata would be enough (for instance, in a GUI that starts with an\n> > > > > > unzoomed view, the application would get the maximum possible crop\n> > > > > > rectangle in the metadata of the first frame), I can imagine we would\n> > > > > > have use cases that need this information before capturing the first\n> > > > >\n> > > > > Indeed. And I also think you should be able to set digital zoom to\n> > > > > take effect *on* the very first frame itself (this spills over into\n> > > > > Naush's most recent patch set...)\n> > > >\n> > > > Yes, I think we should allow that, even if most use cases will likely\n> > > > not need it.\n> > > >\n> > > > > > frame. I initially thought we could report it through the max value of\n> > > > > > the ControlInfo instance for the ScalerCrop control, but this is\n> > > > > > supposed to be static information. As this would be (in my opinion) a\n> > > > > > neat solution, I'd like to investigate the possibility of making\n> > > > > > ControlInfo dynamic, but maybe we will need a different solution. For\n> > > > > > foresee the addition of an API that will create request templates,\n> > > > > > filling them with default control values based on a requested use case,\n> > > > > > and the maximum ScalerCrop could possibly be reported through that\n> > > > > > mechanism. I'm also fine reporting it through a temporary mechanism (for\n> > > > > > instance adding it to CameraConfiguration, or creating a dynamic\n> > > > > > property) for the time being, if you already have pressing use cases for\n> > > > > > knowing the maximum value before capturing the first frame.\n> > > > > >\n> > > > > > Thoughts ? :-)\n> > > > >\n> > > > > I'm glad we're talking about all this stuff!!\n> > > > >\n> > > > > > > > Two additional points I'd like to consider (and which are orthogonal to\n> > > > > > > > the previous one) are:\n> > > > > > > >\n> > > > > > > > - Should we automatically adjust the ScalerCrop rectangle to always\n> > > > > > > > output square pixels, or should we allow modifying the aspect ratio\n> > > > > > > > when scaling ? Most use cases call for square pixels, but I don't\n> > > > > > > > think we necessarily want to create an artificial limitation here (as\n> > > > > > > > long as we make it easy for applications to compute the scaling\n> > > > > > > > parameters that will give them square pixels)/\n> > > > > > >\n> > > > > > > Personally I see no reason to restrict what an application can request. We\n> > > > > > > need to make it easy to request the right aspect ratio (hence those\n> > > > > > > geometry helpers), but if people actually have a use-case for something\n> > > > > > > strange...\n> > > > > >\n> > > > > > Fine with me.\n> > > > > >\n> > > > > > > > - Should we allow a ScalerCrop rectangle smaller than the stream output\n> > > > > > > > size, or should we restrict scaling to down-scaling only ?\n> > > > > > >\n> > > > > > > I think up-scaling is probably the most common use-case for us (though\n> > > > > > > downscaling will happen too). Think of all those (rubbish) 30x zoom\n> > > > > > > pictures that some phones like to produce...!\n> > > > > >\n> > > > > > Rubbish is exactly the work I would have used :-) I'm tempted to try and\n> > > > > > teach users that they shouldn't do this by disabling this feature, but\n> > > > > > that would be pretentious. I suppose there's no good reason to forbid\n> > > > > > this. Should we put a limit on the upscaling factor though ?\n> > > > >\n> > > > > Always tricky. To what extent should we save people from themselves?\n> > > > > I'm pretty sure the Pi imposes some kind of a limit, but don't really\n> > > > > know. In the end, so long as it doesn't blow up, that's kind of OK...\n> > > >\n> > > > Maybe we'll introduce the concept of soft and hard limits in the future,\n> > > > with the soft limit being what the pipeline handler recommends not to\n> > > > exceed to guarantee \"reasonable\" quality, and the hard limit being what\n> > > > you can get out of the hardware. If someone finds a good use case for\n> > > > x100 digital zoom, I'll be curious to hear about it. We unfortunately\n> > > > don't have the same ISPs as in those movies where a 4 pixels detail in\n> > > > an image can be zoomed in full screen :-)\n> > > >\n> > > > > > > > > David Plowman (6):\n> > > > > > > > > libcamera: Add SensorOutputSize property\n> > > > > > > > > libcamera: Initialise the SensorOutputSize property\n> > > > > > > > > libcamera: Add IspCrop control\n> > > > > > > > > libcamera: Add geometry helper functions\n> > > > > > > > > libcamera: pipeline: raspberrypi: Implementation of digital zoom\n> > > > > > > > > cam: Add command line option to test IspCrop control\n> > > > > > > > >\n> > > > > > > > > include/libcamera/geometry.h | 20 +++\n> > > > > > > > > include/libcamera/ipa/raspberrypi.h | 1 +\n> > > > > > > > > src/cam/capture.cpp | 25 +++-\n> > > > > > > > > src/cam/capture.h | 2 +-\n> > > > > > > > > src/cam/main.cpp | 3 +\n> > > > > > > > > src/cam/main.h | 1 +\n> > > > > > > > > src/ipa/raspberrypi/raspberrypi.cpp | 7 +\n> > > > > > > > > src/libcamera/camera_sensor.cpp | 6 +\n> > > > > > > > > src/libcamera/control_ids.yaml | 12 ++\n> > > > > > > > > src/libcamera/geometry.cpp | 129 ++++++++++++++++++\n> > > > > > > > > .../pipeline/raspberrypi/raspberrypi.cpp | 47 +++++++\n> > > > > > > > > src/libcamera/property_ids.yaml | 19 +++\n> > > > > > > > > 12 files changed, 269 insertions(+), 3 deletions(-)","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 94D57BDB13\n\tfor ;\n\tFri, 16 Oct 2020 13:48:53 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 15EB66109C;\n\tFri, 16 Oct 2020 15:48:53 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 49E4F603C1\n\tfor ;\n\tFri, 16 Oct 2020 15:48:51 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id B6037528;\n\tFri, 16 Oct 2020 15:48:50 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"QUoIPVMN\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1602856131;\n\tbh=YQoOrOjN7ZTEVHdeBUL3xLjBuXfppKArT51iiERdqxs=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=QUoIPVMNMP6zCuJeg1Fi/+6TEeDYbo3iHRTKK+CivVsK8/tgtaQa1RQpCK9Ew0eCA\n\teup2aqj2+1nJczBd4WFd4zR81t1nfVxWpX1turGph4OCWcvbyolYMZ2sI+amqjX88J\n\tdDHrjwgA/fwJmHWQf9h7N11XX987yGxkc/GSp6xw=","Date":"Fri, 16 Oct 2020 16:48:04 +0300","From":"Laurent Pinchart ","To":"David Plowman ","Message-ID":"<20201016134804.GM3829@pendragon.ideasonboard.com>","References":"<20200929164000.15429-1-david.plowman@raspberrypi.com>\n\t<20201011214708.GA3944@pendragon.ideasonboard.com>\n\t\n\t<20201013014422.GD3942@pendragon.ideasonboard.com>\n\t\n\t<20201015021650.GH3858@pendragon.ideasonboard.com>\n\t\n\t<20201016063048.GJ3829@pendragon.ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v3 0/6] Digital zoom","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 ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]