Bug 141782

Summary: [GTK] Accelerated compositing should support hidpi
Product: WebKit Reporter: Christopher Bayliss <christopher.j.bayliss>
Component: WebKitGTKAssignee: Gwang Yoon Hwang <yoon>
Status: RESOLVED FIXED    
Severity: Normal CC: berto, cgarcia, christopher.j.bayliss, cmarcelo, commit-queue, fastcat, gns, janousek.jiri, kondapallykalyan, luiz, mcatanzaro, mike, mrobinson, noam, realh69, yoon
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Linux   
See Also: https://bugzilla.gnome.org/show_bug.cgi?id=726507
Attachments:
Description Flags
Patch
none
Patch
none
Patch
cgarcia: review+, cgarcia: commit-queue-
Patch none

Description Christopher Bayliss 2015-02-18 16:59:10 PST
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
Comment 1 Christopher Bayliss 2015-02-19 15:31:41 PST
Two more example sites that are blurry are youtube.com and plus.google.com
Comment 2 Carlos Garcia Campos 2015-04-10 02:51:47 PDT
HiDPI depends on the GTK+ and cairo versions found when building. What version of GTK+ and cairo are you using?
Comment 3 Christopher Bayliss 2015-04-10 17:15:56 PDT
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/
Comment 4 Michael Catanzaro 2015-04-22 10:36:26 PDT
The 2.6.0 release notes say we have "HighDPI support for non-accelerated compositing contents" -- could that be the issue?
Comment 5 Jiří Janoušek 2015-05-04 11:00:39 PDT
Which version of Cairo is required for proper HiDPI support?
Comment 6 Matthew Gabeler-Lee 2015-05-04 11:17:00 PDT
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.
Comment 7 Carlos Garcia Campos 2015-05-04 23:01:30 PDT
It could be that HiDPI is not implemented in accelerated composited mode, so when switching to AC mode, we lose the HiDPI support.
Comment 8 Michael Gratton 2015-07-22 23:45:47 PDT
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.
Comment 9 Gwang Yoon Hwang 2015-07-22 23:55:51 PDT
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.
Comment 10 Michael Catanzaro 2015-09-18 14:31:20 PDT
Yoon, how did this development turn out? You sounded like you almost had a fix last month, but I guess ran into troubles?
Comment 11 Gwang Yoon Hwang 2015-09-21 01:40:40 PDT
Created attachment 261640 [details]
Patch
Comment 12 Gwang Yoon Hwang 2015-09-21 01:51:38 PDT
(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
Comment 13 Michael Catanzaro 2015-09-21 17:30:05 PDT
Awesome, thanks Yoon!
Comment 14 Gwang Yoon Hwang 2015-09-22 03:02:40 PDT
Created attachment 261733 [details]
Patch
Comment 15 WebKit Commit Bot 2015-09-22 03:07:36 PDT
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
Comment 16 Carlos Garcia Campos 2015-09-29 02:58:03 PDT
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.
Comment 17 Martin Robinson 2015-09-29 10:44:45 PDT
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.
Comment 18 Gwang Yoon Hwang 2015-09-30 00:05:54 PDT
Created attachment 262143 [details]
Patch
Comment 19 Gwang Yoon Hwang 2015-09-30 00:22:15 PDT
(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.
Comment 20 Gwang Yoon Hwang 2015-09-30 00:22:54 PDT
(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 :)
Comment 21 Carlos Garcia Campos 2015-09-30 00:59:30 PDT
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 };
Comment 22 Gwang Yoon Hwang 2015-09-30 02:01:32 PDT
Created attachment 262147 [details]
Patch
Comment 23 Gwang Yoon Hwang 2015-09-30 02:03:37 PDT
(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.
Comment 24 WebKit Commit Bot 2015-09-30 02:56:24 PDT
Comment on attachment 262147 [details]
Patch

Clearing flags on attachment: 262147

Committed r190344: <http://trac.webkit.org/changeset/190344>
Comment 25 Michael Catanzaro 2015-09-30 06:06:10 PDT
(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).
Comment 26 Carlos Garcia Campos 2015-09-30 06:18:14 PDT
(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