Andrew Geissler | 87f5cff | 2022-09-30 13:13:31 -0500 | [diff] [blame] | 1 | From 0d3344e17d258106617b0e6d783d073b188a2548 Mon Sep 17 00:00:00 2001 |
| 2 | From: Adrian Perez de Castro <aperez@igalia.com> |
| 3 | Date: Thu, 2 Jun 2022 11:19:06 +0300 |
| 4 | Subject: [PATCH] [ARM][NEON] FELightningNEON.cpp fails to build, NEON fast |
| 5 | path seems unused https://bugs.webkit.org/show_bug.cgi?id=241182 |
| 6 | |
| 7 | Reviewed by NOBODY (OOPS!). |
| 8 | |
| 9 | Move the NEON fast path for the SVG lighting filter effects into |
| 10 | FELightingSoftwareApplier, and arrange to actually use them by |
| 11 | forwarding calls to applyPlatformGeneric() into applyPlatformNeon(). |
| 12 | |
| 13 | Some changes were needed to adapt platformApplyNeon() to the current |
| 14 | state of filters after r286140. This was not detected because the code |
| 15 | bitrotted due to it being guarded with CPU(ARM_TRADITIONAL), which does |
| 16 | not get used much these days: CPU(ARM_THUMB2) is more common. It should |
| 17 | be possible to use the NEON fast paths also in Thumb mode, but that is |
| 18 | left for a follow-up fix. |
| 19 | |
| 20 | * Source/WebCore/platform/graphics/cpu/arm/filters/FELightingNEON.cpp: |
| 21 | (WebCore::FELightingSoftwareApplier::platformApplyNeonWorker): |
| 22 | (WebCore::FELightingSoftwareApplier::getPowerCoefficients): |
| 23 | (WebCore::FELighting::platformApplyNeonWorker): Deleted. |
| 24 | (WebCore::FELighting::getPowerCoefficients): Deleted. |
| 25 | * Source/WebCore/platform/graphics/cpu/arm/filters/FELightingNEON.h: |
| 26 | (WebCore::FELightingSoftwareApplier::applyPlatformNeon): |
| 27 | (WebCore::FELighting::platformApplyNeon): Deleted. |
| 28 | * Source/WebCore/platform/graphics/filters/DistantLightSource.h: |
| 29 | * Source/WebCore/platform/graphics/filters/FELighting.h: |
| 30 | * Source/WebCore/platform/graphics/filters/PointLightSource.h: |
| 31 | * Source/WebCore/platform/graphics/filters/SpotLightSource.h: |
| 32 | * Source/WebCore/platform/graphics/filters/software/FELightingSoftwareApplier.h: |
| 33 | --- |
| 34 | Upstream-Status: Submitted [https://github.com/WebKit/WebKit/pull/1233] |
| 35 | Signed-off-by: Khem Raj <raj.khem@gmail.com> |
| 36 | |
| 37 | .../cpu/arm/filters/FELightingNEON.cpp | 4 +- |
| 38 | .../graphics/cpu/arm/filters/FELightingNEON.h | 54 +++++++++---------- |
| 39 | .../graphics/filters/DistantLightSource.h | 4 ++ |
| 40 | .../platform/graphics/filters/FELighting.h | 7 --- |
| 41 | .../graphics/filters/PointLightSource.h | 4 ++ |
| 42 | .../graphics/filters/SpotLightSource.h | 4 ++ |
| 43 | .../software/FELightingSoftwareApplier.h | 16 ++++++ |
| 44 | 7 files changed, 57 insertions(+), 36 deletions(-) |
| 45 | |
| 46 | --- a/Source/WebCore/platform/graphics/cpu/arm/filters/FELightingNEON.cpp |
| 47 | +++ b/Source/WebCore/platform/graphics/cpu/arm/filters/FELightingNEON.cpp |
| 48 | @@ -49,7 +49,7 @@ short* feLightingConstantsForNeon() |
| 49 | return s_FELightingConstantsForNeon; |
| 50 | } |
| 51 | |
| 52 | -void FELighting::platformApplyNeonWorker(FELightingPaintingDataForNeon* parameters) |
| 53 | +void FELightingSoftwareApplier::platformApplyNeonWorker(FELightingPaintingDataForNeon* parameters) |
| 54 | { |
| 55 | neonDrawLighting(parameters); |
| 56 | } |
| 57 | @@ -464,7 +464,7 @@ TOSTRING(neonDrawLighting) ":" NL |
| 58 | "b .lightStrengthCalculated" NL |
| 59 | ); // NOLINT |
| 60 | |
| 61 | -int FELighting::getPowerCoefficients(float exponent) |
| 62 | +int FELightingSoftwareApplier::getPowerCoefficients(float exponent) |
| 63 | { |
| 64 | // Calling a powf function from the assembly code would require to save |
| 65 | // and reload a lot of NEON registers. Since the base is in range [0..1] |
| 66 | --- a/Source/WebCore/platform/graphics/cpu/arm/filters/FELightingNEON.h |
| 67 | +++ b/Source/WebCore/platform/graphics/cpu/arm/filters/FELightingNEON.h |
| 68 | @@ -24,14 +24,15 @@ |
| 69 | * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. |
| 70 | */ |
| 71 | |
| 72 | -#ifndef FELightingNEON_h |
| 73 | -#define FELightingNEON_h |
| 74 | +#pragma once |
| 75 | |
| 76 | #if CPU(ARM_NEON) && CPU(ARM_TRADITIONAL) && COMPILER(GCC_COMPATIBLE) |
| 77 | |
| 78 | -#include "FELighting.h" |
| 79 | +#include "FELightingSoftwareApplier.h" |
| 80 | +#include "ImageBuffer.h" |
| 81 | #include "PointLightSource.h" |
| 82 | #include "SpotLightSource.h" |
| 83 | +#include <wtf/ObjectIdentifier.h> |
| 84 | #include <wtf/ParallelJobs.h> |
| 85 | |
| 86 | namespace WebCore { |
| 87 | @@ -93,14 +94,14 @@ extern "C" { |
| 88 | void neonDrawLighting(FELightingPaintingDataForNeon*); |
| 89 | } |
| 90 | |
| 91 | -inline void FELighting::platformApplyNeon(const LightingData& data, const LightSource::PaintingData& paintingData) |
| 92 | +inline void FELightingSoftwareApplier::applyPlatformNeon(const FELightingSoftwareApplier::LightingData& data, const LightSource::PaintingData& paintingData) |
| 93 | { |
| 94 | - alignas(16) FELightingFloatArgumentsForNeon floatArguments; |
| 95 | - FELightingPaintingDataForNeon neonData = { |
| 96 | + WebCore::FELightingFloatArgumentsForNeon alignas(16) floatArguments; |
| 97 | + WebCore::FELightingPaintingDataForNeon neonData = { |
| 98 | data.pixels->data(), |
| 99 | 1, |
| 100 | - data.widthDecreasedByOne - 1, |
| 101 | - data.heightDecreasedByOne - 1, |
| 102 | + data.width - 2, |
| 103 | + data.height - 2, |
| 104 | 0, |
| 105 | 0, |
| 106 | 0, |
| 107 | @@ -111,23 +112,23 @@ inline void FELighting::platformApplyNeo |
| 108 | // Set light source arguments. |
| 109 | floatArguments.constOne = 1; |
| 110 | |
| 111 | - auto color = m_lightingColor.toColorTypeLossy<SRGBA<uint8_t>>().resolved(); |
| 112 | + auto color = data.lightingColor.toColorTypeLossy<SRGBA<uint8_t>>().resolved(); |
| 113 | |
| 114 | floatArguments.colorRed = color.red; |
| 115 | floatArguments.colorGreen = color.green; |
| 116 | floatArguments.colorBlue = color.blue; |
| 117 | floatArguments.padding4 = 0; |
| 118 | |
| 119 | - if (m_lightSource->type() == LS_POINT) { |
| 120 | + if (data.lightSource->type() == LS_POINT) { |
| 121 | neonData.flags |= FLAG_POINT_LIGHT; |
| 122 | - PointLightSource& pointLightSource = static_cast<PointLightSource&>(m_lightSource.get()); |
| 123 | + const auto& pointLightSource = *static_cast<const PointLightSource*>(data.lightSource); |
| 124 | floatArguments.lightX = pointLightSource.position().x(); |
| 125 | floatArguments.lightY = pointLightSource.position().y(); |
| 126 | floatArguments.lightZ = pointLightSource.position().z(); |
| 127 | floatArguments.padding2 = 0; |
| 128 | - } else if (m_lightSource->type() == LS_SPOT) { |
| 129 | + } else if (data.lightSource->type() == LS_SPOT) { |
| 130 | neonData.flags |= FLAG_SPOT_LIGHT; |
| 131 | - SpotLightSource& spotLightSource = static_cast<SpotLightSource&>(m_lightSource.get()); |
| 132 | + const auto& spotLightSource = *static_cast<const SpotLightSource*>(data.lightSource); |
| 133 | floatArguments.lightX = spotLightSource.position().x(); |
| 134 | floatArguments.lightY = spotLightSource.position().y(); |
| 135 | floatArguments.lightZ = spotLightSource.position().z(); |
| 136 | @@ -145,7 +146,7 @@ inline void FELighting::platformApplyNeo |
| 137 | if (spotLightSource.specularExponent() == 1) |
| 138 | neonData.flags |= FLAG_CONE_EXPONENT_IS_1; |
| 139 | } else { |
| 140 | - ASSERT(m_lightSource->type() == LS_DISTANT); |
| 141 | + ASSERT(data.lightSource->type() == LS_DISTANT); |
| 142 | floatArguments.lightX = paintingData.initialLightingData.lightVector.x(); |
| 143 | floatArguments.lightY = paintingData.initialLightingData.lightVector.y(); |
| 144 | floatArguments.lightZ = paintingData.initialLightingData.lightVector.z(); |
| 145 | @@ -155,38 +156,39 @@ inline void FELighting::platformApplyNeo |
| 146 | // Set lighting arguments. |
| 147 | floatArguments.surfaceScale = data.surfaceScale; |
| 148 | floatArguments.minusSurfaceScaleDividedByFour = -data.surfaceScale / 4; |
| 149 | - if (m_lightingType == FELighting::DiffuseLighting) |
| 150 | - floatArguments.diffuseConstant = m_diffuseConstant; |
| 151 | + if (data.filterType == FilterEffect::Type::FEDiffuseLighting) |
| 152 | + floatArguments.diffuseConstant = data.diffuseConstant; |
| 153 | else { |
| 154 | neonData.flags |= FLAG_SPECULAR_LIGHT; |
| 155 | - floatArguments.diffuseConstant = m_specularConstant; |
| 156 | - neonData.specularExponent = getPowerCoefficients(m_specularExponent); |
| 157 | - if (m_specularExponent == 1) |
| 158 | + floatArguments.diffuseConstant = data.specularConstant; |
| 159 | + neonData.specularExponent = getPowerCoefficients(data.specularExponent); |
| 160 | + if (data.specularExponent == 1) |
| 161 | neonData.flags |= FLAG_SPECULAR_EXPONENT_IS_1; |
| 162 | } |
| 163 | if (floatArguments.diffuseConstant == 1) |
| 164 | neonData.flags |= FLAG_DIFFUSE_CONST_IS_1; |
| 165 | |
| 166 | - int optimalThreadNumber = ((data.widthDecreasedByOne - 1) * (data.heightDecreasedByOne - 1)) / s_minimalRectDimension; |
| 167 | + static constexpr int minimalRectDimension = 100 * 100; // Empirical data limit for parallel jobs |
| 168 | + int optimalThreadNumber = ((data.width - 2) * (data.height - 2)) / minimalRectDimension; |
| 169 | if (optimalThreadNumber > 1) { |
| 170 | // Initialize parallel jobs |
| 171 | - ParallelJobs<FELightingPaintingDataForNeon> parallelJobs(&WebCore::FELighting::platformApplyNeonWorker, optimalThreadNumber); |
| 172 | + ParallelJobs<FELightingPaintingDataForNeon> parallelJobs(&FELightingSoftwareApplier::platformApplyNeonWorker, optimalThreadNumber); |
| 173 | |
| 174 | // Fill the parameter array |
| 175 | int job = parallelJobs.numberOfJobs(); |
| 176 | if (job > 1) { |
| 177 | int yStart = 1; |
| 178 | - int yStep = (data.heightDecreasedByOne - 1) / job; |
| 179 | + int yStep = (data.height - 2) / job; |
| 180 | for (--job; job >= 0; --job) { |
| 181 | FELightingPaintingDataForNeon& params = parallelJobs.parameter(job); |
| 182 | params = neonData; |
| 183 | params.yStart = yStart; |
| 184 | - params.pixels += (yStart - 1) * (data.widthDecreasedByOne + 1) * 4; |
| 185 | + params.pixels += (yStart - 1) * data.width * 4; |
| 186 | if (job > 0) { |
| 187 | params.absoluteHeight = yStep; |
| 188 | yStart += yStep; |
| 189 | } else |
| 190 | - params.absoluteHeight = data.heightDecreasedByOne - yStart; |
| 191 | + params.absoluteHeight = (data.height - 1) - yStart; |
| 192 | } |
| 193 | parallelJobs.execute(); |
| 194 | return; |
| 195 | @@ -199,5 +201,3 @@ inline void FELighting::platformApplyNeo |
| 196 | } // namespace WebCore |
| 197 | |
| 198 | #endif // CPU(ARM_NEON) && COMPILER(GCC_COMPATIBLE) |
| 199 | - |
| 200 | -#endif // FELightingNEON_h |
| 201 | --- a/Source/WebCore/platform/graphics/filters/DistantLightSource.h |
| 202 | +++ b/Source/WebCore/platform/graphics/filters/DistantLightSource.h |
| 203 | @@ -25,6 +25,10 @@ |
| 204 | #include "LightSource.h" |
| 205 | #include <wtf/Ref.h> |
| 206 | |
| 207 | +namespace WTF { |
| 208 | +class TextStream; |
| 209 | +} // namespace WTF |
| 210 | + |
| 211 | namespace WebCore { |
| 212 | |
| 213 | class DistantLightSource : public LightSource { |
| 214 | --- a/Source/WebCore/platform/graphics/filters/FELighting.h |
| 215 | +++ b/Source/WebCore/platform/graphics/filters/FELighting.h |
| 216 | @@ -35,8 +35,6 @@ |
| 217 | |
| 218 | namespace WebCore { |
| 219 | |
| 220 | -struct FELightingPaintingDataForNeon; |
| 221 | - |
| 222 | class FELighting : public FilterEffect { |
| 223 | public: |
| 224 | const Color& lightingColor() const { return m_lightingColor; } |
| 225 | @@ -67,11 +65,6 @@ protected: |
| 226 | |
| 227 | std::unique_ptr<FilterEffectApplier> createSoftwareApplier() const override; |
| 228 | |
| 229 | -#if CPU(ARM_NEON) && CPU(ARM_TRADITIONAL) && COMPILER(GCC_COMPATIBLE) |
| 230 | - static int getPowerCoefficients(float exponent); |
| 231 | - inline void platformApplyNeon(const LightingData&, const LightSource::PaintingData&); |
| 232 | -#endif |
| 233 | - |
| 234 | Color m_lightingColor; |
| 235 | float m_surfaceScale; |
| 236 | float m_diffuseConstant; |
| 237 | --- a/Source/WebCore/platform/graphics/filters/PointLightSource.h |
| 238 | +++ b/Source/WebCore/platform/graphics/filters/PointLightSource.h |
| 239 | @@ -26,6 +26,10 @@ |
| 240 | #include "LightSource.h" |
| 241 | #include <wtf/Ref.h> |
| 242 | |
| 243 | +namespace WTF { |
| 244 | +class TextStream; |
| 245 | +} // namespace WTF |
| 246 | + |
| 247 | namespace WebCore { |
| 248 | |
| 249 | class PointLightSource : public LightSource { |
| 250 | --- a/Source/WebCore/platform/graphics/filters/SpotLightSource.h |
| 251 | +++ b/Source/WebCore/platform/graphics/filters/SpotLightSource.h |
| 252 | @@ -26,6 +26,10 @@ |
| 253 | #include "LightSource.h" |
| 254 | #include <wtf/Ref.h> |
| 255 | |
| 256 | +namespace WTF { |
| 257 | +class TextStream; |
| 258 | +} // namespace WTF |
| 259 | + |
| 260 | namespace WebCore { |
| 261 | |
| 262 | class SpotLightSource : public LightSource { |
| 263 | --- a/Source/WebCore/platform/graphics/filters/software/FELightingSoftwareApplier.h |
| 264 | +++ b/Source/WebCore/platform/graphics/filters/software/FELightingSoftwareApplier.h |
| 265 | @@ -36,6 +36,7 @@ |
| 266 | namespace WebCore { |
| 267 | |
| 268 | class FELighting; |
| 269 | +struct FELightingPaintingDataForNeon; |
| 270 | |
| 271 | class FELightingSoftwareApplier final : public FilterEffectConcreteApplier<FELighting> { |
| 272 | WTF_MAKE_FAST_ALLOCATED; |
| 273 | @@ -132,8 +133,23 @@ private: |
| 274 | |
| 275 | static void applyPlatformGenericPaint(const LightingData&, const LightSource::PaintingData&, int startY, int endY); |
| 276 | static void applyPlatformGenericWorker(ApplyParameters*); |
| 277 | + |
| 278 | +#if CPU(ARM_NEON) && CPU(ARM_TRADITIONAL) && COMPILER(GCC_COMPATIBLE) |
| 279 | + static int getPowerCoefficients(float exponent); |
| 280 | + static void platformApplyNeonWorker(FELightingPaintingDataForNeon*); |
| 281 | + inline static void applyPlatformNeon(const LightingData&, const LightSource::PaintingData&); |
| 282 | + |
| 283 | + inline static void applyPlatformGeneric(const LightingData& data, const LightSource::PaintingData& paintingData) |
| 284 | + { |
| 285 | + applyPlatformNeon(data, paintingData); |
| 286 | + } |
| 287 | +#else |
| 288 | static void applyPlatformGeneric(const LightingData&, const LightSource::PaintingData&); |
| 289 | +#endif |
| 290 | + |
| 291 | static void applyPlatform(const LightingData&); |
| 292 | }; |
| 293 | |
| 294 | } // namespace WebCore |
| 295 | + |
| 296 | +#include "FELightingNEON.h" |