Bug 162598 - [GTK] Should check whether GDK can use GL before asking it to
Summary: [GTK] Should check whether GDK can use GL before asking it to
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Gtk (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Gustavo Noronha (kov)
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-09-27 01:43 PDT by Gustavo Noronha (kov)
Modified: 2016-09-27 02:09 PDT (History)
2 users (show)

See Also:


Attachments
Patch (4.72 KB, patch)
2016-09-27 01:49 PDT, Gustavo Noronha (kov)
mcatanzaro: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Gustavo Noronha (kov) 2016-09-27 01:43:10 PDT
[GTK] Should check whether GDK can use GL before asking it to
Comment 1 Gustavo Noronha (kov) 2016-09-27 01:49:09 PDT
Created attachment 289924 [details]
Patch
Comment 2 Michael Catanzaro 2016-09-27 01:56:13 PDT
Comment on attachment 289924 [details]
Patch

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

Thanks. OK with a GTK+ bug report on bugzilla.gnome.org.

> Source/WebKit2/UIProcess/gtk/AcceleratedBackingStoreWayland.cpp:79
> +        g_warning("GDK is not able to create a GL context, falling back to glReadPixels: %s", error->message);

I would tweak this slightly to indicate that glReadPixels is slow:

"falling back to glReadPixels (slow!): %s"

> Source/WebKit2/UIProcess/gtk/AcceleratedBackingStoreWayland.h:46
> +    bool canGdkUseGL();

I think this function should probably be static, since it doesn't need any member variables. Otherwise, it should be const.
Comment 3 Carlos Garcia Campos 2016-09-27 02:01:35 PDT
Comment on attachment 289924 [details]
Patch

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

>> Source/WebKit2/UIProcess/gtk/AcceleratedBackingStoreWayland.h:46
>> +    bool canGdkUseGL();
> 
> I think this function should probably be static, since it doesn't need any member variables. Otherwise, it should be const.

And we avoid including gtk.h in the header.
Comment 4 Carlos Garcia Campos 2016-09-27 02:03:32 PDT
(In reply to comment #2)
> Comment on attachment 289924 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=289924&action=review
> 
> Thanks. OK with a GTK+ bug report on bugzilla.gnome.org.
> 
> > Source/WebKit2/UIProcess/gtk/AcceleratedBackingStoreWayland.cpp:79
> > +        g_warning("GDK is not able to create a GL context, falling back to glReadPixels: %s", error->message);
> 
> I would tweak this slightly to indicate that glReadPixels is slow:
> 
> "falling back to glReadPixels (slow!): %s"

I would not, first because it's not accurate, it's fast in most desktops, but also because there's nothing the user can do, I'm afraid. Or what's the reason why GL can't be used? missing libs the user needs to install? or drivers?

> > Source/WebKit2/UIProcess/gtk/AcceleratedBackingStoreWayland.h:46
> > +    bool canGdkUseGL();
> 
> I think this function should probably be static, since it doesn't need any
> member variables. Otherwise, it should be const.
Comment 5 Gustavo Noronha (kov) 2016-09-27 02:09:05 PDT
Committed r206424: <http://trac.webkit.org/changeset/206424>