Bug 78675 – [Qt][WK2] Manage graphics buffers in the web process

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Noam Rosenthal]

Some platforms allow for efficient graphics buffers, like IO buffers on Mac and EGL pixmaps. We should use that when possible instead of SHM, and still use SHM as a fallback.

[Noam Rosenthal]

The plan right now is to contribute this in two patches:
1. Instead of using a ShareableBitmap for each update, use a big ShareableBitmap that's divided to smaller updates, like an atlas.
2. Create a GraphicsSurface class that supports software/hardware rendering and fast (DMA) copying, to be implemented differently for different platforms - first implementation with IOSurfaces for Mac.

[Noam Rosenthal]

Created attachment 136142 [details]
Patch

[Noam Rosenthal]

Created attachment 136241 [details]
Patch

[Noam Rosenthal]

andersca, this adds some #ifdef protected functionality to ShareableBitmap, where it seemed the most natural. If this is unsavory, I can go about it a different way.

[Anders Carlsson]

(In reply to comment #4)
> andersca, this adds some #ifdef protected functionality to ShareableBitmap, where it seemed the most natural. If this is unsavory, I can go about it a different way.

Yeah I don't think ShareableBitmap should be overloaded to handle surfaces as well. I'd rather like to see a separate ShareableSurface class. (In fact, we used to have something like this in WebKit2).

[Noam Rosenthal]

Comment on attachment 136241 [details]
Patch

OK, based on andersca's comments I'll add a ShareableSurface class that can have ShareableBitmap as a fallback-backend.

[Noam Rosenthal]

Created attachment 136398 [details]
Patch

[Kenneth Rohde Christiansen]

Comment on attachment 136398 [details]
Patch

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

> Source/WebCore/ChangeLog:12
> +        TextureMapperGL had to be modified to use GL_UNSIGNED_INT_8_8_8_8_REV

This is for Mac only? or general... could be clarified

> Source/WebCore/platform/graphics/surfaces/GraphicsSurface.cpp:52
> +    int stride = 0;
> +    char* bits = platformLock(rect, stride, lockOptions);

It is not obvious that the stride is being set by the platformLock... why not make it a pointer so you would do &stride ?

> Source/WebCore/platform/graphics/surfaces/GraphicsSurface.h:31
> +#if USE(GRAPHICS_SURFACE)
> +#if OS(DARWIN)

I would add a newline between those two... one is for the whole file.

> Source/WebCore/platform/graphics/surfaces/GraphicsSurface.h:35
> +#endif
> +namespace WebCore {

also here

> Source/WebCore/platform/graphics/surfaces/qt/GraphicsSurfaceQt.cpp:3
> +/*
> + Copyright (C) 2012 Nokia Corporation and/or its subsidiary(-ies)
> +

Are you sure that the split you have between GraphicsSurfaceQt and GraphicsSurfaceMac will be the same for all platforms?

> Source/WebCore/platform/graphics/texmap/TextureMapper.h:67
> +    virtual bool isOpenGLBacked() const { return false; }

isBackedByOpenGL?

> Source/WebCore/platform/graphics/texmap/TextureMapperGL.cpp:390
> +#if OS(DARWIN)
> +#define DEFAULT_TEXTURE_PIXEL_TRANSFER_TYPE GL_UNSIGNED_INT_8_8_8_8_REV
> +#else
> +#define DEFAULT_TEXTURE_PIXEL_TRANSFER_TYPE GL_UNSIGNED_BYTE
> +#endif

I wonder where it makes the most sense to put this

> Source/WebKit2/Shared/ShareableSurface.cpp:83
> +PassRefPtr<ShareableSurface> ShareableSurface::create(const IntSize& size, ShareableBitmap::Flags flags, BackingType preferredBacking)

This backing sounds weird to me, but that might just be me. BackignStore? Backend?

Also when would you prefer the Bitmap backend? And if you want the Bitmap one it needs to be enforced right?

[Kenneth Rohde Christiansen]

Zeno can you have a look at the mac specific code?

[Zeno Albisser]

(In reply to comment #9)
> Zeno can you have a look at the mac specific code?

sure. :)

[Zeno Albisser]

(In reply to comment #10)
> (In reply to comment #9)
> > Zeno can you have a look at the mac specific code?
> 
> sure. :)

The mac specific code looks good to me. Nothing i can nitpick on. ;-)

[Noam Rosenthal]

> > Source/WebCore/ChangeLog:12
> > +        TextureMapperGL had to be modified to use GL_UNSIGNED_INT_8_8_8_8_REV
> 
> This is for Mac only? or general... could be clarified
Yes, for now. Might need to change pixel-formats for other platforms as we add more GraphicsSurface implementations.

> > Source/WebCore/platform/graphics/surfaces/GraphicsSurface.cpp:52
> > +    int stride = 0;
> > +    char* bits = platformLock(rect, stride, lockOptions);
> 
> It is not obvious that the stride is being set by the platformLock... why not make it a pointer so you would do &stride ?
OK

> Are you sure that the split you have between GraphicsSurfaceQt and GraphicsSurfaceMac will be the same for all platforms?

In general, platformLock gives us a pointer (that would be the case on other platforms), and Qt makes it into a GraphicsContext by wrapping that pointer in a QImage. If we find exceptions to that, we can add them as an exceptions

> 
> > Source/WebCore/platform/graphics/texmap/TextureMapper.h:67
> > +    virtual bool isOpenGLBacked() const { return false; }
> 
> isBackedByOpenGL?
> 
> > Source/WebCore/platform/graphics/texmap/TextureMapperGL.cpp:390
> > +#if OS(DARWIN)
> > +#define DEFAULT_TEXTURE_PIXEL_TRANSFER_TYPE GL_UNSIGNED_INT_8_8_8_8_REV
> > +#else
> > +#define DEFAULT_TEXTURE_PIXEL_TRANSFER_TYPE GL_UNSIGNED_BYTE
> > +#endif
> 
> I wonder where it makes the most sense to put this
> Also when would you prefer the Bitmap backend? And if you want the Bitmap one it needs to be enforced right?
It is enforced. See ShareableSurface::create; We skip trying to create a GraphicsSurface if the backing-type is bitmap.

[Noam Rosenthal]

Created attachment 136447 [details]
Patch

[Kenneth Rohde Christiansen]

Comment on attachment 136447 [details]
Patch

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

> Source/WebKit2/Shared/SurfaceUpdateInfo.h:47
> +    float updateScaleFactor;

I think we can remove update* from these, they are part of SurfaceUpdateInfo. Maybe newScaleFactor makes more sense

[Noam Rosenthal]

Created attachment 136451 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 136451 [details]
Patch for landing

Clearing flags on attachment: 136451

Committed r113773: <http://trac.webkit.org/changeset/113773>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Csaba Osztrogonác]

Reopen, because it broke the Qt Mac build:
../../../../Source/WebCore/platform/graphics/surfaces/GraphicsSurface.h: In constructor ‘WebCore::GraphicsSurface::GraphicsSurface(const WebCore::IntSize&, int)’:
../../../../Source/WebCore/platform/graphics/surfaces/GraphicsSurface.h:95: warning: ‘WebCore::GraphicsSurface::m_size’ will be initialized after
../../../../Source/WebCore/platform/graphics/surfaces/GraphicsSurface.h:85: warning:   ‘int WebCore::GraphicsSurface::m_flags’
../../../../Source/WebCore/platform/graphics/surfaces/GraphicsSurface.cpp:62: warning:   when initialized here
g++ -c -Wall -Wextra -Wreturn-type -fno-strict-aliasing -Wchar-subscripts -Wformat-security -Wreturn-type -Wno-unused-parameter -Wno-sign-compare -Wno-switch -Wno-switch-enum -Wundef -Wmissing-noreturn -Winit-self -pipe -O2 -arch x86_64 -Xarch_x86_64 -mmacosx-version-min=10.5 -fvisibility=hidden -fvisibility-inlines-hidden -fPIC -DWTF_USE_GRAPHICS_SURFACE=1 -DWTF_USE_QT4_UNICODE=1 -DENABLE_SVG_FONTS=0 -DENABLE_JAVASCRIPT_DEBUGGER=1 -DENABLE_GAMEPAD=0 -DENABLE_SQL_DATABASE=1 -DENABLE_ICONDATABASE=1 -DENABLE_CHANNEL_MESSAGING=1 -DENABLE_DIRECTORY_UPLOAD=0 -DENABLE_FILE_SYSTEM=0 -DENABLE_QUOTA=0 -DENABLE_DASHBOARD_SUPPORT=0 -DENABLE_FILTERS=1 -DENABLE_CSS_FILTERS=1 -DENABLE_SHARED_WORKERS=1 -DENABLE_SHADOW_DOM=0 -DENABLE_WORKERS=1 -DENABLE_DETAILS=1 -DENABLE_METER_TAG=1 -DENABLE_MHTML=0 -DENABLE_MICRODATA=0 -DENABLE_PROGRESS_TAG=1 -DENABLE_BLOB=1 -DENABLE_LEGACY_NOTIFICATIONS=1 -DENABLE_NOTIFICATIONS=1 -DENABLE_INPUT_TYPE_COLOR=0 -DENABLE_INPUT_SPEECH=0 -DENABLE_SCRIPTED_SPEECH=0 -DENABLE_INSPECTOR=1 -DENABLE_3D_RENDERING=1 -DENABLE_WEB_AUDIO=0 -DENABLE_MEDIA_SOURCE=0 -DENABLE_MEDIA_STATISTICS=0 -DENABLE_MEDIA_STREAM=0 -DENABLE_VIDEO_TRACK=0 -DENABLE_TOUCH_ICON_LOADING=0 -DENABLE_ANIMATION_API=0 -DENABLE_TOUCH_ADJUSTMENT=1 -DENABLE_FAST_MOBILE_SCROLLING=1 -DENABLE_PAGE_VISIBILITY_API=1 -DWTF_USE_QT_IMAGE_DECODER=1 -DENABLE_FTPDIR=1 -DENABLE_SVG=1 -DENABLE_DATALIST=1 -DWTF_USE_TILED_BACKING_STORE=1 -DENABLE_NETSCAPE_PLUGIN_API=1 -DPLUGIN_ARCHITECTURE_UNSUPPORTED=1 -DHAVE_QSTYLE=1 -DENABLE_WEBGL=1 -DENABLE_WEB_SOCKETS=1 -DENABLE_WEB_TIMING=1 -DENABLE_REQUEST_ANIMATION_FRAME=1 -DENABLE_XSLT=1 -DENABLE_GEOLOCATION=1 -DENABLE_ORIENTATION_EVENTS=1 -DENABLE_DEVICE_ORIENTATION=1 -DENABLE_TOUCH_EVENTS=1 -DENABLE_GESTURE_EVENTS=1 -DENABLE_VIDEO=1 -DWTF_USE_QTKIT=1 -DENABLE_FULLSCREEN_API=0 -DHAVE_QQUICK1=1 -DWTF_USE_TEXTURE_MAPPER=1 -DWTF_USE_TEXTURE_MAPPER_GL=1 -DQT_MAKEDLL -DNSGEOMETRY_TYPES_SAME_AS_CGGEOMETRY_TYPES -DWTF_USE_TEXTURE_MAPPER_GL -DQT_OPENGL_SHIMS=1 -D__ASSERT_MACROS_DEFINE_VERSIONS_WITHOUT_UNDERSCORES=0 -DBUILDING_QT__=1 -DNDEBUG -DBUILDING_WebCore -DBUILDING_WEBKIT -DQT_ASCII_CAST_WARNINGS -DQT_NO_DEBUG -DQT_SQL_LIB -DQT_XMLPATTERNS_LIB -DQT_OPENGL_LIB -DQT_GUI_LIB -DQT_NETWORK_LIB -DQT_CORE_LIB -DQT_SHARED -I/usr/local/Trolltech/Qt-4.8.0/mkspecs/macx-g++ -I../../../../Source/WebCore -I/usr/local/Trolltech/Qt-4.8.0/lib/QtCore.framework/Versions/4/Headers -I/usr/local/Trolltech/Qt-4.8.0/include/QtCore -I/usr/local/Trolltech/Qt-4.8.0/lib/QtNetwork.framework/Versions/4/Headers -I/usr/local/Trolltech/Qt-4.8.0/include/QtNetwork -I/usr/local/Trolltech/Qt-4.8.0/lib/QtGui.framework/Versions/4/Headers -I/usr/local/Trolltech/Qt-4.8.0/include/QtGui -I/usr/local/Trolltech/Qt-4.8.0/lib/QtOpenGL.framework/Versions/4/Headers -I/usr/local/Trolltech/Qt-4.8.0/include/QtOpenGL -I/usr/local/Trolltech/Qt-4.8.0/lib/QtXmlPatterns.framework/Versions/4/Headers -I/usr/local/Trolltech/Qt-4.8.0/include/QtXmlPatterns -I/usr/local/Trolltech/Qt-4.8.0/lib/QtSql.framework/Versions/4/Headers -I/usr/local/Trolltech/Qt-4.8.0/include/QtSql -I/usr/local/Trolltech/Qt-4.8.0/include -I/usr/local/qt-mobility-1.2.0/include/QtSensors -I/usr/local/qt-mobility-1.2.0/include/QtLocation -I. -I../../../../Source/WebCore -I../../../../Source/WebCore/Modules/filesystem -I../../../../Source/WebCore/Modules/geolocation -I../../../../Source/WebCore/Modules/indexeddb -I../../../../Source/WebCore/Modules/webaudio -I../../../../Source/WebCore/Modules/webdatabase -I../../../../Source/WebCore/Modules/websockets -I../../../../Source/WebCore/accessibility -I../../../../Source/WebCore/bindings -I../../../../Source/WebCore/bindings/generic -I../../../../Source/WebCore/bridge -I../../../../Source/WebCore/bridge/qt -I../../../../Source/WebCore/css -I../../../../Source/WebCore/dom -I../../../../Source/WebCore/dom/default -I../../../../Source/WebCore/editing -I../../../../Source/WebCore/fileapi -I../../../../Source/WebCore/history -I../../../../Source/WebCore/html -I../../../../Source/WebCore/html/canvas -I../../../../Source/WebCore/html/parser -I../../../../Source/WebCore/html/shadow -I../../../../Source/WebCore/html/track -I../../../../Source/WebCore/inspector -I../../../../Source/WebCore/loader -I../../../../Source/WebCore/loader/appcache -I../../../../Source/WebCore/loader/archive -I../../../../Source/WebCore/loader/cache -I../../../../Source/WebCore/loader/icon -I../../../../Source/WebCore/mathml -I../../../../Source/WebCore/notifications -I../../../../Source/WebCore/page -I../../../../Source/WebCore/page/animation -I../../../../Source/WebCore/page/qt -I../../../../Source/WebCore/page/scrolling -I../../../../Source/WebCore/platform -I../../../../Source/WebCore/platform/animation -I../../../../Source/WebCore/platform/audio -I../../../../Source/WebCore/platform/graphics -I../../../../Source/WebCore/platform/graphics/filters -I../../../../Source/WebCore/platform/graphics/filters/arm -I../../../../Source/WebCore/platform/graphics/opengl -I../../../../Source/WebCore/platform/graphics/qt -I../../../../Source/WebCore/platform/graphics/surfaces -I../../../../Source/WebCore/platform/graphics/texmap -I../../../../Source/WebCore/platform/graphics/transforms -I../../../../Source/WebCore/platform/image-decoders -I../../../../Source/WebCore/platform/leveldb -I../../../../Source/WebCore/platform/mock -I../../../../Source/WebCore/platform/network -I../../../../Source/WebCore/platform/network/qt -I../../../../Source/WebCore/platform/qt -I../../../../Source/WebCore/platform/sql -I../../../../Source/WebCore/platform/text -I../../../../Source/WebCore/platform/text/transcoder -I../../../../Source/WebCore/plugins -I../../../../Source/WebCore/rendering -I../../../../Source/WebCore/rendering/mathml -I../../../../Source/WebCore/rendering/style -I../../../../Source/WebCore/rendering/svg -I../../../../Source/WebCore/storage -I../../../../Source/WebCore/svg -I../../../../Source/WebCore/svg/animation -I../../../../Source/WebCore/svg/graphics -I../../../../Source/WebCore/svg/graphics/filters -I../../../../Source/WebCore/svg/properties -I../../../../Source/WebCore/testing -I/buildbot/lion-qt-release/lion-qt-intel-release/build/Source/WebCore/websockets -I../../../../Source/WebCore/workers -I../../../../Source/WebCore/xml -I../../../../Source/WebCore/xml/parser -I../../../../Source/ThirdParty -I../../../../Source/WebCore/bridge/jsc -I../../../../Source/WebCore/bindings/js -I/buildbot/lion-qt-release/lion-qt-intel-release/build/Source/WebCore/bindings/js/specialization -I../../../../Source/WebCore/bridge/c -I../../../../Source/WebCore/testing/js -Igenerated -I../../../../Source/WebCore/platform/mac -I../../../../Source/WebCore/platform/graphics/mac -I/usr/local/Trolltech/Qt-4.8.0/src/3rdparty/sqlite/ -I../../../../Source/WTF/wtf/qt/compat -I/buildbot/lion-qt-release/lion-qt-intel-release/build/Source/WebCore/../WebKitLibraries/ -I../../../../Source/WebCore/platform/mac -I../../../../Source/WebCore/platform/graphics/gpu -I../../../../Source/ThirdParty/ANGLE/src -I../../../../Source/ThirdParty/ANGLE/include -I/System/Library/Frameworks/CoreFoundation.framework/Versions/A/Headers -I../../../../Source -I../include -I../../../../Source/JavaScriptCore -I../../../../Source -I../../../../Source/WTF -I../../../../Source/JavaScriptCore/assembler -I../../../../Source/JavaScriptCore/bytecode -I../../../../Source/JavaScriptCore/bytecompiler -I../../../../Source/JavaScriptCore/heap -I../../../../Source/JavaScriptCore/dfg -I../../../../Source/JavaScriptCore/debugger -I../../../../Source/JavaScriptCore/interpreter -I../../../../Source/JavaScriptCore/jit -I../../../../Source/JavaScriptCore/llint -I../../../../Source/JavaScriptCore/parser -I../../../../Source/JavaScriptCore/profiler -I../../../../Source/JavaScriptCore/runtime -I../../../../Source/JavaScriptCore/tools -I../../../../Source/JavaScriptCore/yarr -I../../../../Source/JavaScriptCore/API -I../../../../Source/JavaScriptCore/ForwardingHeaders -I../JavaScriptCore/generated -I../../../../Source -I../../../../Source/WTF -I/buildbot/lion-qt-release/lion-qt-intel-release/build/Source/WTF/gobject -I/buildbot/lion-qt-release/lion-qt-intel-release/build/Source/WTF/qt -I/buildbot/lion-qt-release/lion-qt-intel-release/build/Source/WTF/unicode -I../../../../Source/WTF/wtf -I/usr/local/qt-mobility-1.2.0/include -I/usr/local/qt-mobility-1.2.0/include/QtMobility -I/System/Library/Frameworks/OpenGL.framework/Versions/A/Headers -I/System/Library/Frameworks/AGL.framework/Headers -I../../../../Source/WebCore -I. -F/usr/local/Trolltech/Qt-4.8.0/lib -o obj/release/PerspectiveTransformOperation.o ../../../../Source/WebCore/platform/graphics/transforms/PerspectiveTransformOperation.cpp
../../../../Source/WebCore/platform/graphics/surfaces/qt/GraphicsSurfaceQt.cpp: In member function ‘WTF::PassOwnPtr<WebCore::GraphicsContext> WebCore::GraphicsSurface::platformBeginPaint(const WebCore::IntSize&, char*, int)’:
../../../../Source/WebCore/platform/graphics/surfaces/qt/GraphicsSurfaceQt.cpp:42: error: no matching function for call to ‘QImage::QImage(uchar*, int, int, int&, QImage::Format&, void ()(void*), WebCore::GraphicsSurface* const)’
/usr/local/Trolltech/Qt-4.8.0/include/QtGui/qimage.h:139: note: candidates are: QImage::QImage(const QImage&)
/usr/local/Trolltech/Qt-4.8.0/include/QtGui/qimage.h:136: note:                 QImage::QImage(const char*, const char*)
/usr/local/Trolltech/Qt-4.8.0/include/QtGui/qimage.h:134: note:                 QImage::QImage(const QString&, const char*)
/usr/local/Trolltech/Qt-4.8.0/include/QtGui/qimage.h:132: note:                 QImage::QImage(const char* const*)
/usr/local/Trolltech/Qt-4.8.0/include/QtGui/qimage.h:129: note:                 QImage::QImage(const uchar*, int, int, int, QImage::Format)
/usr/local/Trolltech/Qt-4.8.0/include/QtGui/qimage.h:128: note:                 QImage::QImage(uchar*, int, int, int, QImage::Format)
/usr/local/Trolltech/Qt-4.8.0/include/QtGui/qimage.h:127: note:                 QImage::QImage(const uchar*, int, int, QImage::Format)
/usr/local/Trolltech/Qt-4.8.0/include/QtGui/qimage.h:126: note:                 QImage::QImage(uchar*, int, int, QImage::Format)
/usr/local/Trolltech/Qt-4.8.0/include/QtGui/qimage.h:125: note:                 QImage::QImage(int, int, QImage::Format)
/usr/local/Trolltech/Qt-4.8.0/include/QtGui/qimage.h:124: note:                 QImage::QImage(const QSize&, QImage::Format)
/usr/local/Trolltech/Qt-4.8.0/include/QtGui/qimage.h:123: note:                 QImage::QImage()
../../../../Source/WebCore/platform/graphics/surfaces/qt/GraphicsSurfaceQt.cpp: In member function ‘WTF::PassRefPtr<WebCore::Image> WebCore::GraphicsSurface::createReadOnlyImage(const WebCore::IntRect&)’:
../../../../Source/WebCore/platform/graphics/surfaces/qt/GraphicsSurfaceQt.cpp:54: error: no matching function for call to ‘QImage::QImage(uchar*, int, int, int&, QImage::Format&, void ()(void*), WebCore::GraphicsSurface* const)’
/usr/local/Trolltech/Qt-4.8.0/include/QtGui/qimage.h:139: note: candidates are: QImage::QImage(const QImage&)
/usr/local/Trolltech/Qt-4.8.0/include/QtGui/qimage.h:136: note:                 QImage::QImage(const char*, const char*)
/usr/local/Trolltech/Qt-4.8.0/include/QtGui/qimage.h:134: note:                 QImage::QImage(const QString&, const char*)
/usr/local/Trolltech/Qt-4.8.0/include/QtGui/qimage.h:132: note:                 QImage::QImage(const char* const*)
/usr/local/Trolltech/Qt-4.8.0/include/QtGui/qimage.h:129: note:                 QImage::QImage(const uchar*, int, int, int, QImage::Format)
/usr/local/Trolltech/Qt-4.8.0/include/QtGui/qimage.h:128: note:                 QImage::QImage(uchar*, int, int, int, QImage::Format)
/usr/local/Trolltech/Qt-4.8.0/include/QtGui/qimage.h:127: note:                 QImage::QImage(const uchar*, int, int, QImage::Format)
/usr/local/Trolltech/Qt-4.8.0/include/QtGui/qimage.h:126: note:                 QImage::QImage(uchar*, int, int, QImage::Format)
/usr/local/Trolltech/Qt-4.8.0/include/QtGui/qimage.h:125: note:                 QImage::QImage(int, int, QImage::Format)
/usr/local/Trolltech/Qt-4.8.0/include/QtGui/qimage.h:124: note:                 QImage::QImage(const QSize&, QImage::Format)
/usr/local/Trolltech/Qt-4.8.0/include/QtGui/qimage.h:123: note:                 QImage::QImage()

[Noam Rosenthal]

Created attachment 136660 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 136660 [details]
Patch for landing

Clearing flags on attachment: 136660

Committed r113853: <http://trac.webkit.org/changeset/113853>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.