Bug 141782

Summary:

[GTK] Accelerated compositing should support hidpi

Product:

WebKit

Reporter:

Christopher Bayliss <christopher.j.bayliss>

Component:

WebKitGTK

Assignee:

Gwang Yoon Hwang <yoon>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

berto, cgarcia, christopher.j.bayliss, cmarcelo, commit-queue, fastcat, gustavo, janousek.jiri, kondapallykalyan, luiz, mcatanzaro, mike, mrobinson, noam, realh69, yoon

Priority:

Version:

528+ (Nightly build)

Hardware:

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

Christopher Bayliss

Reported 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

Attachments
Patch (26.62 KB, patch) 2015-09-21 01:40 PDT, Gwang Yoon Hwang	no flags	Details Formatted Diff Diff
Patch (22.57 KB, patch) 2015-09-22 03:02 PDT, Gwang Yoon Hwang	no flags	Details Formatted Diff Diff
Patch (22.54 KB, patch) 2015-09-30 00:05 PDT, Gwang Yoon Hwang	cgarcia: review+ cgarcia: commit-queue-	Details Formatted Diff Diff
Patch (22.37 KB, patch) 2015-09-30 02:01 PDT, Gwang Yoon Hwang	no flags	Details Formatted Diff Diff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.

Christopher Bayliss

Comment 1 2015-02-19 15:31:41 PST

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

Carlos Garcia Campos

Comment 2 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?

Christopher Bayliss

Comment 3 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/

Michael Catanzaro

Comment 4 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?

Jiří Janoušek

Comment 5 2015-05-04 11:00:39 PDT

Which version of Cairo is required for proper HiDPI support?

Matthew Gabeler-Lee

Comment 6 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.

Carlos Garcia Campos

Comment 7 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.

Michael Gratton

Comment 8 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.

Gwang Yoon Hwang

Comment 9 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.

Michael Catanzaro

Comment 10 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?

Gwang Yoon Hwang

Comment 11 2015-09-21 01:40:40 PDT

Created attachment 261640 [details] Patch

Gwang Yoon Hwang

Comment 12 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

Michael Catanzaro

Comment 13 2015-09-21 17:30:05 PDT

Awesome, thanks Yoon!

Gwang Yoon Hwang

Comment 14 2015-09-22 03:02:40 PDT

Created attachment 261733 [details] Patch

WebKit Commit Bot

Comment 15 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

Carlos Garcia Campos

Comment 16 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.

Martin Robinson

Comment 17 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.

Gwang Yoon Hwang

Comment 18 2015-09-30 00:05:54 PDT

Created attachment 262143 [details] Patch

Gwang Yoon Hwang

Comment 19 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.

Gwang Yoon Hwang

Comment 20 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 :)

Carlos Garcia Campos

Comment 21 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 };

Gwang Yoon Hwang

Comment 22 2015-09-30 02:01:32 PDT

Created attachment 262147 [details] Patch

Gwang Yoon Hwang

Comment 23 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.

WebKit Commit Bot

Comment 24 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>

Michael Catanzaro

Comment 25 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).

Carlos Garcia Campos

Comment 26 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

Note You need to log in before you can comment on or make changes to this bug.