From 0ebda972c1435393ce1d471a073c35f34d1e4530 Mon Sep 17 00:00:00 2001 From: Iulian Moraru Date: Wed, 22 Jun 2022 22:00:53 +0300 Subject: [PATCH] Backed out changeset 003cad9bbcc6 (bug 1772555) for causing reftest failures on bugs/1315113-1.html. --- gfx/2d/Types.h | 19 ++++ gfx/thebes/gfxUtils.cpp | 16 --- gfx/thebes/gfxUtils.h | 10 +- layout/painting/nsCSSRenderingGradients.cpp | 99 ++++++++----------- layout/painting/nsCSSRenderingGradients.h | 4 +- .../components/style/values/animated/color.rs | 4 +- .../components/style/values/computed/color.rs | 2 +- .../components/style/values/generics/color.rs | 2 +- servo/ports/geckolib/glue.rs | 25 +---- .../gradient-wrong-interpolation-crash.html | 7 -- 10 files changed, 73 insertions(+), 115 deletions(-) delete mode 100644 testing/web-platform/tests/css/css-backgrounds/gradient-wrong-interpolation-crash.html diff --git a/gfx/2d/Types.h b/gfx/2d/Types.h index 6ff4341e514a2..04affbef832c8 100644 --- a/gfx/2d/Types.h +++ b/gfx/2d/Types.h @@ -766,6 +766,25 @@ struct sRGBColor { return a > 0.f ? sRGBColor(r / a, g / a, b / a, a) : *this; } + // Returns aFrac*aC2 + (1 - aFrac)*C1. The interpolation is done in + // unpremultiplied space, which is what SVG gradients and cairo gradients + // expect. + constexpr static sRGBColor InterpolatePremultiplied(const sRGBColor& aC1, + const sRGBColor& aC2, + float aFrac) { + double other = 1 - aFrac; + return sRGBColor( + aC2.r * aFrac + aC1.r * other, aC2.g * aFrac + aC1.g * other, + aC2.b * aFrac + aC1.b * other, aC2.a * aFrac + aC1.a * other); + } + + constexpr static sRGBColor Interpolate(const sRGBColor& aC1, + const sRGBColor& aC2, float aFrac) { + return InterpolatePremultiplied(aC1.Premultiplied(), aC2.Premultiplied(), + aFrac) + .Unpremultiplied(); + } + // The "Unusual" prefix is to avoid unintentionally using this function when // ToABGR(), which is much more common, is needed. uint32_t UnusualToARGB() const { diff --git a/gfx/thebes/gfxUtils.cpp b/gfx/thebes/gfxUtils.cpp index 8d00ab7f9d64e..d403389de2142 100644 --- a/gfx/thebes/gfxUtils.cpp +++ b/gfx/thebes/gfxUtils.cpp @@ -1702,21 +1702,5 @@ DeviceColor ToDeviceColor(const StyleRGBA& aColor) { return ToDeviceColor(aColor.ToColor()); } -sRGBColor ToSRGBColor(const StyleAnimatedRGBA& aColor) { - const auto ToComponent = [](float aF) -> float { - float component = std::min(std::max(0.0f, aF), 1.0f); - if (MOZ_UNLIKELY(!std::isfinite(component))) { - return 0.0f; - } - return component; - }; - return {ToComponent(aColor.red), ToComponent(aColor.green), - ToComponent(aColor.blue), ToComponent(aColor.alpha)}; -} - -DeviceColor ToDeviceColor(const StyleAnimatedRGBA& aColor) { - return ToDeviceColor(ToSRGBColor(aColor)); -} - } // namespace gfx } // namespace mozilla diff --git a/gfx/thebes/gfxUtils.h b/gfx/thebes/gfxUtils.h index 6d7f5e2400b69..8df39bbd2cd51 100644 --- a/gfx/thebes/gfxUtils.h +++ b/gfx/thebes/gfxUtils.h @@ -374,7 +374,6 @@ class gfxUtils { namespace mozilla { struct StyleRGBA; -struct StyleAnimatedRGBA; namespace gfx { @@ -385,12 +384,9 @@ namespace gfx { * color is returned unchanged (other than a type change to Moz2D Color, if * applicable). */ -DeviceColor ToDeviceColor(const sRGBColor&); -DeviceColor ToDeviceColor(const StyleRGBA&); -DeviceColor ToDeviceColor(const StyleAnimatedRGBA&); -DeviceColor ToDeviceColor(nscolor); - -sRGBColor ToSRGBColor(const StyleAnimatedRGBA&); +DeviceColor ToDeviceColor(const sRGBColor& aColor); +DeviceColor ToDeviceColor(const StyleRGBA& aColor); +DeviceColor ToDeviceColor(nscolor aColor); /** * Performs a checked multiply of the given width, height, and bytes-per-pixel diff --git a/layout/painting/nsCSSRenderingGradients.cpp b/layout/painting/nsCSSRenderingGradients.cpp index 89555984c3e10..8bcdb33bb2fad 100644 --- a/layout/painting/nsCSSRenderingGradients.cpp +++ b/layout/painting/nsCSSRenderingGradients.cpp @@ -246,19 +246,6 @@ static float Interpolate(float aF1, float aF2, float aFrac) { return aF1 + aFrac * (aF2 - aF1); } -static StyleAnimatedRGBA Interpolate(const StyleAnimatedRGBA& aLeft, - const StyleAnimatedRGBA& aRight, - float aFrac) { - // NOTE: This has to match the interpolation method that WebRender uses which - // right now is sRGB. In the future we should implement interpolation in more - // gradient color-spaces. - static constexpr auto kMethod = StyleColorInterpolationMethod{ - StyleColorSpace::Srgb, - StyleHueInterpolationMethod::Shorter, - }; - return Servo_InterpolateColor(&kMethod, &aLeft, &aRight, aFrac); -} - static nscoord FindTileStart(nscoord aDirtyCoord, nscoord aTilePos, nscoord aTileDim) { NS_ASSERTION(aTileDim > 0, "Non-positive tile dimension"); @@ -297,7 +284,7 @@ static bool RectIsBeyondLinearGradientEdge(const gfxRect& aRect, const nsTArray& aStops, const gfxPoint& aGradientStart, const gfxPoint& aGradientEnd, - StyleAnimatedRGBA* aOutEdgeColor) { + sRGBColor* aOutEdgeColor) { gfxFloat topLeft = LinearGradientStopPositionForPoint( aGradientStart, aGradientEnd, aPatternMatrix.TransformPoint(aRect.TopLeft())); @@ -335,8 +322,8 @@ static void ResolveMidpoints(nsTArray& stops) { continue; } - const auto& color1 = stops[x - 1].mColor; - const auto& color2 = stops[x + 1].mColor; + sRGBColor color1 = stops[x - 1].mColor; + sRGBColor color2 = stops[x + 1].mColor; float offset1 = stops[x - 1].mPosition; float offset2 = stops[x + 1].mPosition; float offset = stops[x].mPosition; @@ -389,18 +376,16 @@ static void ResolveMidpoints(nsTArray& stops) { // points were chosen since it is the minimum number of stops that always // give the smoothest appearace regardless of midpoint position and // difference in luminance of the end points. - const float relativeOffset = + float relativeOffset = (newStop.mPosition - offset1) / (offset2 - offset1); - const float multiplier = powf(relativeOffset, logf(.5f) / logf(midpoint)); + float multiplier = powf(relativeOffset, logf(.5f) / logf(midpoint)); - const float red = color1.red + multiplier * (color2.red - color1.red); - const float green = - color1.green + multiplier * (color2.green - color1.green); - const float blue = color1.blue + multiplier * (color2.blue - color1.blue); - const float alpha = - color1.alpha + multiplier * (color2.alpha - color1.alpha); + gfx::Float red = color1.r + multiplier * (color2.r - color1.r); + gfx::Float green = color1.g + multiplier * (color2.g - color1.g); + gfx::Float blue = color1.b + multiplier * (color2.b - color1.b); + gfx::Float alpha = color1.a + multiplier * (color2.a - color1.a); - newStop.mColor = {red, green, blue, alpha}; + newStop.mColor = sRGBColor(red, green, blue, alpha); } stops.ReplaceElementsAt(x, 1, newStops, 9); @@ -408,10 +393,9 @@ static void ResolveMidpoints(nsTArray& stops) { } } -static StyleAnimatedRGBA TransparentColor(const StyleAnimatedRGBA& aColor) { - auto color = aColor; - color.alpha = 0.0f; - return color; +static sRGBColor TransparentColor(sRGBColor aColor) { + aColor.a = 0; + return aColor; } // Adjusts and adds color stops in such a way that drawing the gradient with @@ -426,21 +410,21 @@ static void ResolvePremultipliedAlpha(nsTArray& aStops) { // if the left and right stop have the same alpha value, we don't need // to do anything. Hardstops should be instant, and also should never // require dealing with interpolation. - if (leftStop.mColor.alpha == rightStop.mColor.alpha || + if (leftStop.mColor.a == rightStop.mColor.a || leftStop.mPosition == rightStop.mPosition) { continue; } // Is the stop on the left 100% transparent? If so, have it adopt the color // of the right stop - if (leftStop.mColor.alpha == 0) { + if (leftStop.mColor.a == 0) { aStops[x - 1].mColor = TransparentColor(rightStop.mColor); continue; } // Is the stop on the right completely transparent? // If so, duplicate it and assign it the color on the left. - if (rightStop.mColor.alpha == 0) { + if (rightStop.mColor.a == 0) { ColorStop newStop = rightStop; newStop.mColor = TransparentColor(leftStop.mColor); aStops.InsertElementAt(x, newStop); @@ -450,16 +434,20 @@ static void ResolvePremultipliedAlpha(nsTArray& aStops) { // Now handle cases where one or both of the stops are partially // transparent. - if (leftStop.mColor.alpha != 1.0f || rightStop.mColor.alpha != 1.0f) { + if (leftStop.mColor.a != 1.0f || rightStop.mColor.a != 1.0f) { + sRGBColor premulLeftColor = leftStop.mColor.Premultiplied(); + sRGBColor premulRightColor = rightStop.mColor.Premultiplied(); // Calculate how many extra steps. We do a step per 10% transparency. size_t stepCount = - NSToIntFloor(fabsf(leftStop.mColor.alpha - rightStop.mColor.alpha) / + NSToIntFloor(fabsf(leftStop.mColor.a - rightStop.mColor.a) / kAlphaIncrementPerGradientStep); for (size_t y = 1; y < stepCount; y++) { float frac = static_cast(y) / stepCount; ColorStop newStop( Interpolate(leftStop.mPosition, rightStop.mPosition, frac), false, - Interpolate(leftStop.mColor, rightStop.mColor, frac)); + sRGBColor::InterpolatePremultiplied(premulLeftColor, + premulRightColor, frac) + .Unpremultiplied()); aStops.InsertElementAt(x, newStop); x++; } @@ -470,7 +458,7 @@ static void ResolvePremultipliedAlpha(nsTArray& aStops) { static ColorStop InterpolateColorStop(const ColorStop& aFirst, const ColorStop& aSecond, double aPosition, - const StyleAnimatedRGBA& aDefault) { + const sRGBColor& aDefault) { MOZ_ASSERT(aFirst.mPosition <= aPosition); MOZ_ASSERT(aPosition <= aSecond.mPosition); @@ -479,9 +467,10 @@ static ColorStop InterpolateColorStop(const ColorStop& aFirst, return ColorStop(aPosition, false, aDefault); } - return ColorStop(aPosition, false, - Interpolate(aFirst.mColor, aSecond.mColor, - (aPosition - aFirst.mPosition) / delta)); + return ColorStop( + aPosition, false, + sRGBColor::Interpolate(aFirst.mColor, aSecond.mColor, + (aPosition - aFirst.mPosition) / delta)); } // Clamp and extend the given ColorStop array in-place to fit exactly into the @@ -493,8 +482,8 @@ static void ClampColorStops(nsTArray& aStops) { // with a single colour. if (aStops.Length() < 2 || aStops[0].mPosition > 1 || aStops.LastElement().mPosition < 0) { - const auto& c = aStops[0].mPosition > 1 ? aStops[0].mColor - : aStops.LastElement().mColor; + sRGBColor c = aStops[0].mPosition > 1 ? aStops[0].mColor + : aStops.LastElement().mColor; aStops.Clear(); aStops.AppendElement(ColorStop(0, false, c)); return; @@ -540,18 +529,16 @@ static void ClampColorStops(nsTArray& aStops) { namespace mozilla { template -static StyleAnimatedRGBA GetSpecifiedColor( +static sRGBColor GetSpecifiedColor( const StyleGenericGradientItem& aItem, const ComputedStyle& aStyle) { if (aItem.IsInterpolationHint()) { - return {0.0f, 0.0f, 0.0f, 0.0f}; + return sRGBColor(); } - const StyleColor& c = aItem.IsSimpleColorStop() - ? aItem.AsSimpleColorStop() - : aItem.AsComplexColorStop().color; - nscolor color = c.CalcColor(aStyle); - return {NS_GET_R(color) / 255.0f, NS_GET_G(color) / 255.0f, - NS_GET_B(color) / 255.0f, NS_GET_A(color) / 255.0f}; + const StyleColor& color = aItem.IsSimpleColorStop() + ? aItem.AsSimpleColorStop() + : aItem.AsComplexColorStop().color; + return sRGBColor::FromABGR(color.CalcColor(aStyle)); } static Maybe GetSpecifiedGradientPosition( @@ -854,8 +841,8 @@ void nsCSSGradientRenderer::Paint(gfxContext& aContext, const nsRect& aDest, // XXX Color interpolation (in cairo, too) should use the // CSS 'color-interpolation' property! float frac = float((0.0 - pos) / (nextPos - pos)); - mStops[i].mColor = - Interpolate(mStops[i].mColor, mStops[i + 1].mColor, frac); + mStops[i].mColor = sRGBColor::InterpolatePremultiplied( + mStops[i].mColor, mStops[i + 1].mColor, frac); } } } @@ -972,8 +959,8 @@ void nsCSSGradientRenderer::Paint(gfxContext& aContext, const nsRect& aDest, // gradient with radius of 0 -> just paint the last stop color. // We use firstStop offset to keep |stops| with same units (will later // normalize to 0). - auto firstColor(mStops[0].mColor); - auto lastColor(mStops.LastElement().mColor); + sRGBColor firstColor(mStops[0].mColor); + sRGBColor lastColor(mStops.LastElement().mColor); mStops.Clear(); if (!mGradient->Repeating() && !zeroRadius) { @@ -1077,13 +1064,13 @@ void nsCSSGradientRenderer::Paint(gfxContext& aContext, const nsRect& aDest, gfxRect dirtyFillRect = fillRect.Intersect(dirtyAreaToFill); gfxRect fillRectRelativeToTile = dirtyFillRect - tileRect.TopLeft(); - StyleAnimatedRGBA edgeColor{0.0f}; + sRGBColor edgeColor; if (mGradient->IsLinear() && !isRepeat && RectIsBeyondLinearGradientEdge(fillRectRelativeToTile, matrix, mStops, gradientStart, gradientEnd, &edgeColor)) { - edgeColor.alpha *= aOpacity; - aContext.SetColor(ToSRGBColor(edgeColor)); + edgeColor.a *= aOpacity; + aContext.SetColor(edgeColor); } else { aContext.SetMatrixDouble( aContext.CurrentMatrixDouble().Copy().PreTranslate( diff --git a/layout/painting/nsCSSRenderingGradients.h b/layout/painting/nsCSSRenderingGradients.h index fdc4abf7f6b0c..b5d59a120ae1c 100644 --- a/layout/painting/nsCSSRenderingGradients.h +++ b/layout/painting/nsCSSRenderingGradients.h @@ -30,11 +30,11 @@ class DisplayListBuilder; // a color. struct ColorStop { ColorStop() : mPosition(0), mIsMidpoint(false) {} - ColorStop(double aPosition, bool aIsMidPoint, const StyleAnimatedRGBA& aColor) + ColorStop(double aPosition, bool aIsMidPoint, const gfx::sRGBColor& aColor) : mPosition(aPosition), mIsMidpoint(aIsMidPoint), mColor(aColor) {} double mPosition; // along the gradient line; 0=start, 1=end bool mIsMidpoint; - StyleAnimatedRGBA mColor; + gfx::sRGBColor mColor; }; class nsCSSGradientRenderer final { diff --git a/servo/components/style/values/animated/color.rs b/servo/components/style/values/animated/color.rs index e79961661b91c..cdfa45dc4e7d6 100644 --- a/servo/components/style/values/animated/color.rs +++ b/servo/components/style/values/animated/color.rs @@ -19,7 +19,7 @@ use std::f32::consts::PI; /// range `[0.0, 1.0]`. #[derive(Clone, Copy, Debug, MallocSizeOf, PartialEq, ToAnimatedZero, ToAnimatedValue)] #[repr(C)] -pub struct AnimatedRGBA { +pub struct RGBA { /// The red component. pub red: f32, /// The green component. @@ -30,8 +30,6 @@ pub struct AnimatedRGBA { pub alpha: f32, } -use self::AnimatedRGBA as RGBA; - const RAD_PER_DEG: f32 = PI / 180.0; const DEG_PER_RAD: f32 = 180.0 / PI; diff --git a/servo/components/style/values/computed/color.rs b/servo/components/style/values/computed/color.rs index 1229793c71438..573cb6fe500d8 100644 --- a/servo/components/style/values/computed/color.rs +++ b/servo/components/style/values/computed/color.rs @@ -4,7 +4,7 @@ //! Computed color values. -use crate::values::animated::color::AnimatedRGBA; +use crate::values::animated::color::RGBA as AnimatedRGBA; use crate::values::animated::ToAnimatedValue; use crate::values::generics::color::{GenericCaretColor, GenericColor, GenericColorOrAuto}; use crate::values::computed::percentage::Percentage; diff --git a/servo/components/style/values/generics/color.rs b/servo/components/style/values/generics/color.rs index 65acfee9061a7..7ab3880bc1a6e 100644 --- a/servo/components/style/values/generics/color.rs +++ b/servo/components/style/values/generics/color.rs @@ -9,7 +9,7 @@ use style_traits::{CssWriter, ParseError, ToCss}; use crate::values::{Parse, ParserContext, Parser}; use crate::values::specified::percentage::ToPercentage; use crate::values::animated::ToAnimatedValue; -use crate::values::animated::color::AnimatedRGBA; +use crate::values::animated::color::RGBA as AnimatedRGBA; /// This struct represents a combined color from a numeric color and /// the current foreground color (currentcolor keyword). diff --git a/servo/ports/geckolib/glue.rs b/servo/ports/geckolib/glue.rs index eca29a62f4751..c797519b66e2b 100644 --- a/servo/ports/geckolib/glue.rs +++ b/servo/ports/geckolib/glue.rs @@ -136,8 +136,6 @@ use style::traversal::DomTraversal; use style::traversal_flags::{self, TraversalFlags}; use style::use_counters::UseCounters; use style::values::animated::{Animate, Procedure, ToAnimatedZero}; -use style::values::animated::color::AnimatedRGBA; -use style::values::generics::color::ColorInterpolationMethod; use style::values::computed::easing::ComputedLinearStop; use style::values::computed::font::{FontFamily, FontFamilyList, GenericFontFamily, FontWeight, FontStyle, FontStretch}; use style::values::computed::{self, Context, ToComputedValue}; @@ -801,7 +799,7 @@ pub extern "C" fn Servo_AnimationValue_Color( color: structs::nscolor, ) -> Strong { use style::gecko::values::convert_nscolor_to_rgba; - use style::values::animated::color::Color; + use style::values::animated::color::{Color, RGBA as AnimatedRGBA}; let property = LonghandId::from_nscsspropertyid(color_property) .expect("We don't have shorthand property animation value"); @@ -7499,7 +7497,7 @@ pub unsafe extern "C" fn Servo_InvalidateForViewportUnits( } #[no_mangle] -pub extern "C" fn Servo_CreatePiecewiseLinearFunction( +pub unsafe extern "C" fn Servo_CreatePiecewiseLinearFunction( entries: &style::OwnedSlice, result: &mut PiecewiseLinearFunction, ) { @@ -7511,26 +7509,9 @@ pub extern "C" fn Servo_CreatePiecewiseLinearFunction( } #[no_mangle] -pub extern "C" fn Servo_PiecewiseLinearFunctionAt( +pub unsafe extern "C" fn Servo_PiecewiseLinearFunctionAt( function: &PiecewiseLinearFunction, progress: f32, ) -> f32 { function.at(progress) } - -#[no_mangle] -pub extern "C" fn Servo_InterpolateColor( - interpolation: &ColorInterpolationMethod, - left: &AnimatedRGBA, - right: &AnimatedRGBA, - progress: f32, -) -> AnimatedRGBA { - style::values::animated::color::Color::mix( - interpolation, - left, - progress, - right, - 1.0 - progress, - /* normalize_weights = */ false, - ) -} diff --git a/testing/web-platform/tests/css/css-backgrounds/gradient-wrong-interpolation-crash.html b/testing/web-platform/tests/css/css-backgrounds/gradient-wrong-interpolation-crash.html deleted file mode 100644 index 5aefb64297112..0000000000000 --- a/testing/web-platform/tests/css/css-backgrounds/gradient-wrong-interpolation-crash.html +++ /dev/null @@ -1,7 +0,0 @@ - - -