953020 - Setting WebGL canvas size with 0 will allocate unnecessary buffer

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Description

[Jerry Shih[:jerry] (UTC+8) (inactive)]

In
https://bugzilla.mozilla.org/show_bug.cgi?id=951812#c9 ,
we will see some unnecessary buffer allocation when we set canvas's width to zero.

We should avoid this to reduce memory usage.

Comment 1

[Jerry Shih[:jerry] (UTC+8) (inactive)]

For normal webgl rendering, we will call Swap() in WebGLContext::PresentScreenBuffer().

#0  mozilla::gl::GLScreenBuffer::Swap (this=0xb2feafc0, size=...) at /home/bignose/mozilla/gecko/hg/mozilla_central/gfx/gl/GLScreenBuffer.cpp:424
#1  mozilla::gl::GLScreenBuffer::PublishFrame (this=0xb2feafc0, size=...) at /home/bignose/mozilla/gecko/hg/mozilla_central/gfx/gl/GLScreenBuffer.cpp:445
#2  mozilla::gl::GLContext::PublishFrame (this=0xb3acf000) at ../../../gfx/gl/GLContext.cpp:1498
#3  mozilla::WebGLContext::PresentScreenBuffer (this=0xb3c228d0) at /home/bignose/mozilla/gecko/hg/mozilla_central/content/canvas/src/WebGLContext.cpp:1186
#4  mozilla::WebGLContextUserData::PreTransactionCallback (data=0xb3361500) at /home/bignose/mozilla/gecko/hg/mozilla_central/content/canvas/src/WebGLContext.cpp:857
#5  mozilla::layers::CanvasLayer::FirePreTransactionCallback (this=0xb47a4190) at ../../../gfx/layers/Layers.h:1834
#6  mozilla::layers::ClientCanvasLayer::RenderLayer (this=0xb47a4190) at /home/bignose/mozilla/gecko/hg/mozilla_central/gfx/layers/client/ClientCanvasLayer.cpp:120
#7  mozilla::layers::ClientContainerLayer::RenderLayer (this=0xb2e6a600) at ../../../gfx/layers/client/ClientContainerLayer.h:81

In gallery case, we set the webgl context size to 0 to reduce memory usage, but we also call the Swap() in WebGLContext::PresentScreenBuffer().
The Swap() function will alloc additional buffer.
It is not good for memory usage.

This is the buffer alloc debug message when we change the context size:
I/Gecko   (29394): bignose WebGLContext::SetDimensions(1024,0)  //set context height to 0
I/Gecko   (29394): bignose gralloc surface:(1024,1024)          //call Swap() in WebGLContext::PresentScreenBuffer() <- unnecessary call
I/Gecko   (29394): bignose gralloc surface:(1024,1)             //resize buffer size

The call stack:
#0  mozilla::gl::GLScreenBuffer::Swap (this=0xb3a68280, size=...) at /home/bignose/mozilla/gecko/hg/mozilla_central/gfx/gl/GLScreenBuffer.cpp:424
#1  mozilla::gl::GLScreenBuffer::PublishFrame (this=0xb3a68280, size=...) at /home/bignose/mozilla/gecko/hg/mozilla_central/gfx/gl/GLScreenBuffer.cpp:445
#2  mozilla::gl::GLContext::PublishFrame (this=0xb19f7c00) at ../../../gfx/gl/GLContext.cpp:1498
#3  mozilla::WebGLContext::PresentScreenBuffer (this=0xb3c23e20) at /home/bignose/mozilla/gecko/hg/mozilla_central/content/canvas/src/WebGLContext.cpp:1186
#4  mozilla::WebGLContext::SetDimensions (this=0xb3c23e20, width=1024, height=1) at /home/bignose/mozilla/gecko/hg/mozilla_central/content/canvas/src/WebGLContext.cpp:397


If the webgl context is not attached in a layer tree, I think the "mShouldPresent" in PresentScreenBuffer() should be false to prevent calling PublishFrame() function.
http://dxr.mozilla.org/mozilla-central/source/content/canvas/src/WebGLContext.cpp#1181

Can we check if the webgl context is not attached in a layer tree?

Comment 2

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Can we just check mNeedsBlit flag to prevent calling mozilla::gl::GLScreenBuffer::Swap() function?
http://dxr.mozilla.org/mozilla-central/source/gfx/gl/GLScreenBuffer.cpp#332

In gallery context resize case, mNeedsBlit is false.

Comment 3

[Jerry Shih[:jerry] (UTC+8) (inactive)]

If we lost gl context, should we still need to resize the buffer size?
http://dxr.mozilla.org/mozilla-central/source/content/canvas/src/WebGLContext.cpp#399

Comment 4

[Benoit Jacob [:bjacob] (mostly away)]

Jeff Gilbert is going to be a better person than me to answer such questions about the compositing of WebGL contexts.

Jeff: the issue at hand here is that the Gallery application would like to be very economical in terms of what buffer (re-)allocations it causes. It tries to reduce memory usage by setting its canvas width and heith to 0 as soon as it can, which should help reducing memory usage, but unfortunately this triggers code paths around WebGLContext::SetDimensions that cause more buffers to be (re-)allocated.

For instance, as Jerry mentions above, WebGLContext::SetDimensions calls PresentScreenBuffer with the old dimensions, which causes a large buffer allocation that seems possibly useless in the present case. Is there a possibility to change this? What is, in fact, the reason to call PresentScreenBuffer there?

Also see other questions in comment 2 and comment 3 --- I believe that you're the right person to ask these too.

Comment 5

[Benoit Jacob [:bjacob] (mostly away)]

Clearing my needinfo request for now, as Jeff should be strictly better than me for this; don't hesitate to flag me again.

Comment 6

[Kelsey Gilbert [:jgilbert]]

(In reply to Jerry Shih[:jerry] from comment #2)
> Can we just check mNeedsBlit flag to prevent calling
> mozilla::gl::GLScreenBuffer::Swap() function?
> http://dxr.mozilla.org/mozilla-central/source/gfx/gl/GLScreenBuffer.cpp#332
> 
> In gallery context resize case, mNeedsBlit is false.

No.

Comment 7

[Kelsey Gilbert [:jgilbert]]

(In reply to Jerry Shih[:jerry] from comment #3)
> If we lost gl context, should we still need to resize the buffer size?
> http://dxr.mozilla.org/mozilla-central/source/content/canvas/src/
> WebGLContext.cpp#399

If the context is lost, then nothing will happen.

Comment 8

[Kelsey Gilbert [:jgilbert]]

I'll take care of this.

Comment 9

[Jerry Shih[:jerry] (UTC+8) (inactive)]

(In reply to Jeff Gilbert [:jgilbert] from comment #7)
> (In reply to Jerry Shih[:jerry] from comment #3)
> > If we lost gl context, should we still need to resize the buffer size?
> > http://dxr.mozilla.org/mozilla-central/source/content/canvas/src/
> > WebGLContext.cpp#399
> 
> If the context is lost, then nothing will happen.

We will still create a new buffer even the context is lost.
http://dxr.mozilla.org/mozilla-central/source/content/canvas/src/WebGLContext.cpp#554
gl=>null
useOpenGL=>true


The call stack:
#0  SetDimensions (height=1, width=1024, this=0xb3b28010) at ../../../../content/canvas/src/WebGLContext.cpp:555
#1  mozilla::WebGLContext::SetDimensions (this=0xb3b28010, width=<optimized out>, height=<optimized out>) at ../../../../content/canvas/src/WebGLContext.cpp:365
#2  0xb5d08f3e in mozilla::dom::HTMLCanvasElement::UpdateContext (this=0xb2afcd30, aCx=<optimized out>, aNewContextOptions=...) at ../../../../../content/html/content/src/HTMLCanvasElement.cpp:802

Comment 10

[Kelsey Gilbert [:jgilbert]]

Sorry, s/will/should/.

Comment 11

[Kelsey Gilbert [:jgilbert]]

We no longer do this.