[{"id":4198,"web_url":"https://patchwork.libcamera.org/comment/4198/","msgid":"","date":"2020-03-23T13:10:33","subject":"Re: [libcamera-devel] [PATCH LIBCAMERA v3] libcamera: rkisp1: Use\n\tparametered constructor instead of default","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Kaaira,\n\nOn 23/03/2020 08:44, Kaaira Gupta wrote:\n> Use of default constructor StreamConfiguration() is deprecated, hence\n> make it private. Make Stream class a friend of StreamConfiguration as it\n> uses the default constructor.\n> \n> Replace default constructor StreamConfiguration() by it's parametered\n\ns/parametered/parametrized/ ? If so, both here and in the subject line.\n\n> counterpart by using StreamFormats in generateConfiguration() in rkisp1\n> \n> Also, use erase(), instead of replace() in validate() to prevent it from\n> creating a new instance with empty constructor.\n> \n> Signed-off-by: Kaaira Gupta \n> ---\n> \n> Changes since v1:\n> \t- replace resize() by erase() in validate() to prevent it from\n> creating a new instance with empty constructor.\n> Changes since v2:\n> \t- Rearaange the order of declaration of friend class.\n> \t- Add constructor implementation again to stream.cpp\n> \t- Corrections in range of erase()\n> \n> include/libcamera/stream.h | 4 ++-\n> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 39 ++++++++++++++++--------\n> src/libcamera/stream.cpp | 6 ++--\n> 3 files changed, 32 insertions(+), 17 deletions(-)\n> \n> diff --git a/include/libcamera/stream.h b/include/libcamera/stream.h\n> index b1441f8..a379ebb 100644\n> --- a/include/libcamera/stream.h\n> +++ b/include/libcamera/stream.h\n> @@ -37,7 +37,6 @@ private:\n> };\n> \n> struct StreamConfiguration {\n> -\tStreamConfiguration();\n> \tStreamConfiguration(const StreamFormats &formats);\n> \n> \tPixelFormat pixelFormat;\n> @@ -52,6 +51,9 @@ struct StreamConfiguration {\n> \tstd::string toString() const;\n> \n> private:\n> +\tfriend class Stream;\n> +\tStreamConfiguration();\n> +\n\nBecause the change to move the StreamConfiguration(); to being private\ncauses multiple breakage, I think we should have it as a separate\nchange, so this has certainly already become a series :-)\n\nThe series should thus fix all breakages (before it breaks) then move\nthis at the end of the series. Probably along with the removal of the \\todo\n\n\n> \tStream *stream_;\n> \tStreamFormats formats_;\n> };\n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> index 2f909ce..6839e3c 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> @@ -218,6 +218,21 @@ private:\n> \tCamera *activeCamera_;\n> };\n> \n> +namespace {\n> +\n> +static const std::array formats{\n> +\tPixelFormat(DRM_FORMAT_YUYV),\n> +\tPixelFormat(DRM_FORMAT_YVYU),\n> +\tPixelFormat(DRM_FORMAT_VYUY),\n> +\tPixelFormat(DRM_FORMAT_NV16),\n> +\tPixelFormat(DRM_FORMAT_NV61),\n> +\tPixelFormat(DRM_FORMAT_NV21),\n> +\tPixelFormat(DRM_FORMAT_NV12),\n> +\t/* \\todo Add support for 8-bit greyscale to DRM formats */\n> +};\n> +\n> +} /* namespace */\n> +\n> RkISP1Frames::RkISP1Frames(PipelineHandler *pipe)\n> \t: pipe_(dynamic_cast(pipe))\n> {\n> @@ -430,17 +445,6 @@ RkISP1CameraConfiguration::RkISP1CameraConfiguration(Camera *camera,\n> \n> CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n> {\n> -\tstatic const std::array formats{\n> -\t\tPixelFormat(DRM_FORMAT_YUYV),\n> -\t\tPixelFormat(DRM_FORMAT_YVYU),\n> -\t\tPixelFormat(DRM_FORMAT_VYUY),\n> -\t\tPixelFormat(DRM_FORMAT_NV16),\n> -\t\tPixelFormat(DRM_FORMAT_NV61),\n> -\t\tPixelFormat(DRM_FORMAT_NV21),\n> -\t\tPixelFormat(DRM_FORMAT_NV12),\n> -\t\t/* \\todo Add support for 8-bit greyscale to DRM formats */\n> -\t};\n> -\n> \tconst CameraSensor *sensor = data_->sensor_;\n> \tStatus status = Valid;\n> \n> @@ -449,7 +453,7 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n> \n> \t/* Cap the number of entries to the available streams. */\n> \tif (config_.size() > 1) {\n> -\t\tconfig_.resize(1);\n> +\t\tconfig_.erase(config_.begin() + 1, config_.end());\n\nInteresting, how does this differ from .resize(1)?\n\nAh, I see - it's required because if .resize() is used to 'increase' the\nsize, it uses the default constructor, and that is no longer available.\n\nI think it would be useful to add a comment to explain /why/ we use\n.erase() instead of .resize() here...\n\n\n> \t\tstatus = Adjusted;\n> \t}\n> \n> @@ -537,7 +541,16 @@ CameraConfiguration *PipelineHandlerRkISP1::generateConfiguration(Camera *camera\n> \tif (roles.empty())\n> \t\treturn config;\n> \n> -\tStreamConfiguration cfg{};\n> +\tstd::map> pixelformats;\n> +\n> +\tfor (PixelFormat pixelformat : formats) {\n> +\t\tstd::vector sizes{\n\nWhere do the values for the following SizeRange come from? A comment to\nindicate could be beneficial.\n\nPresumably these are the minimum and maximum capabilities of the ISP\noutput ?\n\nIdeally these values should be determined from the hardware driver, but\nmaybe that's not possible right now.\n\n\n> +\t\t\tSizeRange{ { 32, 16 }, { 4416, 3312 } }\n> +\t\t};\n> +\t\tpixelformats[pixelformat] = sizes;\n> +\t}\n\nI think a newline could be added here,\n\n> +\tStreamFormats format(pixelformats);\n> +\tStreamConfiguration cfg(format);\n\nAnd probably here to aid readability.\n\n> \tcfg.pixelFormat = PixelFormat(DRM_FORMAT_NV12);\n> \tcfg.size = data->sensor_->resolution();\n> \n> diff --git a/src/libcamera/stream.cpp b/src/libcamera/stream.cpp\n> index ea3d214..7e41378 100644\n> --- a/src/libcamera/stream.cpp\n> +++ b/src/libcamera/stream.cpp\n> @@ -275,9 +275,9 @@ SizeRange StreamFormats::range(const PixelFormat &pixelformat) const\n> */\n> \n> /**\n> - * \\todo This method is deprecated and should be removed once all pipeline\n> - * handlers provied StreamFormats.\n> - */\n> +* \\todo This method is deprecated and should be removed once all pipeline\n> +* handlers provied StreamFormats.\n> +*/\n\nThis looks like an unintentional change (just removing some whitespace\non the comment?), and shouldn't be in the patch.\n\nOnce the pipeline handlers all provide StreamFormats, a patch on top\nshould remove this comment of course.\n\n> StreamConfiguration::StreamConfiguration()\n> \t: pixelFormat(0), stream_(nullptr)\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 781016041A\n\tfor ;\n\tMon, 23 Mar 2020 14:10:37 +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 B9C33308;\n\tMon, 23 Mar 2020 14:10:36 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1584969037;\n\tbh=t30xzCfeQYhXxiOdISX6KTvsEFczc6wIl4xOIR0/0jc=;\n\th=Reply-To:Subject:To:References:From:Date:In-Reply-To:From;\n\tb=uy7gjYoyUmS2qIfSxKtWjzlSrQC8HL4yyKtREq7Ez7B1kwJOjotn5WhASH2tlvsTj\n\tbjnH5lS1iSzHyVfx3sqKsB0CESuJhK0W+Uao6MkfKd2qmqBqDYwsGNX3MJZIHLgOpl\n\t+rrBNUAw9w4LFUfbos25W4eLd7Pi+5M4hkERuJjI=","Reply-To":"kieran.bingham@ideasonboard.com","To":"Kaaira Gupta , libcamera-devel@lists.libcamera.org,\n\tHelen Koike ,\n\tVaishali Thakkar ","References":"<20200323084430.GA7565@kaaira-HP-Pavilion-Notebook>","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":"","Date":"Mon, 23 Mar 2020 13:10:33 +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":"<20200323084430.GA7565@kaaira-HP-Pavilion-Notebook>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH LIBCAMERA v3] libcamera: rkisp1: Use\n\tparametered constructor instead of default","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 13:10:37 -0000"}},{"id":4200,"web_url":"https://patchwork.libcamera.org/comment/4200/","msgid":"<20200323133610.GA12411@kaaira-HP-Pavilion-Notebook>","date":"2020-03-23T13:36:10","subject":"Re: [libcamera-devel] [PATCH LIBCAMERA v3] libcamera: rkisp1: Use\n\tparametered constructor instead of default","submitter":{"id":39,"url":"https://patchwork.libcamera.org/api/people/39/","name":"Kaaira Gupta","email":"kgupta@es.iitr.ac.in"},"content":"On Mon, Mar 23, 2020 at 01:10:33PM +0000, Kieran Bingham wrote:\n> Hi Kaaira,\n> \n> On 23/03/2020 08:44, Kaaira Gupta wrote:\n> > Use of default constructor StreamConfiguration() is deprecated, hence\n> > make it private. Make Stream class a friend of StreamConfiguration as it\n> > uses the default constructor.\n> > \n> > Replace default constructor StreamConfiguration() by it's parametered\n> \n> s/parametered/parametrized/ ? If so, both here and in the subject line.\n> \n> > counterpart by using StreamFormats in generateConfiguration() in rkisp1\n> > \n> > Also, use erase(), instead of replace() in validate() to prevent it from\n> > creating a new instance with empty constructor.\n> > \n> > Signed-off-by: Kaaira Gupta \n> > ---\n> > \n> > Changes since v1:\n> > \t- replace resize() by erase() in validate() to prevent it from\n> > creating a new instance with empty constructor.\n> > Changes since v2:\n> > \t- Rearaange the order of declaration of friend class.\n> > \t- Add constructor implementation again to stream.cpp\n> > \t- Corrections in range of erase()\n> > \n> > include/libcamera/stream.h | 4 ++-\n> > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 39 ++++++++++++++++--------\n> > src/libcamera/stream.cpp | 6 ++--\n> > 3 files changed, 32 insertions(+), 17 deletions(-)\n> > \n> > diff --git a/include/libcamera/stream.h b/include/libcamera/stream.h\n> > index b1441f8..a379ebb 100644\n> > --- a/include/libcamera/stream.h\n> > +++ b/include/libcamera/stream.h\n> > @@ -37,7 +37,6 @@ private:\n> > };\n> > \n> > struct StreamConfiguration {\n> > -\tStreamConfiguration();\n> > \tStreamConfiguration(const StreamFormats &formats);\n> > \n> > \tPixelFormat pixelFormat;\n> > @@ -52,6 +51,9 @@ struct StreamConfiguration {\n> > \tstd::string toString() const;\n> > \n> > private:\n> > +\tfriend class Stream;\n> > +\tStreamConfiguration();\n> > +\n> \n> Because the change to move the StreamConfiguration(); to being private\n> causes multiple breakage, I think we should have it as a separate\n> change, so this has certainly already become a series :-)\n\nExactly, it does cause a lot of breakage. But I am having a hard time\nfiguring out what all they are because I think I haven't figured it out\nright how I can run tests on one thing at a time or maybe 'stop' the\ntests on a particular thing? Can you please help me with it? :/\n\n> \n> The series should thus fix all breakages (before it breaks) then move\n> this at the end of the series. Probably along with the removal of the \\todo\n> \n> \n> > \tStream *stream_;\n> > \tStreamFormats formats_;\n> > };\n> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > index 2f909ce..6839e3c 100644\n> > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > @@ -218,6 +218,21 @@ private:\n> > \tCamera *activeCamera_;\n> > };\n> > \n> > +namespace {\n> > +\n> > +static const std::array formats{\n> > +\tPixelFormat(DRM_FORMAT_YUYV),\n> > +\tPixelFormat(DRM_FORMAT_YVYU),\n> > +\tPixelFormat(DRM_FORMAT_VYUY),\n> > +\tPixelFormat(DRM_FORMAT_NV16),\n> > +\tPixelFormat(DRM_FORMAT_NV61),\n> > +\tPixelFormat(DRM_FORMAT_NV21),\n> > +\tPixelFormat(DRM_FORMAT_NV12),\n> > +\t/* \\todo Add support for 8-bit greyscale to DRM formats */\n> > +};\n> > +\n> > +} /* namespace */\n> > +\n> > RkISP1Frames::RkISP1Frames(PipelineHandler *pipe)\n> > \t: pipe_(dynamic_cast(pipe))\n> > {\n> > @@ -430,17 +445,6 @@ RkISP1CameraConfiguration::RkISP1CameraConfiguration(Camera *camera,\n> > \n> > CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n> > {\n> > -\tstatic const std::array formats{\n> > -\t\tPixelFormat(DRM_FORMAT_YUYV),\n> > -\t\tPixelFormat(DRM_FORMAT_YVYU),\n> > -\t\tPixelFormat(DRM_FORMAT_VYUY),\n> > -\t\tPixelFormat(DRM_FORMAT_NV16),\n> > -\t\tPixelFormat(DRM_FORMAT_NV61),\n> > -\t\tPixelFormat(DRM_FORMAT_NV21),\n> > -\t\tPixelFormat(DRM_FORMAT_NV12),\n> > -\t\t/* \\todo Add support for 8-bit greyscale to DRM formats */\n> > -\t};\n> > -\n> > \tconst CameraSensor *sensor = data_->sensor_;\n> > \tStatus status = Valid;\n> > \n> > @@ -449,7 +453,7 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n> > \n> > \t/* Cap the number of entries to the available streams. */\n> > \tif (config_.size() > 1) {\n> > -\t\tconfig_.resize(1);\n> > +\t\tconfig_.erase(config_.begin() + 1, config_.end());\n> \n> Interesting, how does this differ from .resize(1)?\n> \n> Ah, I see - it's required because if .resize() is used to 'increase' the\n> size, it uses the default constructor, and that is no longer available.\n\nYes\n\n> \n> I think it would be useful to add a comment to explain /why/ we use\n> .erase() instead of .resize() here...\n\nOkay I will\n\n> \n> \n> > \t\tstatus = Adjusted;\n> > \t}\n> > \n> > @@ -537,7 +541,16 @@ CameraConfiguration *PipelineHandlerRkISP1::generateConfiguration(Camera *camera\n> > \tif (roles.empty())\n> > \t\treturn config;\n> > \n> > -\tStreamConfiguration cfg{};\n> > +\tstd::map> pixelformats;\n> > +\n> > +\tfor (PixelFormat pixelformat : formats) {\n> > +\t\tstd::vector sizes{\n> \n> Where do the values for the following SizeRange come from? A comment to\n> indicate could be beneficial.\n> \n> Presumably these are the minimum and maximum capabilities of the ISP\n> output ?\n\nYes, I took them from here in validate() :\n\n cfg.size.width = std::max(32U, std::min(4416U, cfg.size.width));\n cfg.size.height = std::max(16U, std::min(3312U, cfg.size.height));\n\n\n> \n> Ideally these values should be determined from the hardware driver, but\n> maybe that's not possible right now.\n> \n> \n> > +\t\t\tSizeRange{ { 32, 16 }, { 4416, 3312 } }\n> > +\t\t};\n> > +\t\tpixelformats[pixelformat] = sizes;\n> > +\t}\n> \n> I think a newline could be added here,\n> \n> > +\tStreamFormats format(pixelformats);\n> > +\tStreamConfiguration cfg(format);\n> \n> And probably here to aid readability.\n> \n> > \tcfg.pixelFormat = PixelFormat(DRM_FORMAT_NV12);\n> > \tcfg.size = data->sensor_->resolution();\n> > \n> > diff --git a/src/libcamera/stream.cpp b/src/libcamera/stream.cpp\n> > index ea3d214..7e41378 100644\n> > --- a/src/libcamera/stream.cpp\n> > +++ b/src/libcamera/stream.cpp\n> > @@ -275,9 +275,9 @@ SizeRange StreamFormats::range(const PixelFormat &pixelformat) const\n> > */\n> > \n> > /**\n> > - * \\todo This method is deprecated and should be removed once all pipeline\n> > - * handlers provied StreamFormats.\n> > - */\n> > +* \\todo This method is deprecated and should be removed once all pipeline\n> > +* handlers provied StreamFormats.\n> > +*/\n> \n> This looks like an unintentional change (just removing some whitespace\n> on the comment?), and shouldn't be in the patch.\n\nYes, I'll fix all the whitespace errors.\n\n> \n> Once the pipeline handlers all provide StreamFormats, a patch on top\n> should remove this comment of course.\n> \n> > StreamConfiguration::StreamConfiguration()\n> > \t: pixelFormat(0), stream_(nullptr)\n> > {\n> > \n> \n> -- \n> Regards\n> --\n> Kieran","headers":{"Return-Path":"","Received":["from mail-pf1-x444.google.com (mail-pf1-x444.google.com\n\t[IPv6:2607:f8b0:4864:20::444])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D35336041A\n\tfor ;\n\tMon, 23 Mar 2020 14:36:19 +0100 (CET)","by mail-pf1-x444.google.com with SMTP id b72so7490658pfb.11\n\tfor ;\n\tMon, 23 Mar 2020 06:36:19 -0700 (PDT)","from kaaira-HP-Pavilion-Notebook ([103.113.213.154])\n\tby smtp.gmail.com with ESMTPSA id\n\te126sm13295508pfa.122.2020.03.23.06.36.15\n\t(version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256);\n\tMon, 23 Mar 2020 06:36:17 -0700 (PDT)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=es-iitr-ac-in.20150623.gappssmtp.com; s=20150623;\n\th=from:date:to:cc:subject:message-id:references:mime-version\n\t:content-disposition:in-reply-to:user-agent;\n\tbh=odwgKog6purj/BMdOJoeEj+E79KMuY4BaiBnG9Cnthc=;\n\tb=Rc6OQr/WMBr30ajT/vGI6DeaG//dTEtKL30/XYdF+McCuYyQD60M1aNXZHC9v/A4Mc\n\tJK9T+KDrsM2tRhnfDw5ayNmbihDpgmfPsMxpM3h+ZeP+S1bvkAiXuOA2cMBBQUA1OYQe\n\tklnWFu2ki6bMstrmD2jDwGczXSl2VBDYAQOJ21pO+CsI02RWaGV2HvqkkkZVH/FweAwP\n\ttMOVDFJjvQ09Lqki+wK/nzhsSgjiLTrpDRqzM40fzo6Gu75nnBXTP1qxKu0yRm8TkkmJ\n\t4B08V0skSVd6YMpl6YdM9FkQ3/WhAMuhzmFrzlPrR9rimMTtx9iKXFw/6qhF6T+5oB71\n\tY23w==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:from:date:to:cc:subject:message-id:references\n\t:mime-version:content-disposition:in-reply-to:user-agent;\n\tbh=odwgKog6purj/BMdOJoeEj+E79KMuY4BaiBnG9Cnthc=;\n\tb=YxwNfQbEyQWo7jpkuAxP73sA9rmACPzFH3n+hBltGRXOqKAXkfLwjD3h90ciEtwdM5\n\t77bbwbimktsdBxxqVgmhBOrtg22lM54ZceMVCcAmveEIz1b/LNsXs5ocsBpTTEnUXKs+\n\tjBBCAhP4aw+sVSW8xl2nEInwebWGRd2yjCfz+zzpeGEWTEwcODEF5nt1/hu8T1N2FFmU\n\thVTJBjue6Cp4vfXCmK2Ph0dMFiH8cA/v4rXsPTxXKOe7RgI+CKRe5QdTa3VvCFqCe0qM\n\t+BRrP0DPWQntM5MJol9JKw7umMZfEnRjav41DMrgXJITXbmdTqq/4gct03enFO6A2NuS\n\t6dBQ==","X-Gm-Message-State":"ANhLgQ2Bwz9KycKDWq/RODFvk+tmVr7wKSNX1XIeGhKOv5B/AkzqhBdy\n\tFHGiUlTMH0hhZQ9PrRb7WHh2BA==","X-Google-Smtp-Source":"ADFU+vuKB96QrnvuxjTHObdED7r9kGTSsaPHJgyGPQA2OpQdKkl/c+CSNla+ugg3TaTZtjGgkQ6H+Q==","X-Received":"by 2002:a62:aa01:: with SMTP id e1mr5511839pff.123.1584970578024;\n\tMon, 23 Mar 2020 06:36:18 -0700 (PDT)","From":"Kaaira Gupta ","X-Google-Original-From":"Kaaira Gupta ","Date":"Mon, 23 Mar 2020 19:06:10 +0530","To":"Kieran Bingham ","Cc":"Kaaira Gupta , libcamera-devel@lists.libcamera.org,\n\tHelen Koike ,\n\tVaishali Thakkar ","Message-ID":"<20200323133610.GA12411@kaaira-HP-Pavilion-Notebook>","References":"<20200323084430.GA7565@kaaira-HP-Pavilion-Notebook>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"","User-Agent":"Mutt/1.9.4 (2018-02-28)","Subject":"Re: [libcamera-devel] [PATCH LIBCAMERA v3] libcamera: rkisp1: Use\n\tparametered constructor instead of default","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 13:36:20 -0000"}},{"id":4228,"web_url":"https://patchwork.libcamera.org/comment/4228/","msgid":"<1f94a99a-a966-918b-b3d9-bb3a98fde6f4@ideasonboard.com>","date":"2020-03-23T16:28:24","subject":"Re: [libcamera-devel] [PATCH LIBCAMERA v3] libcamera: rkisp1: Use\n\tparametered constructor instead of default","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Kaaira,\n\nOn 23/03/2020 13:36, Kaaira Gupta wrote:\n> On Mon, Mar 23, 2020 at 01:10:33PM +0000, Kieran Bingham wrote:\n>> Hi Kaaira,\n>>\n>> On 23/03/2020 08:44, Kaaira Gupta wrote:\n>>> Use of default constructor StreamConfiguration() is deprecated, hence\n>>> make it private. Make Stream class a friend of StreamConfiguration as it\n>>> uses the default constructor.\n>>>\n>>> Replace default constructor StreamConfiguration() by it's parametered\n>>\n>> s/parametered/parametrized/ ? If so, both here and in the subject line.\n>>\n>>> counterpart by using StreamFormats in generateConfiguration() in rkisp1\n>>>\n>>> Also, use erase(), instead of replace() in validate() to prevent it from\n>>> creating a new instance with empty constructor.\n>>>\n>>> Signed-off-by: Kaaira Gupta \n>>> ---\n>>>\n>>> Changes since v1:\n>>> \t- replace resize() by erase() in validate() to prevent it from\n>>> creating a new instance with empty constructor.\n>>> Changes since v2:\n>>> \t- Rearaange the order of declaration of friend class.\n>>> \t- Add constructor implementation again to stream.cpp\n>>> \t- Corrections in range of erase()\n>>>\n>>> include/libcamera/stream.h | 4 ++-\n>>> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 39 ++++++++++++++++--------\n>>> src/libcamera/stream.cpp | 6 ++--\n>>> 3 files changed, 32 insertions(+), 17 deletions(-)\n>>>\n>>> diff --git a/include/libcamera/stream.h b/include/libcamera/stream.h\n>>> index b1441f8..a379ebb 100644\n>>> --- a/include/libcamera/stream.h\n>>> +++ b/include/libcamera/stream.h\n>>> @@ -37,7 +37,6 @@ private:\n>>> };\n>>> \n>>> struct StreamConfiguration {\n>>> -\tStreamConfiguration();\n>>> \tStreamConfiguration(const StreamFormats &formats);\n>>> \n>>> \tPixelFormat pixelFormat;\n>>> @@ -52,6 +51,9 @@ struct StreamConfiguration {\n>>> \tstd::string toString() const;\n>>> \n>>> private:\n>>> +\tfriend class Stream;\n>>> +\tStreamConfiguration();\n>>> +\n>>\n>> Because the change to move the StreamConfiguration(); to being private\n>> causes multiple breakage, I think we should have it as a separate\n>> change, so this has certainly already become a series :-)\n> \n> Exactly, it does cause a lot of breakage. But I am having a hard time\n> figuring out what all they are because I think I haven't figured it out\n> right how I can run tests on one thing at a time or maybe 'stop' the\n> tests on a particular thing? Can you please help me with it? :/\n\nI assume here you mean that one of the tests gets broken with the\nchange? (test/v4l2_videodevice/buffer_cache I think)\n\nThe important thing to remember here is that the change you make to the\nrkisp1 should still work without the change to the StreamConfiguration\nbeing made private, so I would recommend splitting that part out already.\n\nYou should be able to test your change to RKISP1 (by ensuring that it\ncompiles cleanly, - you don't have hardware to test it running) entirely\nby *not* including the patch which makes StreamConfiguration private.\n\nAnd indeed, we could expect a few patches that can be tested\nindependently before the StreamConfiguration is moved...\n\nThe series would end up looking something like this:\n\n\n - [1/n] pipeline: Remove use of .resize() in validate() calls.\n - [2/n] pipeline: rkisp1: Use paramaterized StreamConfiguration\n - [3/n] pipeline: ipu3: Use paramaterized StreamConfiguration\n - [4/n] test: Fix v4l2_videodevice/buffer_cache test\n - [5/n] stream: Make default constructor of StreamConfiguration private\n\n\nPatch 1 should fix all uses of the .resize() which we know won't work\n(so that's vimc, uvc, rkisp1, ipu3).\n\nPatch 2 should then handle the rkisp1 changes as you've already identified.\n\nYou should be able to compile cleanly and run the test suite on both of\nthose patches independently.\n\n\nPatch 3 can then apply the similar changes you've made to the RKISP..\n\n\nPatch 4 should then fix the issue that will be faced in the buffer_cache\ntest. I haven't looked at that yet to determine the solution. Is this\nthe point you are currently blocked?\n\n\nIf there are any other fixes needed, they'd go in here, and patch 5\nwould come later of course...\n\n\nPatch 5 should handle the privatisation of the StreamConfiguration\ndefault constructor.\n\n\n\n>> The series should thus fix all breakages (before it breaks) then move\n>> this at the end of the series. Probably along with the removal of the \\todo\n>>\n>>\n>>> \tStream *stream_;\n>>> \tStreamFormats formats_;\n>>> };\n>>> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n>>> index 2f909ce..6839e3c 100644\n>>> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n>>> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n>>> @@ -218,6 +218,21 @@ private:\n>>> \tCamera *activeCamera_;\n>>> };\n>>> \n>>> +namespace {\n>>> +\n>>> +static const std::array formats{\n>>> +\tPixelFormat(DRM_FORMAT_YUYV),\n>>> +\tPixelFormat(DRM_FORMAT_YVYU),\n>>> +\tPixelFormat(DRM_FORMAT_VYUY),\n>>> +\tPixelFormat(DRM_FORMAT_NV16),\n>>> +\tPixelFormat(DRM_FORMAT_NV61),\n>>> +\tPixelFormat(DRM_FORMAT_NV21),\n>>> +\tPixelFormat(DRM_FORMAT_NV12),\n>>> +\t/* \\todo Add support for 8-bit greyscale to DRM formats */\n>>> +};\n>>> +\n>>> +} /* namespace */\n>>> +\n>>> RkISP1Frames::RkISP1Frames(PipelineHandler *pipe)\n>>> \t: pipe_(dynamic_cast(pipe))\n>>> {\n>>> @@ -430,17 +445,6 @@ RkISP1CameraConfiguration::RkISP1CameraConfiguration(Camera *camera,\n>>> \n>>> CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n>>> {\n>>> -\tstatic const std::array formats{\n>>> -\t\tPixelFormat(DRM_FORMAT_YUYV),\n>>> -\t\tPixelFormat(DRM_FORMAT_YVYU),\n>>> -\t\tPixelFormat(DRM_FORMAT_VYUY),\n>>> -\t\tPixelFormat(DRM_FORMAT_NV16),\n>>> -\t\tPixelFormat(DRM_FORMAT_NV61),\n>>> -\t\tPixelFormat(DRM_FORMAT_NV21),\n>>> -\t\tPixelFormat(DRM_FORMAT_NV12),\n>>> -\t\t/* \\todo Add support for 8-bit greyscale to DRM formats */\n>>> -\t};\n>>> -\n>>> \tconst CameraSensor *sensor = data_->sensor_;\n>>> \tStatus status = Valid;\n>>> \n>>> @@ -449,7 +453,7 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n>>> \n>>> \t/* Cap the number of entries to the available streams. */\n>>> \tif (config_.size() > 1) {\n>>> -\t\tconfig_.resize(1);\n>>> +\t\tconfig_.erase(config_.begin() + 1, config_.end());\n>>\n>> Interesting, how does this differ from .resize(1)?\n>>\n>> Ah, I see - it's required because if .resize() is used to 'increase' the\n>> size, it uses the default constructor, and that is no longer available.\n> \n> Yes\n> \n>>\n>> I think it would be useful to add a comment to explain /why/ we use\n>> .erase() instead of .resize() here...\n> \n> Okay I will\n\nIf you group the vimc/uvc element of this change, it could potentially\nbe a single patch making this change for all of the pipeline handlers at\nthe same time, each one doing the 'right' change and adding the same\ncomment or such.\n\nThat patch would then come before the changes to generateConfiguration\nin this pipeline handler.\n\n\n> \n>>\n>>\n>>> \t\tstatus = Adjusted;\n>>> \t}\n>>> \n>>> @@ -537,7 +541,16 @@ CameraConfiguration *PipelineHandlerRkISP1::generateConfiguration(Camera *camera\n>>> \tif (roles.empty())\n>>> \t\treturn config;\n>>> \n>>> -\tStreamConfiguration cfg{};\n>>> +\tstd::map> pixelformats;\n>>> +\n>>> +\tfor (PixelFormat pixelformat : formats) {\n>>> +\t\tstd::vector sizes{\n>>\n>> Where do the values for the following SizeRange come from? A comment to\n>> indicate could be beneficial.\n>>\n>> Presumably these are the minimum and maximum capabilities of the ISP\n>> output ?\n> \n> Yes, I took them from here in validate() :\n> \n> cfg.size.width = std::max(32U, std::min(4416U, cfg.size.width));\n> cfg.size.height = std::max(16U, std::min(3312U, cfg.size.height));\n\nAh yes, it would be nice if these were in a common location - but I\ndon't think you need to worry about that for now in this patch.\n\n>> Ideally these values should be determined from the hardware driver, but\n>> maybe that's not possible right now.\n>>\n>>\n>>> +\t\t\tSizeRange{ { 32, 16 }, { 4416, 3312 } }\n>>> +\t\t};\n>>> +\t\tpixelformats[pixelformat] = sizes;\n>>> +\t}\n>>\n>> I think a newline could be added here,\n>>\n>>> +\tStreamFormats format(pixelformats);\n>>> +\tStreamConfiguration cfg(format);\n>>\n>> And probably here to aid readability.\n>>\n>>> \tcfg.pixelFormat = PixelFormat(DRM_FORMAT_NV12);\n>>> \tcfg.size = data->sensor_->resolution();\n>>> \n>>> diff --git a/src/libcamera/stream.cpp b/src/libcamera/stream.cpp\n>>> index ea3d214..7e41378 100644\n>>> --- a/src/libcamera/stream.cpp\n>>> +++ b/src/libcamera/stream.cpp\n>>> @@ -275,9 +275,9 @@ SizeRange StreamFormats::range(const PixelFormat &pixelformat) const\n>>> */\n>>> \n>>> /**\n>>> - * \\todo This method is deprecated and should be removed once all pipeline\n>>> - * handlers provied StreamFormats.\n>>> - */\n>>> +* \\todo This method is deprecated and should be removed once all pipeline\n>>> +* handlers provied StreamFormats.\n>>> +*/\n>>\n>> This looks like an unintentional change (just removing some whitespace\n>> on the comment?), and shouldn't be in the patch.\n> \n> Yes, I'll fix all the whitespace errors.\n> \n>>\n>> Once the pipeline handlers all provide StreamFormats, a patch on top\n>> should remove this comment of course.\n>>\n>>> StreamConfiguration::StreamConfiguration()\n>>> \t: pixelFormat(0), stream_(nullptr)\n>>> {\n>>>\n>>\n>> -- \n>> Regards\n>> --\n>> Kieran","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 4C70160429\n\tfor ;\n\tMon, 23 Mar 2020 17:28:28 +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 9B1D3308;\n\tMon, 23 Mar 2020 17:28:27 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1584980907;\n\tbh=ro1lgPvwXsebx5ZTtEvGEVGXy11yYaLjZyffCOVmyVw=;\n\th=Reply-To:Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=kHreqjKkiJdmGHhgNmumamf/+a6tJKu/ZmwthRYe9+9+RoW3jpuJkxs4zH8EL6jAL\n\tFPn6NnTU5687G2le1hHMVqiLXHFQuPOBVUZEZMRpB6LyRutXazY8Mm8AD+PNxiWi+Y\n\tUchyMlcgFQvaLi1YLUu9boYuxuopbt+Cv+u5mv4M=","Reply-To":"kieran.bingham@ideasonboard.com","To":"Kaaira Gupta ","Cc":"libcamera-devel@lists.libcamera.org,\n\tHelen Koike ,\n\tVaishali Thakkar ","References":"<20200323084430.GA7565@kaaira-HP-Pavilion-Notebook>\n\t\n\t<20200323133610.GA12411@kaaira-HP-Pavilion-Notebook>","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":"<1f94a99a-a966-918b-b3d9-bb3a98fde6f4@ideasonboard.com>","Date":"Mon, 23 Mar 2020 16:28:24 +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":"<20200323133610.GA12411@kaaira-HP-Pavilion-Notebook>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH LIBCAMERA v3] libcamera: rkisp1: Use\n\tparametered constructor instead of default","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:28:28 -0000"}},{"id":4245,"web_url":"https://patchwork.libcamera.org/comment/4245/","msgid":"<20200323191016.GA19096@kaaira-HP-Pavilion-Notebook>","date":"2020-03-23T19:10:16","subject":"Re: [libcamera-devel] [PATCH LIBCAMERA v3] libcamera: rkisp1: Use\n\tparametered constructor instead of default","submitter":{"id":39,"url":"https://patchwork.libcamera.org/api/people/39/","name":"Kaaira Gupta","email":"kgupta@es.iitr.ac.in"},"content":"On Mon, Mar 23, 2020 at 04:28:24PM +0000, Kieran Bingham wrote:\n> Hi Kaaira,\n> \n> On 23/03/2020 13:36, Kaaira Gupta wrote:\n> > On Mon, Mar 23, 2020 at 01:10:33PM +0000, Kieran Bingham wrote:\n> >> Hi Kaaira,\n> >>\n> >> On 23/03/2020 08:44, Kaaira Gupta wrote:\n> >>> Use of default constructor StreamConfiguration() is deprecated, hence\n> >>> make it private. Make Stream class a friend of StreamConfiguration as it\n> >>> uses the default constructor.\n> >>>\n> >>> Replace default constructor StreamConfiguration() by it's parametered\n> >>\n> >> s/parametered/parametrized/ ? If so, both here and in the subject line.\n> >>\n> >>> counterpart by using StreamFormats in generateConfiguration() in rkisp1\n> >>>\n> >>> Also, use erase(), instead of replace() in validate() to prevent it from\n> >>> creating a new instance with empty constructor.\n> >>>\n> >>> Signed-off-by: Kaaira Gupta \n> >>> ---\n> >>>\n> >>> Changes since v1:\n> >>> \t- replace resize() by erase() in validate() to prevent it from\n> >>> creating a new instance with empty constructor.\n> >>> Changes since v2:\n> >>> \t- Rearaange the order of declaration of friend class.\n> >>> \t- Add constructor implementation again to stream.cpp\n> >>> \t- Corrections in range of erase()\n> >>>\n> >>> include/libcamera/stream.h | 4 ++-\n> >>> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 39 ++++++++++++++++--------\n> >>> src/libcamera/stream.cpp | 6 ++--\n> >>> 3 files changed, 32 insertions(+), 17 deletions(-)\n> >>>\n> >>> diff --git a/include/libcamera/stream.h b/include/libcamera/stream.h\n> >>> index b1441f8..a379ebb 100644\n> >>> --- a/include/libcamera/stream.h\n> >>> +++ b/include/libcamera/stream.h\n> >>> @@ -37,7 +37,6 @@ private:\n> >>> };\n> >>> \n> >>> struct StreamConfiguration {\n> >>> -\tStreamConfiguration();\n> >>> \tStreamConfiguration(const StreamFormats &formats);\n> >>> \n> >>> \tPixelFormat pixelFormat;\n> >>> @@ -52,6 +51,9 @@ struct StreamConfiguration {\n> >>> \tstd::string toString() const;\n> >>> \n> >>> private:\n> >>> +\tfriend class Stream;\n> >>> +\tStreamConfiguration();\n> >>> +\n> >>\n> >> Because the change to move the StreamConfiguration(); to being private\n> >> causes multiple breakage, I think we should have it as a separate\n> >> change, so this has certainly already become a series :-)\n> > \n> > Exactly, it does cause a lot of breakage. But I am having a hard time\n> > figuring out what all they are because I think I haven't figured it out\n> > right how I can run tests on one thing at a time or maybe 'stop' the\n> > tests on a particular thing? Can you please help me with it? :/\n> \n> I assume here you mean that one of the tests gets broken with the\n> change? (test/v4l2_videodevice/buffer_cache I think)\n> \n> The important thing to remember here is that the change you make to the\n> rkisp1 should still work without the change to the StreamConfiguration\n> being made private, so I would recommend splitting that part out already.\n> \n> You should be able to test your change to RKISP1 (by ensuring that it\n> compiles cleanly, - you don't have hardware to test it running) entirely\n> by *not* including the patch which makes StreamConfiguration private.\n> \n> And indeed, we could expect a few patches that can be tested\n> independently before the StreamConfiguration is moved...\n> \n> The series would end up looking something like this:\n> \n> \n> - [1/n] pipeline: Remove use of .resize() in validate() calls.\n> - [2/n] pipeline: rkisp1: Use paramaterized StreamConfiguration\n> - [3/n] pipeline: ipu3: Use paramaterized StreamConfiguration\n> - [4/n] test: Fix v4l2_videodevice/buffer_cache test\n> - [5/n] stream: Make default constructor of StreamConfiguration private\n> \n> \n> Patch 1 should fix all uses of the .resize() which we know won't work\n> (so that's vimc, uvc, rkisp1, ipu3).\n> \n> Patch 2 should then handle the rkisp1 changes as you've already identified.\n> \n> You should be able to compile cleanly and run the test suite on both of\n> those patches independently.\n> \n> \n> Patch 3 can then apply the similar changes you've made to the RKISP..\n> \n> \n> Patch 4 should then fix the issue that will be faced in the buffer_cache\n> test. I haven't looked at that yet to determine the solution. Is this\n> the point you are currently blocked?\n\nYes, I was stuck here. Your mail clears this of-course..I'll send in the\nthree patches, and we'll come on this after that.\nThanks!\n\n> \n> \n> If there are any other fixes needed, they'd go in here, and patch 5\n> would come later of course...\n> \n> \n> Patch 5 should handle the privatisation of the StreamConfiguration\n> default constructor.\n> \n> \n> \n> >> The series should thus fix all breakages (before it breaks) then move\n> >> this at the end of the series. Probably along with the removal of the \\todo\n> >>\n> >>\n> >>> \tStream *stream_;\n> >>> \tStreamFormats formats_;\n> >>> };\n> >>> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> >>> index 2f909ce..6839e3c 100644\n> >>> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> >>> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> >>> @@ -218,6 +218,21 @@ private:\n> >>> \tCamera *activeCamera_;\n> >>> };\n> >>> \n> >>> +namespace {\n> >>> +\n> >>> +static const std::array formats{\n> >>> +\tPixelFormat(DRM_FORMAT_YUYV),\n> >>> +\tPixelFormat(DRM_FORMAT_YVYU),\n> >>> +\tPixelFormat(DRM_FORMAT_VYUY),\n> >>> +\tPixelFormat(DRM_FORMAT_NV16),\n> >>> +\tPixelFormat(DRM_FORMAT_NV61),\n> >>> +\tPixelFormat(DRM_FORMAT_NV21),\n> >>> +\tPixelFormat(DRM_FORMAT_NV12),\n> >>> +\t/* \\todo Add support for 8-bit greyscale to DRM formats */\n> >>> +};\n> >>> +\n> >>> +} /* namespace */\n> >>> +\n> >>> RkISP1Frames::RkISP1Frames(PipelineHandler *pipe)\n> >>> \t: pipe_(dynamic_cast(pipe))\n> >>> {\n> >>> @@ -430,17 +445,6 @@ RkISP1CameraConfiguration::RkISP1CameraConfiguration(Camera *camera,\n> >>> \n> >>> CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n> >>> {\n> >>> -\tstatic const std::array formats{\n> >>> -\t\tPixelFormat(DRM_FORMAT_YUYV),\n> >>> -\t\tPixelFormat(DRM_FORMAT_YVYU),\n> >>> -\t\tPixelFormat(DRM_FORMAT_VYUY),\n> >>> -\t\tPixelFormat(DRM_FORMAT_NV16),\n> >>> -\t\tPixelFormat(DRM_FORMAT_NV61),\n> >>> -\t\tPixelFormat(DRM_FORMAT_NV21),\n> >>> -\t\tPixelFormat(DRM_FORMAT_NV12),\n> >>> -\t\t/* \\todo Add support for 8-bit greyscale to DRM formats */\n> >>> -\t};\n> >>> -\n> >>> \tconst CameraSensor *sensor = data_->sensor_;\n> >>> \tStatus status = Valid;\n> >>> \n> >>> @@ -449,7 +453,7 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n> >>> \n> >>> \t/* Cap the number of entries to the available streams. */\n> >>> \tif (config_.size() > 1) {\n> >>> -\t\tconfig_.resize(1);\n> >>> +\t\tconfig_.erase(config_.begin() + 1, config_.end());\n> >>\n> >> Interesting, how does this differ from .resize(1)?\n> >>\n> >> Ah, I see - it's required because if .resize() is used to 'increase' the\n> >> size, it uses the default constructor, and that is no longer available.\n> > \n> > Yes\n> > \n> >>\n> >> I think it would be useful to add a comment to explain /why/ we use\n> >> .erase() instead of .resize() here...\n> > \n> > Okay I will\n> \n> If you group the vimc/uvc element of this change, it could potentially\n> be a single patch making this change for all of the pipeline handlers at\n> the same time, each one doing the 'right' change and adding the same\n> comment or such.\n\nOkay\n\n> \n> That patch would then come before the changes to generateConfiguration\n> in this pipeline handler.\n> \n> \n> > \n> >>\n> >>\n> >>> \t\tstatus = Adjusted;\n> >>> \t}\n> >>> \n> >>> @@ -537,7 +541,16 @@ CameraConfiguration *PipelineHandlerRkISP1::generateConfiguration(Camera *camera\n> >>> \tif (roles.empty())\n> >>> \t\treturn config;\n> >>> \n> >>> -\tStreamConfiguration cfg{};\n> >>> +\tstd::map> pixelformats;\n> >>> +\n> >>> +\tfor (PixelFormat pixelformat : formats) {\n> >>> +\t\tstd::vector sizes{\n> >>\n> >> Where do the values for the following SizeRange come from? A comment to\n> >> indicate could be beneficial.\n> >>\n> >> Presumably these are the minimum and maximum capabilities of the ISP\n> >> output ?\n> > \n> > Yes, I took them from here in validate() :\n> > \n> > cfg.size.width = std::max(32U, std::min(4416U, cfg.size.width));\n> > cfg.size.height = std::max(16U, std::min(3312U, cfg.size.height));\n> \n> Ah yes, it would be nice if these were in a common location - but I\n> don't think you need to worry about that for now in this patch.\n> \n> >> Ideally these values should be determined from the hardware driver, but\n> >> maybe that's not possible right now.\n> >>\n> >>\n> >>> +\t\t\tSizeRange{ { 32, 16 }, { 4416, 3312 } }\n> >>> +\t\t};\n> >>> +\t\tpixelformats[pixelformat] = sizes;\n> >>> +\t}\n> >>\n> >> I think a newline could be added here,\n> >>\n> >>> +\tStreamFormats format(pixelformats);\n> >>> +\tStreamConfiguration cfg(format);\n> >>\n> >> And probably here to aid readability.\n> >>\n> >>> \tcfg.pixelFormat = PixelFormat(DRM_FORMAT_NV12);\n> >>> \tcfg.size = data->sensor_->resolution();\n> >>> \n> >>> diff --git a/src/libcamera/stream.cpp b/src/libcamera/stream.cpp\n> >>> index ea3d214..7e41378 100644\n> >>> --- a/src/libcamera/stream.cpp\n> >>> +++ b/src/libcamera/stream.cpp\n> >>> @@ -275,9 +275,9 @@ SizeRange StreamFormats::range(const PixelFormat &pixelformat) const\n> >>> */\n> >>> \n> >>> /**\n> >>> - * \\todo This method is deprecated and should be removed once all pipeline\n> >>> - * handlers provied StreamFormats.\n> >>> - */\n> >>> +* \\todo This method is deprecated and should be removed once all pipeline\n> >>> +* handlers provied StreamFormats.\n> >>> +*/\n> >>\n> >> This looks like an unintentional change (just removing some whitespace\n> >> on the comment?), and shouldn't be in the patch.\n> > \n> > Yes, I'll fix all the whitespace errors.\n> > \n> >>\n> >> Once the pipeline handlers all provide StreamFormats, a patch on top\n> >> should remove this comment of course.\n> >>\n> >>> StreamConfiguration::StreamConfiguration()\n> >>> \t: pixelFormat(0), stream_(nullptr)\n> >>> {\n> >>>\n> >>\n> >> -- \n> >> Regards\n> >> --\n> >> Kieran\n> \n> -- \n> Regards\n> --\n> Kieran\n\nThanks!\n--\nKaaira","headers":{"Return-Path":"","Received":["from mail-pg1-x541.google.com (mail-pg1-x541.google.com\n\t[IPv6:2607:f8b0:4864:20::541])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E636360429\n\tfor ;\n\tMon, 23 Mar 2020 20:10:25 +0100 (CET)","by mail-pg1-x541.google.com with SMTP id a32so7685627pga.4\n\tfor ;\n\tMon, 23 Mar 2020 12:10:25 -0700 (PDT)","from kaaira-HP-Pavilion-Notebook ([103.113.213.154])\n\tby smtp.gmail.com with ESMTPSA id\n\tbt19sm329119pjb.3.2020.03.23.12.10.20\n\t(version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256);\n\tMon, 23 Mar 2020 12:10:22 -0700 (PDT)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=es-iitr-ac-in.20150623.gappssmtp.com; s=20150623;\n\th=from:date:to:cc:subject:message-id:references:mime-version\n\t:content-disposition:in-reply-to:user-agent;\n\tbh=TBQqcDm/293n0upjSmQjW55YWl6vWg/f74wJ8BDCWgA=;\n\tb=WinZvDSjkIsqah7yPM78mdbx4NIFXMK0APJeHtRZoH3lzu5+Rc1pmtOmMyp30k0jzx\n\t0WSdIQLHXIB9UPG4qVJapVSm6eRKPd/WDkSgq+RECLpuWUySxEyiJDwUYWaOpBkhMaBo\n\t5FhbRQu+VHltVQjcfqAmslCCfiKrtKtB7SGGtnrS+oJLzJztLQkrJg7oTOXjJDviDvpp\n\tMTWOtf03+FkScRn27zyO1mB/Yc8MkYCs56PIjSBEsBbtLXQ+nRd9G2EQny/SuQevtVVE\n\tyUpEOzq+k054sroDJtYEAPMFqUSjnNsRT0hIeAIw6Idiqyy0dUnp45qKp8L9vOKnCccL\n\tOWsQ==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:from:date:to:cc:subject:message-id:references\n\t:mime-version:content-disposition:in-reply-to:user-agent;\n\tbh=TBQqcDm/293n0upjSmQjW55YWl6vWg/f74wJ8BDCWgA=;\n\tb=gPpwgEllCSZLykM85Z4ZdVohuy/XT5PC5vKaUIfJeEVcSCZqtYndFTmweUeMjWxH5L\n\teNS5Ubhc5XVU8NnZcNa2hrb8MROnCLLQ421NvxqOT++R3k5E6p0DXOy14WM3l4luQtTM\n\tpB9AnEZ4xzE7fGWGgK0rGlQIPD7QjzyzHTKVXiZbb5Gljm3LtATCVdrGkBPPoKEmvxhE\n\tubKPyaKW7JLT2lkxBLsLAowzdiKc3aKbQ9j62sgdROA9d/8WeqDnz941sAHI2bQtvBZW\n\tHww0BeE8gpXQ9PooPjPpmYCrWXDGVgYJEKMDzaKw9loChltU1OOmDw8/vPFdevA3HGsv\n\tu4Yg==","X-Gm-Message-State":"ANhLgQ02BoWo96DZmUxKVbXke/XzMXTJHeCn2J+yRv7y6PAGCjRH5vRq\n\tSfqlKKLMuRFjSIjp16X5YrC7NA==","X-Google-Smtp-Source":"ADFU+vs2R+qE7SfugmoqKQc9Uorxhuygru1lU+TRJvmzPlyxz05MQdvNkKKI8Y9egN0gwI/iwmnufQ==","X-Received":"by 2002:a63:3446:: with SMTP id\n\tb67mr23354531pga.69.1584990623569; \n\tMon, 23 Mar 2020 12:10:23 -0700 (PDT)","From":"Kaaira Gupta ","X-Google-Original-From":"Kaaira Gupta ","Date":"Tue, 24 Mar 2020 00:40:16 +0530","To":"Kieran Bingham ","Cc":"Kaaira Gupta , libcamera-devel@lists.libcamera.org,\n\tHelen Koike ,\n\tVaishali Thakkar ","Message-ID":"<20200323191016.GA19096@kaaira-HP-Pavilion-Notebook>","References":"<20200323084430.GA7565@kaaira-HP-Pavilion-Notebook>\n\t\n\t<20200323133610.GA12411@kaaira-HP-Pavilion-Notebook>\n\t<1f94a99a-a966-918b-b3d9-bb3a98fde6f4@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<1f94a99a-a966-918b-b3d9-bb3a98fde6f4@ideasonboard.com>","User-Agent":"Mutt/1.9.4 (2018-02-28)","Subject":"Re: [libcamera-devel] [PATCH LIBCAMERA v3] libcamera: rkisp1: Use\n\tparametered constructor instead of default","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 19:10:26 -0000"}}]