Bug 97813 – Expose some GPU Information

• Component: WebGL
• Status: RESOLVED FIXED

[Dean Jackson]

We have a few places where we're doing GPU vendor detection to work around bugs or enable features. I want to have a central place to store this information. Chromium has something like this in its feature_info/FeatureInfo class.

I'd appreciate any advice on how to design this. I was going to add another class, say GPUInfo, that would hang off GraphicsContext3D. But we already have Extensions3D, so it seems weird to have two classes doing feature detection. I suggest renaming Extensions3D (and it's subclasses) to Features3D (or something) and adding new generic methods at the top, for things like isNVidia(), and maybe needsBuiltInFunctionTranslation(). The existing extension-related methods might need some renaming. e.g. supports() could potentially become supportsExtension() - although I don't think that's necessary.

[Radar WebKit Bug Importer]

<rdar://problem/12388705>

[Kenneth Russell]

zmo necessarily added GPUInfo concepts to Chromium's implementation in order to implement the GPU blacklist. He might have some suggestions.

One argument for adding a new GPUInfo class, and leaving Extensions3D alone, is that Extensions3D and subclasses necessarily have port-specific code. If the GPUInfo will just be a simple container for information that can be queried via the OpenGL API (or even the GraphicsContext3D API), then it may make sense to add it as a new concept in the port-independent code.

[Dean Jackson]

(In reply to comment #2)
> zmo necessarily added GPUInfo concepts to Chromium's implementation in order to implement the GPU blacklist. He might have some suggestions.
> 
> One argument for adding a new GPUInfo class, and leaving Extensions3D alone, is that Extensions3D and subclasses necessarily have port-specific code. If the GPUInfo will just be a simple container for information that can be queried via the OpenGL API (or even the GraphicsContext3D API), then it may make sense to add it as a new concept in the port-independent code.

That's what I originally thought, except when I started to fill out the implementation of GPUInfo I noticed I was writing port-specific code :)

For example, I wanted a helper to detect if ANGLE should translate built-in functions. On OS X that depends on the card and the version of the OS :(

[Kenneth Russell]

(In reply to comment #3)
> 
> That's what I originally thought, except when I started to fill out the implementation of GPUInfo I noticed I was writing port-specific code :)
> 
> For example, I wanted a helper to detect if ANGLE should translate built-in functions. On OS X that depends on the card and the version of the OS :(

Darn.

If we just need a couple of additional query methods then maybe the least painful course of action would be to add them to Extensions3D. Renaming that class to something like OptionalFeatures3D doesn't seem to me to add much semantic value, especially given that the class would still be holding all of the extensions' entry points.

[Zhenyao Mo]

I am fine with adding these functions to Extension3D or GraphicsContext3D.  Functions like isNVidia() seems fit GraphicsContext3D better.

I suspect we will end up writing port specific code anyway when we ask questions like needsBuiltInFunctionTranslation().  For chromium, the answer is passed down from browser instead of being decided at webkit level.

[Dean Jackson]

Thanks. I'll take that approach then - adding to GC3D or renaming Extensions3D.

[Dean Jackson]

Created attachment 166109 [details]
Patch

[Simon Hausmann]

Comment on attachment 166109 [details]
Patch

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

> Source/WebCore/platform/graphics/opengl/Extensions3DOpenGLCommon.cpp:71
> +#if PLATFORM(MAC)

I think it would be nice if here (as well as in the other places in this patch) it would be

#if OS(DARWIN) && !PLATFORM(CHROMIUM)

because while Chromium may defer this due to its libGL wrapper I believe the other ports are all running on top of the platform libGL directly and would benefit from this kind of feature detection.

Hm, oh wait, I don't think this file is event used in the Chromium build (in favour of Extensions3DChromium), so it might be even simpler and just OS(DARWIN) :)

[Dean Jackson]

(In reply to comment #8)
> (From update of attachment 166109 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=166109&action=review
> 
> > Source/WebCore/platform/graphics/opengl/Extensions3DOpenGLCommon.cpp:71
> > +#if PLATFORM(MAC)
> 
> I think it would be nice if here (as well as in the other places in this patch) it would be
> 
> #if OS(DARWIN) && !PLATFORM(CHROMIUM)
> 
> because while Chromium may defer this due to its libGL wrapper I believe the other ports are all running on top of the platform libGL directly and would benefit from this kind of feature detection.
> 
> Hm, oh wait, I don't think this file is event used in the Chromium build (in favour of Extensions3DChromium), so it might be even simpler and just OS(DARWIN) :)

I tend to be conservative in enabling/disabling features in other ports. For Chromium, you're right that it detects using another method, which is why the maySupportMultisampling returns true by default.

I'd be slightly happier for other ports themselves to make the change you recommended. Obviously I suggest they follow our lead but, for example, they may decide that it is worth the risk to enable multisampling on all AMD cards on all OS builds.

[Dean Jackson]

Anyone listening want to review this? It's no change in functionality, and I'm very open to suggestions on alternate APIs.

[Kenneth Russell]

Comment on attachment 166109 [details]
Patch

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

Sorry for the delay looking at this. Looks fine overall; a few comments.

> Source/WebCore/platform/graphics/Extensions3D.h:198
> +    virtual bool maySupportMultisampling() = 0;

How about a little documentation for this method? i.e., "If this method returns false then the system definitely does not support multisampling. Otherwise, callers must attempt to construct multisampled renderbuffers and check framebuffer completeness."

> Source/WebCore/platform/graphics/Extensions3D.h:199
> +    virtual bool requiresBuiltInFunctionEmultation() = 0;

Typo: Emultation -> Emulation. Here and throughout.

> Source/WebCore/platform/graphics/opengl/Extensions3DOpenGLCommon.cpp:65
> +        m_isNVidia = true;

Are you sure the capitalization is correct? GL_VENDOR is "NVIDIA Corporation" on my laptop.

> Source/WebCore/platform/graphics/opengl/Extensions3DOpenGLCommon.cpp:66
> +    if (m_vendor.contains("ATI") || m_vendor.contains("AMD"))

You'll need to be very careful if you address the above issue by lowercasing the GL_VENDOR string: watch out for "NVIDIA CorporATIon". Chromium now tokenizes the lowercased GL_VENDOR string and checks for strict equality with these vendors' names.

[Dean Jackson]

(In reply to comment #11)
> > Source/WebCore/platform/graphics/Extensions3D.h:198
> > +    virtual bool maySupportMultisampling() = 0;
> 
> How about a little documentation for this method? i.e., "If this method returns false then the system definitely does not support multisampling. Otherwise, callers must attempt to construct multisampled renderbuffers and check framebuffer completeness."

Good suggestion.

> > Source/WebCore/platform/graphics/Extensions3D.h:199
> > +    virtual bool requiresBuiltInFunctionEmultation() = 0;
> 
> Typo: Emultation -> Emulation. Here and throughout.

Yikes!

> 
> > Source/WebCore/platform/graphics/opengl/Extensions3DOpenGLCommon.cpp:65
> > +        m_isNVidia = true;
> 
> Are you sure the capitalization is correct? GL_VENDOR is "NVIDIA Corporation" on my laptop.

You're correct. Shame on me for not testing on NV.

> > Source/WebCore/platform/graphics/opengl/Extensions3DOpenGLCommon.cpp:66
> > +    if (m_vendor.contains("ATI") || m_vendor.contains("AMD"))
> 
> You'll need to be very careful if you address the above issue by lowercasing the GL_VENDOR string: watch out for "NVIDIA CorporATIon". Chromium now tokenizes the lowercased GL_VENDOR string and checks for strict equality with these vendors' names.

Also a great point. Tim reminded me that Linux also returns the full name (e.g. "Advanced Micro Devices"). I'm not sure what I can do about this without Linux to test on.

[Dean Jackson]

Created attachment 166776 [details]
Patch

[Kenneth Russell]

Comment on attachment 166776 [details]
Patch

Looks good to me.

[Tim Horton]

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

> Source/WebCore/platform/graphics/Extensions3D.h:192
> +    // Some helper methods to detect GPU functionality

Comments end with periods, and I'm not sure this one is actually useful (and also, I wouldn't consider vendors "functionality", though the stuff below them is).

> Source/WebCore/platform/graphics/Extensions3D.h:193
> +    virtual bool isNVidia() = 0;

NVIDIA spells their name with all caps.

> Source/WebCore/platform/graphics/Extensions3D.h:200
> +    // multisampled renderbuffers and check framebuffer completeness.

Completeness?

> Source/WebCore/platform/graphics/opengl/Extensions3DOpenGLCommon.cpp:71
> +    if (vendorComponents.contains("nvidia"))
> +        m_isNVidia = true;
> +    if (vendorComponents.contains("ati") || vendorComponents.contains("amd"))
> +        m_isAMD = true;
> +    if (vendorComponents.contains("intel"))
> +        m_isIntel = true;

This still won't work for some platforms, but, as you say, we can let them work it out, since we don't have their vendor names.

> Source/WebCore/platform/graphics/opengl/Extensions3DOpenGLCommon.h:60
> +    // Some helper methods to detect GPU functionality

Comments end with periods. Also not sure this one is really needed.

> Source/WebCore/platform/graphics/opengl/Extensions3DOpenGLCommon.h:83
> +    bool m_isNVidia;

Allll the capitalization.

[Dean Jackson]

Committed r130235: <http://trac.webkit.org/changeset/130235>

[Viatcheslav Ostapenko]

Comment on attachment 166776 [details]
Patch

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

> Source/WebCore/platform/graphics/opengl/Extensions3DOpenGLCommon.cpp:71
> +    if (vendorComponents.contains("nvidia"))
> +        m_isNVidia = true;
> +    if (vendorComponents.contains("ati") || vendorComponents.contains("amd"))
> +        m_isAMD = true;
> +    if (vendorComponents.contains("intel"))
> +        m_isIntel = true;

Is it possible to have several vendors at the same time? Can both m_isAMD and m_isIntel (or m_isNVIDIA and m_isAMD) be true?
If not, may be create single enum instead of 3 flags?

Also, if multimple vendors are not possible then let's do "else if" chain placing AMD/ATI at the end in order to minimize detection errors.

[Dean Jackson]

(In reply to comment #17)
> 
> Is it possible to have several vendors at the same time? Can both m_isAMD and m_isIntel (or m_isNVIDIA and m_isAMD) be true?
> If not, may be create single enum instead of 3 flags?
> 
> Also, if multimple vendors are not possible then let's do "else if" chain placing AMD/ATI at the end in order to minimize detection errors.

I did it this way in preparation for code that may well have multiple GPUs. Of course, that probably won't be exposed on this object and, as you say, not with two vendors on teh

[Viatcheslav Ostapenko]

(In reply to comment #18)
> (In reply to comment #17)
> > 
> > Is it possible to have several vendors at the same time? Can both m_isAMD and m_isIntel (or m_isNVIDIA and m_isAMD) be true?
> > If not, may be create single enum instead of 3 flags?
> > 
> > Also, if multimple vendors are not possible then let's do "else if" chain placing AMD/ATI at the end in order to minimize detection errors.
> 
> I did it this way in preparation for code that may well have multiple GPUs. Of course, that probably won't be exposed on this object and, as you say, not with two vendors on teh

Is it possible to get context that talks to multiple devices?

[Kenneth Russell]

(In reply to comment #19)
> (In reply to comment #18)
> > (In reply to comment #17)
> > > 
> > > Is it possible to have several vendors at the same time? Can both m_isAMD and m_isIntel (or m_isNVIDIA and m_isAMD) be true?
> > > If not, may be create single enum instead of 3 flags?
> > > 
> > > Also, if multimple vendors are not possible then let's do "else if" chain placing AMD/ATI at the end in order to minimize detection errors.
> > 
> > I did it this way in preparation for code that may well have multiple GPUs. Of course, that probably won't be exposed on this object and, as you say, not with two vendors on teh
> 
> Is it possible to get context that talks to multiple devices?

Not today with the way that dual-GPU support is implemented on any operating system.

[Dean Jackson]

(In reply to comment #20)
> (In reply to comment #19)

> > > I did it this way in preparation for code that may well have multiple GPUs. Of course, that probably won't be exposed on this object and, as you say, not with two vendors on teh
> > 
> > Is it possible to get context that talks to multiple devices?
> 
> Not today with the way that dual-GPU support is implemented on any operating system.

Yeah, sorry. It seems the last bit of my comment got cut off, but that is what I was saying. You've made good suggestions on the coding style. I'll fix it up when I start using the GPU vendor detection in practice.

[Roger Fong]

Extensions3DOpenGLCommon::Extensions3DOpenGLCommon(GraphicsContext3D* context)
    : m_initializedAvailableExtensions(false)
    , m_context(context)
    , m_isNVIDIA(false)
    , m_isAMD(false)
    , m_isIntel(false)
    , 
    , m_requiresBuiltInFunctionEmulation(false)

did you mean, 
m_maySupportMultisampling(false)  ?

[Dean Jackson]

(In reply to comment #22)
> Extensions3DOpenGLCommon::Extensions3DOpenGLCommon(GraphicsContext3D* context)
>     : m_initializedAvailableExtensions(false)
>     , m_context(context)
>     , m_isNVIDIA(false)
>     , m_isAMD(false)
>     , m_isIntel(false)
>     , 
>     , m_requiresBuiltInFunctionEmulation(false)
> 
> did you mean, 
> m_maySupportMultisampling(false)  ?

I probably did :(

But at the same time, I was probably intending to make sure I didn't turn it off for other platforms. I can only make assumptions about Mac.