951812 - [Gallery] Release webgl context when we leave editor mode

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Gallery, defect)

Description

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

This bug comes from bug 939962 comment 96.

If we don't need the webgl context, we can use webgl_context.getExtension('WEBGL_lose_context').loseContext() to remove the context explicitly.

Thus, we can recycle the memory before GC waking up.

Comment 1

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

Sorry... typo
This bug comes from bug 939962 comment 69.

Comment 2

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

Attachment: print_shared_surface_alloc_log.patch

Comment 3

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

Attachment: 0001-add-debug-log.patch

Comment 4

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

Attachment: 0003-use-webgl_lose_context-ext.patch

Comment 5

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

Attachment: gallery-reduce-memory-usage.patch

1. remove unnecessary canvas size assignment.
2. destroy the webgl context explicitly.

Comment 6

[David Flanagan [:djf]]

Comment on attachment 8350213 [details] [diff] [review]
gallery-reduce-memory-usage.patch

Review of attachment 8350213 [details] [diff] [review]:
-----------------------------------------------------------------

r- because this patch sets the crop canvas size to 0.

In general, I'm very excited to land any patch that improves gallery memory usage.  But I'd like to understand better what this patch is doing and why it saves memory before I give final r+.

Does loseContext() really free the context or just simulate losing the context?

Why do you move the code that sets canvas size so that it comes before the getContext() call?

Why are you removing the code that sets the canvas size to 0? My hope was that that code might cause memory to be freed sooner.  I could be completely wrong about that, though.

Basically, if you fix the crop canvas and explain what the patch does and have measured that it saves memory then I'm happy to give r+ to land.

You asked if I thought this needed a unit test.  I can't actually imagine how you would write a unit test for this.  Most of the image editor has no tests, so I don't think we should require one for this patch.  If you can think of a way to test it, that would be great, though.

::: apps/gallery/js/ImageEditor.js
@@ +548,4 @@
>  
>    canvas.width = previewWidth;
>    canvas.height = previewHeight;
> +  var context = canvas.getContext('2d');

Why move this here?  Is it inefficient to change the size of a canvas once the context has already been created?

I'm fine with moving it, but I like to keep the canvas and the context creation code close to each other. So if you move the call to getContext() here, please move the call to createElement() to line 548, too.

@@ -775,4 @@
>        // The processed image is in our offscreen canvas, so we don't need
>        // the WebGL stuff anymore.
>        processor.destroy();
> -      tile.width = tile.height = 0;

Why remove this? Does it cause any memory inefficiency?

I don't know anything about the implementation, but assume that there is a chance that setting the size to 0 will free some memory that would otherwise not be freed until a gc occurs.

If you know more about the implementation and are certain that this code does nothing, then go ahead and remove it.

@@ -780,5 @@
>  
>        // Now get the canvas contents as a file and pass to the callback
>        canvas.toBlob(function(blobData) {
>          // Now that we've got the blob, we don't need the canvas anymore
> -        canvas.width = canvas.height = 0;

If there is any chance that this line will cause memory to be freed before the done() callback is called, then I think we should free it.

Please only delete the line if you are certain that it is cauing problems or does nothing.

@@ +808,5 @@
>    var self = this;
>  
>    var canvas = this.cropCanvas = document.createElement('canvas');
> +  canvas.width = canvas.clientWidth;
> +  canvas.height = canvas.clientHeight;

The canvas has not been inserted into the doucment yet, so clientWidth and clientHeight are 0. So I think this change completely breaks cropping.

@@ +813,1 @@
>    var context = this.cropContext = canvas.getContext('2d');

You could move this line after the width and height are set, if that makes a difference, but you have to put the width and height setting code after the appendChild() call.

@@ -1466,4 @@
>  // Destroy all the stuff we allocated
>  ImageProcessor.prototype.destroy = function() {
>    var gl = this.context;
> -  this.context = null; // Don't retain a reference to it

I'd prefer to leave this line where it is.  The new code you add below should still work.

@@ +1469,5 @@
>    gl.deleteTexture(this.sourceTexture);
>    gl.deleteBuffer(this.rectangleBuffer);
>    gl.viewport(0, 0, 0, 0);
> +
> +  // Destroy webgl context explicitly. Not wait for GC cleaning up.

Are you certain (like have you checked with Benoit Jacob) that this actually destroys the context?  I know nothing about it, but I notice that the spec at http://www.khronos.org/registry/webgl/extensions/WEBGL_lose_context/ says something about "simulate losing the context".  So does it really free resources or just pretend to?

Is a more detailed comment needed here?

@@ +1472,5 @@
> +
> +  // Destroy webgl context explicitly. Not wait for GC cleaning up.
> +  var loseContextExt = gl.getExtension('WEBGL_lose_context');
> +  if (loseContextExt) {
> +    loseContextExt.loseContext();

I did not know that anything like this existed. For bug 939962 I could never duplicate the OOM issue. I would just see a message in logcat saying something like "can't get a context". So I'm hoping that calling loseContext here will at least fix that issue!

@@ +1474,5 @@
> +  var loseContextExt = gl.getExtension('WEBGL_lose_context');
> +  if (loseContextExt) {
> +    loseContextExt.loseContext();
> +  }
> +  this.context = null; // Don't retain a reference to it

Remove this line, since you left it in its original location above.

Comment 7

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Testing Notes : https://etherpad.mozilla.org/gallery-memory-patch-1-4

Summary:
Pro: 
* Seems like it has a very slight change in terms of memory usage for the better ( less memory usage ).  It is slightly noticible.

Con: 
* Markers didn't seem to show in gallery edit.  Not sure what broke it.  His patch shouldn't have broken it.
* Also I think the APZ is broken for Gallery, I can zoom in and out where I shouldn't.  I'm not sure if it has to do with this patch or not with APZ turned on.

Comment 8

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

(In reply to David Flanagan [:djf] from comment #6)
> Comment on attachment 8350213 [details] [diff] [review]
> gallery-reduce-memory-usage.patch
> 
> Review of attachment 8350213 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r- because this patch sets the crop canvas size to 0.

Sorry, I do not test the crop mode. Sorry....
 
> In general, I'm very excited to land any patch that improves gallery memory
> usage.  But I'd like to understand better what this patch is doing and why
> it saves memory before I give final r+.
> 
> Does loseContext() really free the context or just simulate losing the
> context?

Yes, loseContext() will really free the buffer for webgl context in b2g.
We can recycle the webgl buffer memory for other purpose.
Unfortunately, we don't have such kind of api for 2d canvas. We need to wait GC to recycle the 2d buffer memory.
 
> Why do you move the code that sets canvas size so that it comes before the
> getContext() call?

If we change the canvas size after the context created, we will call SetDimensions() in b2g many times.
Furthermore, for webgl canvas, it will cause buffer alloc for each different dimension change.
So, if we set canvas size before we create context, it just call SetDimensions() once.

> Why are you removing the code that sets the canvas size to 0? My hope was
> that that code might cause memory to be freed sooner.  I could be completely
> wrong about that, though.

I will show some log later.

1. for 3D canvas:
I think we just relase the context via loseContext(). No need to assign the size with zero. Just assign null for object.
Also, it will cause buffer alloc for each assignment, and we have a bug in 3d canvas buffer alloc.
It will create some unnecessary buffer in this situation.
I will create another bug to solve this problem.

2. for 2D canvas:
It will not affect the buffer size in b2g. B2G will change the buffer size when we call drawImage().
Zero assignment will not change the buffer size in this situation.
I don't know when the GC clean object.
In my experiment, I can't see the memory to be freed sooner when we set zero.
I just want to sync the code path as 3d canvas doing. 


> Basically, if you fix the crop canvas and explain what the patch does and
> have measured that it saves memory then I'm happy to give r+ to land.
> 
> You asked if I thought this needed a unit test.  I can't actually imagine
> how you would write a unit test for this.  Most of the image editor has no
> tests, so I don't think we should require one for this patch.  If you can
> think of a way to test it, that would be great, though.
>

Comment 9

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

Here are the log for buffer alloc and SetDimensions().
Test image size: 384*640

orig:
I/Gecko   (29394): bignose WebGLContext::SetDimensions(354,395)
I/Gecko   (29394): bignose gralloc surface:(354,395)
I/Gecko   (29394): bignose gralloc surface:(354,395)
I/Gecko   (29394): bignose CanvasRenderingContext2D::SetDimensions(300,150)
I/Gecko   (29394): bignose CanvasRenderingContext2D::SetDimensions(237,150)
I/Gecko   (29394): bignose CanvasRenderingContext2D::SetDimensions(237,395)  //call SetDimensions() many times
I/Gecko   (29394): bignose gralloc surface:(237,395)
I/Gecko   (29394): bignose gralloc surface:(354,395)
I/Gecko   (29394): bignose CanvasRenderingContext2D::SetDimensions(384,640)  //full image size 2d canvas
I/Gecko   (29394): bignose gralloc surface:(384,640)                         //full image size 2d canvas
I/Gecko   (29394): bignose WebGLContext::SetDimensions(1024,1024)            //the webgl tile canvas
I/Gecko   (29394): bignose gralloc surface:(1024,1024)                       //the webgl tile canvas
I/Gecko   (29394): bignose WebGLContext::SetDimensions(1024,0)               //tile.width=tile.height=0
I/Gecko   (29394): bignose gralloc surface:(1024,1024)             //unnecessary buffer cause by zero assign 
I/Gecko   (29394): bignose gralloc surface:(1024,1)                //unnecessary buffer cause by zero assign
I/Gecko   (29394): bignose WebGLContext::SetDimensions(0,0)
I/Gecko   (29394): bignose CanvasRenderingContext2D::SetDimensions(384,0)
I/Gecko   (29394): bignose gralloc remove surface:(384,640)
I/Gecko   (29394): bignose CanvasRenderingContext2D::SetDimensions(0,0)
I/Gecko   (29394): bignose CanvasRenderingContext2D::SetDimensions(300,150)
I/Gecko   (29394): bignose CanvasRenderingContext2D::SetDimensions(285,150)
I/Gecko   (29394): bignose CanvasRenderingContext2D::SetDimensions(285,285)
I/Gecko   (29394): bignose gralloc remove surface:(237,395)
I/Gecko   (29394): bignose gralloc surface:(285,285)

We can't see the webgl tile buffer to be freed immediately here.


after modify:
I/Gecko   (30956): bignose WebGLContext::SetDimensions(354,395)
I/Gecko   (30956): bignose gralloc surface:(354,395)
I/Gecko   (30956): bignose gralloc surface:(354,395)
I/Gecko   (30956): bignose CanvasRenderingContext2D::SetDimensions(237,395)    //set dim only once
I/Gecko   (30956): bignose gralloc surface:(237,395)
I/Gecko   (30956): bignose gralloc surface:(354,395)
I/Gecko   (30956): bignose CanvasRenderingContext2D::SetDimensions(384,640)
I/Gecko   (30956): bignose gralloc surface:(384,640)
I/Gecko   (30956): bignose WebGLContext::SetDimensions(1024,1024)
I/Gecko   (30956): bignose gralloc surface:(1024,1024)
I/Gecko   (30956): bignose gralloc remove surface:(1024,1024)      //we remove webgl tile buffer here
I/Gecko   (30956): bignose gralloc remove surface:(354,395)  //we also remove the editor preview canvas buffer
I/Gecko   (30956): bignose gralloc remove surface:(354,395)  //editor preview canvas
I/Gecko   (30956): bignose gralloc remove surface:(354,395)  //editor preview canvas
I/Gecko   (30956): bignose CanvasRenderingContext2D::SetDimensions(300,150)  //I think there is another
I/Gecko   (30956): bignose CanvasRenderingContext2D::SetDimensions(285,150)  //setting size after getContext,
I/Gecko   (30956): bignose CanvasRenderingContext2D::SetDimensions(285,285)  //but I can't find the code.
I/Gecko   (30956): bignose gralloc remove surface:(237,395)
I/Gecko   (30956): bignose gralloc surface:(285,285)

comparing with the orig one, it just saves 1024*1024 + 1024*1024*1 memory size.
And it recycles the webgl buffer soon.

Comment 10

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

Attachment: gallery.patch

modify with David's comment

Comment 11

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

(In reply to Naoki Hirata :nhirata (please use needinfo instead of cc) from comment #7)
> Testing Notes : https://etherpad.mozilla.org/gallery-memory-patch-1-4
> 
> Summary:
> Pro: 
> * Seems like it has a very slight change in terms of memory usage for the
> better ( less memory usage ).  It is slightly noticible.
> 
> Con: 
> * Markers didn't seem to show in gallery edit.  Not sure what broke it.  His
> patch shouldn't have broken it.
> * Also I think the APZ is broken for Gallery, I can zoom in and out where I
> shouldn't.  I'm not sure if it has to do with this patch or not with APZ
> turned on.

Sorry, I made a mistake in editor crop mode.
I fixed it in attachment 8350532 [details] [diff] [review]

Comment 12

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

The markers do show now.

Here's my summary of testing the second patch:
Memory usage only seems to be restored after the image is some how destroyed (ie cancel, save, gallery app killed) and even then it's only some.  This still helps especially when saving the image or accepting the cropped image.

Having said that with the patch, it seems like messing with the crop will use additional memory with the patch than without:
ie 
After each step run adb shell b2g-ps |grep gallery
1. Launch Gallery 
2. Photo Edit
3. switch to Crop tool
4. change size
5. switch to brightness tool
6. Changed brightness
7. Switched back to crop
8. Undo Crop
9. Save the image

I expected the memory not to increase.  With the patch the memory increases.

Other issues
brightness slider jumps in landscape on first use:
doesn't happen w/o patch; not sure why...

Comment 13

[David Flanagan [:djf]]

Comment on attachment 8350532 [details] [diff] [review]
gallery.patch

Review of attachment 8350532 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for pointing out the inefficiency of setting width and height (in two distinct steps) after context creation.  The one you could see in your debug log but could not find the code for is probably gallery/js/MetadataParser.js:38.  That code would be triggered to create a new preview image and/or thumbnail for a newly edited and saved image, and that might be what you were seeing. Feel free to fix that one too.

If I ran this patch with an old gecko, it would crash when I cropped an image or when it called loseContext().  But when I updated to m-c, it works fine.  Without the patch, I see errors like these in my logcat on hamachi:

[JavaScript Warning: "Error: WebGL: Can't get a usable WebGL context" {file: "app://gallery.gaiamobile.org/js/ImageEditor.js" line: 1396}]

With the patch, I don't see this error. So even if this patch does not solve the memory usage, it seems like it is very important to land it.  (But don't uplift it unless gecko has been patch to support loseContext() on other branches.)

Comment 14

[David Flanagan [:djf]]

(In reply to Naoki Hirata :nhirata (please use needinfo instead of cc) from comment #12)

> Other issues
> brightness slider jumps in landscape on first use:
> doesn't happen w/o patch; not sure why...

I can reproduce that without the patch, in landscape and in portrait mode.

Go into edit mode, drag the slider way to the left. Click X to exit edit mode then come right back to it. Drag the slider and it will jump way back to the left where you left it.

If this was not happening before, it is because the exitEditMode() function was aborting early with a webgl extension.

I think this is a regression, but not cause by Jerry's patch.

Comment 15

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

Sorry, I am wrong.

When we set the canvas size, the buffer will be released at the same time.
See: 
http://dxr.mozilla.org/mozilla-central/source/content/canvas/src/CanvasRenderingContext2D.cpp#619

The call stack:
#0  mozilla::dom::CanvasRenderingContext2D::Reset (this=0x43ecb800)
    at ../../../../content/canvas/src/CanvasRenderingContext2D.cpp:609
#1  0x40b40862 in mozilla::dom::CanvasRenderingContext2D::ClearTarget (this=0x43ecb800)
    at ../../../../content/canvas/src/CanvasRenderingContext2D.cpp:953
#2  0x40b408de in mozilla::dom::CanvasRenderingContext2D::SetDimensions (this=0x43ecb800, width=2560, height=0)
    at ../../../../content/canvas/src/CanvasRenderingContext2D.cpp:932
#3  0x40b769ae in mozilla::dom::HTMLCanvasElement::UpdateContext (this=0x43f74180, aCx=<optimized out>, 
    aNewContextOptions=$jsval(-nan(0xfff8600000000)))
    at ../../../../../content/html/content/src/HTMLCanvasElement.cpp:802


So, we should reserve the canvas resize here.
For webgl canvas case, there is a bug 953020 to fix the unnecessary buffer problem.
We also can reserve the webgl canvas resize.

Comment 16

[David Flanagan [:djf]]

Jerry, bug 939962 has been resolved, but I think it still makes sense to land your fixes in this bug.

Setting needinfo for you since I notice that you didn't assign this bug to yourself.

Comment 17

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

Attachment: gallery.patch

1. reorder canvas size setting
2. call loseContext() when we leave editor mode

----
The zero size assignment problem will be fix at bug 953020

Comment 18

[David Flanagan [:djf]]

Comment on attachment 8357002 [details] [diff] [review]
gallery.patch

Review of attachment 8357002 [details] [diff] [review]:
-----------------------------------------------------------------

I did not test, but this looks good to me.

Comment 19

[David Flanagan [:djf]]

This bug continues work that started in the koi+ bug 939962. It is an almost trivial patch with measurable memory performance improvements, and I think we should uplift it to 1.3 (or even 1.2).

Comment 20

[Hema Koka [:hema]]

Update from 1/8 triage: Given the performance benefits and a safe patch (based on comment 19), marking this a 1.3+

Comment 21

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

Convert attachment 8357002 [details] [diff] [review] to pull request
https://github.com/mozilla-b2g/gaia/pull/15145


Travis was green: https://travis-ci.org/mozilla-b2g/gaia/builds/16641232
merge to master: https://github.com/mozilla-b2g/gaia/commit/e20ca482b4eb862da2e1052ce6e09bd89e332e7b

Comment 22

[John Ford [:jhford] CET/CEST Berlin Time]

Uplifted e20ca482b4eb862da2e1052ce6e09bd89e332e7b to:
v1.3: 8c45d72f724a75be754745a86001c64a9b5d23c0