Bug 61444

Created attachment 94849 [details] proposed patch

Comment on attachment 94849 [details] proposed patch This is partially duplicating even more code from DrawingBuffer :(

(In reply to comment #2) > (From update of attachment 94849 [details]) > This is partially duplicating even more code from DrawingBuffer :( All this code will go away once we delegate the creation of frame-buffer to SKIA. I have sent a proposal to skia team to make skia GrContext::createPlatformSurface more generic.

Comment on attachment 94849 [details] proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=94849&action=review As I'm sure you know, I'd much prefer that you reuse existing code and refactor it to meet your needs rather than partially reimplementing it. Do you want to try falling back to color+stencil if the packed extension is unavailable? I think that in practice color+stencil is very rare, and it only works in ANGLE because it's allocating a packed buffer behind the scenes, but it might be worth trying. > Source/WebCore/platform/graphics/chromium/LayerTextureUpdaterCanvas.cpp:163 > + if (m_stencilBuffer) { this member variable is now misnamed - it's really a depthStencilBuffer > Source/WebCore/platform/graphics/chromium/LayerTextureUpdaterCanvas.cpp:195 > + Extensions3D* extensions = context()->getExtensions(); > + if (!extensions->supports("GL_OES_packed_depth_stencil")) > + return false; > + extensions->ensureEnabled("GL_OES_packed_depth_stencil"); you should query+cached this once per context, not once per fbo

(In reply to comment #3) > (In reply to comment #2) > > (From update of attachment 94849 [details] [details]) > > This is partially duplicating even more code from DrawingBuffer :( > > All this code will go away once we delegate the creation of frame-buffer to SKIA. I have sent a proposal to skia team to make skia GrContext::createPlatformSurface more generic. Yes but currently you and I are both wasting time maintaining it.

Comment on attachment 94849 [details] proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=94849&action=review Ideally you should try all the following configs: 1. STENCIL_INDEX8 2. DEPTH24_STENCIL8 3. STENCIL_INDEX4 4. STENCIL_INDEX16 5. STENCIL_INDEX 6. DEPTH_STENCIL Since we are not checking all the possible configs, I would rather keep it simple and just try the most common one. SKIA does a pretty good job of iterating through all configs. So once we delegate the creation of frame-buffer to SKIA, we would not need to worry about it. >> Source/WebCore/platform/graphics/chromium/LayerTextureUpdaterCanvas.cpp:163 >> + if (m_stencilBuffer) { > > this member variable is now misnamed - it's really a depthStencilBuffer Done. >> Source/WebCore/platform/graphics/chromium/LayerTextureUpdaterCanvas.cpp:195 >> + extensions->ensureEnabled("GL_OES_packed_depth_stencil"); > > you should query+cached this once per context, not once per fbo Caching (sort of) is already being done by Extensions3DChromium. It does not hit the IPC or command-buffer; just a hash-table look-up.

Created attachment 94869 [details] proposed patch renamed m_stencilBuffer to m_depthStencilBuffer

The commit-queue encountered the following flaky tests while processing attachment 94869 [details]: http/tests/websocket/tests/frame-length-overflow.html bug 61507 (author: abarth@webkit.org) The commit-queue is continuing to process your patch.

Comment on attachment 94869 [details] proposed patch Clearing flags on attachment: 94869 Committed r87365: <http://trac.webkit.org/changeset/87365>

All reviewed patches have been landed. Closing bug.