[{"id":32015,"web_url":"https://patchwork.libcamera.org/comment/32015/","msgid":"<871pzrvsqw.fsf@redhat.com>","date":"2024-11-04T17:00:39","subject":"Re: [PATCH 6/8] libcamera: swstats_cpu: Add support for YUV420","submitter":{"id":177,"url":"https://patchwork.libcamera.org/api/people/177/","name":"Milan Zamazal","email":"mzamazal@redhat.com"},"content":"Hi Hans,\n\nHans de Goede writes:\n\n> Add support for processing YUV420 data.\n>\n> Signed-off-by: Hans de Goede \n> ---\n> .../internal/software_isp/swstats_cpu.h | 6 ++\n> src/libcamera/software_isp/swstats_cpu.cpp | 89 +++++++++++++++++++\n> 2 files changed, 95 insertions(+)\n>\n> diff --git a/include/libcamera/internal/software_isp/swstats_cpu.h b/include/libcamera/internal/software_isp/swstats_cpu.h\n> index fa47cec9..a043861c 100644\n> --- a/include/libcamera/internal/software_isp/swstats_cpu.h\n> +++ b/include/libcamera/internal/software_isp/swstats_cpu.h\n> @@ -71,6 +71,7 @@ public:\n> private:\n> \tusing statsProcessFn = void (SwStatsCpu::*)(const uint8_t *src[]);\n> \tusing processFrameFn = void (SwStatsCpu::*)(MappedFrameBuffer &in);\n> +\tusing finishFrameFn = void (SwStatsCpu::*)();\n> \n> \tint setupStandardBayerOrder(BayerFormat::Order order);\n> \t/* Bayer 8 bpp unpacked */\n> @@ -82,10 +83,15 @@ private:\n> \t/* Bayer 10 bpp packed */\n> \tvoid statsBGGR10PLine0(const uint8_t *src[]);\n> \tvoid statsGBRG10PLine0(const uint8_t *src[]);\n> +\t/* YUV420 3 planes */\n> +\tvoid statsYUV420Line0(const uint8_t *src[]);\n> \n> \tvoid processBayerFrame2(MappedFrameBuffer &in);\n> +\tvoid processYUV420Frame(MappedFrameBuffer &in);\n> +\tvoid finishYUV420Frame();\n> \n> \tprocessFrameFn processFrame_;\n> +\tfinishFrameFn finishFrame_;\n> \n> \t/* Variables set by configure(), used every line */\n> \tstatsProcessFn stats0_;\n> diff --git a/src/libcamera/software_isp/swstats_cpu.cpp b/src/libcamera/software_isp/swstats_cpu.cpp\n> index 1ff15f5b..e81c96a2 100644\n> --- a/src/libcamera/software_isp/swstats_cpu.cpp\n> +++ b/src/libcamera/software_isp/swstats_cpu.cpp\n> @@ -13,6 +13,7 @@\n> \n> #include \n> \n> +#include \n> #include \n> \n> #include \"libcamera/internal/bayer_format.h\"\n> @@ -288,6 +289,40 @@ void SwStatsCpu::statsGBRG10PLine0(const uint8_t *src[])\n> \tSWSTATS_FINISH_LINE_STATS()\n> }\n> \n> +void SwStatsCpu::statsYUV420Line0(const uint8_t *src[])\n> +{\n> +\tuint64_t sumY = 0;\n> +\tuint64_t sumU = 0;\n> +\tuint64_t sumV = 0;\n> +\tuint8_t y, u, v;\n> +\n> +\t/* Adjust src[] for starting at window_.x */\n> +\tsrc[0] += window_.x;\n> +\tsrc[1] += window_.x / 2;\n> +\tsrc[2] += window_.x / 2;\n> +\n> +\t/* x += 4 sample every other 2x2 block */\n> +\tfor (int x = 0; x < (int)window_.width; x += 4) {\n\nI've probably already asked about using a signed int in line processors\nfor other patterns but I fail to see again why `x' cannot be unsigned\nhere.\n\n> +\t\t/*\n> +\t\t * Take y from the top left corner of the 2x2 block instead\n> +\t\t * of averaging 4 y-s.\n\nOK.\n\n> +\t\t */\n> +\t\ty = src[0][x];\n> +\t\tu = src[1][x];\n> +\t\tv = src[2][x];\n> +\n> +\t\tsumY += y;\n> +\t\tsumU += u;\n> +\t\tsumV += v;\n> +\n> +\t\tstats_.yHistogram[y * SwIspStats::kYHistogramSize / 256]++;\n> +\t}\n> +\n> +\tstats_.sumR_ += sumY;\n> +\tstats_.sumG_ += sumU;\n> +\tstats_.sumB_ += sumV;\n\nThis is above my confusion acceptation threshold :-), data shouldn't\ncontain something completely different than what the name suggests. How\nabout changing stats_.sum* to stats_.sum[3]? And defining constants for\nr/g/b and y/u/v array index access to make clear what we work with at\nthe given places?\n\n> +}\n> +\n> /**\n> * \\brief Reset state to start statistics gathering for a new frame\n> *\n> @@ -313,6 +348,9 @@ void SwStatsCpu::startFrame(void)\n> */\n> void SwStatsCpu::finishFrame(uint32_t frame, uint32_t bufferId)\n> {\n> +\tif (finishFrame_)\n> +\t\t(this->*finishFrame_)();\n> +\n> \t*sharedStats_ = stats_;\n> \tstatsReady.emit(frame, bufferId);\n> }\n> @@ -362,6 +400,20 @@ int SwStatsCpu::setupStandardBayerOrder(BayerFormat::Order order)\n> int SwStatsCpu::configure(const StreamConfiguration &inputCfg)\n> {\n> \tstride_ = inputCfg.stride;\n> +\tfinishFrame_ = NULL;\n\nnullptr\n\n> +\n> +\tif (inputCfg.pixelFormat == formats::YUV420) {\n> +\t\tpatternSize_.height = 2;\n> +\t\tpatternSize_.width = 2;\n> +\t\t/* Skip every 3th and 4th line, sample every other 2x2 block */\n> +\t\tySkipMask_ = 0x02;\n> +\t\txShift_ = 0;\n> +\t\tswapLines_ = false;\n> +\t\tstats0_ = &SwStatsCpu::statsYUV420Line0;\n> +\t\tprocessFrame_ = &SwStatsCpu::processYUV420Frame;\n> +\t\tfinishFrame_ = &SwStatsCpu::finishYUV420Frame;\n> +\t\treturn 0;\n> +\t}\n> \n> \tBayerFormat bayerFormat =\n> \t\tBayerFormat::fromPixelFormat(inputCfg.pixelFormat);\n> @@ -430,6 +482,43 @@ void SwStatsCpu::setWindow(const Rectangle &window)\n> \twindow_.height &= ~(patternSize_.height - 1);\n> }\n> \n> +void SwStatsCpu::processYUV420Frame(MappedFrameBuffer &in)\n> +{\n> +\tconst uint8_t *linePointers[3];\n> +\n> +\tlinePointers[0] = in.planes()[0].data();\n> +\tlinePointers[1] = in.planes()[1].data();\n> +\tlinePointers[2] = in.planes()[2].data();\n> +\n> +\t/* Adjust linePointers for starting at window_.y */\n> +\tlinePointers[0] += window_.y * stride_;\n> +\tlinePointers[1] += window_.y * stride_ / 4;\n> +\tlinePointers[2] += window_.y * stride_ / 4;\n> +\n> +\tfor (unsigned int y = 0; y < window_.height; y += 2) {\n> +\t\tif (!(y & ySkipMask_))\n> +\t\t\t(this->*stats0_)(linePointers);\n> +\n> +\t\tlinePointers[0] += stride_ * 2;\n> +\t\tlinePointers[1] += stride_ / 2;\n> +\t\tlinePointers[2] += stride_ / 2;\n> +\t}\n\n4:2:0, 2x2 blocks, skip mask, ..., headache :-). OK, the stride_\nmultipliers & dividers look correct.\n\n> +}\n> +\n> +void SwStatsCpu::finishYUV420Frame()\n> +{\n> +\t/* sumR_ / G_ / B_ contain Y / U / V sums convert this */\n> +\tdouble divider = (uint64_t)window_.width * window_.height * 256 / 16;\n\nWhy 256 / 16 ? To convert to 0..1 range and to adjust for sampling\nevery 16th pixel (one from 4 * every other horizontally * every other\nvertically)? It should have an explanation here.\n\n> +\tdouble Y = (double)stats_.sumR_ / divider;\n> +\t/* U and V 0 - 255 values represent -128 - 127 range */\n\nBy shifting? I.e. 0 represents -128 and 128 represents 0?\n\n> +\tdouble U = (double)stats_.sumG_ / divider - 0.5;\n> +\tdouble V = (double)stats_.sumB_ / divider - 0.5;\n> +\n> +\tstats_.sumR_ = (Y + 1.140 * V) * divider;\n> +\tstats_.sumG_ = (Y - 0.395 * U - 0.581 * V) * divider;\n> +\tstats_.sumB_ = (Y + 2.032 * U) * divider;\n> +}\n> +\n> void SwStatsCpu::processBayerFrame2(MappedFrameBuffer &in)\n> {\n> \tconst uint8_t *src = in.planes()[0].data();","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 514CFBDC71\n\tfor ;\n\tMon, 4 Nov 2024 17:00:51 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 68B49653DC;\n\tMon, 4 Nov 2024 18:00:50 +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 E966565399\n\tfor ;\n\tMon, 4 Nov 2024 18:00:47 +0100 (CET)","from mail-ej1-f71.google.com (mail-ej1-f71.google.com\n\t[209.85.218.71]) by relay.mimecast.com with ESMTP with STARTTLS\n\t(version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id\n\tus-mta-582-JgEVYD-jPLSo5xeqslK5Ug-1; Mon, 04 Nov 2024 12:00:45 -0500","by mail-ej1-f71.google.com with SMTP id\n\ta640c23a62f3a-a9a2ccb77ceso346140266b.2\n\tfor ;\n\tMon, 04 Nov 2024 09:00:45 -0800 (PST)","from nuthatch (ip-77-48-47-2.net.vodafone.cz. [77.48.47.2])\n\tby smtp.gmail.com with ESMTPSA id\n\ta640c23a62f3a-a9eb17d7535sm4924566b.98.2024.11.04.09.00.41\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tMon, 04 Nov 2024 09:00:41 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=redhat.com header.i=@redhat.com\n\theader.b=\"iO2IFEQL\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1730739646;\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=nu1/kmNaO8p4delI93vvQp+WuGrDDb9OISwk9mxWfI0=;\n\tb=iO2IFEQLeEuUBs3q4rxIiS/EA4vuB748jvzlczj1ahZvD45XwB0ESIbKfvZivFe143hrbo\n\tBWrq8GwsM0H2PhbtWTiZvhnOsnpdvbxknw+VwocqSH1/KneL1dZ6/Hig4EdCi9omn2SR/T\n\tYHoQ6DiUm+GlHvDaGNacfELWcJzdzEs=","X-MC-Unique":"JgEVYD-jPLSo5xeqslK5Ug-1","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1730739644; x=1731344444;\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=nu1/kmNaO8p4delI93vvQp+WuGrDDb9OISwk9mxWfI0=;\n\tb=wyTUTry0mI2vCZyxThmqDGI/uM9HX3lgU/Iv1GcTYMbYSlKeC4j/tctYQaYcAgtknp\n\tmVam4jruRAsqCgjSe2F0j3Z5knPxQDF9Qek3VJ2Pq3IhravNSCFTuueInYDtdKkVWDcg\n\tkLIdHai2PpsuSzYW0Y9vp3bb/mrH+h0ckXF7HIHQegHnAQ8kmxnTdqHUFTqw9KEMNGyD\n\tN/xJatS/3JcWLHDU/U4ci//orC1BonF91mpUa17Vgbz8rDbvlBbKGhJ/abo3jQgA3oru\n\t1t6q0NPbZMJzxIIYjjKH6lk+36DN/xxis/76eYJN6CVeqgTIE/atdD0YCQXKXoBs9fD+\n\tHMiA==","X-Gm-Message-State":"AOJu0YzfdsMWrXLqWdsssqyzpYj50Rhjigpbkb8e4hRjH5X8D+KxQd03\n\tQ7n7UU0qcIWv8Hgk7kjh/ZvGPtxOhMghwhOwDs6Gapi0bUFnYM4vE4fUI7N6ZnKgJiBl0ovVL78\n\tbwJ3Wa1hPfXtmqj4bM/cvntBUVsBg2A8/Pj2r/Qh8NxrVkrATS5k5jmSiOlLMpzh7ms2Zic8=","X-Received":["by 2002:a17:907:7f29:b0:a99:eedd:6466 with SMTP id\n\ta640c23a62f3a-a9de5ecdfe5mr3145242266b.19.1730739642662; \n\tMon, 04 Nov 2024 09:00:42 -0800 (PST)","by 2002:a17:907:7f29:b0:a99:eedd:6466 with SMTP id\n\ta640c23a62f3a-a9de5ecdfe5mr3145239566b.19.1730739642161; \n\tMon, 04 Nov 2024 09:00:42 -0800 (PST)"],"X-Google-Smtp-Source":"AGHT+IHDDwc8bfkyEM7QcPzMcOrLZ0C13crDMwTz6eDMdSlkEZr0bGyOJ1sRZUKtpEFbmP6411uq6A==","From":"Milan Zamazal ","To":"Hans de Goede ","Cc":"libcamera-devel@lists.libcamera.org, Maxime Ripard ","Subject":"Re: [PATCH 6/8] libcamera: swstats_cpu: Add support for YUV420","In-Reply-To":"<20241103152205.29219-7-hdegoede@redhat.com> (Hans de Goede's\n\tmessage of \"Sun, 3 Nov 2024 16:22:03 +0100\")","References":"<20241103152205.29219-1-hdegoede@redhat.com>\n\t<20241103152205.29219-7-hdegoede@redhat.com>","Date":"Mon, 04 Nov 2024 18:00:39 +0100","Message-ID":"<871pzrvsqw.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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]