[{"id":28826,"web_url":"https://patchwork.libcamera.org/comment/28826/","msgid":"<875xy2aq7w.fsf@redhat.com>","date":"2024-03-04T16:58:43","subject":"Re: [PATCH v4 08/18] libcamera: software_isp: Add DebayerCpu class","submitter":{"id":177,"url":"https://patchwork.libcamera.org/api/people/177/","name":"Milan Zamazal","email":"mzamazal@redhat.com"},"content":"Hans de Goede writes:\n\n> Add CPU based debayering implementation. This initial implementation\n> only supports debayering packed 10 bits per pixel bayer data in\n> the 4 standard bayer orders.\n>\n> Doxygen documentation by Dennis Bonke.\n>\n> Tested-by: Bryan O'Donoghue # sc8280xp Lenovo x13s\n> Tested-by: Pavel Machek \n> Reviewed-by: Pavel Machek \n> Co-developed-by: Dennis Bonke \n> Signed-off-by: Dennis Bonke \n> Co-developed-by: Andrey Konovalov \n> Signed-off-by: Andrey Konovalov \n> Co-developed-by: Pavel Machek \n> Signed-off-by: Pavel Machek \n> Signed-off-by: Hans de Goede \n\nThis patch is quite difficult to understand and I wonder what happens if this\npiece of code needs to be modified a year or two later. My primary problem with\nit is tracking all the line pointers and where they point to at each given\nmoment. I think more verbosity (code comments) is desirable unless other\nreviewers think it's crystal clear.\n\n> ---\n> Changes in v3:\n> - Move debayer_cpu.h to src/libcamera/software_isp/\n> - Move documentation to .cpp file\n> - Document how/why an array of src pointers is passed to\n> the debayer functions\n> ---\n> src/libcamera/software_isp/debayer_cpu.cpp | 619 +++++++++++++++++++++\n> src/libcamera/software_isp/debayer_cpu.h | 143 +++++\n> src/libcamera/software_isp/meson.build | 1 +\n> 3 files changed, 763 insertions(+)\n> create mode 100644 src/libcamera/software_isp/debayer_cpu.cpp\n> create mode 100644 src/libcamera/software_isp/debayer_cpu.h\n>\n> diff --git a/src/libcamera/software_isp/debayer_cpu.cpp b/src/libcamera/software_isp/debayer_cpu.cpp\n> new file mode 100644\n> index 00000000..53e90776\n> --- /dev/null\n> +++ b/src/libcamera/software_isp/debayer_cpu.cpp\n\n[...]\n\n> +/**\n> + * \\brief Constructs a DebayerCpu object.\n> + * \\param[in] stats Pointer to the stats object to use.\n> + */\n> +DebayerCpu::DebayerCpu(std::unique_ptr stats)\n> +\t: stats_(std::move(stats)), gamma_correction_(1.0)\n> +{\n> +#ifdef __x86_64__\n> +\tenableInputMemcpy_ = false;\n> +#else\n> +\tenableInputMemcpy_ = true;\n> +#endif\n> +\t/* Initialize gamma to 1.0 curve */\n> +\tfor (unsigned int i = 0; i < kGammaLookupSize; i++)\n> +\t\tgamma_[i] = i / 4;\n\nWhat is this division by 4? Is it related to kGammaLookupSize being 1024?\nIf yes then it should derive from that constant.\n\n[...]\n\n> +void DebayerCpu::debayer10P_RGRG_BGR888(uint8_t *dst, const uint8_t *src[])\n> +{\n> +\tconst int width_in_bytes = window_.width * 5 / 4;\n> +\tconst uint8_t *prev = (const uint8_t *)src[0];\n> +\tconst uint8_t *curr = (const uint8_t *)src[1];\n> +\tconst uint8_t *next = (const uint8_t *)src[2];\n> +\n> +\tfor (int x = 0; x < width_in_bytes; x++) {\n> +\t\t/* Even pixel */\n> +\t\tRGGB_BGR888(2, 1, 1)\n> +\t\t/* Odd pixel RGGB -> GRBG*/\n\nMissing space before */.\n\n[...]\n\n> +void DebayerCpu::process2(const uint8_t *src, uint8_t *dst)\n> +{\n> +\tunsigned int y_end = window_.y + window_.height;\n> +\t/* Holds [0] previous- [1] current- [2] next-line */\n> +\tconst uint8_t *linePointers[3];\n> +\n> +\t/* Adjust src to top left corner of the window */\n> +\tsrc += window_.y * inputConfig_.stride + window_.x * inputConfig_.bpp / 8;\n> +\n> +\t/* [x] becomes [x - 1] after initial shiftLinePointers() call */\n> +\tif (window_.y) {\n> +\t\tlinePointers[1] = src - inputConfig_.stride; /* previous-line */\n> +\t\tlinePointers[2] = src;\n> +\t} else {\n> +\t\t/* window_.y == 0, use the next line as prev line */\n\nWhy is this needed here and not in process4?\n\n> +\t\tlinePointers[1] = src + inputConfig_.stride;\n> +\t\tlinePointers[2] = src;\n> +\t\t/* Last 2 lines also need special handling */\n> +\t\ty_end -= 2;\n> +\t}\n> +\n> +\tsetupInputMemcpy(linePointers);\n> +\n> +\tfor (unsigned int y = window_.y; y < y_end; y += 2) {\n> +\t\tshiftLinePointers(linePointers, src);\n> +\t\tmemcpyNextLine(linePointers);\n> +\t\tstats_->processLine0(y, linePointers);\n> +\t\t(this->*debayer0_)(dst, linePointers);\n> +\t\tsrc += inputConfig_.stride;\n> +\t\tdst += outputConfig_.stride;\n> +\n> +\t\tshiftLinePointers(linePointers, src);\n> +\t\tmemcpyNextLine(linePointers);\n> +\t\t(this->*debayer1_)(dst, linePointers);\n> +\t\tsrc += inputConfig_.stride;\n> +\t\tdst += outputConfig_.stride;\n> +\t}\n> +\n> +\tif (window_.y == 0) {\n> +\t\tshiftLinePointers(linePointers, src);\n> +\t\tmemcpyNextLine(linePointers);\n> +\t\tstats_->processLine0(y_end, linePointers);\n> +\t\t(this->*debayer0_)(dst, linePointers);\n> +\t\tsrc += inputConfig_.stride;\n> +\t\tdst += outputConfig_.stride;\n> +\n> +\t\tshiftLinePointers(linePointers, src);\n> +\t\t/* next line may point outside of src, use prev. */\n> +\t\tlinePointers[2] = linePointers[0];\n\nAt least this deserves a more elaborate comment; honestly, I'm lost what's\nhappening here exactly.\n\n> +\t\t(this->*debayer1_)(dst, linePointers);\n> +\t\tsrc += inputConfig_.stride;\n> +\t\tdst += outputConfig_.stride;\n> +\t}\n> +}\n> +\n> +void DebayerCpu::process4(const uint8_t *src, uint8_t *dst)\n> +{\n> +\tconst unsigned int y_end = window_.y + window_.height;\n> +\t/*\n> +\t * This holds pointers to [0] 2-lines-up [1] 1-line-up [2] current-line\n> +\t * [3] 1-line-down [4] 2-lines-down.\n> +\t */\n> +\tconst uint8_t *linePointers[5];\n> +\n> +\t/* Adjust src to top left corner of the window */\n> +\tsrc += window_.y * inputConfig_.stride + window_.x * inputConfig_.bpp / 8;\n> +\n> +\t/* [x] becomes [x - 1] after initial shiftLinePointers() call */\n> +\tlinePointers[1] = src - 2 * inputConfig_.stride;\n> +\tlinePointers[2] = src - inputConfig_.stride;\n> +\tlinePointers[3] = src;\n> +\tlinePointers[4] = src + inputConfig_.stride;\n> +\n> +\tsetupInputMemcpy(linePointers);\n> +\n> +\tfor (unsigned int y = window_.y; y < y_end; y += 4) {\n> +\t\tshiftLinePointers(linePointers, src);\n> +\t\tmemcpyNextLine(linePointers);\n> +\t\tstats_->processLine0(y, linePointers);\n> +\t\t(this->*debayer0_)(dst, linePointers);\n> +\t\tsrc += inputConfig_.stride;\n> +\t\tdst += outputConfig_.stride;\n> +\n> +\t\tshiftLinePointers(linePointers, src);\n> +\t\tmemcpyNextLine(linePointers);\n> +\t\t(this->*debayer1_)(dst, linePointers);\n> +\t\tsrc += inputConfig_.stride;\n> +\t\tdst += outputConfig_.stride;\n> +\n> +\t\tshiftLinePointers(linePointers, src);\n> +\t\tmemcpyNextLine(linePointers);\n> +\t\tstats_->processLine2(y, linePointers);\n> +\t\t(this->*debayer2_)(dst, linePointers);\n> +\t\tsrc += inputConfig_.stride;\n> +\t\tdst += outputConfig_.stride;\n> +\n> +\t\tshiftLinePointers(linePointers, src);\n> +\t\tmemcpyNextLine(linePointers);\n> +\t\t(this->*debayer3_)(dst, linePointers);\n> +\t\tsrc += inputConfig_.stride;\n> +\t\tdst += outputConfig_.stride;\n> +\t}\n> +}\n> +\n\n[...]\n\n> diff --git a/src/libcamera/software_isp/debayer_cpu.h b/src/libcamera/software_isp/debayer_cpu.h\n> new file mode 100644\n> index 00000000..98782838\n> --- /dev/null\n> +++ b/src/libcamera/software_isp/debayer_cpu.h\n\n[...]\n\n> +\tint configure(const StreamConfiguration &inputCfg,\n> +\t\t const std::vector> &outputCfgs);\n> +\tSize patternSize(PixelFormat inputFormat);\n> +\tstd::vector formats(PixelFormat input);\n> +\tstd::tuple\n> +\tstrideAndFrameSize(const PixelFormat &outputFormat, const Size &size);\n> +\tvoid process(FrameBuffer *input, FrameBuffer *output, DebayerParams params);\n> +\tSizeRange sizes(PixelFormat inputFormat, const Size &inputSize);\n> +\n> +\t/**\n> +\t * \\brief Get the file descriptor for the statistics.\n> +\t *\n> +\t * \\return the file descriptor pointing to the statistics.\n> +\t */\n> +\tconst SharedFD &getStatsFD() { return stats_->getStatsFD(); }\n> +\n> +\t/**\n> +\t * \\brief Get the output frame size.\n> +\t *\n> +\t * \\return The output frame size.\n> +\t */\n> +\tunsigned int frameSize() { return outputConfig_.frameSize; }\n> +\n> +private:\n> +\t/**\n> +\t * \\brief Called to debayer 1 line of Bayer input data to output format\n> +\t * \\param[out] dst Pointer to the start of the output line to write\n> +\t * \\param[in] src The input data\n> +\t *\n> +\t * Input data is an array of (patternSize_.height + 1) src\n> +\t * pointers each pointing to a line in the Bayer source. The middle\n> +\t * element of the array will point to the actual line being processed.\n> +\t * Earlier element(s) will point to the previous line(s) and later\n> +\t * element(s) to the next line(s).\n> +\t *\n> +\t * These functions take an array of src pointers, rather then\n\nthen -> than\n\n[...]","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 7C18ABD160\n\tfor ;\n\tMon, 4 Mar 2024 16:58:51 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id BC323627FC;\n\tMon, 4 Mar 2024 17:58:50 +0100 (CET)","from us-smtp-delivery-124.mimecast.com\n\t(us-smtp-delivery-124.mimecast.com [170.10.129.124])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id BBBAF627FC\n\tfor ;\n\tMon, 4 Mar 2024 17:58:49 +0100 (CET)","from mail-wm1-f71.google.com (mail-wm1-f71.google.com\n\t[209.85.128.71]) by relay.mimecast.com with ESMTP with STARTTLS\n\t(version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id\n\tus-mta-311-XCDHBVRUN7O88gZbnEjdAw-1; Mon, 04 Mar 2024 11:58:47 -0500","by mail-wm1-f71.google.com with SMTP id\n\t5b1f17b1804b1-412c7ee0c97so21870725e9.0\n\tfor ;\n\tMon, 04 Mar 2024 08:58:47 -0800 (PST)","from nuthatch (nat-pool-brq-t.redhat.com. [213.175.37.10])\n\tby smtp.gmail.com with ESMTPSA id\n\tm19-20020a056000181300b0033d3b8820f8sm12624535wrh.109.2024.03.04.08.58.44\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tMon, 04 Mar 2024 08:58:44 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=redhat.com header.i=@redhat.com\n\theader.b=\"D240mMpc\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1709571528;\n\th=from:from:reply-to:subject:subject:date:date:message-id:message-id:\n\tto:to:cc:cc:mime-version:mime-version:content-type:content-type:\n\tin-reply-to:in-reply-to:references:references;\n\tbh=yT5pXmfvysRLzDbHBfe7i6Rg+0oMXLjvO3nkVoDqsb0=;\n\tb=D240mMpcSOnIcPjIiM1anRy3FI00HQfYUVgI98xgC7pFnU4eMKGpHZkqY0RxI74WzXJk8a\n\t2yymcKjvTxfZyvihTF9K5JdSzqiWGAn94pTEySKk9MWa6iA63ZZ5Fc8ynnqSZTBhBWhKCW\n\tQGMteA91IIPiuKHQyrbhlQRQ86J470M=","X-MC-Unique":"XCDHBVRUN7O88gZbnEjdAw-1","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1709571525; x=1710176325;\n\th=mime-version:user-agent:message-id:date:references:in-reply-to\n\t:subject:cc:to:from:x-gm-message-state:from:to:cc:subject:date\n\t:message-id:reply-to;\n\tbh=yT5pXmfvysRLzDbHBfe7i6Rg+0oMXLjvO3nkVoDqsb0=;\n\tb=qyWmxzCSNcS8tCD3JDyqAIRGAw2O5tIjsES41uHmCawZSlGazSMhFHCBjj9oR+WfYN\n\t43RCsxKiINk4w8bj8X6xyQOgUqGuOxp2qjpgv6eA63+CfZLGT4sLy0CRVSB4zqJ6nw87\n\t4PfzOzGkgMeWGjEsej0KQ8a+McOi+blSCSOYNaEsVSB1/Eiplnggyq9k8/0wfDl7y9bG\n\teiWOGFmAMzA0KrahqoyjIz37hF1FuqT4x+ZA81KWpc8W0i5qw50S/zur6okV23UAegHz\n\tY/68STlAFyUOrqwFp9GaNkxQVXynhpnOwY1buzMjgDdHUzVHXXIpGqqZkr+kTQXLmXFc\n\tpUyg==","X-Gm-Message-State":"AOJu0YyKSxbWaD0CCLf4iZvVcqhJLWU4YAVytqnrjsMpRsWw3vzWgnfG\n\tpz5kCozSCTTKM9hjABki+r4wuIb4UWKJ18nX9k3h6mgfX2Q/VKgfSF+iomjwtHE9BgPIJophkbq\n\tStWjVOOQszme14pG7c+0D1wc+Pc0ZwPET4Y9ThyjJwiBlzZiBQ/u8a+2XKuE5ZNTSMrEhF+6Z9O\n\tDj/g8=","X-Received":["by 2002:a05:600c:ccf:b0:412:ea74:2ee8 with SMTP id\n\tfk15-20020a05600c0ccf00b00412ea742ee8mr79838wmb.30.1709571525606; \n\tMon, 04 Mar 2024 08:58:45 -0800 (PST)","by 2002:a05:600c:ccf:b0:412:ea74:2ee8 with SMTP id\n\tfk15-20020a05600c0ccf00b00412ea742ee8mr79818wmb.30.1709571525256; \n\tMon, 04 Mar 2024 08:58:45 -0800 (PST)"],"X-Google-Smtp-Source":"AGHT+IGlsPMaV6dOF8zmSIRTuylYXzN4kzqWZiE4vIeVnWDlXmH/F1uzXOzk+btMMVwrtlb76+ouZQ==","From":"Milan Zamazal ","To":"Hans de Goede ","Subject":"Re: [PATCH v4 08/18] libcamera: software_isp: Add DebayerCpu class","In-Reply-To":"<20240229183654.7206-9-hdegoede@redhat.com> (Hans de Goede's\n\tmessage of \"Thu, 29 Feb 2024 19:36:19 +0100\")","References":"<20240229183654.7206-1-hdegoede@redhat.com>\n\t<20240229183654.7206-9-hdegoede@redhat.com>","Date":"Mon, 04 Mar 2024 17:58:43 +0100","Message-ID":"<875xy2aq7w.fsf@redhat.com>","User-Agent":"Gnus/5.13 (Gnus v5.13)","MIME-Version":"1.0","X-Mimecast-Spam-Score":"0","X-Mimecast-Originator":"redhat.com","Content-Type":"text/plain","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"Maxime Ripard , libcamera-devel@lists.libcamera.org, \n\tPavel Machek ,\n\tBryan O'Donoghue , \n\tDennis Bonke ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":28907,"web_url":"https://patchwork.libcamera.org/comment/28907/","msgid":"<9497f0ea-7b52-4745-a9ec-c6a871522425@redhat.com>","date":"2024-03-11T10:10:57","subject":"Re: [PATCH v4 08/18] libcamera: software_isp: Add DebayerCpu class","submitter":{"id":102,"url":"https://patchwork.libcamera.org/api/people/102/","name":"Hans de Goede","email":"hdegoede@redhat.com"},"content":"Hi Milan,\n\nThnak you for all the reviews. For reviews where\nI have not responded I have applied all suggested changes\nfor the next version.\n\nOn 3/4/24 5:58 PM, Milan Zamazal wrote:\n> Hans de Goede writes:\n> \n>> Add CPU based debayering implementation. This initial implementation\n>> only supports debayering packed 10 bits per pixel bayer data in\n>> the 4 standard bayer orders.\n>>\n>> Doxygen documentation by Dennis Bonke.\n>>\n>> Tested-by: Bryan O'Donoghue # sc8280xp Lenovo x13s\n>> Tested-by: Pavel Machek \n>> Reviewed-by: Pavel Machek \n>> Co-developed-by: Dennis Bonke \n>> Signed-off-by: Dennis Bonke \n>> Co-developed-by: Andrey Konovalov \n>> Signed-off-by: Andrey Konovalov \n>> Co-developed-by: Pavel Machek \n>> Signed-off-by: Pavel Machek \n>> Signed-off-by: Hans de Goede \n> \n> This patch is quite difficult to understand and I wonder what happens if this\n> piece of code needs to be modified a year or two later. My primary problem with\n> it is tracking all the line pointers and where they point to at each given\n> moment. I think more verbosity (code comments) is desirable unless other\n> reviewers think it's crystal clear.\n\nI tried to explain how this works already in the new comments in this\nv4 series. I think it might be best for someone less familiar with\nthe code to add extra comments since to me the working is already clear.\n\nMaybe you can prepare a follow-up patch to add some extra comments\nin places where you believe that this is necessary ?\n\n\n> \n>> ---\n>> Changes in v3:\n>> - Move debayer_cpu.h to src/libcamera/software_isp/\n>> - Move documentation to .cpp file\n>> - Document how/why an array of src pointers is passed to\n>> the debayer functions\n>> ---\n>> src/libcamera/software_isp/debayer_cpu.cpp | 619 +++++++++++++++++++++\n>> src/libcamera/software_isp/debayer_cpu.h | 143 +++++\n>> src/libcamera/software_isp/meson.build | 1 +\n>> 3 files changed, 763 insertions(+)\n>> create mode 100644 src/libcamera/software_isp/debayer_cpu.cpp\n>> create mode 100644 src/libcamera/software_isp/debayer_cpu.h\n>>\n>> diff --git a/src/libcamera/software_isp/debayer_cpu.cpp b/src/libcamera/software_isp/debayer_cpu.cpp\n>> new file mode 100644\n>> index 00000000..53e90776\n>> --- /dev/null\n>> +++ b/src/libcamera/software_isp/debayer_cpu.cpp\n> \n> [...]\n> \n>> +/**\n>> + * \\brief Constructs a DebayerCpu object.\n>> + * \\param[in] stats Pointer to the stats object to use.\n>> + */\n>> +DebayerCpu::DebayerCpu(std::unique_ptr stats)\n>> +\t: stats_(std::move(stats)), gamma_correction_(1.0)\n>> +{\n>> +#ifdef __x86_64__\n>> +\tenableInputMemcpy_ = false;\n>> +#else\n>> +\tenableInputMemcpy_ = true;\n>> +#endif\n>> +\t/* Initialize gamma to 1.0 curve */\n>> +\tfor (unsigned int i = 0; i < kGammaLookupSize; i++)\n>> +\t\tgamma_[i] = i / 4;\n> \n> What is this division by 4? Is it related to kGammaLookupSize being 1024?\n> If yes then it should derive from that constant.\n\nGood point, I will replace this with a a calculation using kGammaLookupSize\nfor the next version.\n\n\n> \n> [...]\n> \n>> +void DebayerCpu::debayer10P_RGRG_BGR888(uint8_t *dst, const uint8_t *src[])\n>> +{\n>> +\tconst int width_in_bytes = window_.width * 5 / 4;\n>> +\tconst uint8_t *prev = (const uint8_t *)src[0];\n>> +\tconst uint8_t *curr = (const uint8_t *)src[1];\n>> +\tconst uint8_t *next = (const uint8_t *)src[2];\n>> +\n>> +\tfor (int x = 0; x < width_in_bytes; x++) {\n>> +\t\t/* Even pixel */\n>> +\t\tRGGB_BGR888(2, 1, 1)\n>> +\t\t/* Odd pixel RGGB -> GRBG*/\n> \n> Missing space before */.\n\nAck, will fix.\n\n> [...]\n> \n>> +void DebayerCpu::process2(const uint8_t *src, uint8_t *dst)\n>> +{\n>> +\tunsigned int y_end = window_.y + window_.height;\n>> +\t/* Holds [0] previous- [1] current- [2] next-line */\n>> +\tconst uint8_t *linePointers[3];\n>> +\n>> +\t/* Adjust src to top left corner of the window */\n>> +\tsrc += window_.y * inputConfig_.stride + window_.x * inputConfig_.bpp / 8;\n>> +\n>> +\t/* [x] becomes [x - 1] after initial shiftLinePointers() call */\n>> +\tif (window_.y) {\n>> +\t\tlinePointers[1] = src - inputConfig_.stride; /* previous-line */\n>> +\t\tlinePointers[2] = src;\n>> +\t} else {\n>> +\t\t/* window_.y == 0, use the next line as prev line */\n> \n> Why is this needed here and not in process4?\n\nThis comes from:\n\nhttps://github.com/jwrdegoede/libcamera/commit/df717903fd99b371efde20314991df2e08e4c22b\n\nWhich was later squashed into the main commit adding the DebayerCpu class.\n\nIf you look at: DebayerCpu::sizes() then you see that normally\nit uses:\n\n\tunsigned int border_height = pattern_size.height;\n\nAnd there is a special case for bayer patterns with a pattern height of 2\n(which are actually all standard bayer patterns):\n\n\t/* No need for top/bottom border with a pattern height of 2 */\n\tif (pattern_size.height == 2)\n\t\tborder_height = 0;\n\nSo for the pattern-height = 4 case there is a top and bottom border of\n4 extra pixels and doing e.g. \"src - 2 * srcStride\" still points to\nvalid data because the initial src value starts at line 4 or more\nof the raw src data.\n\nThe code used to do the same thing for pattern-height = 2, but testing on a HP\nlaptop with a hi556 sensor showed that that sensor (or at least the current\ndriver for that sensor) can produce a max raw-bayer data height of 722,\nso we could not do 1280x720 there after adding a 2 byte border to both\nthe top and bottom which in turn confused the webrtc code in firefox.\n\nSince I already introduced the line-pointers for the sliding window\nwith memcpy-ed buffers, the top/bottom padding was no longer strictly\nnecessary. So I decided to remove it to make things work with the hi556\nsensor:\n\nhttps://github.com/jwrdegoede/libcamera/commit/df717903fd99b371efde20314991df2e08e4c22b\n\n>> +\t\tlinePointers[1] = src + inputConfig_.stride;\n>> +\t\tlinePointers[2] = src;\n>> +\t\t/* Last 2 lines also need special handling */\n>> +\t\ty_end -= 2;\n>> +\t}\n>> +\n>> +\tsetupInputMemcpy(linePointers);\n>> +\n>> +\tfor (unsigned int y = window_.y; y < y_end; y += 2) {\n>> +\t\tshiftLinePointers(linePointers, src);\n>> +\t\tmemcpyNextLine(linePointers);\n>> +\t\tstats_->processLine0(y, linePointers);\n>> +\t\t(this->*debayer0_)(dst, linePointers);\n>> +\t\tsrc += inputConfig_.stride;\n>> +\t\tdst += outputConfig_.stride;\n>> +\n>> +\t\tshiftLinePointers(linePointers, src);\n>> +\t\tmemcpyNextLine(linePointers);\n>> +\t\t(this->*debayer1_)(dst, linePointers);\n>> +\t\tsrc += inputConfig_.stride;\n>> +\t\tdst += outputConfig_.stride;\n>> +\t}\n>> +\n>> +\tif (window_.y == 0) {\n>> +\t\tshiftLinePointers(linePointers, src);\n>> +\t\tmemcpyNextLine(linePointers);\n\nWithout memcpy-ing (1) we have the following linePointer values\n(with lines[] being a virtual array which contains pointers\nto the start of each line):\n\nlinePointers[0] = lines[window_.height - 3];\nlinePointers[1] = lines[window_.height - 2];\nlinePointers[2] = lines[window_.height - 1];\n\nNote that linePointers[2] now points to the very last line of\nthe input buffer. Since there is no bottom border we cannot\ngo further then this!\n\n>> +\t\tstats_->processLine0(y_end, linePointers);\n>> +\t\t(this->*debayer0_)(dst, linePointers);\n>> +\t\tsrc += inputConfig_.stride;\n>> +\t\tdst += outputConfig_.stride;\n>> +\n>> +\t\tshiftLinePointers(linePointers, src);\n>> +\t\t/* next line may point outside of srclinePointers[2], use prev. */\n>> +\t\tlinePointers[2] = linePointers[0];\n> \n> At least this deserves a more elaborate comment; honestly, I'm lost what's\n> happening here exactly.\n\nAfter the shiftLinePointers() we now have:\n\nlinePointers[0] = lines[window_.height - 2];\nlinePointers[1] = lines[window_.height - 1];\nlinePointers[2] = lines[window_.height];\n\nNote linePointers[2] now points outside of the valid area of our\nsrc buffer!\n\nSo we need to fixup linePointers[2]. And it needs to point to\na line matching the expected bayer pattern (which has different\ncolors in odd/even lines) so to keep linePointers[2] pointing\nto an even line we do:\n\n\t\tlinePointers[2] = linePointers[0];\n\nresulting in:\n\nlinePointers[0] = lines[window_.height - 2];\nlinePointers[1] = lines[window_.height - 1];\nlinePointers[2] = lines[window_.height - 2];\n\nSo instead of giving the debayer code a pointer to the line above and\nbelow the current line for interpolating, we are giving it the line\nabove the current line twice (since there is no line below to give it)\nsince the line above and below have the same bayer order this will still\nresult in correct output.\n\nThis really is just a small trick to avoid the need for a border / for\nextra rows at the top/bottom of the src buffer.\n\n\n1) With memcpy-ing the linePointers each point to one of the 3 lineBuffers_\nholding the contents of these lines.\n\nAlso note that we don't need to do the memcpy for debayering the last line\nsince we are simply making linePointers[2] point to the lineBuffers_[] entry\nwhich already contains a copy of lines[window_.height - 2]\n\nRegards,\n\nHans","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 29F65BD1F1\n\tfor ;\n\tMon, 11 Mar 2024 10:11:08 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 431D762C83;\n\tMon, 11 Mar 2024 11:11:07 +0100 (CET)","from us-smtp-delivery-124.mimecast.com\n\t(us-smtp-delivery-124.mimecast.com [170.10.133.124])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 78F1561C7F\n\tfor ;\n\tMon, 11 Mar 2024 11:11:04 +0100 (CET)","from mail-lf1-f69.google.com (mail-lf1-f69.google.com\n\t[209.85.167.69]) by relay.mimecast.com with ESMTP with STARTTLS\n\t(version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id\n\tus-mta-472-BEGYZT9LM82LR0KJzGaL1w-1; Mon, 11 Mar 2024 06:11:01 -0400","by mail-lf1-f69.google.com with SMTP id\n\t2adb3069b0e04-513b15ac4e0so321520e87.1\n\tfor ;\n\tMon, 11 Mar 2024 03:11:00 -0700 (PDT)","from [10.40.98.142] ([78.108.130.194])\n\tby smtp.gmail.com with ESMTPSA id\n\ty7-20020ac24e67000000b005134b2b45easm1013704lfs.232.2024.03.11.03.10.57\n\t(version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128);\n\tMon, 11 Mar 2024 03:10:58 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=redhat.com header.i=@redhat.com\n\theader.b=\"DN8WRSZT\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1710151863;\n\th=from:from:reply-to:subject:subject:date:date:message-id:message-id:\n\tto:to:cc:cc:mime-version:mime-version:content-type:content-type:\n\tcontent-transfer-encoding:content-transfer-encoding:\n\tin-reply-to:in-reply-to:references:references;\n\tbh=6nieAPYeDQxDSxocdD+4XDL/yXG4JYr+zEyDFFftvpA=;\n\tb=DN8WRSZTFlUNWDcYpI456S1KZorcvgGD159+SzsI8ivu+1vwfZ1XG9roVHEsz1e3UXpWCo\n\t+MdKhKuM1oiZ747kClOSnXUHGO0ys11VdfyDDZczXJ0REOeT1FVqvnZKVNXNtYconOu930\n\tQcQnuyz7uBY/cOVTucjBKtCFhcCaH9M=","X-MC-Unique":"BEGYZT9LM82LR0KJzGaL1w-1","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1710151860; x=1710756660;\n\th=content-transfer-encoding:in-reply-to:from:content-language\n\t:references:cc:to:subject:user-agent:mime-version:date:message-id\n\t:x-gm-message-state:from:to:cc:subject:date:message-id:reply-to;\n\tbh=6nieAPYeDQxDSxocdD+4XDL/yXG4JYr+zEyDFFftvpA=;\n\tb=nmLA/x0ke17/3XJY6LjDHRwazCmgyN9crlx1g4zlBB+wwb/XDnQQjSxhbdA/AjKbx+\n\t2zHkt9HOn91OXvUoaxplhGwR4rvSpcvBhzkinKliBGVfKorURtWXRrdIEpKGFtQa35mp\n\tv1mtw4zbqtpOZFO0RP+k5sEWFPMLJUa/noanPVYYGJhKBXjTcsnLyXBSwgq2kFLWGBvs\n\tUkbDSqNcbQ6MC8EHLyMtV/ql0JYIg6CnI7DTw8tLHXzIsnOFHyUUXKGM4MnpqXct71PK\n\tMZ6Z34kp+zEJMzJJyyaUCmbiXt9lPCnwoQ801ErreoQRPnoCgpMRtuz71SVZYYmMmk+7\n\txYkw==","X-Gm-Message-State":"AOJu0YytONWSlaOjQpSmzYCYV30Gk8Y9aZ5X2Ltj4iuDt8OV/RG7HJE4\n\t1XJsiVLKLmT3ZISo5uUwEdRAol+EmvtMGAM18B5AZXaoVMrT7qda/UzHqb82I3cXqyq+s40e0T6\n\tAUYsut26tJ9gBQak6tpALY0xQ4vVI0A3N5SJVpU5IEzLLvT7pwx+9RjH4341vr5Gx5nZzfPs=","X-Received":["by 2002:ac2:54ad:0:b0:513:5eea:2942 with SMTP id\n\tw13-20020ac254ad000000b005135eea2942mr3708817lfk.37.1710151859692; \n\tMon, 11 Mar 2024 03:10:59 -0700 (PDT)","by 2002:ac2:54ad:0:b0:513:5eea:2942 with SMTP id\n\tw13-20020ac254ad000000b005135eea2942mr3708806lfk.37.1710151859226; \n\tMon, 11 Mar 2024 03:10:59 -0700 (PDT)"],"X-Google-Smtp-Source":"AGHT+IGqo+wYfftYjf4F/VsfmSAiB5SlHSVNFaHQymGGOz1RotKHEdDAlCy8tTHO8iIl+AkNcRxwkQ==","Message-ID":"<9497f0ea-7b52-4745-a9ec-c6a871522425@redhat.com>","Date":"Mon, 11 Mar 2024 11:10:57 +0100","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH v4 08/18] libcamera: software_isp: Add DebayerCpu class","To":"Milan Zamazal ","References":"<20240229183654.7206-1-hdegoede@redhat.com>\n\t<20240229183654.7206-9-hdegoede@redhat.com>\n\t<875xy2aq7w.fsf@redhat.com>","From":"Hans de Goede ","In-Reply-To":"<875xy2aq7w.fsf@redhat.com>","X-Mimecast-Spam-Score":"0","X-Mimecast-Originator":"redhat.com","Content-Language":"en-US","Content-Type":"text/plain; charset=UTF-8","Content-Transfer-Encoding":"7bit","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"Maxime Ripard , libcamera-devel@lists.libcamera.org, \n\tPavel Machek ,\n\tBryan O'Donoghue , \n\tDennis Bonke ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":28911,"web_url":"https://patchwork.libcamera.org/comment/28911/","msgid":"<87jzm97xh6.fsf@redhat.com>","date":"2024-03-11T12:40:37","subject":"Re: [PATCH v4 08/18] libcamera: software_isp: Add DebayerCpu class","submitter":{"id":177,"url":"https://patchwork.libcamera.org/api/people/177/","name":"Milan Zamazal","email":"mzamazal@redhat.com"},"content":"Hans de Goede writes:\n\n> On 3/4/24 5:58 PM, Milan Zamazal wrote:\n\n[...]\n \n>> This patch is quite difficult to understand and I wonder what happens if this\n>> piece of code needs to be modified a year or two later. My primary problem with\n>> it is tracking all the line pointers and where they point to at each given\n>> moment. I think more verbosity (code comments) is desirable unless other\n>> reviewers think it's crystal clear.\n>\n> I tried to explain how this works already in the new comments in this\n> v4 series. I think it might be best for someone less familiar with\n> the code to add extra comments since to me the working is already clear.\n>\n> Maybe you can prepare a follow-up patch to add some extra comments\n> in places where you believe that this is necessary ?\n\nHi Hans,\n\ngood idea, added to my TODO (this can be added anytime later, let's keep focus\non getting the branch merged for now).\n\nThank you for the expanded explanations of the vertical border handling, saving\nthem as a reference for the future comments.\n\nRegards,\nMilan","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 47407BD160\n\tfor ;\n\tMon, 11 Mar 2024 12:40:46 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8881A62868;\n\tMon, 11 Mar 2024 13:40:45 +0100 (CET)","from us-smtp-delivery-124.mimecast.com\n\t(us-smtp-delivery-124.mimecast.com [170.10.129.124])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id AD93061C7C\n\tfor ;\n\tMon, 11 Mar 2024 13:40:43 +0100 (CET)","from mail-lj1-f199.google.com (mail-lj1-f199.google.com\n\t[209.85.208.199]) by relay.mimecast.com with ESMTP with STARTTLS\n\t(version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id\n\tus-mta-513-Eo4qDyVdOKuczSpztDD1_Q-1; Mon, 11 Mar 2024 08:40:40 -0400","by mail-lj1-f199.google.com with SMTP id\n\t38308e7fff4ca-2d33e6f838dso35484171fa.0\n\tfor ;\n\tMon, 11 Mar 2024 05:40:40 -0700 (PDT)","from nuthatch (ip-77-48-47-2.net.vodafone.cz. [77.48.47.2])\n\tby smtp.gmail.com with ESMTPSA id\n\tf6-20020a05600c154600b004131ae36ac4sm11164246wmg.20.2024.03.11.05.40.38\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tMon, 11 Mar 2024 05:40:38 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=redhat.com header.i=@redhat.com\n\theader.b=\"GEVeIgqd\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1710160842;\n\th=from:from:reply-to:subject:subject:date:date:message-id:message-id:\n\tto:to:cc:cc:mime-version:mime-version:content-type:content-type:\n\tin-reply-to:in-reply-to:references:references;\n\tbh=I0PXi8T3fsTUeRp4dynihtj09+aCNiSmWERhkRhK+Fk=;\n\tb=GEVeIgqdia6SgSSURFBoyYP0+srR4zAYDnto+w9S97lnubiR8amKyTp7irnNSnFAt+tzOh\n\tagGJuSpZKGTBZZ9oTV1HqlyVc5dpVSEZ2PNMGXxeuYiv/HEChRTLmlQvXmCfOI/MuEGK3W\n\tiK1JYTKNb1YwNVlXeygP5ivuysshf2I=","X-MC-Unique":"Eo4qDyVdOKuczSpztDD1_Q-1","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1710160839; x=1710765639;\n\th=mime-version:user-agent:message-id:date:references:in-reply-to\n\t:subject:cc:to:from:x-gm-message-state:from:to:cc:subject:date\n\t:message-id:reply-to;\n\tbh=I0PXi8T3fsTUeRp4dynihtj09+aCNiSmWERhkRhK+Fk=;\n\tb=pRREDYxbwCsA9BPi4XeaJ55dm0LY9HznarD0+36yPfNbomUvkdjzbDS61++Qa1BE84\n\tkMusZrgBZkFN5+uiJdjz8GCPmn0iWLaT1aS7AtRBuvsIGtzPuyTTFv57z6aJrGxPWPuP\n\tMWwCMnngJ6UxmK7GrM6u6CRu0HKoga1wXs0LOt29hF/loBhlz8FXJQZxBrOyWlNP/2qz\n\tnI+cGAvNGCJrdgtTNd3mGP3yKZ45Q4FY3/3cNxnVqnEzhhUxRe3nPI4PD43tn3Hr0B5r\n\tQbuoGQdQcJfEff/q8zaAlTkdqp2lyxySQC7uL6f8Lwe2Zw59nIiby3MUdAt9PcTJ9eeb\n\tw0Kg==","X-Gm-Message-State":"AOJu0YwyhZs4qZARXcxoW8dFOWIsiwCMM0CGPY5N2flOIp9N1/6Db9uu\n\tN5ZW0o6Opjwuhy+cFonNuXYI5rMZLU1ns4VJ2VWZWGsyAljGOB9qXPSKZZdtzERE93QN4jvFcui\n\tH2bRn+1F8wWcI0j8Gd79oQm5weF2Bphd6Q6wvRK2CINMkh/X4wGUpl4WygmkBaWk9FywK1yQ=","X-Received":["by 2002:a2e:b0ef:0:b0:2d3:d71:98d3 with SMTP id\n\th15-20020a2eb0ef000000b002d30d7198d3mr259291ljl.29.1710160839301; \n\tMon, 11 Mar 2024 05:40:39 -0700 (PDT)","by 2002:a2e:b0ef:0:b0:2d3:d71:98d3 with SMTP id\n\th15-20020a2eb0ef000000b002d30d7198d3mr259282ljl.29.1710160838881; \n\tMon, 11 Mar 2024 05:40:38 -0700 (PDT)"],"X-Google-Smtp-Source":"AGHT+IH6XrJiQPayyIdw4WrsyEQzsWuB6xCNCnfrwF7veWuepV4kxFeZpJnPVROC0RhrSgRDSDV/Jw==","From":"Milan Zamazal ","To":"Hans de Goede ","Subject":"Re: [PATCH v4 08/18] libcamera: software_isp: Add DebayerCpu class","In-Reply-To":"<9497f0ea-7b52-4745-a9ec-c6a871522425@redhat.com> (Hans de\n\tGoede's message of \"Mon, 11 Mar 2024 11:10:57 +0100\")","References":"<20240229183654.7206-1-hdegoede@redhat.com>\n\t<20240229183654.7206-9-hdegoede@redhat.com>\n\t<875xy2aq7w.fsf@redhat.com>\n\t<9497f0ea-7b52-4745-a9ec-c6a871522425@redhat.com>","Date":"Mon, 11 Mar 2024 13:40:37 +0100","Message-ID":"<87jzm97xh6.fsf@redhat.com>","User-Agent":"Gnus/5.13 (Gnus v5.13)","MIME-Version":"1.0","X-Mimecast-Spam-Score":"0","X-Mimecast-Originator":"redhat.com","Content-Type":"text/plain","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"Maxime Ripard , libcamera-devel@lists.libcamera.org, \n\tPavel Machek ,\n\tBryan O'Donoghue , \n\tDennis Bonke ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]