Bug 86214 - Fix and enable WebGL for WebKit2 on Qt
Summary: Fix and enable WebGL for WebKit2 on Qt
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebGL (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Zeno Albisser
URL:
Keywords:
Depends on: 87725 87738
Blocks: 79169
  Show dependency treegraph
 
Reported: 2012-05-11 07:06 PDT by Zeno Albisser
Modified: 2012-06-02 07:01 PDT (History)
15 users (show)

See Also:


Attachments
WIP patch. (28.27 KB, patch)
2012-05-11 07:55 PDT, Zeno Albisser
webkit-ews: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-01 (114.63 KB, application/zip)
2012-05-11 09:33 PDT, WebKit Review Bot
no flags Details
patch for review. (28.93 KB, patch)
2012-05-30 01:40 PDT, Zeno Albisser
noam: review-
webkit-ews: commit-queue-
Details | Formatted Diff | Diff
patch for review. - fixed issues as commented before. (28.53 KB, patch)
2012-05-30 08:30 PDT, Zeno Albisser
webkit-ews: commit-queue-
Details | Formatted Diff | Diff
patch for review. (29.36 KB, patch)
2012-06-01 13:45 PDT, Zeno Albisser
zeno: review-
Details | Formatted Diff | Diff
patch for review. (29.41 KB, patch)
2012-06-01 13:54 PDT, Zeno Albisser
webkit-ews: commit-queue-
Details | Formatted Diff | Diff
patch for review. (29.43 KB, patch)
2012-06-01 17:17 PDT, Zeno Albisser
noam: review+
noam: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Zeno Albisser 2012-05-11 07:06:04 PDT
We should start with a GraphicsSurface (IOSurface) based implementation for mac.
From there we can then also approach other platforms.
Comment 1 Zeno Albisser 2012-05-11 07:55:54 PDT
Created attachment 141419 [details]
WIP patch.

This is a work-in-progress patch.
It therefore lacks a proper changelog.
Comment 2 Early Warning System Bot 2012-05-11 08:41:58 PDT
Comment on attachment 141419 [details]
WIP patch.

Attachment 141419 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/12675236
Comment 3 WebKit Review Bot 2012-05-11 09:33:17 PDT
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
Comment 4 WebKit Review Bot 2012-05-11 09:33:22 PDT
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
Comment 5 Gyuyoung Kim 2012-05-11 09:44:16 PDT
Comment on attachment 141419 [details]
WIP patch.

Attachment 141419 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/12671404
Comment 6 Alexey Proskuryakov 2012-05-11 10:13:56 PDT
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?
Comment 7 Noam Rosenthal 2012-05-11 10:15:58 PDT
(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.
Comment 8 Alexey Proskuryakov 2012-05-11 10:28:03 PDT
I think that this kind of comment is appropriate for "WIP" patches, too.
Comment 9 Noam Rosenthal 2012-05-11 10:31:41 PDT
(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.
Comment 10 Zeno Albisser 2012-05-29 03:48:49 PDT
(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.
Comment 11 Zeno Albisser 2012-05-30 01:40:00 PDT
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.
Comment 12 Early Warning System Bot 2012-05-30 02:09:07 PDT
Comment on attachment 144752 [details]
patch for review.

Attachment 144752 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/12842823
Comment 13 Early Warning System Bot 2012-05-30 02:19:20 PDT
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
Comment 14 Noam Rosenthal 2012-05-30 07:03:43 PDT
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?
Comment 15 Zeno Albisser 2012-05-30 08:27:12 PDT
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.
Comment 16 Zeno Albisser 2012-05-30 08:30:59 PDT
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.
Comment 17 Early Warning System Bot 2012-05-30 09:54:06 PDT
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
Comment 18 Early Warning System Bot 2012-05-30 10:26:04 PDT
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
Comment 19 Zeno Albisser 2012-06-01 13:45:07 PDT
Created attachment 145373 [details]
patch for review.
Comment 20 Zeno Albisser 2012-06-01 13:49:29 PDT
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)
Comment 21 Zeno Albisser 2012-06-01 13:54:39 PDT
Created attachment 145374 [details]
patch for review.
Comment 22 Early Warning System Bot 2012-06-01 14:02:48 PDT
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
Comment 23 Noam Rosenthal 2012-06-01 14:48:30 PDT
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
Comment 24 Noam Rosenthal 2012-06-01 14:55:17 PDT
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.
Comment 25 Zeno Albisser 2012-06-01 16:55:33 PDT
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.
Comment 26 Noam Rosenthal 2012-06-01 16:57:50 PDT
(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.
Comment 27 Zeno Albisser 2012-06-01 17:17:26 PDT
Created attachment 145409 [details]
patch for review.

includes check for m_hostWindow.
Comment 28 Noam Rosenthal 2012-06-01 17:31:33 PDT
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;
Comment 29 Zeno Albisser 2012-06-02 07:01:40 PDT
Committed r119318: <http://trac.webkit.org/changeset/119318>