Bug 45087 – [chromium] Disable subpixel rendering in Linux when GPU compositor is active

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Alexey Marinichev]

[chromium] Disable subpixel rendering in Linux when GPU compositor is active

[Alexey Marinichev]

Created attachment 66316 [details]
Patch

[Alexey Marinichev]

Created attachment 66317 [details]
Patch

[Alexey Marinichev]

webkit-patch kindly created a Changelog file, but didn't add it to the commit.

[Vangelis Kokkevis]

Comment on attachment 66317 [details]
Patch

Looks good!

[Vangelis Kokkevis]

Comment on attachment 66317 [details]
Patch

Ooops, please disregard my previous response. I got mixed up with the bugs.

> diff --git a/WebCore/ChangeLog b/WebCore/ChangeLog
> index c8f9bcf61fa93600c1aac821e070adcb3f67a2d9..55a2df599434fc3fdb374b088460fbb543ad22ca 100644
> --- a/WebCore/ChangeLog
> +++ b/WebCore/ChangeLog
> @@ -1,3 +1,29 @@
> [phone]  Alexey Marinichev  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        [chromium] Disable subpixel rendering in Linux when GPU compositor is active
> +        https://bugs.webkit.org/show_bug.cgi?id=45087
> +
> +        No new tests. (OOPS!)

We'll probably need some test here...

> +
> +        * platform/graphics/chromium/ContentLayerChromium.cpp:
> +        (WebCore::ContentLayerChromium::updateContents):
> +        * platform/graphics/chromium/FontLinux.cpp:
> +        (WebCore::adjustTextRenderMode): Added a check to see if the compositor is active.
> +        (WebCore::Font::drawGlyphs):
> +        (WebCore::Font::drawComplexText):
> +        * platform/graphics/chromium/LayerRendererChromium.cpp:
> +        (WebCore::LayerRendererChromium::setRootLayerCanvasSize):
> +        * platform/graphics/chromium/VideoLayerChromium.cpp:
> +        (WebCore::VideoLayerChromium::updateContents):
> +        * platform/graphics/skia/ImageBufferSkia.cpp:
> +        (WebCore::ImageBuffer::ImageBuffer):
> +        * platform/graphics/skia/PlatformContextSkia.cpp:
> +        (WebCore::PlatformContextSkia::PlatformContextSkia): Made setDrawingToImageBuffer and
> +        isDrawingToImageBuffer available to Linux.
> +        * platform/graphics/skia/PlatformContextSkia.h:
> +
>  2010-08-27  Simon Fraser  <[e-mail]>
>  
>          Reviewed by Tony Chang.
> diff --git a/WebCore/platform/graphics/chromium/ContentLayerChromium.cpp b/WebCore/platform/graphics/chromium/ContentLayerChromium.cpp
> index 974933d45b779cc9394319cc46d3f06dce91b6bf..259a1fd6345f31d1836c499bd84eac72be1bc0bb 100644
> --- a/WebCore/platform/graphics/chromium/ContentLayerChromium.cpp
> +++ b/WebCore/platform/graphics/chromium/ContentLayerChromium.cpp
> @@ -174,12 +174,11 @@ void ContentLayerChromium::updateContents()
>      canvas.set(new skia::PlatformCanvas(dirtyRect.width(), dirtyRect.height(), false));
>      skiaContext.set(new PlatformContextSkia(canvas.get()));
>  
> -#if OS(WINDOWS)
> +#if OS(WINDOWS) || OS(LINUX)

I don't think that we need to test for OS(WINDOWS) || OS(LINUX) anymore. Can't this method be available to all OS's that use PLATFORM(SKIA)? 

>      // This is needed to get text to show up correctly. Without it,
>      // GDI renders with zero alpha and the text becomes invisible.
>      // Unfortunately, setting this to true disables cleartype.
>      // FIXME: Does this take us down a very slow text rendering path?
> -    // FIXME: why is this is a windows-only call ?
>      skiaContext->setDrawingToImageBuffer(true);
>  #endif
>  
> diff --git a/WebCore/platform/graphics/chromium/FontLinux.cpp b/WebCore/platform/graphics/chromium/FontLinux.cpp
> index ec79b82bb52171c9ef95071aa0d016403439bf65..239266db098cb618905b02fd2d4afc0c65a60de0 100644
> --- a/WebCore/platform/graphics/chromium/FontLinux.cpp
> +++ b/WebCore/platform/graphics/chromium/FontLinux.cpp
> @@ -65,13 +65,13 @@ static bool isCanvasMultiLayered(SkCanvas* canvas)
>      return !layerIterator.done();
>  }
>  
> -static void adjustTextRenderMode(SkPaint* paint, bool isCanvasMultiLayered)
> +static void adjustTextRenderMode(SkPaint* paint, GraphicsContext* gc)
>  {
>      // Our layers only have a single alpha channel. This means that subpixel
>      // rendered text cannot be compositied correctly when the layer is
>      // collapsed. Therefore, subpixel text is disabled when we are drawing
> -    // onto a layer.
> -    if (isCanvasMultiLayered)
> +    // onto a layer or when the compositor is being used.
> +    if (isCanvasMultiLayered(gc->platformContext()->canvas()) || gc->platformContext()->isDrawingToImageBuffer())
>          paint->setLCDRenderText(false);
>  }
>  
> @@ -102,14 +102,13 @@ void Font::drawGlyphs(GraphicsContext* gc, const SimpleFontData* font,
>  
>      SkCanvas* canvas = gc->platformContext()->canvas();
>      int textMode = gc->platformContext()->getTextDrawingMode();
> -    bool haveMultipleLayers = isCanvasMultiLayered(canvas);
>  
>      // We draw text up to two times (once for fill, once for stroke).
>      if (textMode & cTextFill) {
>          SkPaint paint;
>          gc->platformContext()->setupPaintForFilling(&paint);
>          font->platformData().setupPaint(&paint);
> -        adjustTextRenderMode(&paint, haveMultipleLayers);
> +        adjustTextRenderMode(&paint, gc);
>          paint.setTextEncoding(SkPaint::kGlyphID_TextEncoding);
>          paint.setColor(gc->fillColor().rgb());
>          canvas->drawPosText(glyphs, numGlyphs << 1, pos, paint);
> @@ -122,7 +121,7 @@ void Font::drawGlyphs(GraphicsContext* gc, const SimpleFontData* font,
>          SkPaint paint;
>          gc->platformContext()->setupPaintForStroking(&paint, 0, 0);
>          font->platformData().setupPaint(&paint);
> -        adjustTextRenderMode(&paint, haveMultipleLayers);
> +        adjustTextRenderMode(&paint, gc);
>          paint.setTextEncoding(SkPaint::kGlyphID_TextEncoding);
>          paint.setColor(gc->strokeColor().rgb());
>  
> @@ -637,7 +636,6 @@ void Font::drawComplexText(GraphicsContext* gc, const TextRun& run,
>      }
>  
>      TextRunWalker walker(run, point.x(), this);
> -    bool haveMultipleLayers = isCanvasMultiLayered(canvas);
>      walker.setWordSpacingAdjustment(wordSpacing());
>      walker.setLetterSpacingAdjustment(letterSpacing());
>      walker.setPadding(run.padding());
> @@ -645,13 +643,13 @@ void Font::drawComplexText(GraphicsContext* gc, const TextRun& run,
>      while (walker.nextScriptRun()) {
>          if (fill) {
>              walker.fontPlatformDataForScriptRun()->setupPaint(&fillPaint);
> -            adjustTextRenderMode(&fillPaint, haveMultipleLayers);
> +            adjustTextRenderMode(&fillPaint, gc);
>              canvas->drawPosTextH(walker.glyphs(), walker.length() << 1, walker.xPositions(), point.y(), fillPaint);
>          }
>  
>          if (stroke) {
>              walker.fontPlatformDataForScriptRun()->setupPaint(&strokePaint);
> -            adjustTextRenderMode(&strokePaint, haveMultipleLayers);
> +            adjustTextRenderMode(&strokePaint, gc);
>              canvas->drawPosTextH(walker.glyphs(), walker.length() << 1, walker.xPositions(), point.y(), strokePaint);
>          }
>      }
> diff --git a/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp b/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp
> index e87d63e1f2765ce36e5e5f524bace2b73596c7df..a501a281102bd4e9a5982e5e36d4b46a66eb2ca8 100644
> --- a/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp
> +++ b/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp
> @@ -118,7 +118,7 @@ void LayerRendererChromium::setRootLayerCanvasSize(const IntSize& size)
>      // the old ones.
>      m_rootLayerCanvas = new skia::PlatformCanvas(size.width(), size.height(), false);
>      m_rootLayerSkiaContext = new PlatformContextSkia(m_rootLayerCanvas.get());
> -#if OS(WINDOWS)
> +#if OS(WINDOWS) || OS(LINUX)
>      // FIXME: why is this is a windows-only call ?

FIXME is no longer valid

>      m_rootLayerSkiaContext->setDrawingToImageBuffer(true);
>  #endif
> diff --git a/WebCore/platform/graphics/chromium/VideoLayerChromium.cpp b/WebCore/platform/graphics/chromium/VideoLayerChromium.cpp
> index 7ff98b93fd03ada2da68fc4d1f6e29b5740ff4be..62530274f227e829888a0e51e42f14f87f2392db 100644
> --- a/WebCore/platform/graphics/chromium/VideoLayerChromium.cpp
> +++ b/WebCore/platform/graphics/chromium/VideoLayerChromium.cpp
> @@ -96,7 +96,7 @@ void VideoLayerChromium::updateContents()
>          m_canvas = new skia::PlatformCanvas(dirtyRect.width(), dirtyRect.height(), true);
>          m_skiaContext = new PlatformContextSkia(m_canvas.get());
>  
> -#if OS(WINDOWS)
> +#if OS(WINDOWS) || OS(LINUX)
>          // This is needed to get text to show up correctly. Without it,
>          // GDI renders with zero alpha and the text becomes invisible.
Please remove the GDI comment here if this will be called on Linux as well

>          // Unfortunately, setting this to true disables cleartype.
> diff --git a/WebCore/platform/graphics/skia/ImageBufferSkia.cpp b/WebCore/platform/graphics/skia/ImageBufferSkia.cpp
> index 9403406300a7fe79fe04fe21cf14955e72a259db..0f4172e38a9bf0733331db5d37c958dcb5e454dd 100644
> --- a/WebCore/platform/graphics/skia/ImageBufferSkia.cpp
> +++ b/WebCore/platform/graphics/skia/ImageBufferSkia.cpp
> @@ -67,7 +67,7 @@ ImageBuffer::ImageBuffer(const IntSize& size, ImageColorSpace imageColorSpace, b
>  
>      m_data.m_platformContext.setCanvas(&m_data.m_canvas);
>      m_context.set(new GraphicsContext(&m_data.m_platformContext));
> -#if OS(WINDOWS)
> +#if OS(WINDOWS) || OS(LINUX)
>      m_context->platformContext()->setDrawingToImageBuffer(true);
>  #endif
>  
> diff --git a/WebCore/platform/graphics/skia/PlatformContextSkia.cpp b/WebCore/platform/graphics/skia/PlatformContextSkia.cpp
> index af0c4c41ac79aebe2f088911be31120869c30c57..5078826cae172deb03054f100a76c516f91eb01c 100644
> --- a/WebCore/platform/graphics/skia/PlatformContextSkia.cpp
> +++ b/WebCore/platform/graphics/skia/PlatformContextSkia.cpp
> @@ -204,7 +204,7 @@ SkColor PlatformContextSkia::State::applyAlpha(SkColor c) const
>  // Danger: canvas can be NULL.
>  PlatformContextSkia::PlatformContextSkia(skia::PlatformCanvas* canvas)
>      : m_canvas(canvas)
> -#if OS(WINDOWS)
> +#if OS(WINDOWS) || OS(LINUX)
>      , m_drawingToImageBuffer(false)
>  #endif
>      , m_useGPU(false)
> @@ -230,7 +230,7 @@ void PlatformContextSkia::setCanvas(skia::PlatformCanvas* canvas)
>      m_canvas = canvas;
>  }
>  
> -#if OS(WINDOWS)
> +#if OS(WINDOWS) || OS(LINUX)
>  void PlatformContextSkia::setDrawingToImageBuffer(bool value)
>  {
>      m_drawingToImageBuffer = value;
> diff --git a/WebCore/platform/graphics/skia/PlatformContextSkia.h b/WebCore/platform/graphics/skia/PlatformContextSkia.h
> index fc0fb0b13e0e5cb89972ae0f4789b192a2481b36..8e82ebf2796fcfd32d65021254754ff066698567 100644
> --- a/WebCore/platform/graphics/skia/PlatformContextSkia.h
> +++ b/WebCore/platform/graphics/skia/PlatformContextSkia.h
> @@ -78,7 +78,7 @@ public:
>      // to the constructor.
>      void setCanvas(skia::PlatformCanvas*);
>  
> -#if OS(WINDOWS)
> +#if OS(WINDOWS) || OS(LINUX)

Why not on have it on all OS's that use PlatformContextSkia? 

>      // If false we're rendering to a GraphicsContext for a web page, if false
>      // we're not (as is the case when rendering to a canvas object).
>      // If this is true the contents have not been marked up with the magic
> @@ -222,7 +222,7 @@ private:
>      // Values are used in ImageSkia.cpp
>      IntSize m_imageResamplingHintSrcSize;
>      FloatSize m_imageResamplingHintDstSize;
> -#if OS(WINDOWS)
> +#if OS(WINDOWS) || OS(LINUX)
>      bool m_drawingToImageBuffer;
>  #endif
>      bool m_useGPU;

[Alexey Marinichev]

Created attachment 66427 [details]
Patch

[Alexey Marinichev]

Comments addressed.

[James Robinson]

Comment on attachment 66427 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=66427&action=prettypatch

> WebCore/platform/graphics/chromium/FontLinux.cpp:68
> -static void adjustTextRenderMode(SkPaint* paint, bool isCanvasMultiLayered)
> +static void adjustTextRenderMode(SkPaint* paint, GraphicsContext* gc)
I think it would be a marginal improvement if this took a PlatformContextSkia* instead of a GraphicsContext*.  This function is only concerned with Skia-specific state, not general WebCore::GraphicsContext stuff.

> WebCore/platform/graphics/skia/ImageBufferSkia.cpp:70
> +#if OS(WINDOWS) || OS(LINUX)
Unnecessary.  OS(WINDOWS) || OS(LINUX) === skia and this is a skia-only file.

> WebCore/platform/graphics/skia/PlatformContextSkia.cpp:207
> +#if OS(LINUX) || OS(WINDOWS)
remove

> WebCore/platform/graphics/skia/PlatformContextSkia.cpp:233
> +#if OS(LINUX) || OS(WINDOWS)
remove

> WebCore/platform/graphics/skia/PlatformContextSkia.h:81
> +#if OS(LINUX) || OS(WINDOWS)
remove

> WebCore/platform/graphics/skia/PlatformContextSkia.h:225
> +#if OS(WINDOWS) || OS(LINUX)
remove

[Alexey Marinichev]

Created attachment 67106 [details]
Patch

[James Robinson]

Comment on attachment 67106 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=67106&action=prettypatch

> WebCore/platform/graphics/chromium/FontLinux.cpp:68
> +static void adjustTextRenderMode(SkPaint* paint, PlatformGraphicsContext* pgc)
PlatformContextSkia, not PlatformGraphicsContext

[Alexey Marinichev]

Created attachment 67112 [details]
Patch

[James Robinson]

Comment on attachment 67112 [details]
Patch

R=me.

[James Robinson]

Committed r67126: <http://trac.webkit.org/changeset/67126>

[James Robinson]

Comment on attachment 67112 [details]
Patch

Patch has landed, clearing flags.

[WebKit Review Bot]

http://trac.webkit.org/changeset/67126 might have broken Chromium Mac Release
The following changes are on the blame list:
http://trac.webkit.org/changeset/67128
http://trac.webkit.org/changeset/67129
http://trac.webkit.org/changeset/67126
http://trac.webkit.org/changeset/67127

[WebKit Review Bot]

http://trac.webkit.org/changeset/67127 might have broken Chromium Mac Release
The following changes are on the blame list:
http://trac.webkit.org/changeset/67128
http://trac.webkit.org/changeset/67129
http://trac.webkit.org/changeset/67126
http://trac.webkit.org/changeset/67127

[James Robinson]

This broke the chromium mac compile.  Apparently it attempts to compile PlatformContextSkia.cpp, but it appears that it's not actually used.  I'm going to expand the exclusion list for mac and try to re-land.

[James Robinson]

Committed r67161: <http://trac.webkit.org/changeset/67161>