Bug 86214 – Fix and enable WebGL for WebKit2 on Qt

• Component: WebGL
• Status: RESOLVED FIXED

[Zeno Albisser]

We should start with a GraphicsSurface (IOSurface) based implementation for mac.
From there we can then also approach other platforms.

[Zeno Albisser]

Created attachment 141419 [details]
WIP patch.

This is a work-in-progress patch.
It therefore lacks a proper changelog.

[Early Warning System Bot]

Comment on attachment 141419 [details]
WIP patch.

Attachment 141419 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/12675236

[WebKit Review Bot]

Comment on attachment 141419 [details]
WIP patch.

Attachment 141419 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12676235

New failing tests:
compositing/reflections/nested-reflection-opacity.html
compositing/reflections/become-simple-composited-reflection.html
compositing/reflections/nested-reflection-size-change.html
compositing/reflections/backface-hidden-reflection.html
compositing/reflections/reflection-on-composited.html
compositing/reflections/reflection-positioning2.html
compositing/reflections/nested-reflection-on-overflow.html
compositing/plugins/invalidate_rect.html
compositing/reflections/empty-reflection-with-mask.html
compositing/reflections/nested-reflection-anchor-point.html
compositing/reflections/nested-reflection.html
compositing/reflections/nested-reflection-mask-change.html
compositing/reflections/nested-reflection-transformed.html
compositing/reflections/nested-reflection-transformed2.html
compositing/reflections/nested-reflection-animated.html
compositing/overflow/overflow-scaled-descendant-overlapping.html
http/tests/inspector/resource-har-pages.html
compositing/overflow/theme-affects-visual-overflow.html
compositing/reflections/reflection-positioning.html
compositing/sibling-positioning.html
compositing/reflections/reflection-ordering.html

[WebKit Review Bot]

Created attachment 141432 [details]
Archive of layout-test-results from ec2-cr-linux-01

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-01  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Gyuyoung Kim]

Comment on attachment 141419 [details]
WIP patch.

Attachment 141419 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/12671404

[Alexey Proskuryakov]

This patch does much more than enabling a feature, re-titled accordingly. Also, it changes cross platform code, so [Qt] prefix on the bug was wrong and misleading. These prefixes exist so that people not interested in a specific port could quickly skip a bug while looking through bug list - that's most definitely not the case here!

This patch changes cross-platform RenderBoxModelObject.cpp, mac/GraphicsSurfaceMac.cpp, WebKit2 cross-platform layer tree code, and needs careful review from multiple core and Mac developers. Changes in cross-platform GraphicsContext3DOpenGLCommon.cpp are under a PLATFORM(QT) guard, but without any explanation of what is Qt specific there.

Is it possible to split the patch into smaller logical chunks?

[Noam Rosenthal]

(In reply to comment #6)
> This patch does much more than enabling a feature, re-titled accordingly. Also, it changes cross platform code, so [Qt] prefix on the bug was wrong and misleading. These prefixes exist so that people not interested in a specific port could quickly skip a bug while looking through bug list - that's most definitely not the case here!
> 
> This patch changes cross-platform RenderBoxModelObject.cpp, mac/GraphicsSurfaceMac.cpp, WebKit2 cross-platform layer tree code, and needs careful review from multiple core and Mac developers. Changes in cross-platform GraphicsContext3DOpenGLCommon.cpp are under a PLATFORM(QT) guard, but without any explanation of what is Qt specific there.
> 
> Is it possible to split the patch into smaller logical chunks?

It's still a WIP patch, like Zeno has mentioned... For now we're just reviewing the general direction.

[Alexey Proskuryakov]

I think that this kind of comment is appropriate for "WIP" patches, too.

[Noam Rosenthal]

(In reply to comment #8)
> I think that this kind of comment is appropriate for "WIP" patches, too.

Fair enough :)

GraphicsSurfaceMac.cpp is enabled only for Qt for now... 
I agree that the RenderBoxModelObject change should be done in a separate patch.

Otherwise, I think this patch is architecturally in the right direction.

[Zeno Albisser]

(In reply to comment #6)
> This patch does much more than enabling a feature, re-titled accordingly. Also, it changes cross platform code, so [Qt] prefix on the bug was wrong and misleading. These prefixes exist so that people not interested in a specific port could quickly skip a bug while looking through bug list - that's most definitely not the case here!
> 
> This patch changes cross-platform RenderBoxModelObject.cpp, mac/GraphicsSurfaceMac.cpp, WebKit2 cross-platform layer tree code, and needs careful review from multiple core and Mac developers. Changes in cross-platform GraphicsContext3DOpenGLCommon.cpp are under a PLATFORM(QT) guard, but without any explanation of what is Qt specific there.
> 
> Is it possible to split the patch into smaller logical chunks?

I'll try to split it up in smaller, more reasonable chunks.

[Zeno Albisser]

Created attachment 144752 [details]
patch for review.

I split the original patch up into several chunks. - I hope that looks more reasonable now.
EWS will most likely fail (if executed), because this patch depends on Bug87738, and the according patch has not been landed yet.

[Early Warning System Bot]

Comment on attachment 144752 [details]
patch for review.

Attachment 144752 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/12842823

[Early Warning System Bot]

Comment on attachment 144752 [details]
patch for review.

Attachment 144752 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/12844809

[Noam Rosenthal]

Comment on attachment 144752 [details]
patch for review.

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

Awesome progress, see comments.

> Source/WebCore/platform/graphics/qt/GraphicsContext3DQt.cpp:115
> +        window->show();

You don't need to show the window. Just call create(). Then you don't have to set its geometry to outside the screen.

> Source/WebCore/platform/graphics/qt/GraphicsContext3DQt.cpp:130
> +            GraphicsSurface::SupportsSoftwareWrite
> +            | GraphicsSurface::SupportsSoftwareRead

Why these two?

> Source/WebCore/platform/graphics/qt/GraphicsContext3DQt.cpp:132
> +            | GraphicsSurface::SupportsCopyFromTexture

Seems like you're not really using CopyToTexture, but rather something like TextureTarget.

> Source/WebCore/platform/graphics/qt/GraphicsContext3DQt.cpp:292
> +    m_graphicsSurface = GraphicsSurface::create(contextSize, m_surfaceFlags);

In which case would it be 0?
Shouldn't we destroy the surface if it's 0?

[Zeno Albisser]

Comment on attachment 144752 [details]
patch for review.

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

>> Source/WebCore/platform/graphics/qt/GraphicsContext3DQt.cpp:115
>> +        window->show();
> 
> You don't need to show the window. Just call create(). Then you don't have to set its geometry to outside the screen.

setGeometry is necessary for me. Otherwise you will have an invalid drawable.
But i can call create() instead of show().

>> Source/WebCore/platform/graphics/qt/GraphicsContext3DQt.cpp:130
>> +            | GraphicsSurface::SupportsSoftwareRead
> 
> Why these two?

These are leftovers from debugging.
GraphicsSurface::SupportsTextureTarget | GraphicsSurface::SupportsSharing should be sufficient i think.

>> Source/WebCore/platform/graphics/qt/GraphicsContext3DQt.cpp:292
>> +    m_graphicsSurface = GraphicsSurface::create(contextSize, m_surfaceFlags);
> 
> In which case would it be 0?
> Shouldn't we destroy the surface if it's 0?

That is certainly a more sane approach. - fixed.

[Zeno Albisser]

Created attachment 144825 [details]
patch for review. - fixed issues as commented before.

EWS will not pass as long as the patches for the blocking bugs are not landed.

[Early Warning System Bot]

Comment on attachment 144825 [details]
patch for review. - fixed issues as commented before.

Attachment 144825 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/12848915

[Early Warning System Bot]

Comment on attachment 144825 [details]
patch for review. - fixed issues as commented before.

Attachment 144825 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/12844923

[Zeno Albisser]

Created attachment 145373 [details]
patch for review.

[Zeno Albisser]

Comment on attachment 145373 [details]
patch for review.

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

> Source/WebCore/platform/graphics/qt/GraphicsContext3DQt.cpp:40
> +#include <QWindow>

This needs an #if HAVE(QT5)

> Source/WebCore/platform/graphics/qt/GraphicsContext3DQt.cpp:112
> +        QWindow* window = new QWindow;

#if HAVE(QT5)

[Zeno Albisser]

Created attachment 145374 [details]
patch for review.

[Early Warning System Bot]

Comment on attachment 145374 [details]
patch for review.

Attachment 145374 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/12862726

[Noam Rosenthal]

Comment on attachment 145374 [details]
patch for review.

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

> Source/WebCore/ChangeLog:20
> +

Redundant line

> Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLCommon.cpp:230
> +

Redundant line

> Source/WebCore/platform/graphics/qt/GraphicsContext3DQt.cpp:112
> +#if HAVE(QT5)

I think this is in the wrong place. the #if statement should be outside the if() statement, and allow for early returns. In general, this code could be more readable.

> Source/WebCore/platform/graphics/qt/GraphicsContext3DQt.cpp:222
> +        return 0;
> +    blitMultisampleFramebufferAndRestoreContext();

Extra line here please

> Source/WebCore/platform/graphics/qt/GraphicsContext3DQt.cpp:230
> +    RefPtr<GraphicsSurface> temp = m_frontBufferGraphicsSurface;
> +    m_frontBufferGraphicsSurface = m_backBufferGraphicsSurface;
> +    m_backBufferGraphicsSurface = temp;

std::swap

> Source/WebCore/platform/graphics/qt/GraphicsContext3DQt.cpp:296
> +void GraphicsContext3DPrivate::reshape(int width, int height)

why don't we explicitly call this createGraphicsSurfaces?

> Source/WebCore/platform/graphics/qt/GraphicsContext3DQt.cpp:483
> +void GraphicsContext3D::reshapeSurface(int width, int height)

createGraphicsSurfaces

[Noam Rosenthal]

Comment on attachment 145374 [details]
patch for review.

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

> Source/WebCore/platform/graphics/qt/GraphicsContext3DQt.cpp:110
>      QWebPageClient* webPageClient = m_hostWindow->platformPageClient();

We should also account for having no m_hostWindow.

[Zeno Albisser]

Comment on attachment 145374 [details]
patch for review.

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

>> Source/WebCore/platform/graphics/qt/GraphicsContext3DQt.cpp:110
>>      QWebPageClient* webPageClient = m_hostWindow->platformPageClient();
> 
> We should also account for having no m_hostWindow.

As far as I understand, it should not be possible not to have a HostWindow.
I will add an assert for that.

>> Source/WebCore/platform/graphics/qt/GraphicsContext3DQt.cpp:112
>> +#if HAVE(QT5)
> 
> I think this is in the wrong place. the #if statement should be outside the if() statement, and allow for early returns. In general, this code could be more readable.

I replaced the HAVE(QT5) with USE(GRAPHICS_SURFACE), because currently we can't make use of one without the other.
I tried to make it a bit more readable as well.

[Noam Rosenthal]

(In reply to comment #25)
> (From update of attachment 145374 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=145374&action=review
> 
> >> Source/WebCore/platform/graphics/qt/GraphicsContext3DQt.cpp:110
> >>      QWebPageClient* webPageClient = m_hostWindow->platformPageClient();
> > 
> > We should also account for having no m_hostWindow.
> 
> As far as I understand, it should not be possible not to have a HostWindow.
> I will add an assert for that.
That's going to change... and in this case we don't need the HostWindow.

[Zeno Albisser]

Created attachment 145409 [details]
patch for review.

includes check for m_hostWindow.

[Noam Rosenthal]

Comment on attachment 145409 [details]
patch for review.

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

OK, this is great! only some minor nitpicks.

> Source/WebCore/platform/graphics/qt/GraphicsContext3DQt.cpp:115
> +            return;

Extra line after return

> Source/WebCore/platform/graphics/qt/GraphicsContext3DQt.cpp:117
> +    }

Extra line after brace. you can do an early return and avoid the else

> Source/WebCore/platform/graphics/qt/GraphicsContext3DQt.cpp:119
> +      else {

bad indentation

> Source/WebCore/platform/graphics/qt/GraphicsContext3DQt.cpp:296
> +        m_frontBufferGraphicsSurface = 0;
> +        m_backBufferGraphicsSurface = 0;

= 0 --> .clear()

> Source/WebKit2/WebProcess/WebCoreSupport/WebGraphicsLayer.cpp:488
> +    if (!m_canvasNeedsDisplay)
> +        return;
> +    if (!m_canvasPlatformLayer)
> +        return;

Extra lines after return;

[Zeno Albisser]

Committed r119318: <http://trac.webkit.org/changeset/119318>