Changeset 207681 in webkit


Ignore:
Timestamp:
Oct 21, 2016 10:10:31 AM (8 years ago)
Author:
Brent Fulgham
Message:

[Win][Direct2D] Correct some memory leaks and other minor bugs
​https://bugs.webkit.org/show_bug.cgi?id=163769

Reviewed by Alex Christensen.

Several D2D handles were being leaked.

Direct2D sometimes returns an infinite rect containing { -inf, -inf, FloatMax, FloatMax },
sometimes { -FloatMax, -FloatMax, inf, inf }, and various combinations thereof. This caused
most SVG drawing to decide no screen rect was contained in the "infinite rect" so nothing
would be drawn.

Tested by existing layout tests.

  • platform/graphics/GraphicsContext.h:
  • platform/graphics/win/FloatRectDirect2D.cpp:

(WebCore::isInfiniteRect): Recognize various infinite rects in Windows.
(WebCore::FloatRect::FloatRect): Convert a Windows infinite rect to the style
we use inside WebKit.

  • platform/graphics/win/FontCascadeDirect2D.cpp:

(WebCore::FontCascade::drawGlyphs): Use cached brushes if possible.

  • platform/graphics/win/GlyphPageTreeNodeDirect2D.cpp:

(WebCore::GlyphPage::fill): Don't terminate on this error case.

  • platform/graphics/win/GradientDirect2D.cpp:

(WebCore::Gradient::generateGradient): Don't leak gradients.

  • platform/graphics/win/GraphicsContextDirect2D.cpp:

(WebCore::GraphicsContextPlatformPrivate::brushWithColor): Added.
(WebCore::GraphicsContext::brushWithColor): Added.
(WebCore::GraphicsContextPlatformPrivate::concatCTM): Perform transform multiplication
in the right order (hint: it's not distributive).
(WebCore::GraphicsContext::drawWithShadow): Use convenience method.
(WebCore::GraphicsContext::fillRect): Ditto.
(WebCore::GraphicsContext::platformFillRoundedRect): Ditto.
(WebCore::GraphicsContext::clearRect): Ditto.
(WebCore::GraphicsContext::setPlatformStrokeColor): Ditto.
(WebCore::GraphicsContext::setPlatformFillColor): Ditto.

  • platform/graphics/win/PathDirect2D.cpp:

(WebCore::Path::polygonPathFromPoints): No need to convert manually.
(WebCore::Path::~Path): Don't leak ID2D1Geometry entities.
(WebCore::Path::appendGeometry): Ditto.
(WebCore::Path::createGeometryWithFillMode): Ditto.
(WebCore::Path::Path): Ditto.
(WebCore::Path::operator=): Ditto.
(WebCore::Path::strokeBoundingRect): Provide an implementation.
(WebCore::Path::addRect): No need for manual casting here.

Location:
trunk/Source/WebCore
Files:
8 edited

Legend:

Unmodified
Added
Removed

  • trunk/Source/WebCore/ChangeLog

    r207680 r207681  
     12016-10-20  Brent Fulgham  <bfulgham@apple.com>
     2
     3        [Win][Direct2D] Correct some memory leaks and other minor bugs
     4        https://bugs.webkit.org/show_bug.cgi?id=163769
     5
     6        Reviewed by Alex Christensen.
     7
     8        Several D2D handles were being leaked.
     9 
     10        Direct2D sometimes returns an infinite rect containing { -inf, -inf, FloatMax, FloatMax },
     11        sometimes { -FloatMax, -FloatMax, inf, inf }, and various combinations thereof. This caused
     12        most SVG drawing to decide no screen rect was contained in the "infinite rect" so nothing
     13        would be drawn.
     14       
     15        Tested by existing layout tests.
     16
     17        * platform/graphics/GraphicsContext.h:
     18        * platform/graphics/win/FloatRectDirect2D.cpp:
     19        (WebCore::isInfiniteRect): Recognize various infinite rects in Windows.
     20        (WebCore::FloatRect::FloatRect): Convert a Windows infinite rect to the style
     21        we use inside WebKit.
     22        * platform/graphics/win/FontCascadeDirect2D.cpp:
     23        (WebCore::FontCascade::drawGlyphs): Use cached brushes if possible.
     24        * platform/graphics/win/GlyphPageTreeNodeDirect2D.cpp:
     25        (WebCore::GlyphPage::fill): Don't terminate on this error case.
     26        * platform/graphics/win/GradientDirect2D.cpp:
     27        (WebCore::Gradient::generateGradient): Don't leak gradients.
     28        * platform/graphics/win/GraphicsContextDirect2D.cpp:
     29        (WebCore::GraphicsContextPlatformPrivate::brushWithColor): Added.
     30        (WebCore::GraphicsContext::brushWithColor): Added.
     31        (WebCore::GraphicsContextPlatformPrivate::concatCTM): Perform transform multiplication
     32        in the right order (hint: it's not distributive).
     33        (WebCore::GraphicsContext::drawWithShadow): Use convenience method.
     34        (WebCore::GraphicsContext::fillRect): Ditto.
     35        (WebCore::GraphicsContext::platformFillRoundedRect): Ditto.
     36        (WebCore::GraphicsContext::clearRect): Ditto.
     37        (WebCore::GraphicsContext::setPlatformStrokeColor): Ditto.
     38        (WebCore::GraphicsContext::setPlatformFillColor): Ditto.
     39        * platform/graphics/win/PathDirect2D.cpp:
     40        (WebCore::Path::polygonPathFromPoints): No need to convert manually.
     41        (WebCore::Path::~Path): Don't leak ID2D1Geometry entities.
     42        (WebCore::Path::appendGeometry): Ditto.
     43        (WebCore::Path::createGeometryWithFillMode): Ditto.
     44        (WebCore::Path::Path): Ditto.
     45        (WebCore::Path::operator=): Ditto.
     46        (WebCore::Path::strokeBoundingRect): Provide an implementation.
     47        (WebCore::Path::addRect): No need for manual casting here.
     48
    1492016-10-21  Wenson Hsieh  <wenson_hsieh@apple.com>
    250

  • trunk/Source/WebCore/platform/graphics/GraphicsContext.h

    r207140 r207681  
    4343interface ID2D1RenderTarget;
    4444interface ID2D1Factory;
     45interface ID2D1SolidColorBrush;
    4546typedef ID2D1RenderTarget PlatformGraphicsContext;
    4647#elif USE(CAIRO)
    … …  
    560561    ID2D1Brush* patternStrokeBrush() const;
    561562    ID2D1Brush* patternFillBrush() const;
     563
     564    ID2D1SolidColorBrush* brushWithColor(const Color&);
    562565#endif
    563566#else // PLATFORM(WIN)

  • trunk/Source/WebCore/platform/graphics/win/FloatRectDirect2D.cpp

    r205871 r207681  
    3434namespace WebCore {
    3535
     36static bool isInfiniteRect(const D2D1_RECT_F& rect)
     37{
     38    if (!std::isinf(rect.top) && rect.top != -std::numeric_limits<float>::max())
     39        return false;
     40
     41    if (!std::isinf(rect.left) && rect.left != -std::numeric_limits<float>::max())
     42        return false;
     43
     44    if (!std::isinf(rect.bottom) && rect.bottom != std::numeric_limits<float>::max())
     45        return false;
     46
     47    if (!std::isinf(rect.right) && rect.right != std::numeric_limits<float>::max())
     48        return false;
     49
     50    return true;
     51}
     52
    3653FloatRect::FloatRect(const D2D1_RECT_F& r)
    37     : m_location(FloatPoint(r.left, r.top))
    38     , m_size(FloatSize(r.right - r.left, r.bottom - r.top))
    3954{
     55    // Infinite Rect case:
     56    if (isInfiniteRect(r)) {
     57        *this = infiniteRect();
     58        return;
     59    }
     60
     61    m_location = FloatPoint(r.left, r.top);
     62    m_size = FloatSize(r.right - r.left, r.bottom - r.top);
    4063}
    4164

  • trunk/Source/WebCore/platform/graphics/win/FontCascadeDirect2D.cpp

    r206597 r207681  
    130130        float shadowTextY = point.y() + translation.height() + shadowOffset.height() * (graphicsContext.shadowsIgnoreTransforms() ? -1 : 1);
    131131
    132         COMPtr<ID2D1SolidColorBrush> shadowBrush;
    133         if (!SUCCEEDED(context->CreateSolidColorBrush(graphicsContext.colorWithGlobalAlpha(shadowFillColor), &shadowBrush)))
    134             return;
    135 
    136         context->DrawGlyphRun(D2D1::Point2F(shadowTextX, shadowTextY), &glyphRun, shadowBrush.get());
     132        auto shadowBrush = graphicsContext.brushWithColor(shadowFillColor);
     133        context->DrawGlyphRun(D2D1::Point2F(shadowTextX, shadowTextY), &glyphRun, shadowBrush);
    137134        if (font.syntheticBoldOffset())
    138             context->DrawGlyphRun(D2D1::Point2F(shadowTextX + font.syntheticBoldOffset(), shadowTextY), &glyphRun, shadowBrush.get());
     135            context->DrawGlyphRun(D2D1::Point2F(shadowTextX + font.syntheticBoldOffset(), shadowTextY), &glyphRun, shadowBrush);
    139136    }
    140137

  • trunk/Source/WebCore/platform/graphics/win/GlyphPageTreeNodeDirect2D.cpp

    r206830 r207681  
    6868        &helper.m_analysis, nullptr, nullptr, nullptr, nullptr, 0, GlyphPage::size, clusterMap, textProperties.data(),
    6969        localGlyphBuffer, glyphProperties.data(), &returnedCount);
    70     RELEASE_ASSERT(SUCCEEDED(hr));
     70    if (!SUCCEEDED(hr))
     71        return false;
    7172
    7273    for (unsigned i = 0; i < GlyphPage::size; i++) {

  • trunk/Source/WebCore/platform/graphics/win/GradientDirect2D.cpp

    r207028 r207681  
    6666    HRESULT hr = renderTarget->CreateGradientStopCollection(gradientStops.data(), gradientStops.size(), &gradientStopCollection);
    6767    RELEASE_ASSERT(SUCCEEDED(hr));
     68
     69    if (m_gradient) {
     70        m_gradient->Release();
     71        m_gradient = nullptr;
     72    }
    6873
    6974    if (m_radial) {

  • trunk/Source/WebCore/platform/graphics/win/GraphicsContextDirect2D.cpp

    r207357 r207681  
    275275
    276276    auto existingBrush = m_solidColoredBrushCache.ensure(colorKey, [this, color] {
    277         ID2D1SolidColorBrush* colorBrush = nullptr;
     277        COMPtr<ID2D1SolidColorBrush> colorBrush;
    278278        m_renderTarget->CreateSolidColorBrush(color, &colorBrush);
    279279        return colorBrush;
    … …  
    281281
    282282    return existingBrush.iterator->value;
     283}
     284
     285ID2D1SolidColorBrush* GraphicsContext::brushWithColor(const Color& color)
     286{
     287    return m_data->brushWithColor(colorWithGlobalAlpha(color)).get();
    283288}
    284289
    … …  
    321326    m_renderTarget->GetTransform(&currentTransform);
    322327
    323     m_renderTarget->SetTransform(affineTransform * AffineTransform(currentTransform));
     328    D2D1_MATRIX_3X2_F transformToConcat = affineTransform;
     329    m_renderTarget->SetTransform(transformToConcat * currentTransform);
    324330}
    325331
    … …  
    883889    deviceContext->BeginDraw();
    884890    deviceContext->DrawImage(compositor.get(), D2D1_INTERPOLATION_MODE_LINEAR);
    885     deviceContext->EndDraw();
     891    hr = deviceContext->EndDraw();
     892    ASSERT(SUCCEEDED(hr));
    886893}
    887894
    … …  
    10521059    drawWithoutShadow(rect, [this, rect, color](ID2D1RenderTarget* renderTarget) {
    10531060        const D2D1_RECT_F d2dRect = rect;
    1054         renderTarget->FillRectangle(&d2dRect, m_data->brushWithColor(colorWithGlobalAlpha(color)).get());
     1061        renderTarget->FillRectangle(&d2dRect, brushWithColor(color));
    10551062    });
    10561063}
    … …  
    10861093    if (!hasCustomFill && equalWidths && equalHeights && radii.topLeft().width() * 2 == r.width() && radii.topLeft().height() * 2 == r.height()) {
    10871094        auto roundedRect = D2D1::RoundedRect(r, radii.topLeft().width(), radii.topLeft().height());
    1088         COMPtr<ID2D1SolidColorBrush> fillBrush = m_data->brushWithColor(colorWithGlobalAlpha(color));
    1089         context->FillRoundedRectangle(roundedRect, fillBrush.get());
     1095        context->FillRoundedRectangle(roundedRect, brushWithColor(color));
    10901096    } else {
    10911097        D2DContextStateSaver stateSaver(*m_data);
    … …  
    13461352        renderTarget->SetTags(1, __LINE__);
    13471353        rectToClear.intersect(renderTargetRect);
    1348         renderTarget->FillRectangle(rectToClear, m_data->brushWithColor(colorWithGlobalAlpha(fillColor())).get());
     1354        renderTarget->FillRectangle(rectToClear, solidFillBrush());
    13491355    });
    13501356}
    … …  
    16691675    m_data->m_solidStrokeBrush = nullptr;
    16701676
    1671     m_data->m_solidStrokeBrush = m_data->brushWithColor(colorWithGlobalAlpha(strokeColor()));
     1677    m_data->m_solidStrokeBrush = brushWithColor(strokeColor());
    16721678}
    16731679
    … …  
    16841690    m_data->m_solidFillBrush = nullptr;
    16851691
    1686     m_data->m_solidFillBrush = m_data->brushWithColor(colorWithGlobalAlpha(fillColor()));
     1692    m_data->m_solidFillBrush = brushWithColor(fillColor());
    16871693}
    16881694

  • trunk/Source/WebCore/platform/graphics/win/PathDirect2D.cpp

    r207020 r207681  
    5353    d2dPoints.reserveInitialCapacity(points.size() - 1);
    5454    for (auto point : points)
    55         d2dPoints.uncheckedAppend(D2D1::Point2F(point.x(), point.y()));
     55        d2dPoints.uncheckedAppend(point);
    5656
    5757    path.moveTo(points.first());
    … …  
    7171Path::~Path()
    7272{
     73    if (m_path)
     74        m_path->Release();
    7375}
    7476
    … …  
    9092    Vector<ID2D1Geometry*> geometries(geometryCount, nullptr);
    9193
     94    // Note: 'GetSourceGeometries' returns geometries that have a +1 ref count.
     95    // so they must be released before we return.
    9296    if (geometryCount)
    9397        m_path->GetSourceGeometries(geometries.data(), geometryCount);
    9498
     99    geometry->AddRef();
    95100    geometries.append(geometry);
    96101
    97102    auto fillMode = m_path ? m_path->GetFillMode() : D2D1_FILL_MODE_WINDING;
    98103
    99     COMPtr<ID2D1GeometryGroup> protectedPath = m_path;
     104    COMPtr<ID2D1GeometryGroup> protectedPath = adoptCOM(m_path);
    100105    m_path = nullptr;
    101106
    102107    HRESULT hr = GraphicsContext::systemFactory()->CreateGeometryGroup(fillMode, geometries.data(), geometries.size(), &m_path);
    103108    RELEASE_ASSERT(SUCCEEDED(hr));
     109
     110    for (auto entry : geometries)
     111        entry->Release();
    104112}
    105113
    … …  
    119127    Vector<ID2D1Geometry*> geometries(geometryCount, nullptr);
    120128    ASSERT(geometryCount);
     129
     130    // Note: 'GetSourceGeometries' returns geometries that have a +1 ref count.
     131    // so they must be released before we return.
    121132    m_path->GetSourceGeometries(geometries.data(), geometryCount);
    122133
    123134    HRESULT hr = GraphicsContext::systemFactory()->CreateGeometryGroup(fillMode, geometries.data(), geometries.size(), &path);
    124135    RELEASE_ASSERT(SUCCEEDED(hr));
     136
     137    for (auto entry : geometries)
     138        entry->Release();
    125139}
    126140
    … …  
    134148        Vector<ID2D1Geometry*> geometries(geometryCount, nullptr);
    135149        ASSERT(geometryCount);
     150
     151        // Note: 'GetSourceGeometries' returns geometries that have a +1 ref count.
     152        // so they must be released before we return.
    136153        otherPath->GetSourceGeometries(geometries.data(), geometryCount);
    137154
    138155        HRESULT hr = GraphicsContext::systemFactory()->CreateGeometryGroup(other.m_path->GetFillMode(), geometries.data(), geometryCount, &m_path);
    139156        RELEASE_ASSERT(SUCCEEDED(hr));
     157
     158        for (auto entry : geometries)
     159            entry->Release();
    140160    }
    141161}
    … …  
    143163Path& Path::operator=(const Path& other)
    144164{
     165    if (m_path)
     166        m_path->Release();
     167
    145168    m_path = other.m_path;
    146169    if (m_path)
    … …  
    265288        return FloatRect();
    266289
    267     return FloatRect(bounds.left, bounds.bottom, bounds.right - bounds.left, bounds.top - bounds.bottom);
     290    return bounds;
    268291}
    269292
    … …  
    277300        return FloatRect();
    278301
    279     return FloatRect(bounds.left, bounds.bottom, bounds.right - bounds.left, bounds.top - bounds.bottom);
     302    return bounds;
    280303}
    281304
    … …  
    284307    if (isNull())
    285308        return FloatRect();
     309
     310    if (!applier)
     311        return boundingRect();
    286312
    287313    UNUSED_PARAM(applier);
    288314    notImplemented();
    289     return FloatRect();
     315
     316    // Just return regular bounding rect for now.
     317    return boundingRect();
    290318}
    291319
    … …  
    381409}
    382410
    383 static void drawArcSection(ID2D1GeometrySink* sink, FloatPoint center, float radius, float startAngle, float endAngle, bool clockwise)
     411static FloatPoint arcStart(const FloatPoint& center, float radius, float startAngle)
     412{
     413    FloatPoint startingPoint = center;
     414    float startX = radius * std::cos(startAngle);
     415    float startY = radius * std::sin(startAngle);
     416    startingPoint.move(startX, startY);
     417    return startingPoint;
     418}
     419
     420static void drawArcSection(ID2D1GeometrySink* sink, const FloatPoint& center, float radius, float startAngle, float endAngle, bool clockwise)
    384421{
    385422    // Direct2D wants us to specify the end point of the arc, not the center. It will be drawn from
    … …  
    397434void Path::addArc(const FloatPoint& center, float radius, float startAngle, float endAngle, bool clockwise)
    398435{
    399     if (!m_activePath) {
    400         float startX = radius * std::cos(startAngle);
    401         float startY = radius * std::sin(startAngle);
    402 
    403         FloatPoint start = center;
    404         start.move(startX, startY);
    405         moveTo(start);
     436    auto arcStartPoint = arcStart(center, radius, startAngle);
     437    if (!m_activePath)
     438        moveTo(arcStartPoint);
     439    else if (!areEssentiallyEqual(currentPoint(), arcStartPoint)) {
     440        // If the arc defined by the center and radius does not intersect the current position,
     441        // we need to draw a line from the current position to the starting point of the arc.
     442        addLineTo(arcStartPoint);
    406443    }
    407444
    … …  
    424461{
    425462    COMPtr<ID2D1RectangleGeometry> rectangle;
    426     HRESULT hr = GraphicsContext::systemFactory()->CreateRectangleGeometry(static_cast<D2D1_RECT_F>(r), &rectangle);
     463    HRESULT hr = GraphicsContext::systemFactory()->CreateRectangleGeometry(r, &rectangle);
    427464    RELEASE_ASSERT(SUCCEEDED(hr));
    428465    appendGeometry(rectangle.get());
Note: See TracChangeset for help on using the changeset viewer.