Bug 76654 – WebGL must allocate smaller drawing buffer if allocation fails

• Status: RESOLVED
• Resolution: FIXED
• Component: WebGL

[Kenneth Russell]

Per http://www.khronos.org/registry/webgl/specs/latest/#2.2 ("The Drawing Buffer"), the WebGL implementation must allocate a drawing buffer with a smaller than requested width and height if the requested size could not be allocated. The fix for Bug 76239 exposes a problem where the reallocation is not being retried.

I recall that zmo implemented this functionality once in Chromium's command buffer port, but I believe it needs to be reimplemented now after the conversion to DrawingBuffer.

[Zhenyao Mo]

Assigned to yongsheng.  Thanks for picking up this bug.  I've been thinking of working on it, but kept being distracted.

[yongsheng]

(In reply to comment #1)
> Assigned to yongsheng.  Thanks for picking up this bug.  I've been thinking of working on it, but kept being distracted.

I posted my thinkings about the policy to allocate smalller buffer than requested:
1) allocate all of the remaining buffer for the request
2) multiple the requested width and heighth with a ratio(such as 0.5) in the loop until the size of requested buffer is met

which one is preferred? or any better idea?

[Zhenyao Mo]

I don't think I fully understand what option 1) stands, but option 2) sounds reasonable.

My original implementation in chromium side is

1) if it's multisampled buffer, try a non-multisampled buffer with the same size
2) if 1) fails, try multisampled buffer with half size
3) if 2) fails, try non-multisampled buffer with half size
4) if 3) fails, goto 2) again

However, I think just doing half the size looping is good enough.

[Kenneth Russell]

Also, the first step should be to retry the allocation at the original size. I believe that we currently try to reallocate the new drawing buffer while holding on to its original resources, which fails and causes the drawing buffer to be released. The allocation should be retried after releasing the drawing buffer. All of this logic should probably be restructured.

[Kenneth Russell]

Yongsheng, do you have an update on this bug? The conformance suite failures in WebKit must be resolved soon. Either we need to push forward a fix for this or roll out bug 76239's patch for now.

[yongsheng]

oh, I'll try to do it today. The late response is because I'm on Chinese New Year.

[yongsheng]

(In reply to comment #3)
> I don't think I fully understand what option 1) stands, but option 2) sounds reasonable.
> My original implementation in chromium side is
> 
> 1) if it's multisampled buffer, try a non-multisampled buffer with the same size
> 2) if 1) fails, try multisampled buffer with half size
> 3) if 2) fails, try non-multisampled buffer with half size
> 4) if 3) fails, goto 2) again
> 
> However, I think just doing half the size looping is good enough.
okay, I'll follow the simple option 2).

[yongsheng]

Created attachment 124099 [details]
Patch

Please review the patch.

[Zhenyao Mo]

Comment on attachment 124099 [details]
Patch

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

> Source/WebCore/platform/graphics/gpu/DrawingBuffer.cpp:248
> +        while ((s_currentResourceUsePixels + pixelDelta) > s_maximumResourceUsePixels) {

I don't think making sure less than s_maximumResourceUsePixels is enough.

You should try creating a buffer with adjustedSize, check if it succeeds, if not, try with half the size.  Do this until you get a valid backbuffer.

[Kenneth Russell]

Comment on attachment 124099 [details]
Patch

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

Thanks for working on this during the Chinese new year, but it needs more work.

> Source/WebCore/ChangeLog:4
> +        the allocation fails. 

Try to keep the synopsis on one line.

> Source/WebCore/platform/graphics/gpu/DrawingBuffer.cpp:52
> +static float s_resourceAdjustedRatio = 0.5;

Should be const, not static.

>> Source/WebCore/platform/graphics/gpu/DrawingBuffer.cpp:248
>> +        while ((s_currentResourceUsePixels + pixelDelta) > s_maximumResourceUsePixels) {
> 
> I don't think making sure less than s_maximumResourceUsePixels is enough.
> 
> You should try creating a buffer with adjustedSize, check if it succeeds, if not, try with half the size.  Do this until you get a valid backbuffer.

I agree with Mo's assessment.

[yongsheng]

(In reply to comment #9)
> (From update of attachment 124099 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=124099&action=review
> 
> > Source/WebCore/platform/graphics/gpu/DrawingBuffer.cpp:248
> > +        while ((s_currentResourceUsePixels + pixelDelta) > s_maximumResourceUsePixels) {
> 
> I don't think making sure less than s_maximumResourceUsePixels is enough.
> 
> You should try creating a buffer with adjustedSize, check if it succeeds, if not, try with half the size.  Do this until you get a valid backbuffer.

ok, since there are two checkings of the framebuffer status in the function, if there is any failure, then try with a smaller size and do all of allocation again util all are okay, am I right?

[yongsheng]

(In reply to comment #10)
> (From update of attachment 124099 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=124099&action=review
> 
> Thanks for working on this during the Chinese new year, but it needs more work.
> 
> > Source/WebCore/ChangeLog:4
> > +        the allocation fails. 
> 
> Try to keep the synopsis on one line.
> 
> > Source/WebCore/platform/graphics/gpu/DrawingBuffer.cpp:52
> > +static float s_resourceAdjustedRatio = 0.5;
> 
> Should be const, not static.
okay, should be 'static const'. 
> >> Source/WebCore/platform/graphics/gpu/DrawingBuffer.cpp:248
> >> +        while ((s_currentResourceUsePixels + pixelDelta) > s_maximumResourceUsePixels) {
> > 
> > I don't think making sure less than s_maximumResourceUsePixels is enough.
> > 
> > You should try creating a buffer with adjustedSize, check if it succeeds, if not, try with half the size.  Do this until you get a valid backbuffer.
> 
> I agree with Mo's assessment.
thanks for your and Mo's comments. I'll refine it.

[yongsheng]

Created attachment 124280 [details]
Patch

[yongsheng]

The 'drawingbuffer-test.html' can be passed. However, there might be a stderr message "Source/WebKit/chromium/third_party/tcmalloc/chromium/src/system-alloc.cc:423] SbrkSysAllocator failed.". Does anyone know this?

[WebKit Review Bot]

Attachment 124280 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/platform/graphics/gpu/DrawingBuffer.cpp:314:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[yongsheng]

Created attachment 124301 [details]
Patch

[Kenneth Russell]

(In reply to comment #14)
> The 'drawingbuffer-test.html' can be passed. However, there might be a stderr message "Source/WebKit/chromium/third_party/tcmalloc/chromium/src/system-alloc.cc:423] SbrkSysAllocator failed.". Does anyone know this?

Is this new behavior that is caused by your patch?

Have you set a breakpoint at that point in the renderer process to see what made the call that failed? I don't think that we should be trying to do huge allocations.

Have you run all of the WebGL conformance tests with this patch? Does it fix all of the intended failures?

The code looks fine overall but you need to remove the associated line from LayoutTests/platform/chromium/test_expectations.txt . r- because of this issue.

[Kenneth Russell]

Note, I just ran the canvas/ subset of the WebGL conformance tests with a debug Chromium build and didn't see any tcmalloc messages printed to stderr.

[Jeff Timanus]

(In reply to comment #18)
> Note, I just ran the canvas/ subset of the WebGL conformance tests with a debug Chromium build and didn't see any tcmalloc messages printed to stderr.

I ran the layout tests fast/canvas/webgl, and drawing-buffer-test.html passed with this change applied.

Output:

twiz@visitor-1950-259h:/usr/local/google/twiz/chrome_src/src/webkit/tools/layout_tests$ ./run_webkit_tests.py fast/canvas/webgl
  fast/canvas/webgl/drawingbuffer-test.html -> unexpected pass
  fast/canvas/webgl/invalid-passed-params.html -> unexpected pass
98 tests ran as expected, 2 didn't:                 


Expected to fail, but passed: (2)
  fast/canvas/webgl/drawingbuffer-test.html
  fast/canvas/webgl/invalid-passed-params.html

[yongsheng]

(In reply to comment #17)
> (In reply to comment #14)
> > The 'drawingbuffer-test.html' can be passed. However, there might be a stderr message "Source/WebKit/chromium/third_party/tcmalloc/chromium/src/system-alloc.cc:423] SbrkSysAllocator failed.". Does anyone know this?
> 
> Is this new behavior that is caused by your patch?
Not very sure. It's a random issue(10%~20%) and only occurs when running the conformance test (that means it never occur when running the single case). Seems not reproducible on your platforms. 
> Have you set a breakpoint at that point in the renderer process to see what made the call that failed? I don't think that we should be trying to do huge allocations.
I'll try to have a look. Any hint to use 
> Have you run all of the WebGL conformance tests with this patch? Does it fix all of the intended failures?
yes, all are okay for me.
> 
> The code looks fine overall but you need to remove the associated line from LayoutTests/platform/chromium/test_expectations.txt . r- because of this issue.
okay, I'll do it.

[yongsheng]

Created attachment 124425 [details]
Patch

[yongsheng]

> twiz@visitor-1950-259h:/usr/local/google/twiz/chrome_src/src/webkit/tools/layout_tests$ ./run_webkit_tests.py fast/canvas/webgl
>   fast/canvas/webgl/drawingbuffer-test.html -> unexpected pass
>   fast/canvas/webgl/invalid-passed-params.html -> unexpected pass
> 98 tests ran as expected, 2 didn't:                 
on my testing environment, the 'invalid-passed-params.html' is 'expected pass'.

[yongsheng]

> > Is this new behavior that is caused by your patch?
> Not very sure. It's a random issue(10%~20%) and only occurs when running the conformance test (that means it never occur when running the single case). Seems not reproducible on your platforms. 

The error is from the GPU side. It resizes its framebuffer with the passed size {width=1, height=1}. It seems the driver allocates a real big buffer(11804672).
I encountered this issue without the patch. but the occurrence frequency is lower than with the patch.

Here is the call stack.
#0  DefaultSysAllocator::Alloc (this=0xb7b911d8, size=11804672, actual_size=0xbfccbacc, alignment=4096)
    at third_party/tcmalloc/chromium/src/system-alloc.cc:423
#1  0xb09991b1 in TCMalloc_SystemAlloc (size=11804672, actual_size=0xbfccbacc, alignment=4096)
    at third_party/tcmalloc/chromium/src/system-alloc.cc:473
#2  0xb0993685 in tcmalloc::PageHeap::GrowHeap (this=0xb7b8c0c0, n=2882)
    at third_party/tcmalloc/chromium/src/page_heap.cc:549
#3  0xb09916c0 in tcmalloc::PageHeap::New (this=0xb7b8c0c0, n=2882) at third_party/tcmalloc/chromium/src/page_heap.cc:102
#4  0xb0980728 in (anonymous namespace)::do_malloc_pages (heap=0xb7f37000, size=11804672)
    at third_party/tcmalloc/chromium/src/tcmalloc.cc:1140
#5  0xb098087c in (anonymous namespace)::do_malloc (size=11804428) at third_party/tcmalloc/chromium/src/tcmalloc.cc:1171
#6  0xb09813f9 in (anonymous namespace)::cpp_alloc (size=11804424, nothrow=true)
    at third_party/tcmalloc/chromium/src/tcmalloc.cc:1453
#7  0xb0980534 in (anonymous namespace)::do_malloc_or_cpp_alloc (size=11804424)
    at third_party/tcmalloc/chromium/src/tcmalloc.cc:1102
#8  0xb09808e9 in (anonymous namespace)::do_calloc (n=1, elem_size=11804424)
    at third_party/tcmalloc/chromium/src/tcmalloc.cc:1183
#9  0xb44908b3 in tc_calloc (n=1, elem_size=11804424) at third_party/tcmalloc/chromium/src/tcmalloc.cc:1602
#10 0xae56a345 in ralloc_size () from /usr/lib/i386-linux-gnu/dri/libglsl.so
#11 0xae56a3ee in rzalloc_size () from /usr/lib/i386-linux-gnu/dri/libglsl.so
#12 0xae56a4c8 in rzalloc_array_size () from /usr/lib/i386-linux-gnu/dri/libglsl.so
#13 0xae90a72c in do_wm_prog () from /usr/lib/i386-linux-gnu/dri/i965_dri.so
#14 0xae90ab48 in ?? () from /usr/lib/i386-linux-gnu/dri/i965_dri.so
#15 0xae9002fd in brw_validate_state () from /usr/lib/i386-linux-gnu/dri/i965_dri.so
#16 0xae8ecec2 in brw_draw_prims () from /usr/lib/i386-linux-gnu/dri/i965_dri.so
#17 0xae7680d1 in ?? () from /usr/lib/i386-linux-gnu/dri/libdricore.so
#18 0xae80129a in _mesa_meta_Clear () from /usr/lib/i386-linux-gnu/dri/libdricore.so
#19 0xae8c8e15 in ?? () from /usr/lib/i386-linux-gnu/dri/i965_dri.so
#20 0xae6996b1 in _mesa_Clear () from /usr/lib/i386-linux-gnu/dri/libdricore.so
#21 0xb3514b2b in gpu::gles2::GLES2DecoderImpl::ResizeOffscreenFrameBuffer (this=0xbd467e00, size=...)   <--- size = {width=1, height =1}
    at gpu/command_buffer/service/gles2_cmd_decoder.cc:2967
#22 0xb351180f in gpu::gles2::GLES2DecoderImpl::Initialize (this=0xbd467e00, surface=..., context=..., size=...,
    disallowed_features=..., allowed_extensions=0xb87a1bcc "*", attribs=...)
    at gpu/command_buffer/service/gles2_cmd_decoder.cc:2155
#23 0xb23a61d0 in GpuCommandBufferStub::OnInitialize (this=0xbd9f0a90, reply_message=0xb87e2d20)
    at content/common/gpu/gpu_command_buffer_stub.cc:234

[Kenneth Russell]

Comment on attachment 124425 [details]
Patch

Thanks, looks good. r=me

[Kenneth Russell]

(In reply to comment #23)
> > > Is this new behavior that is caused by your patch?
> > Not very sure. It's a random issue(10%~20%) and only occurs when running the conformance test (that means it never occur when running the single case). Seems not reproducible on your platforms. 
> 
> The error is from the GPU side. It resizes its framebuffer with the passed size {width=1, height=1}. It seems the driver allocates a real big buffer(11804672).
> I encountered this issue without the patch. but the occurrence frequency is lower than with the patch.
> 
> Here is the call stack.
> #0  DefaultSysAllocator::Alloc (this=0xb7b911d8, size=11804672, actual_size=0xbfccbacc, alignment=4096)
>     at third_party/tcmalloc/chromium/src/system-alloc.cc:423
> #1  0xb09991b1 in TCMalloc_SystemAlloc (size=11804672, actual_size=0xbfccbacc, alignment=4096)
>     at third_party/tcmalloc/chromium/src/system-alloc.cc:473
> #2  0xb0993685 in tcmalloc::PageHeap::GrowHeap (this=0xb7b8c0c0, n=2882)
>     at third_party/tcmalloc/chromium/src/page_heap.cc:549
> #3  0xb09916c0 in tcmalloc::PageHeap::New (this=0xb7b8c0c0, n=2882) at third_party/tcmalloc/chromium/src/page_heap.cc:102
> #4  0xb0980728 in (anonymous namespace)::do_malloc_pages (heap=0xb7f37000, size=11804672)
>     at third_party/tcmalloc/chromium/src/tcmalloc.cc:1140
> #5  0xb098087c in (anonymous namespace)::do_malloc (size=11804428) at third_party/tcmalloc/chromium/src/tcmalloc.cc:1171
> #6  0xb09813f9 in (anonymous namespace)::cpp_alloc (size=11804424, nothrow=true)
>     at third_party/tcmalloc/chromium/src/tcmalloc.cc:1453
> #7  0xb0980534 in (anonymous namespace)::do_malloc_or_cpp_alloc (size=11804424)
>     at third_party/tcmalloc/chromium/src/tcmalloc.cc:1102
> #8  0xb09808e9 in (anonymous namespace)::do_calloc (n=1, elem_size=11804424)
>     at third_party/tcmalloc/chromium/src/tcmalloc.cc:1183
> #9  0xb44908b3 in tc_calloc (n=1, elem_size=11804424) at third_party/tcmalloc/chromium/src/tcmalloc.cc:1602
> #10 0xae56a345 in ralloc_size () from /usr/lib/i386-linux-gnu/dri/libglsl.so
> #11 0xae56a3ee in rzalloc_size () from /usr/lib/i386-linux-gnu/dri/libglsl.so
> #12 0xae56a4c8 in rzalloc_array_size () from /usr/lib/i386-linux-gnu/dri/libglsl.so
> #13 0xae90a72c in do_wm_prog () from /usr/lib/i386-linux-gnu/dri/i965_dri.so
> #14 0xae90ab48 in ?? () from /usr/lib/i386-linux-gnu/dri/i965_dri.so
> #15 0xae9002fd in brw_validate_state () from /usr/lib/i386-linux-gnu/dri/i965_dri.so
> #16 0xae8ecec2 in brw_draw_prims () from /usr/lib/i386-linux-gnu/dri/i965_dri.so
> #17 0xae7680d1 in ?? () from /usr/lib/i386-linux-gnu/dri/libdricore.so
> #18 0xae80129a in _mesa_meta_Clear () from /usr/lib/i386-linux-gnu/dri/libdricore.so
> #19 0xae8c8e15 in ?? () from /usr/lib/i386-linux-gnu/dri/i965_dri.so
> #20 0xae6996b1 in _mesa_Clear () from /usr/lib/i386-linux-gnu/dri/libdricore.so
> #21 0xb3514b2b in gpu::gles2::GLES2DecoderImpl::ResizeOffscreenFrameBuffer (this=0xbd467e00, size=...)   <--- size = {width=1, height =1}
>     at gpu/command_buffer/service/gles2_cmd_decoder.cc:2967
> #22 0xb351180f in gpu::gles2::GLES2DecoderImpl::Initialize (this=0xbd467e00, surface=..., context=..., size=...,
>     disallowed_features=..., allowed_extensions=0xb87a1bcc "*", attribs=...)
>     at gpu/command_buffer/service/gles2_cmd_decoder.cc:2155
> #23 0xb23a61d0 in GpuCommandBufferStub::OnInitialize (this=0xbd9f0a90, reply_message=0xb87e2d20)
>     at content/common/gpu/gpu_command_buffer_stub.cc:234

Well, it doesn't look like your change to DrawingBuffer::reset() is causing this, at least not directly, since DrawingBuffer::reset() only allocates textures and renderbuffers, and the above stack trace is likely occurring during initialization of the WebGLRenderingContext. However, the behavior clearly looks wrong and I would suggest you raise this issue with Intel's driver team, as they may be able to track down the bug in Mesa or wherever it is.

[WebKit Review Bot]

Comment on attachment 124425 [details]
Patch

Clearing flags on attachment: 124425

Committed r106376: <http://trac.webkit.org/changeset/106376>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Jeff Timanus]

*** Bug 76562 has been marked as a duplicate of this bug. ***

[Kenneth Russell]

http://trac.webkit.org/changeset/106376 broke Chromium's webkit_unit_tests and is being rolled out under https://bugs.webkit.org/show_bug.cgi?id=77481 . Reopening this bug.

[Kenneth Russell]

Yongsheng, please build and test Chromium's webkit_unit_tests target with your original patch. Either the patch or the unit test needs to be updated.

[yongsheng]

(In reply to comment #30)
> Yongsheng, please build and test Chromium's webkit_unit_tests target with your original patch. Either the patch or the unit test needs to be updated.
The reason is that when the newSize is empty, and adjustedSize will also be empty, so the code treat it as a failure. However, it should not.
So the fix is to call 'clear()' when the newSize is not empty and adjustedSize is empty.

Tested it with webkit_unit_tests, gpu_tests, layout tests.

[yongsheng]

Created attachment 124892 [details]
Patch

thanks to the unit tests to find this issue.

[yongsheng]

Created attachment 124952 [details]
Patch

reupload it. Please review

[yongsheng]

any comments?

[Jeff Timanus]

Comment on attachment 124952 [details]
Patch

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

> Source/WebCore/platform/graphics/gpu/DrawingBuffer.cpp:324
> +        if (!newSize.isEmpty() && adjustedSize.isEmpty()) {

This change looks ok to me.  Would it have been possible to early-out if newSize is empty?  The need to dynamically shrink by 1/2 and test allocation does not seem necessary when we are starting at 0.

[yongsheng]

(In reply to comment #35)
> (From update of attachment 124952 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=124952&action=review
> 
> > Source/WebCore/platform/graphics/gpu/DrawingBuffer.cpp:324
> > +        if (!newSize.isEmpty() && adjustedSize.isEmpty()) {
> 
> This change looks ok to me.  Would it have been possible to early-out if newSize is empty?  The need to dynamically shrink by 1/2 and test allocation does not seem necessary when we are starting at 0.

I just want to follow up the previous implementation. If the newSize is empty, the old code would also do allocation. In new impl, I want to do that part of code too and then jump out the loop. Make sense?

[Kenneth Russell]

Comment on attachment 124952 [details]
Patch

Looks fine as long as it passes the test. r=me again

[WebKit Review Bot]

Comment on attachment 124952 [details]
Patch

Clearing flags on attachment: 124952

Committed r106888: <http://trac.webkit.org/changeset/106888>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.