[{"id":4149,"web_url":"https://patchwork.libcamera.org/comment/4149/","msgid":"<4e96dabe-24c3-013d-dd05-3abcc6d4adcf@ideasonboard.com>","date":"2020-03-20T15:40:31","subject":"Re: [libcamera-devel] [PATCH v2 5/6] libcamera: controls: Update\n\tusage and description for existing controls","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Naush,\n\nOn 09/03/2020 12:33, Naushir Patuck wrote:\n> Switch to using floats for constrast and saturation control to be more\n\ns/constrast/contrast/\n\n> in-line with end-user expectations.\n> \n> Expand on the descrption for the brightness, contrast and saturation\n\ns/descrption/description/\n\nI really should find some time to resume work on adding spell check to\ncheckstyle.py :-S - it would be useful to everyone I'm sure.\n\nMaybe a task to give to an internship or put on our todo pages ;-)\n\n> controls.\n> \n> Signed-off-by: Naushir Patuck \n> ---\n> src/libcamera/control_ids.yaml | 17 ++++++++++++-----\n> 1 file changed, 12 insertions(+), 5 deletions(-)\n> \n> diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml\n> index 9a33094a..3d1b058f 100644\n> --- a/src/libcamera/control_ids.yaml\n> +++ b/src/libcamera/control_ids.yaml\n> @@ -192,13 +192,20 @@ controls:\n> \n> - Brightness:\n> type: int32_t\n> - description: Specify a fixed brightness parameter\n> + description: |\n> + Specify a fixed brightness parameter. Positive values (up to 65535)\n> + produce brighter images; negative values (up to -65536) produce darker\n\nI think 'up to' still makes sense here ... but only asking to be devils\nadvocate... should this be 'down to -65536' ?\n\nI assume it's then up to the pipelines to convert that brightness range\nto whatever capabilities the hardware has anyway.\n\nWouldn't the control specify the range through a ControlRange to\nindicate what range is actually supported?\n\n> + images and 0 leaves pixels unchanged.\n> \n> - Contrast:\n> - type: int32_t\n> - description: Specify a fixed contrast parameter\n> + type: float\n> + description: |\n> + Specify a fixed contrast parameter. Normal contrast is given by the\n> + value 1.0; larger values produce images with more contrast.\n\nWhat does a value less than 1.0 provide? Do we need to document that?\n\nIf normal contrast is 1.0, and increasing the contrast is the range:\n (1.0 > ~)\ndoes that mean 'decreasing' the contrast is only the range:\n 0.0 > 1.0 ?\n\nPresumably 0 contrast would then give a single colour image?\n\nWould this be more effective to use a scale of 0 == no change, > 0\nincreases contrast, and < 0 decreases?\n\n> \n> - Saturation:\n> - type: int32_t\n> - description: Specify a fixed saturation parameter\n> + type: float\n> + description: |\n> + Specify a fixed saturation parameter. Normal saturation is given by\n> + the value 1.0; larger values produce more saturated colours.\n> ...\n\nSame comments I as above I think.\n\n>","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 60B0B60416\n\tfor ;\n\tFri, 20 Mar 2020 16:40:35 +0100 (CET)","from [192.168.0.20]\n\t(cpc89242-aztw30-2-0-cust488.18-1.cable.virginm.net [86.31.129.233])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id BE453504;\n\tFri, 20 Mar 2020 16:40:34 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1584718834;\n\tbh=ZpTxpMRfSGtcJ8n/r5I3qkosB6hu3tZxb//rITm2d+k=;\n\th=Reply-To:Subject:To:References:From:Date:In-Reply-To:From;\n\tb=QEdFu36RgombBVsogSwT6wnbAhkoVCdwHOVn7aN8pRmO71wZ4kfkyrSZ9aup0Gqo/\n\t3fUQo/Lcarq/H7aUDdOrfIMvzglOE1sZJ6jBLKT/X6eAijiRXFiExI4GSaW3nKpppz\n\tLZkNY57NpTk3T+K07BJxzhvv8QYCamYkbE/+TexA=","Reply-To":"kieran.bingham@ideasonboard.com","To":"Naushir Patuck ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20200309123319.630-1-naush@raspberrypi.com>\n\t<20200309123319.630-6-naush@raspberrypi.com>","From":"Kieran Bingham ","Openpgp":"preference=signencrypt","Autocrypt":"addr=kieran.bingham@ideasonboard.com; keydata=\n\tmQINBFYE/WYBEACs1PwjMD9rgCu1hlIiUA1AXR4rv2v+BCLUq//vrX5S5bjzxKAryRf0uHat\n\tV/zwz6hiDrZuHUACDB7X8OaQcwhLaVlq6byfoBr25+hbZG7G3+5EUl9cQ7dQEdvNj6V6y/SC\n\trRanWfelwQThCHckbobWiQJfK9n7rYNcPMq9B8e9F020LFH7Kj6YmO95ewJGgLm+idg1Kb3C\n\tpotzWkXc1xmPzcQ1fvQMOfMwdS+4SNw4rY9f07Xb2K99rjMwZVDgESKIzhsDB5GY465sCsiQ\n\tcSAZRxqE49RTBq2+EQsbrQpIc8XiffAB8qexh5/QPzCmR4kJgCGeHIXBtgRj+nIkCJPZvZtf\n\tKr2EAbc6tgg6DkAEHJb+1okosV09+0+TXywYvtEop/WUOWQ+zo+Y/OBd+8Ptgt1pDRyOBzL8\n\tRXa8ZqRf0Mwg75D+dKntZeJHzPRJyrlfQokngAAs4PaFt6UfS+ypMAF37T6CeDArQC41V3ko\n\tlPn1yMsVD0p+6i3DPvA/GPIksDC4owjnzVX9kM8Zc5Cx+XoAN0w5Eqo4t6qEVbuettxx55gq\n\t8K8FieAjgjMSxngo/HST8TpFeqI5nVeq0/lqtBRQKumuIqDg+Bkr4L1V/PSB6XgQcOdhtd36\n\tOe9X9dXB8YSNt7VjOcO7BTmFn/Z8r92mSAfHXpb07YJWJosQOQARAQABtDBLaWVyYW4gQmlu\n\tZ2hhbSA8a2llcmFuLmJpbmdoYW1AaWRlYXNvbmJvYXJkLmNvbT6JAlcEEwEKAEECGwMFCwkI\n\tBwIGFQgJCgsCBBYCAwECHgECF4ACGQEWIQSQLdeYP70o/eNy1HqhHkZyEKRh/QUCXWTtygUJ\n\tCyJXZAAKCRChHkZyEKRh/f8dEACTDsbLN2nioNZMwyLuQRUAFcXNolDX48xcUXsWS2QjxaPm\n\tVsJx8Uy8aYkS85mdPBh0C83OovQR/OVbr8AxhGvYqBs3nQvbWuTl/+4od7DfK2VZOoKBAu5S\n\tQK2FYuUcikDqYcFWJ8DQnubxfE8dvzojHEkXw0sA4igINHDDFX3HJGZtLio+WpEFQtCbfTAG\n\tYZslasz1YZRbwEdSsmO3/kqy5eMnczlm8a21A3fKUo3g8oAZEFM+f4DUNzqIltg31OAB/kZS\n\tenKZQ/SWC8PmLg/ZXBrReYakxXtkP6w3FwMlzOlhGxqhIRNiAJfXJBaRhuUWzPOpEDE9q5YJ\n\tBmqQL2WJm1VSNNVxbXJHpaWMH1sA2R00vmvRrPXGwyIO0IPYeUYQa3gsy6k+En/aMQJd27dp\n\taScf9am9PFICPY5T4ppneeJLif2lyLojo0mcHOV+uyrds9XkLpp14GfTkeKPdPMrLLTsHRfH\n\tfA4I4OBpRrEPiGIZB/0im98MkGY/Mu6qxeZmYLCcgD6qz4idOvfgVOrNh+aA8HzIVR+RMW8H\n\tQGBN9f0E3kfwxuhl3omo6V7lDw8XOdmuWZNC9zPq1UfryVHANYbLGz9KJ4Aw6M+OgBC2JpkD\n\thXMdHUkC+d20dwXrwHTlrJi1YNp6rBc+xald3wsUPOZ5z8moTHUX/uPA/qhGsbkCDQRWBP1m\n\tARAAzijkb+Sau4hAncr1JjOY+KyFEdUNxRy+hqTJdJfaYihxyaj0Ee0P0zEi35CbE6lgU0Uz\n\ttih9fiUbSV3wfsWqg1Ut3/5rTKu7kLFp15kF7eqvV4uezXRD3Qu4yjv/rMmEJbbD4cTvGCYI\n\td6MDC417f7vK3hCbCVIZSp3GXxyC1LU+UQr3fFcOyCwmP9vDUR9JV0BSqHHxRDdpUXE26Dk6\n\tmhf0V1YkspE5St814ETXpEus2urZE5yJIUROlWPIL+hm3NEWfAP06vsQUyLvr/GtbOT79vXl\n\tEn1aulcYyu20dRRxhkQ6iILaURcxIAVJJKPi8dsoMnS8pB0QW12AHWuirPF0g6DiuUfPmrA5\n\tPKe56IGlpkjc8cO51lIxHkWTpCMWigRdPDexKX+Sb+W9QWK/0JjIc4t3KBaiG8O4yRX8ml2R\n\t+rxfAVKM6V769P/hWoRGdgUMgYHFpHGSgEt80OKK5HeUPy2cngDUXzwrqiM5Sz6Od0qw5pCk\n\tNlXqI0W/who0iSVM+8+RmyY0OEkxEcci7rRLsGnM15B5PjLJjh1f2ULYkv8s4SnDwMZ/kE04\n\t/UqCMK/KnX8pwXEMCjz0h6qWNpGwJ0/tYIgQJZh6bqkvBrDogAvuhf60Sogw+mH8b+PBlx1L\n\toeTK396wc+4c3BfiC6pNtUS5GpsPMMjYMk7kVvEAEQEAAYkCPAQYAQoAJgIbDBYhBJAt15g/\n\tvSj943LUeqEeRnIQpGH9BQJdizzIBQkLSKZiAAoJEKEeRnIQpGH9eYgQAJpjaWNgqNOnMTmD\n\tMJggbwjIotypzIXfhHNCeTkG7+qCDlSaBPclcPGYrTwCt0YWPU2TgGgJrVhYT20ierN8LUvj\n\t6qOPTd+Uk7NFzL65qkh80ZKNBFddx1AabQpSVQKbdcLb8OFs85kuSvFdgqZwgxA1vl4TFhNz\n\tPZ79NAmXLackAx3sOVFhk4WQaKRshCB7cSl+RIng5S/ThOBlwNlcKG7j7W2MC06BlTbdEkUp\n\tECzuuRBv8wX4OQl+hbWbB/VKIx5HKlLu1eypen/5lNVzSqMMIYkkZcjV2SWQyUGxSwq0O/sx\n\tS0A8/atCHUXOboUsn54qdxrVDaK+6jIAuo8JiRWctP16KjzUM7MO0/+4zllM8EY57rXrj48j\n\tsbEYX0YQnzaj+jO6kJtoZsIaYR7rMMq9aUAjyiaEZpmP1qF/2sYenDx0Fg2BSlLvLvXM0vU8\n\tpQk3kgDu7kb/7PRYrZvBsr21EIQoIjXbZxDz/o7z95frkP71EaICttZ6k9q5oxxA5WC6sTXc\n\tMW8zs8avFNuA9VpXt0YupJd2ijtZy2mpZNG02fFVXhIn4G807G7+9mhuC4XG5rKlBBUXTvPU\n\tAfYnB4JBDLmLzBFavQfvonSfbitgXwCG3vS+9HEwAjU30Bar1PEOmIbiAoMzuKeRm2LVpmq4\n\tWZw01QYHU/GUV/zHJSFk","Organization":"Ideas on Board","Message-ID":"<4e96dabe-24c3-013d-dd05-3abcc6d4adcf@ideasonboard.com>","Date":"Fri, 20 Mar 2020 15:40:31 +0000","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101\n\tThunderbird/60.9.1","MIME-Version":"1.0","In-Reply-To":"<20200309123319.630-6-naush@raspberrypi.com>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH v2 5/6] libcamera: controls: Update\n\tusage and description for existing controls","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","X-List-Received-Date":"Fri, 20 Mar 2020 15:40:35 -0000"}},{"id":4222,"web_url":"https://patchwork.libcamera.org/comment/4222/","msgid":"","date":"2020-03-23T15:53:26","subject":"Re: [libcamera-devel] [PATCH v2 5/6] libcamera: controls: Update\n\tusage and description for existing controls","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi Kieran,\n\n\nOn Fri, 20 Mar 2020 at 15:40, Kieran Bingham\n wrote:\n>\n> Hi Naush,\n>\n> On 09/03/2020 12:33, Naushir Patuck wrote:\n> > Switch to using floats for constrast and saturation control to be more\n>\n> s/constrast/contrast/\n>\n> > in-line with end-user expectations.\n> >\n> > Expand on the descrption for the brightness, contrast and saturation\n>\n> s/descrption/description/\n>\n> I really should find some time to resume work on adding spell check to\n> checkstyle.py :-S - it would be useful to everyone I'm sure.\n>\n> Maybe a task to give to an internship or put on our todo pages ;-)\n>\n> > controls.\n> >\n> > Signed-off-by: Naushir Patuck \n> > ---\n> > src/libcamera/control_ids.yaml | 17 ++++++++++++-----\n> > 1 file changed, 12 insertions(+), 5 deletions(-)\n> >\n> > diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml\n> > index 9a33094a..3d1b058f 100644\n> > --- a/src/libcamera/control_ids.yaml\n> > +++ b/src/libcamera/control_ids.yaml\n> > @@ -192,13 +192,20 @@ controls:\n> >\n> > - Brightness:\n> > type: int32_t\n> > - description: Specify a fixed brightness parameter\n> > + description: |\n> > + Specify a fixed brightness parameter. Positive values (up to 65535)\n> > + produce brighter images; negative values (up to -65536) produce darker\n>\n> I think 'up to' still makes sense here ... but only asking to be devils\n> advocate... should this be 'down to -65536' ?\n>\n\nYes, that sounds correct :)\n\n> I assume it's then up to the pipelines to convert that brightness range\n> to whatever capabilities the hardware has anyway.\n>\n> Wouldn't the control specify the range through a ControlRange to\n> indicate what range is actually supported?\n\nSo this was something we were questioning ourselves. Should the\nlibcamera controls be explicit about range independent of the IPA? If\nno, listing the range here is not the right thing. If yes, then it\nensures any libcamera application will use a defined range, and the\nIPAs translate this to something they understand.\n\n>\n> > + images and 0 leaves pixels unchanged.\n> >\n> > - Contrast:\n> > - type: int32_t\n> > - description: Specify a fixed contrast parameter\n> > + type: float\n> > + description: |\n> > + Specify a fixed contrast parameter. Normal contrast is given by the\n> > + value 1.0; larger values produce images with more contrast.\n>\n> What does a value less than 1.0 provide? Do we need to document that?\n>\n> If normal contrast is 1.0, and increasing the contrast is the range:\n> (1.0 > ~)\n> does that mean 'decreasing' the contrast is only the range:\n> 0.0 > 1.0 ?\n>\n> Presumably 0 contrast would then give a single colour image?\n>\n> Would this be more effective to use a scale of 0 == no change, > 0\n> increases contrast, and < 0 decreases?\n>\n\nEssentially contrast controls the slope of the output pixel to input\npixel mapping and brightness the offset. So contrast values of < 1.0\nis certainly possible (but perhaps not that meaningful). Perhaps we\nshould expand on the description a bit more?\n\n> >\n> > - Saturation:\n> > - type: int32_t\n> > - description: Specify a fixed saturation parameter\n> > + type: float\n> > + description: |\n> > + Specify a fixed saturation parameter. Normal saturation is given by\n> > + the value 1.0; larger values produce more saturated colours.\n> > ...\n>\n> Same comments I as above I think.\n\nAgain, saturation is essentially a multiplier on our colour matrix,\nhence 1.0 is no change.\n\nFor all these controls, we were looking to make the usage as\nmathematically \"correct\" (i.e, the values apply in some plausible\nway), but try to keep them user friendly :)\n\n>\n> >\n>\n> --\n> Regards\n> --\n> Kieran\n\nThanks,\nNaush","headers":{"Return-Path":"","Received":["from mail-lf1-x143.google.com (mail-lf1-x143.google.com\n\t[IPv6:2a00:1450:4864:20::143])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 51D4A60417\n\tfor ;\n\tMon, 23 Mar 2020 16:53:44 +0100 (CET)","by mail-lf1-x143.google.com with SMTP id c5so5175561lfp.5\n\tfor ;\n\tMon, 23 Mar 2020 08:53:44 -0700 (PDT)"],"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=D3K+4bbXMqZg7gnMRpmpGRq3juRbWj4aEP6C7OMMFsg=;\n\tb=Qp0ynpKRYmqcFINtz5rWRkYYW4tiNqyMxiaC3cKRLbYjPWddKa7pf9wL5R3kPZdaoh\n\tD749fMKrRNuiGnfGEwIZItiWQJW3TZbaQ8kQYN3IAZOQ2rXbItDM+1wkerCWhZJfQuG7\n\tZV63OHxEpC/SPUAKjcPHi7L1er95N1GRvjFFftKBcRKBUuisnI6AhEes1/MrnRva7Rho\n\t2/bWgoskbM0+dvu3iodwCWkRoRdsTSYameOmP8+kYut6Gh7UNX087EoGCRZABHXGApCa\n\t8tXnSLb7h4UScEOS8dscbvlxMdH+r4rdNwV5iX/2hAfFCCpkbp4MUj1XusOUdpRd331D\n\tE1yw==","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=D3K+4bbXMqZg7gnMRpmpGRq3juRbWj4aEP6C7OMMFsg=;\n\tb=kZrnxudAmtv0KFeu2STeqkf8/+oupEnKMD0gYyMQYqcBan7X0cjU9GkEADO3AKH0Wu\n\tteSBrT7xKIT0z3QN470xJF+2x42KUGwHrqEcCBHiPTFoeXzR9xFo2Re3aiK5PXd+6Zyc\n\t9gpmyB2eBsYHiF+nixHcz7lfJcxMZ5OugSm1NJXj6TdqZhDuRWNgYTSILmP/FFQsbALN\n\t4Rjs1xddEzXnGbfotmCoN/Qm+wAp5IcAOG2R4na/DBCDIWWsIMgipo0pm9etXe2l+H7I\n\tYjfVPEWG/YxOWIVMuVWHm5tj11Ufc3C2H1Nok3tCEM2vwJutb+xvW8/XXqgAxqOee4EL\n\tU8+A==","X-Gm-Message-State":"ANhLgQ3z+O+InSHC2FCVPsF9z4V+/Xi0+1BxSBWwD5cXCgabC0Br1k6s\n\tw/DyK4j9vvPM/lUqyu61WvWrHx6kx98qJ0hjA7CfcQ==","X-Google-Smtp-Source":"ADFU+vtHcHs9yd/kW28T+7oT1Inp0AO6sfFJtGp4sRJEShQ+Usw1q/kcgFvyCyyWvWT00IGwjlMmUBtdYqHyfE7PpuM=","X-Received":"by 2002:a19:6749:: with SMTP id\n\te9mr13082348lfj.122.1584978823455; \n\tMon, 23 Mar 2020 08:53:43 -0700 (PDT)","MIME-Version":"1.0","References":"<20200309123319.630-1-naush@raspberrypi.com>\n\t<20200309123319.630-6-naush@raspberrypi.com>\n\t<4e96dabe-24c3-013d-dd05-3abcc6d4adcf@ideasonboard.com>","In-Reply-To":"<4e96dabe-24c3-013d-dd05-3abcc6d4adcf@ideasonboard.com>","From":"Naushir Patuck ","Date":"Mon, 23 Mar 2020 15:53:26 +0000","Message-ID":"","To":"Kieran Bingham ","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH v2 5/6] libcamera: controls: Update\n\tusage and description for existing controls","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","X-List-Received-Date":"Mon, 23 Mar 2020 15:53:44 -0000"}},{"id":4225,"web_url":"https://patchwork.libcamera.org/comment/4225/","msgid":"","date":"2020-03-23T16:15:29","subject":"Re: [libcamera-devel] [PATCH v2 5/6] libcamera: controls: Update\n\tusage and description for existing controls","submitter":{"id":42,"url":"https://patchwork.libcamera.org/api/people/42/","name":"David Plowman","email":"david.plowman@raspberrypi.com"},"content":"Just to add to some of that.\n\nThe brightness/contrast controls do the following to an image:\n\npixel_out = (pixel_in - 32768) * contrast + 32768\n\nwhich is a relatively \"standard\" thing (our APIs always treat pixel\nvalues as 16-bit). I'd be very much in favour of controls _not_ using\n\"arbitrary\" ranges as applications would then have to remember, and\nyou suspect different people would choose different ranges and things\nmight wind up non-portable. So trying to stick close to the real\npixels in some mathematically meaningful way is my preference - not\nleast because you can explain it! But I think there's a discussion to\nhave there.\n\nFor reference that saturation control does:\n\nRGB_out = YCbCr2RGB * Diag(1, saturation, saturation) * RGB2YCbCr * RGB_in\n\nwhere:\nRGB2YCbCr is a 3x3 matrix that converts RGB to Y plus (signed) blue\nand red chrominance values,\nDiag(a, b, c) makes a 3x3 matrix with a, b, c, down the diagonal, and\nYCbCr2RGB turns the Y plus chrominance back to RGB (inverse matrix of\nRGB2YCbCr).\n\nDavid\n\n\nOn Mon, 23 Mar 2020 at 15:53, Naushir Patuck wrote:\n>\n> Hi Kieran,\n>\n>\n> On Fri, 20 Mar 2020 at 15:40, Kieran Bingham\n> wrote:\n> >\n> > Hi Naush,\n> >\n> > On 09/03/2020 12:33, Naushir Patuck wrote:\n> > > Switch to using floats for constrast and saturation control to be more\n> >\n> > s/constrast/contrast/\n> >\n> > > in-line with end-user expectations.\n> > >\n> > > Expand on the descrption for the brightness, contrast and saturation\n> >\n> > s/descrption/description/\n> >\n> > I really should find some time to resume work on adding spell check to\n> > checkstyle.py :-S - it would be useful to everyone I'm sure.\n> >\n> > Maybe a task to give to an internship or put on our todo pages ;-)\n> >\n> > > controls.\n> > >\n> > > Signed-off-by: Naushir Patuck \n> > > ---\n> > > src/libcamera/control_ids.yaml | 17 ++++++++++++-----\n> > > 1 file changed, 12 insertions(+), 5 deletions(-)\n> > >\n> > > diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml\n> > > index 9a33094a..3d1b058f 100644\n> > > --- a/src/libcamera/control_ids.yaml\n> > > +++ b/src/libcamera/control_ids.yaml\n> > > @@ -192,13 +192,20 @@ controls:\n> > >\n> > > - Brightness:\n> > > type: int32_t\n> > > - description: Specify a fixed brightness parameter\n> > > + description: |\n> > > + Specify a fixed brightness parameter. Positive values (up to 65535)\n> > > + produce brighter images; negative values (up to -65536) produce darker\n> >\n> > I think 'up to' still makes sense here ... but only asking to be devils\n> > advocate... should this be 'down to -65536' ?\n> >\n>\n> Yes, that sounds correct :)\n>\n> > I assume it's then up to the pipelines to convert that brightness range\n> > to whatever capabilities the hardware has anyway.\n> >\n> > Wouldn't the control specify the range through a ControlRange to\n> > indicate what range is actually supported?\n>\n> So this was something we were questioning ourselves. Should the\n> libcamera controls be explicit about range independent of the IPA? If\n> no, listing the range here is not the right thing. If yes, then it\n> ensures any libcamera application will use a defined range, and the\n> IPAs translate this to something they understand.\n>\n> >\n> > > + images and 0 leaves pixels unchanged.\n> > >\n> > > - Contrast:\n> > > - type: int32_t\n> > > - description: Specify a fixed contrast parameter\n> > > + type: float\n> > > + description: |\n> > > + Specify a fixed contrast parameter. Normal contrast is given by the\n> > > + value 1.0; larger values produce images with more contrast.\n> >\n> > What does a value less than 1.0 provide? Do we need to document that?\n> >\n> > If normal contrast is 1.0, and increasing the contrast is the range:\n> > (1.0 > ~)\n> > does that mean 'decreasing' the contrast is only the range:\n> > 0.0 > 1.0 ?\n> >\n> > Presumably 0 contrast would then give a single colour image?\n> >\n> > Would this be more effective to use a scale of 0 == no change, > 0\n> > increases contrast, and < 0 decreases?\n> >\n>\n> Essentially contrast controls the slope of the output pixel to input\n> pixel mapping and brightness the offset. So contrast values of < 1.0\n> is certainly possible (but perhaps not that meaningful). Perhaps we\n> should expand on the description a bit more?\n>\n> > >\n> > > - Saturation:\n> > > - type: int32_t\n> > > - description: Specify a fixed saturation parameter\n> > > + type: float\n> > > + description: |\n> > > + Specify a fixed saturation parameter. Normal saturation is given by\n> > > + the value 1.0; larger values produce more saturated colours.\n> > > ...\n> >\n> > Same comments I as above I think.\n>\n> Again, saturation is essentially a multiplier on our colour matrix,\n> hence 1.0 is no change.\n>\n> For all these controls, we were looking to make the usage as\n> mathematically \"correct\" (i.e, the values apply in some plausible\n> way), but try to keep them user friendly :)\n>\n> >\n> > >\n> >\n> > --\n> > Regards\n> > --\n> > Kieran\n>\n> Thanks,\n> Naush\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"","Received":["from mail-oi1-x236.google.com (mail-oi1-x236.google.com\n\t[IPv6:2607:f8b0:4864:20::236])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D758B60429\n\tfor ;\n\tMon, 23 Mar 2020 17:15:41 +0100 (CET)","by mail-oi1-x236.google.com with SMTP id w2so4290253oic.5\n\tfor ;\n\tMon, 23 Mar 2020 09:15:41 -0700 (PDT)"],"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=kMNEYPnJpaYCQy3i9j0uzuf9qWSIzBj2ix7gW/4opAI=;\n\tb=k7CBrClt3evVZbXwKx0D5IoIMQg01xYK8VXR9rAxs8yVEGmvdU+nCVmp8uBmn1fw62\n\tC2dy615S92ApHrGzjEDEMNfT8kgSXf2JxkuYYYUE/zyPt8vl/LmbHUpleFXL4P4LO0x8\n\tk01D233RHvBxiQ6XuQVis/10fFHPeJqO3CXAmRYpkkxZAkL3h1hA8EROqZsrBitAq8L+\n\tOJyTihXGUXzGx6n5RBtpMOPjFeDduKydgWJ7UAum+hmAUW2otvHwAHXVxeye2dzZ5Zhj\n\tFCigXys/4ASCRUfpCv1mG/5swNiBNv4GJ83c/E4hZvG2/ml+ORgEOU6KOpeVlPNI2a4R\n\tBFlQ==","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=kMNEYPnJpaYCQy3i9j0uzuf9qWSIzBj2ix7gW/4opAI=;\n\tb=bT6esBv3vwDQeHSCWxKBeLeEUH1YxT+RVGolQirOqiW42CNFh2QG38C6tI1w6/ZRYt\n\tqDK0LxiqOqUHvOOjXcQSmKMN5qFs8+f3O17Sj3hODs7AXkU3U6oxQaf8Bmxt36YYTutJ\n\tDhUpy4riGm9oPtqGDKJc5Ex691ugqR/zK4LCXoMl44G3CLvWtHf+GYcGXyqxj+OFp2t8\n\tnXTAuwNAdCTEYO3a0WlZ1NLLvB5r9nxWH8hMfokW3GbZt6u2JA9/pZ0ScSQAlHZn4AA6\n\tz62uKV8KmBX7CXV46w86U1PPRtscU+b2/S5Z/2c+BkX2oh7dCs2c8cQ8c0qvP+Yj6/Wk\n\trInQ==","X-Gm-Message-State":"ANhLgQ2vRCl0tD+BVcrIsNYen36oR8kF1kU3flD4CVNw6ZC0MaRM0kaz\n\t47cUjtFFo7MbCfSfwr+i3vmo0rtGl3QNKqR7d8yJFg==","X-Google-Smtp-Source":"ADFU+vvZyF2l0WIqgANPV0Hv/JAPk+Xcb9T/XEs29RRQJi0nGqTPcMYyXBpZHZGxmQUyx26g9mbvCQo2dST5GkmK7Fk=","X-Received":"by 2002:aca:1812:: with SMTP id h18mr45196oih.107.1584980140464; \n\tMon, 23 Mar 2020 09:15:40 -0700 (PDT)","MIME-Version":"1.0","References":"<20200309123319.630-1-naush@raspberrypi.com>\n\t<20200309123319.630-6-naush@raspberrypi.com>\n\t<4e96dabe-24c3-013d-dd05-3abcc6d4adcf@ideasonboard.com>\n\t","In-Reply-To":"","From":"David Plowman ","Date":"Mon, 23 Mar 2020 16:15:29 +0000","Message-ID":"","To":"Naushir Patuck ","Cc":"Kieran Bingham ,\n\tlibcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH v2 5/6] libcamera: controls: Update\n\tusage and description for existing controls","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","X-List-Received-Date":"Mon, 23 Mar 2020 16:15:42 -0000"}},{"id":4314,"web_url":"https://patchwork.libcamera.org/comment/4314/","msgid":"<20200326161104.GU20581@pendragon.ideasonboard.com>","date":"2020-03-26T16:11:04","subject":"Re: [libcamera-devel] [PATCH v2 5/6] libcamera: controls: Update\n\tusage and description for existing controls","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hello,\n\nOn Mon, Mar 23, 2020 at 04:15:29PM +0000, David Plowman wrote:\n> Just to add to some of that.\n> \n> The brightness/contrast controls do the following to an image:\n> \n> pixel_out = (pixel_in - 32768) * contrast + 32768\n> \n> which is a relatively \"standard\" thing (our APIs always treat pixel\n> values as 16-bit). I'd be very much in favour of controls _not_ using\n> \"arbitrary\" ranges as applications would then have to remember, and\n> you suspect different people would choose different ranges and things\n> might wind up non-portable. So trying to stick close to the real\n> pixels in some mathematically meaningful way is my preference - not\n> least because you can explain it! But I think there's a discussion to\n> have there.\n\nI agree regarding ranges, if we can standardize them, that's best.\n\n> For reference that saturation control does:\n> \n> RGB_out = YCbCr2RGB * Diag(1, saturation, saturation) * RGB2YCbCr * RGB_in\n> \n> where:\n> RGB2YCbCr is a 3x3 matrix that converts RGB to Y plus (signed) blue\n> and red chrominance values,\n> Diag(a, b, c) makes a 3x3 matrix with a, b, c, down the diagonal, and\n> YCbCr2RGB turns the Y plus chrominance back to RGB (inverse matrix of\n> RGB2YCbCr).\n> \n> On Mon, 23 Mar 2020 at 15:53, Naushir Patuck wrote:\n> > On Fri, 20 Mar 2020 at 15:40, Kieran Bingham wrote:\n> > > On 09/03/2020 12:33, Naushir Patuck wrote:\n> > > > Switch to using floats for constrast and saturation control to be more\n> > >\n> > > s/constrast/contrast/\n> > >\n> > > > in-line with end-user expectations.\n> > > >\n> > > > Expand on the descrption for the brightness, contrast and saturation\n> > >\n> > > s/descrption/description/\n> > >\n> > > I really should find some time to resume work on adding spell check to\n> > > checkstyle.py :-S - it would be useful to everyone I'm sure.\n> > >\n> > > Maybe a task to give to an internship or put on our todo pages ;-)\n> > >\n> > > > controls.\n> > > >\n> > > > Signed-off-by: Naushir Patuck \n> > > > ---\n> > > > src/libcamera/control_ids.yaml | 17 ++++++++++++-----\n> > > > 1 file changed, 12 insertions(+), 5 deletions(-)\n> > > >\n> > > > diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml\n> > > > index 9a33094a..3d1b058f 100644\n> > > > --- a/src/libcamera/control_ids.yaml\n> > > > +++ b/src/libcamera/control_ids.yaml\n> > > > @@ -192,13 +192,20 @@ controls:\n> > > >\n> > > > - Brightness:\n> > > > type: int32_t\n> > > > - description: Specify a fixed brightness parameter\n> > > > + description: |\n> > > > + Specify a fixed brightness parameter. Positive values (up to 65535)\n> > > > + produce brighter images; negative values (up to -65536) produce darker\n> > >\n> > > I think 'up to' still makes sense here ... but only asking to be devils\n> > > advocate... should this be 'down to -65536' ?\n> >\n> > Yes, that sounds correct :)\n> >\n> > > I assume it's then up to the pipelines to convert that brightness range\n> > > to whatever capabilities the hardware has anyway.\n> > >\n> > > Wouldn't the control specify the range through a ControlRange to\n> > > indicate what range is actually supported?\n> >\n> > So this was something we were questioning ourselves. Should the\n> > libcamera controls be explicit about range independent of the IPA? If\n> > no, listing the range here is not the right thing. If yes, then it\n> > ensures any libcamera application will use a defined range, and the\n> > IPAs translate this to something they understand.\n> >\n> > >\n> > > > + images and 0 leaves pixels unchanged.\n> > > >\n> > > > - Contrast:\n> > > > - type: int32_t\n> > > > - description: Specify a fixed contrast parameter\n> > > > + type: float\n> > > > + description: |\n> > > > + Specify a fixed contrast parameter. Normal contrast is given by the\n> > > > + value 1.0; larger values produce images with more contrast.\n> > >\n> > > What does a value less than 1.0 provide? Do we need to document that?\n> > >\n> > > If normal contrast is 1.0, and increasing the contrast is the range:\n> > > (1.0 > ~)\n> > > does that mean 'decreasing' the contrast is only the range:\n> > > 0.0 > 1.0 ?\n> > >\n> > > Presumably 0 contrast would then give a single colour image?\n> > >\n> > > Would this be more effective to use a scale of 0 == no change, > 0\n> > > increases contrast, and < 0 decreases?\n> >\n> > Essentially contrast controls the slope of the output pixel to input\n> > pixel mapping and brightness the offset. So contrast values of < 1.0\n> > is certainly possible (but perhaps not that meaningful). Perhaps we\n> > should expand on the description a bit more?\n> >\n> > > > - Saturation:\n> > > > - type: int32_t\n> > > > - description: Specify a fixed saturation parameter\n> > > > + type: float\n> > > > + description: |\n> > > > + Specify a fixed saturation parameter. Normal saturation is given by\n> > > > + the value 1.0; larger values produce more saturated colours.\n> > > > ...\n> > >\n> > > Same comments I as above I think.\n> >\n> > Again, saturation is essentially a multiplier on our colour matrix,\n> > hence 1.0 is no change.\n> >\n> > For all these controls, we were looking to make the usage as\n> > mathematically \"correct\" (i.e, the values apply in some plausible\n> > way), but try to keep them user friendly :)\n\nI've been thinking about these controls. I would first like to point out\nthat they were added more as initial placeholders than as real controls,\nto have something to experiment with. If there's any control that you\nthink doesn't make much sense, please feel free to say so.\n\nThen, regarding contrast and brightness, I wonder if we should go for\nsupport of more complex tonemap curves. Gamma comes to mind in this\ncontext, and the Android camera HAL exposes a configurable curve as a\nset of points (with the camera reporting the number of points it\nsupports, so a camera that only supports contrast and brightness would\nonly accept two points), maybe that could be worth considering. It\ndoesn't have to be done now, we can change it later, as long as we do so\nbefore freezing the API.\n\nFor saturation I similarly wonder if we shouldn't handle it through a\nconfigurable RGB to RGB matrix.","headers":{"Return-Path":"","Received":["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 8AE4260414\n\tfor ;\n\tThu, 26 Mar 2020 17:11:08 +0100 (CET)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 16AC22DC;\n\tThu, 26 Mar 2020 17:11:08 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"KSbq+SS+\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1585239068;\n\tbh=QZUU95EVYVKXVjtDEO3lfflYcmeoaVi6vz9YxMElNLw=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=KSbq+SS+pms/uOhBjRR692aRGuWwHelbjLyyr6+0Ctx1i/O5Bxj/frzYl9KOfldEB\n\toWxj9ajAkZ1b+6Bdk3oXi0wNjDFwpMWBNQD1ZWJv2dp06n2QRO14X8Z91/JaYibaaz\n\tPOXBqPj0yr/48c5alPPEDVAo6PJX2ne474Ue8ero=","Date":"Thu, 26 Mar 2020 18:11:04 +0200","From":"Laurent Pinchart ","To":"David Plowman ","Cc":"Naushir Patuck ,\n\tlibcamera-devel@lists.libcamera.org","Message-ID":"<20200326161104.GU20581@pendragon.ideasonboard.com>","References":"<20200309123319.630-1-naush@raspberrypi.com>\n\t<20200309123319.630-6-naush@raspberrypi.com>\n\t<4e96dabe-24c3-013d-dd05-3abcc6d4adcf@ideasonboard.com>\n\t\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v2 5/6] libcamera: controls: Update\n\tusage and description for existing controls","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","X-List-Received-Date":"Thu, 26 Mar 2020 16:11:08 -0000"}},{"id":4341,"web_url":"https://patchwork.libcamera.org/comment/4341/","msgid":"","date":"2020-03-27T11:35:32","subject":"Re: [libcamera-devel] [PATCH v2 5/6] libcamera: controls: Update\n\tusage and description for existing controls","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi Laurent,\n\n\n\nOn Thu, 26 Mar 2020 at 16:11, Laurent Pinchart\n wrote:\n>\n> Hello,\n>\n> On Mon, Mar 23, 2020 at 04:15:29PM +0000, David Plowman wrote:\n> > Just to add to some of that.\n> >\n> > The brightness/contrast controls do the following to an image:\n> >\n> > pixel_out = (pixel_in - 32768) * contrast + 32768\n> >\n> > which is a relatively \"standard\" thing (our APIs always treat pixel\n> > values as 16-bit). I'd be very much in favour of controls _not_ using\n> > \"arbitrary\" ranges as applications would then have to remember, and\n> > you suspect different people would choose different ranges and things\n> > might wind up non-portable. So trying to stick close to the real\n> > pixels in some mathematically meaningful way is my preference - not\n> > least because you can explain it! But I think there's a discussion to\n> > have there.\n>\n> I agree regarding ranges, if we can standardize them, that's best.\n>\n> > For reference that saturation control does:\n> >\n> > RGB_out = YCbCr2RGB * Diag(1, saturation, saturation) * RGB2YCbCr * RGB_in\n> >\n> > where:\n> > RGB2YCbCr is a 3x3 matrix that converts RGB to Y plus (signed) blue\n> > and red chrominance values,\n> > Diag(a, b, c) makes a 3x3 matrix with a, b, c, down the diagonal, and\n> > YCbCr2RGB turns the Y plus chrominance back to RGB (inverse matrix of\n> > RGB2YCbCr).\n> >\n> > On Mon, 23 Mar 2020 at 15:53, Naushir Patuck wrote:\n> > > On Fri, 20 Mar 2020 at 15:40, Kieran Bingham wrote:\n> > > > On 09/03/2020 12:33, Naushir Patuck wrote:\n> > > > > Switch to using floats for constrast and saturation control to be more\n> > > >\n> > > > s/constrast/contrast/\n> > > >\n> > > > > in-line with end-user expectations.\n> > > > >\n> > > > > Expand on the descrption for the brightness, contrast and saturation\n> > > >\n> > > > s/descrption/description/\n> > > >\n> > > > I really should find some time to resume work on adding spell check to\n> > > > checkstyle.py :-S - it would be useful to everyone I'm sure.\n> > > >\n> > > > Maybe a task to give to an internship or put on our todo pages ;-)\n> > > >\n> > > > > controls.\n> > > > >\n> > > > > Signed-off-by: Naushir Patuck \n> > > > > ---\n> > > > > src/libcamera/control_ids.yaml | 17 ++++++++++++-----\n> > > > > 1 file changed, 12 insertions(+), 5 deletions(-)\n> > > > >\n> > > > > diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml\n> > > > > index 9a33094a..3d1b058f 100644\n> > > > > --- a/src/libcamera/control_ids.yaml\n> > > > > +++ b/src/libcamera/control_ids.yaml\n> > > > > @@ -192,13 +192,20 @@ controls:\n> > > > >\n> > > > > - Brightness:\n> > > > > type: int32_t\n> > > > > - description: Specify a fixed brightness parameter\n> > > > > + description: |\n> > > > > + Specify a fixed brightness parameter. Positive values (up to 65535)\n> > > > > + produce brighter images; negative values (up to -65536) produce darker\n> > > >\n> > > > I think 'up to' still makes sense here ... but only asking to be devils\n> > > > advocate... should this be 'down to -65536' ?\n> > >\n> > > Yes, that sounds correct :)\n> > >\n> > > > I assume it's then up to the pipelines to convert that brightness range\n> > > > to whatever capabilities the hardware has anyway.\n> > > >\n> > > > Wouldn't the control specify the range through a ControlRange to\n> > > > indicate what range is actually supported?\n> > >\n> > > So this was something we were questioning ourselves. Should the\n> > > libcamera controls be explicit about range independent of the IPA? If\n> > > no, listing the range here is not the right thing. If yes, then it\n> > > ensures any libcamera application will use a defined range, and the\n> > > IPAs translate this to something they understand.\n> > >\n> > > >\n> > > > > + images and 0 leaves pixels unchanged.\n> > > > >\n> > > > > - Contrast:\n> > > > > - type: int32_t\n> > > > > - description: Specify a fixed contrast parameter\n> > > > > + type: float\n> > > > > + description: |\n> > > > > + Specify a fixed contrast parameter. Normal contrast is given by the\n> > > > > + value 1.0; larger values produce images with more contrast.\n> > > >\n> > > > What does a value less than 1.0 provide? Do we need to document that?\n> > > >\n> > > > If normal contrast is 1.0, and increasing the contrast is the range:\n> > > > (1.0 > ~)\n> > > > does that mean 'decreasing' the contrast is only the range:\n> > > > 0.0 > 1.0 ?\n> > > >\n> > > > Presumably 0 contrast would then give a single colour image?\n> > > >\n> > > > Would this be more effective to use a scale of 0 == no change, > 0\n> > > > increases contrast, and < 0 decreases?\n> > >\n> > > Essentially contrast controls the slope of the output pixel to input\n> > > pixel mapping and brightness the offset. So contrast values of < 1.0\n> > > is certainly possible (but perhaps not that meaningful). Perhaps we\n> > > should expand on the description a bit more?\n> > >\n> > > > > - Saturation:\n> > > > > - type: int32_t\n> > > > > - description: Specify a fixed saturation parameter\n> > > > > + type: float\n> > > > > + description: |\n> > > > > + Specify a fixed saturation parameter. Normal saturation is given by\n> > > > > + the value 1.0; larger values produce more saturated colours.\n> > > > > ...\n> > > >\n> > > > Same comments I as above I think.\n> > >\n> > > Again, saturation is essentially a multiplier on our colour matrix,\n> > > hence 1.0 is no change.\n> > >\n> > > For all these controls, we were looking to make the usage as\n> > > mathematically \"correct\" (i.e, the values apply in some plausible\n> > > way), but try to keep them user friendly :)\n>\n> I've been thinking about these controls. I would first like to point out\n> that they were added more as initial placeholders than as real controls,\n> to have something to experiment with. If there's any control that you\n> think doesn't make much sense, please feel free to say so.\n>\n> Then, regarding contrast and brightness, I wonder if we should go for\n> support of more complex tonemap curves. Gamma comes to mind in this\n> context, and the Android camera HAL exposes a configurable curve as a\n> set of points (with the camera reporting the number of points it\n> supports, so a camera that only supports contrast and brightness would\n> only accept two points), maybe that could be worth considering. It\n> doesn't have to be done now, we can change it later, as long as we do so\n> before freezing the API.\n\nI could see the advantages of doing this!\n\n>\n> For saturation I similarly wonder if we shouldn't handle it through a\n> configurable RGB to RGB matrix.\n>\n\nPerhaps both controls could be present and the application chooses how\nit wants to control this?\n\n> --\n> Regards,\n>\n> Laurent Pinchart\n\nRegards,\nNaush","headers":{"Return-Path":"","Received":["from mail-lf1-x142.google.com (mail-lf1-x142.google.com\n\t[IPv6:2a00:1450:4864:20::142])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id AC0656040E\n\tfor ;\n\tFri, 27 Mar 2020 12:35:51 +0100 (CET)","by mail-lf1-x142.google.com with SMTP id e7so7567818lfq.1\n\tfor ;\n\tFri, 27 Mar 2020 04:35:51 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"sEIXdWBD\"; 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=bQK5nv6ZRAEpPB65vN3Cww5e4A7dYJ0BH1gVI20hCoU=;\n\tb=sEIXdWBDrPBP9Y4s1h/qAffeaF0pQaKRQRQrjsN7dcufdATy6BiOvHKPMv7A/cF67w\n\tyybXNsnuSpHDhSzam78qfAIHSKQId9zXl50Lf1W70nrxxOcqfvsE4kvM3L5fOIQLf0Lj\n\teFbPDD1iKVoz/I/c9mt9e+QhTJH/URFQWkohrTY3D62qL0JtgWEI0mNPqIcitDr85oz1\n\tbzu3o6Mm1/Gmz0FseXCF8V+3PbqwWS49UkRVp/HUFf3Mwm4yaOjua05CZ7x+7NpWsdyh\n\tFBpWIo9WrlfGegQAYTIZIoB0uc3zvusohr54VO31yZOKTHK/S4zBHa9aAIH14Ecqv5e9\n\tW8NQ==","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=bQK5nv6ZRAEpPB65vN3Cww5e4A7dYJ0BH1gVI20hCoU=;\n\tb=OEpcSu6slRssoU4fLNQ9bNZxQawSVivNWSA+bYOOm4n+YT1CRh6s5yv+h+cd6ERdnR\n\tPjv6rSObDH3pFLUxKNhxj8gLzMETmL5JhhoDz9covmVzWe2grGzR1+9PG7KZ5gp2LsaO\n\tyvGC6v6FElUunkdyTVIFdQ2jhOoM2gV/h9UU1mwjZbgjb+ebtvkoTAYF3d9TM1xbOb7H\n\tlK6eNlDpqYaWxuflyXLTpCiTwL8MPEHqgQtOSYE1F41LKRTNfV0qvBZv4U4aa0qgyMTE\n\tgkwHzC8zNn9h6grW1fVByESLWGYqNs/OfB4W0F8LWGKTj2A7JSoq74blL90xD76Nmhy1\n\tdHvQ==","X-Gm-Message-State":"ANhLgQ1qxMRiJH1pcd7JILmeqXPqs846KyvLPrtHI46yixNcjeNbNSR8\n\t30/2uJ2g6TzrObb8460ZgsKkwZXDO7SqBwmGiqzY+g==","X-Google-Smtp-Source":"ADFU+vs+UxEkcmxlA/p8+4WtdXp7VEpyX0FKpmcBauvpqsh60gcnQxTzcXUa/G5qDnhb6GSq9r25pb2MqWM5aLyf+2o=","X-Received":"by 2002:a05:6512:3391:: with SMTP id\n\th17mr8741315lfg.181.1585308950795; \n\tFri, 27 Mar 2020 04:35:50 -0700 (PDT)","MIME-Version":"1.0","References":"<20200309123319.630-1-naush@raspberrypi.com>\n\t<20200309123319.630-6-naush@raspberrypi.com>\n\t<4e96dabe-24c3-013d-dd05-3abcc6d4adcf@ideasonboard.com>\n\t\n\t\n\t<20200326161104.GU20581@pendragon.ideasonboard.com>","In-Reply-To":"<20200326161104.GU20581@pendragon.ideasonboard.com>","From":"Naushir Patuck ","Date":"Fri, 27 Mar 2020 11:35:32 +0000","Message-ID":"","To":"Laurent Pinchart ","Cc":"David Plowman ,\n\tlibcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH v2 5/6] libcamera: controls: Update\n\tusage and description for existing controls","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","X-List-Received-Date":"Fri, 27 Mar 2020 11:35:51 -0000"}},{"id":4342,"web_url":"https://patchwork.libcamera.org/comment/4342/","msgid":"<20200327132737.GH5040@pendragon.ideasonboard.com>","date":"2020-03-27T13:27:37","subject":"Re: [libcamera-devel] [PATCH v2 5/6] libcamera: controls: Update\n\tusage and description for existing controls","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Naush,\n\nOn Fri, Mar 27, 2020 at 11:35:32AM +0000, Naushir Patuck wrote:\n> On Thu, 26 Mar 2020 at 16:11, Laurent Pinchart wrote:\n> > On Mon, Mar 23, 2020 at 04:15:29PM +0000, David Plowman wrote:\n> >> Just to add to some of that.\n> >>\n> >> The brightness/contrast controls do the following to an image:\n> >>\n> >> pixel_out = (pixel_in - 32768) * contrast + 32768\n> >>\n> >> which is a relatively \"standard\" thing (our APIs always treat pixel\n> >> values as 16-bit). I'd be very much in favour of controls _not_ using\n> >> \"arbitrary\" ranges as applications would then have to remember, and\n> >> you suspect different people would choose different ranges and things\n> >> might wind up non-portable. So trying to stick close to the real\n> >> pixels in some mathematically meaningful way is my preference - not\n> >> least because you can explain it! But I think there's a discussion to\n> >> have there.\n> >\n> > I agree regarding ranges, if we can standardize them, that's best.\n> >\n> >> For reference that saturation control does:\n> >>\n> >> RGB_out = YCbCr2RGB * Diag(1, saturation, saturation) * RGB2YCbCr * RGB_in\n> >>\n> >> where:\n> >> RGB2YCbCr is a 3x3 matrix that converts RGB to Y plus (signed) blue\n> >> and red chrominance values,\n> >> Diag(a, b, c) makes a 3x3 matrix with a, b, c, down the diagonal, and\n> >> YCbCr2RGB turns the Y plus chrominance back to RGB (inverse matrix of\n> >> RGB2YCbCr).\n> >>\n> >> On Mon, 23 Mar 2020 at 15:53, Naushir Patuck wrote:\n> >>> On Fri, 20 Mar 2020 at 15:40, Kieran Bingham wrote:\n> >>>> On 09/03/2020 12:33, Naushir Patuck wrote:\n> >>>>> Switch to using floats for constrast and saturation control to be more\n> >>>>\n> >>>> s/constrast/contrast/\n> >>>>\n> >>>>> in-line with end-user expectations.\n> >>>>>\n> >>>>> Expand on the descrption for the brightness, contrast and saturation\n> >>>>\n> >>>> s/descrption/description/\n> >>>>\n> >>>> I really should find some time to resume work on adding spell check to\n> >>>> checkstyle.py :-S - it would be useful to everyone I'm sure.\n> >>>>\n> >>>> Maybe a task to give to an internship or put on our todo pages ;-)\n> >>>>\n> >>>>> controls.\n> >>>>>\n> >>>>> Signed-off-by: Naushir Patuck \n> >>>>> ---\n> >>>>> src/libcamera/control_ids.yaml | 17 ++++++++++++-----\n> >>>>> 1 file changed, 12 insertions(+), 5 deletions(-)\n> >>>>>\n> >>>>> diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml\n> >>>>> index 9a33094a..3d1b058f 100644\n> >>>>> --- a/src/libcamera/control_ids.yaml\n> >>>>> +++ b/src/libcamera/control_ids.yaml\n> >>>>> @@ -192,13 +192,20 @@ controls:\n> >>>>>\n> >>>>> - Brightness:\n> >>>>> type: int32_t\n> >>>>> - description: Specify a fixed brightness parameter\n> >>>>> + description: |\n> >>>>> + Specify a fixed brightness parameter. Positive values (up to 65535)\n> >>>>> + produce brighter images; negative values (up to -65536) produce darker\n> >>>>\n> >>>> I think 'up to' still makes sense here ... but only asking to be devils\n> >>>> advocate... should this be 'down to -65536' ?\n> >>>\n> >>> Yes, that sounds correct :)\n> >>>\n> >>>> I assume it's then up to the pipelines to convert that brightness range\n> >>>> to whatever capabilities the hardware has anyway.\n> >>>>\n> >>>> Wouldn't the control specify the range through a ControlRange to\n> >>>> indicate what range is actually supported?\n> >>>\n> >>> So this was something we were questioning ourselves. Should the\n> >>> libcamera controls be explicit about range independent of the IPA? If\n> >>> no, listing the range here is not the right thing. If yes, then it\n> >>> ensures any libcamera application will use a defined range, and the\n> >>> IPAs translate this to something they understand.\n> >>>\n> >>>>\n> >>>>> + images and 0 leaves pixels unchanged.\n> >>>>>\n> >>>>> - Contrast:\n> >>>>> - type: int32_t\n> >>>>> - description: Specify a fixed contrast parameter\n> >>>>> + type: float\n> >>>>> + description: |\n> >>>>> + Specify a fixed contrast parameter. Normal contrast is given by the\n> >>>>> + value 1.0; larger values produce images with more contrast.\n> >>>>\n> >>>> What does a value less than 1.0 provide? Do we need to document that?\n> >>>>\n> >>>> If normal contrast is 1.0, and increasing the contrast is the range:\n> >>>> (1.0 > ~)\n> >>>> does that mean 'decreasing' the contrast is only the range:\n> >>>> 0.0 > 1.0 ?\n> >>>>\n> >>>> Presumably 0 contrast would then give a single colour image?\n> >>>>\n> >>>> Would this be more effective to use a scale of 0 == no change, > 0\n> >>>> increases contrast, and < 0 decreases?\n> >>>\n> >>> Essentially contrast controls the slope of the output pixel to input\n> >>> pixel mapping and brightness the offset. So contrast values of < 1.0\n> >>> is certainly possible (but perhaps not that meaningful). Perhaps we\n> >>> should expand on the description a bit more?\n> >>>\n> >>>>> - Saturation:\n> >>>>> - type: int32_t\n> >>>>> - description: Specify a fixed saturation parameter\n> >>>>> + type: float\n> >>>>> + description: |\n> >>>>> + Specify a fixed saturation parameter. Normal saturation is given by\n> >>>>> + the value 1.0; larger values produce more saturated colours.\n> >>>>> ...\n> >>>>\n> >>>> Same comments I as above I think.\n> >>>\n> >>> Again, saturation is essentially a multiplier on our colour matrix,\n> >>> hence 1.0 is no change.\n> >>>\n> >>> For all these controls, we were looking to make the usage as\n> >>> mathematically \"correct\" (i.e, the values apply in some plausible\n> >>> way), but try to keep them user friendly :)\n> >\n> > I've been thinking about these controls. I would first like to point out\n> > that they were added more as initial placeholders than as real controls,\n> > to have something to experiment with. If there's any control that you\n> > think doesn't make much sense, please feel free to say so.\n> >\n> > Then, regarding contrast and brightness, I wonder if we should go for\n> > support of more complex tonemap curves. Gamma comes to mind in this\n> > context, and the Android camera HAL exposes a configurable curve as a\n> > set of points (with the camera reporting the number of points it\n> > supports, so a camera that only supports contrast and brightness would\n> > only accept two points), maybe that could be worth considering. It\n> > doesn't have to be done now, we can change it later, as long as we do so\n> > before freezing the API.\n> \n> I could see the advantages of doing this!\n> \n> > For saturation I similarly wonder if we shouldn't handle it through a\n> > configurable RGB to RGB matrix.\n> \n> Perhaps both controls could be present and the application chooses how\n> it wants to control this?\n\nI've been giving this some more thoughts. I would like to avoid IPAs\nhaving to handle multiple controls for the same purpose if possible. Do\nyou think it would be a good idea to handle this in upper layers\ninstead, with helpers for applications to set brightness, contrast and\nsaturation, that would fill the tonemap and RGB2RGB controls in the\nrequest ?","headers":{"Return-Path":"","Received":["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 BB8666040E\n\tfor ;\n\tFri, 27 Mar 2020 14:27:41 +0100 (CET)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 1EA6D2DC;\n\tFri, 27 Mar 2020 14:27:41 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"E0NEzYWS\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1585315661;\n\tbh=Q/9EF9v/zsoX8XHmkug1IfGWtwuA0gEg/iVT4mv5w3Q=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=E0NEzYWSwKj71xG63vTnF9S7OQNL7guFG/cl+FH5QQ9Dds8rUFjxwPTf73CERacHU\n\tbPH0Sqc8yeNBjTY7HjmQGEu5ifd/AWm13Braa26NTTtWHmSUWsxFt7U031VsIpnVNP\n\tLeCFv7+w9JsEhjaZefVd4li458QrbmPsLF/loc08=","Date":"Fri, 27 Mar 2020 15:27:37 +0200","From":"Laurent Pinchart ","To":"Naushir Patuck ","Cc":"David Plowman ,\n\tlibcamera-devel@lists.libcamera.org","Message-ID":"<20200327132737.GH5040@pendragon.ideasonboard.com>","References":"<20200309123319.630-1-naush@raspberrypi.com>\n\t<20200309123319.630-6-naush@raspberrypi.com>\n\t<4e96dabe-24c3-013d-dd05-3abcc6d4adcf@ideasonboard.com>\n\t\n\t\n\t<20200326161104.GU20581@pendragon.ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v2 5/6] libcamera: controls: Update\n\tusage and description for existing controls","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","X-List-Received-Date":"Fri, 27 Mar 2020 13:27:42 -0000"}}]