Bug 61444 - [chromium] Cannot create stencil render-buffer for accelerated drawing on desktop GL
Summary: [chromium] Cannot create stencil render-buffer for accelerated drawing on des...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Alok Priyadarshi
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-05-25 09:31 PDT by Alok Priyadarshi
Modified: 2011-05-26 00:01 PDT (History)
5 users (show)

See Also:


Attachments
proposed patch (4.06 KB, patch)
2011-05-25 13:29 PDT, Alok Priyadarshi
jamesr: review-
Details | Formatted Diff | Diff
proposed patch (5.41 KB, patch)
2011-05-25 15:18 PDT, Alok Priyadarshi
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alok Priyadarshi 2011-05-25 09:31:37 PDT
LayerTextureUpdaterCanvas.cpp:137: ASSERT(context()->checkFramebufferStatus(GraphicsContext3D::FRAMEBUFFER) == GraphicsContext3D::FRAMEBUFFER_COMPLETE);

This asserts for every composited page with desktop GL and --enable-accelerated-drawing. It basically fails to create a stencil render buffer.
Comment 1 Alok Priyadarshi 2011-05-25 13:29:00 PDT
Created attachment 94849 [details]
proposed patch
Comment 2 James Robinson 2011-05-25 14:35:16 PDT
Comment on attachment 94849 [details]
proposed patch

This is partially duplicating even more code from DrawingBuffer :(
Comment 3 Alok Priyadarshi 2011-05-25 14:39:00 PDT
(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 4 James Robinson 2011-05-25 14:45:29 PDT
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
Comment 5 James Robinson 2011-05-25 14:49:45 PDT
(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 6 Alok Priyadarshi 2011-05-25 15:15:41 PDT
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.
Comment 7 Alok Priyadarshi 2011-05-25 15:18:14 PDT
Created attachment 94869 [details]
proposed patch

renamed m_stencilBuffer to m_depthStencilBuffer
Comment 8 WebKit Commit Bot 2011-05-25 23:59:59 PDT
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 9 WebKit Commit Bot 2011-05-26 00:01:19 PDT
Comment on attachment 94869 [details]
proposed patch

Clearing flags on attachment: 94869

Committed r87365: <http://trac.webkit.org/changeset/87365>
Comment 10 WebKit Commit Bot 2011-05-26 00:01:24 PDT
All reviewed patches have been landed.  Closing bug.