Bug 141782 – [GTK] Accelerated compositing should support hidpi

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Christopher Bayliss]

Some sites do not display at HiDPI when using WebKit2GTK+. An example is https://duckduckgo.com/ It looks blurry on my HiDPI screen whereas https://google.com/ is fine.

The version of WebKit2GTK+ is 2.6.5 https://www.archlinux.org/packages/extra/x86_64/webkit2gtk/

It can be reproduced using GNOME Web (Epiphany) or my own test web-browser: https://github.com/cjbayliss/web-browser

[Christopher Bayliss]

Two more example sites that are blurry are youtube.com and plus.google.com

[Carlos Garcia Campos]

HiDPI depends on the GTK+ and cairo versions found when building. What version of GTK+ and cairo are you using?

[Christopher Bayliss]

I'm now using Arch linux with GTK+ 3.16.1, Cario 1.14.2, and WebKit2GTK+ 2.8.0. I still have the same problem where some site are not HiDPI.

I had this problem on Fedora 21 too. (GTK+ 3.14.x, WebKit2GTK+ 2.6.x, and I don't know the Cario version.)

refs:
https://www.archlinux.org/packages/extra/x86_64/cairo/
https://www.archlinux.org/packages/extra/x86_64/gtk3/

[Michael Catanzaro]

The 2.6.0 release notes say we have "HighDPI support for non-accelerated compositing contents" -- could that be the issue?

[Jiří Janoušek]

Which version of Cairo is required for proper HiDPI support?

[Matthew Gabeler-Lee]

I've noticed that some google sites (esp. the music player) initially start in HiDpi, and then drop down to "blurry" mode as the rest of the page starts to load.

On the other hand, arstechnica.com seems to stay in hidpi mode.

[Carlos Garcia Campos]

It could be that HiDPI is not implemented in accelerated composited mode, so when switching to AC mode, we lose the HiDPI support.

[Michael Gratton]

Github is another example I've noticed that can be initially crisp then degrading while still loading.

I'm finding that with v2.8.3 the page load starts crisp, then presumably when switching to accelerated compositing the content area flashes with some random corruption, then repaints itself blurry.

(In reply to comment #5)
> Which version of Cairo is required for proper HiDPI support?

Cairo 1.14 introduced support for device scaling.

[Gwang Yoon Hwang]

The reason for this problem 
is laking support of HiDPI from TextureMapper and TiledBackingStore.

It renders contents in to unscaled surface(x 1) and composite it via
scaled from (x 2) so the contents looks blurry.

Now I'm trying to fix this because my new laptop has a HiDPI screen.

[Michael Catanzaro]

Yoon, how did this development turn out? You sounded like you almost had a fix last month, but I guess ran into troubles?

[Gwang Yoon Hwang]

Created attachment 261640 [details]
Patch

[Gwang Yoon Hwang]

(In reply to comment #10)
> Yoon, how did this development turn out? You sounded like you almost had a
> fix last month, but I guess ran into troubles?

After devoting amount of time, one problem left.
You can see ugly part in my patch at DrawingAreaImpl.cpp

[Michael Catanzaro]

Awesome, thanks Yoon!

[Gwang Yoon Hwang]

Created attachment 261733 [details]
Patch

[WebKit Commit Bot]

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

[Carlos Garcia Campos]

Comment on attachment 261733 [details]
Patch

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

Thank you very much yoon. Patch works perfectly for me, I have only a few general questions/comments, since I'm not graphics expert.

> Source/WebCore/ChangeLog:11
> +        to apply transforms currectly.

currectly -> correctly.

> Source/WebCore/ChangeLog:14
> +        TextureMapperBackingStore, and GraphicsLayerTextureMapper do not handles

do not handles - does not handle

> Source/WebCore/platform/graphics/texmap/TextureMapperTiledBackingStore.cpp:35
> +    : m_contentsScale(1)
> +    , m_isScaleDirty(false)
> +    , m_isImage(false)

Move these initializations to the header

> Source/WebCore/platform/graphics/texmap/TextureMapperTiledBackingStore.cpp:93
> +    if (!m_isImage)

Could we just check if (m_image) ? and then we don't need the m_isImage member?

> Source/WebCore/platform/graphics/texmap/TextureMapperTiledBackingStore.cpp:173
> +    m_isImage = !!m_image;

I don't get why we need m_isImage now.

[Martin Robinson]

Comment on attachment 261733 [details]
Patch

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

Looks good to me, minus a few minor nits.

> Source/WebCore/platform/graphics/texmap/BitmapTexture.h:72
> +    virtual void updateContents(TextureMapper*, GraphicsLayer*, const IntRect& target, const IntPoint& offset, UpdateContentsFlag, float scale = 1.0f);

I think this can just be "scale = 1" here.

> Source/WebCore/platform/graphics/texmap/TextureMapperTile.h:43
> +    void updateContents(TextureMapper*, GraphicsLayer*, const IntRect&, BitmapTexture::UpdateContentsFlag UpdateCanModifyOriginalImageData, float scale = 1.0f);

Ditto.

[Gwang Yoon Hwang]

Created attachment 262143 [details]
Patch

[Gwang Yoon Hwang]

(In reply to comment #16)
> Comment on attachment 261733 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=261733&action=review
> 
> Thank you very much yoon. Patch works perfectly for me, I have only a few
> general questions/comments, since I'm not graphics expert.
> 

Nice!

> > Source/WebCore/ChangeLog:11
> > +        to apply transforms currectly.
> 
> currectly -> correctly.
> 

Thanks, I should enable a spell checker for my changelog editor.

> > Source/WebCore/ChangeLog:14
> > +        TextureMapperBackingStore, and GraphicsLayerTextureMapper do not handles
> 
> do not handles - does not handle
>
Fixed.
 
> > Source/WebCore/platform/graphics/texmap/TextureMapperTiledBackingStore.cpp:35
> > +    : m_contentsScale(1)
> > +    , m_isScaleDirty(false)
> > +    , m_isImage(false)
> 
> Move these initializations to the header
>
Done.
 
> > Source/WebCore/platform/graphics/texmap/TextureMapperTiledBackingStore.cpp:93
> > +    if (!m_isImage)
> 
> Could we just check if (m_image) ? and then we don't need the m_isImage
> member?
> 
> > Source/WebCore/platform/graphics/texmap/TextureMapperTiledBackingStore.cpp:173
> > +    m_isImage = !!m_image;
> 
> I don't get why we need m_isImage now.

Nice catch. It was added to support seperated path for image uploading / and tile creations from my previous working set.
It is not needed for this patch anywhere.

It is Removed.

[Gwang Yoon Hwang]

(In reply to comment #17)
> Comment on attachment 261733 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=261733&action=review
> 
> Looks good to me, minus a few minor nits.
> 
> > Source/WebCore/platform/graphics/texmap/BitmapTexture.h:72
> > +    virtual void updateContents(TextureMapper*, GraphicsLayer*, const IntRect& target, const IntPoint& offset, UpdateContentsFlag, float scale = 1.0f);
> 
> I think this can just be "scale = 1" here.
> 
> > Source/WebCore/platform/graphics/texmap/TextureMapperTile.h:43
> > +    void updateContents(TextureMapper*, GraphicsLayer*, const IntRect&, BitmapTexture::UpdateContentsFlag UpdateCanModifyOriginalImageData, float scale = 1.0f);
> 
> Ditto.

Thanks, these are updated :)

[Carlos Garcia Campos]

Comment on attachment 262143 [details]
Patch

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

> Source/WebCore/platform/graphics/texmap/TextureMapperTiledBackingStore.cpp:166
> +void TextureMapperTiledBackingStore::setContentsToImage(Image* image)
> +{
> +    m_image = image;
>  }

We can leave this in the header, as it currently is.

> Source/WebCore/platform/graphics/texmap/TextureMapperTiledBackingStore.h:54
> +    TextureMapperTiledBackingStore()
> +        : m_contentsScale(1), m_isScaleDirty(false)
> +    { }

This is not what I meant, see below.

> Source/WebCore/platform/graphics/texmap/TextureMapperTiledBackingStore.h:69
> +    float m_contentsScale;
> +    bool m_isScaleDirty;

float m_contentsScale { 1 };
bool m_isScaleDirty { false };

[Gwang Yoon Hwang]

Created attachment 262147 [details]
Patch

[Gwang Yoon Hwang]

(In reply to comment #21)
> Comment on attachment 262143 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=262143&action=review
> 
> > Source/WebCore/platform/graphics/texmap/TextureMapperTiledBackingStore.cpp:166
> > +void TextureMapperTiledBackingStore::setContentsToImage(Image* image)
> > +{
> > +    m_image = image;
> >  }
> 
> We can leave this in the header, as it currently is.
>
Done.
 
> > Source/WebCore/platform/graphics/texmap/TextureMapperTiledBackingStore.h:54
> > +    TextureMapperTiledBackingStore()
> > +        : m_contentsScale(1), m_isScaleDirty(false)
> > +    { }
> 
> This is not what I meant, see below.
> 
> > Source/WebCore/platform/graphics/texmap/TextureMapperTiledBackingStore.h:69
> > +    float m_contentsScale;
> > +    bool m_isScaleDirty;
> 
> float m_contentsScale { 1 };
> bool m_isScaleDirty { false };

Ah, it looks like I'm far behind. Modified.

[WebKit Commit Bot]

Comment on attachment 262147 [details]
Patch

Clearing flags on attachment: 262147

Committed r190344: <http://trac.webkit.org/changeset/190344>

[Michael Catanzaro]

(In reply to comment #21)
> > Source/WebCore/platform/graphics/texmap/TextureMapperTiledBackingStore.h:69
> > +    float m_contentsScale;
> > +    bool m_isScaleDirty;
> 
> float m_contentsScale { 1 };
> bool m_isScaleDirty { false };

You prefer this to 

float m_contentsScale = 1;
bool m_isScaleDirty = false;

? I know the C++ folks are saying to use bracket initialization everywhere to prevent narrowing, but we don't do that anywhere in WebKit and the style checker usually complains (I guess not in a header file, though).

[Carlos Garcia Campos]

(In reply to comment #25)
> (In reply to comment #21)
> > > Source/WebCore/platform/graphics/texmap/TextureMapperTiledBackingStore.h:69
> > > +    float m_contentsScale;
> > > +    bool m_isScaleDirty;
> > 
> > float m_contentsScale { 1 };
> > bool m_isScaleDirty { false };
> 
> You prefer this to 
> 
> float m_contentsScale = 1;
> bool m_isScaleDirty = false;
> 
> ? I know the C++ folks are saying to use bracket initialization everywhere
> to prevent narrowing, but we don't do that anywhere in WebKit and the style
> checker usually complains (I guess not in a header file, though).

We use brackets everywhere in newly written WebKit code