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.
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.
Created attachment 136142 [details] Patch
Created attachment 136241 [details] Patch
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.
(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).
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.
Created attachment 136398 [details] Patch
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?
Zeno can you have a look at the mac specific code?
(In reply to comment #9) > Zeno can you have a look at the mac specific code? sure. :)
(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. ;-)
> > 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.
Created attachment 136447 [details] Patch
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
Created attachment 136451 [details] Patch for landing
Comment on attachment 136451 [details] Patch for landing Clearing flags on attachment: 136451 Committed r113773: <http://trac.webkit.org/changeset/113773>
All reviewed patches have been landed. Closing bug.
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()
Created attachment 136660 [details] Patch for landing
Comment on attachment 136660 [details] Patch for landing Clearing flags on attachment: 136660 Committed r113853: <http://trac.webkit.org/changeset/113853>