989847 - Handle webgl context lose/restore event for gallery editor mode

Status: RESOLVED FIXED

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

Description

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

This bug comes from bug 974807.
https://bugzilla.mozilla.org/show_bug.cgi?id=974807#c35

We don't handle the webgl context lost/restore event. When we turn off the device in gallery editor mode, the image preview will lose when we go back.

Comment 1

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

Attachment: handle lose/restore webgl context event in gallery (gaia master)

Comment 2

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

Comment on attachment 8399256 [details] [review]
handle lose/restore webgl context event in gallery (gaia master)

Hi David,
I think this is the final version for this problem.........

--
Use the gecko-master and gaia-master build.

Rewrite context handling.

I also add the debug message for testing.
https://github.com/JerryShih/gaia/commit/460679dfc0538a4eb72013b96c0cd1636d78b9cf

Comment 3

[David Flanagan [:djf]]

I'm pretty sure we need this fix in 1.4 as well as on master, right Jerry?

Comment 4

[Jason Smith [:jsmith]]

QA Wanted - Is this problem present on 1.3?

Comment 5

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

Sorry, I don't know which gecko git branch is 1.4.
With this commit, the webgl context will lose when we turn off the device.
http://hg.mozilla.org/mozilla-central/rev/3d18224c5065

Jason, you can check whether this change merges to gecko 1.4 branch.
If yes, we need this fix.

Comment 6

[David Flanagan [:djf]]

Comment on attachment 8399256 [details] [review]
handle lose/restore webgl context event in gallery (gaia master)

I am finally able to reproduce the original bug and can see that this patch does fix it.

r-, however, because it changes the visual appearance of the image editor.  When I view an image in the editor, the image has black bars to the left and the right that aren't there in other cases.

Also, if I click on crop and then return to the brightness adjustment, the image disappears.

Also, I never actually see the context being lost in the case where we are saving an edited image, so I'm not sure that we need this fix there.  Though I suppose it is probably better to keep it.

Comment 7

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

(In reply to David Flanagan [:djf] from comment #6)
> Comment on attachment 8399256 [details] [review]
> handle lose/restore webgl context event in gallery (gaia master)
> 
> I am finally able to reproduce the original bug and can see that this patch
> does fix it.
> 
> r-, however, because it changes the visual appearance of the image editor. 
> When I view an image in the editor, the image has black bars to the left and
> the right that aren't there in other cases.

sorry, could you take a picture for this?

> 
> Also, if I click on crop and then return to the brightness adjustment, the
> image disappears.

the bug 981005 fix this problem.

> 
> Also, I never actually see the context being lost in the case where we are
> saving an edited image, so I'm not sure that we need this fix there.  Though
> I suppose it is probably better to keep it.

If you enable the debug message, you will see the lost message when you turn off the screen during saving process.

Comment 8

[Jason Smith [:jsmith]]

(In reply to Jerry Shih[:jerry] from comment #5)
> Sorry, I don't know which gecko git branch is 1.4.
> With this commit, the webgl context will lose when we turn off the device.
> http://hg.mozilla.org/mozilla-central/rev/3d18224c5065
> 
> Jason, you can check whether this change merges to gecko 1.4 branch.
> If yes, we need this fix.

Ok - that commit you are referencing is present in 1.4, as it landed on 2/15. So this is a fallout from bug 971728 then.

Comment 9

[Jason Smith [:jsmith]]

Jerry - If this is caused by bug 971728, wouldn't we also need this on 1.3T? That patch landed there to my understanding.

Comment 10

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

(In reply to Jerry Shih[:jerry] from comment #7)
> (In reply to David Flanagan [:djf] from comment #6)
> > r-, however, because it changes the visual appearance of the image editor. 
> > When I view an image in the editor, the image has black bars to the left and
> > the right that aren't there in other cases.
> 
> sorry, could you take a picture for this?
> 

Ohh... I see the difference. The preview image's left and right side have a black rect.

Comment 11

[Gabriele Svelto [:gsvelto]]

(In reply to Jason Smith [:jsmith] from comment #8)
> Ok - that commit you are referencing is present in 1.4, as it landed on
> 2/15. So this is a fallout from bug 971728 then.

When I landed that patch I hadn't noticed that the behavior of a WebGL canvas changed depending on what type of memory-pressure event it received. Before we were passing "heap-minimize" as the event's data, this did not cause the context to be dropped as it was doing so based on a preference which is set to false by default, see the code here:

hg.mozilla.org/mozilla-central/file/7bacc9e903b0/content/canvas/src/WebGLContext.cpp#l86

After bug 971728 the data payload of the memory-pressure event sent to the newly backgrounded app was changed to "low-memory" which causes the WebGLMemoryPressureObserver to unconditionally drop the context. If this is not a desirable result then we can simply change the type of memory pressure event again to prevent this from happening.

Comment 12

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

(In reply to Jerry Shih[:jerry] from comment #10)
> (In reply to Jerry Shih[:jerry] from comment #7)
> > (In reply to David Flanagan [:djf] from comment #6)
> > > r-, however, because it changes the visual appearance of the image editor. 
> > > When I view an image in the editor, the image has black bars to the left and
> > > the right that aren't there in other cases.
> > 
> > sorry, could you take a picture for this?
> > 
> 
> Ohh... I see the difference. The preview image's left and right side have a
> black rect.

Sorry, I try to turn off the alpha to save the memory usage. I don't notice this change.
I add the alpha back and this black area is gone.

Comment 13

[Jason Smith [:jsmith]]

Ok - sounds like we need this on Tarako as well, since this patch landed on 1.3T along with trunk when 1.4 was active.

Comment 15

[Joe Cheng [:jcheng] (please needinfo)]

1.3T+ for regression

Comment 16

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

Attachment: handle lose/restore webgl context event in gallery (1.3T)

rebase to 1.3T

Comment 17

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

I test on these environment:

gecko: 1.3t
commit c6bff249c9aa156cea1c827dc773aa7e6175cc56
Date:   Tue Apr 8 02:51:11 2014 -0700

gaia: 1.3t
commit 643f3e6676cbb89c62708a9f7cbef2edc795a552
Date:   Mon Apr 7 22:44:26 2014 -0400

With/without my 1.3T patch, if we enable lockscreen, the gallery process will disappear when I turn off and turn on the screen.
After unlock from lockscreen, I just go back to homescreen, not gallery.

Comment 18

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

Hi Jason,
This patch just handle the webgl context, not the gallery process lost.
If I disable lockscreen, I can see the gallery editor mode's screen when I turn on the screen.
Is it enough for this bug?

Comment 19

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

Comment on attachment 8399256 [details] [review]
handle lose/restore webgl context event in gallery (gaia master)

For master.
Let the webgl alpha back.

---
If this patch is ok, I will remove the debug message.
The problem of crop mode to brightness adjustment is fixed by bug 981005, and it was landed.

Comment 20

[David Flanagan [:djf]]

Comment on attachment 8399256 [details] [review]
handle lose/restore webgl context event in gallery (gaia master)

Looks good Jerry. Thanks for all your work on this. I thought it was going to be impossible to handle the lost context without completely rewriting everything, and you figured out how to do it.

Don't forget to remove the debug messages and squash your commits.

Comment 21

[Jason Smith [:jsmith]]

(In reply to Jerry Shih[:jerry] from comment #18)
> Hi Jason,
> This patch just handle the webgl context, not the gallery process lost.
> If I disable lockscreen, I can see the gallery editor mode's screen when I
> turn on the screen.
> Is it enough for this bug?

That's fine.

Comment 22

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

Comment on attachment 8402216 [details] [review]
handle lose/restore webgl context event in gallery (1.3T)

For 1.3T.

The only difference between master and 1.3T is "self.needsUpload = true;".

https://github.com/mozilla-b2g/gaia/pull/17207/files#diff-6e48fa715fc315697388e7075d015a41R506

https://github.com/mozilla-b2g/gaia/pull/18001/files#diff-6e48fa715fc315697388e7075d015a41R466

Comment 23

[David Flanagan [:djf]]

Comment on attachment 8402216 [details] [review]
handle lose/restore webgl context event in gallery (1.3T)

The code looks fine.  I haven't figured out how to root my Tarako, so I can't try the patch out myself, so I'm assuming you have tested it carefully.

Comment 25

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

Attachment: 1.3T tarako.mp4

With attachment 8402216 [details] [review], I still get another problem.
The editor preview will suddenly disapper when I turn on screen.
I only see this situation on 1.3T tarako, and I can't 100% reproduce this issue.
This is the video for this issue.

Comment 26

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

Hi David,
I want to land the attachment 8399256 [details] [review] for 1.4 and master first, and then check the 1.3T issue for comment 25.
What should I do for "1.4-approval-needed"?

Comment 27

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

(In reply to Jerry Shih[:jerry] from comment #25)
> Created attachment 8405208 [details]
> 1.3T tarako.mp4
> 
> With attachment 8402216 [details] [review], I still get another problem.
> The editor preview will suddenly disapper when I turn on screen.
> I only see this situation on 1.3T tarako, and I can't 100% reproduce this
> issue.
> This is the video for this issue.

I test the same version gecko(1.3T) and gaia(1.3T) on helix, but I can't see this issue.

Comment 28

[David Flanagan [:djf]]

Jerry,

Bug 993535 is a dupe of this bug and was marked 1.4+, so please just go ahead and uplift your patch.

Comment 29

[David Flanagan [:djf]]

Its the weekend in Taipei already, so I'll go ahead and land the patch to master and uplift to 1.4.

Landed on master: https://github.com/JerryShih/gaia/commit/a9eff96207bf97a65ac3de0a7533277cc2e10b35

Leaving the bug open because the 1.3T patch is still pending

Comment 30

[David Flanagan [:djf]]

This is the correct link to what landed in master: https://github.com/mozilla-b2g/gaia/commit/a55eb67d0659327a8e4b64099fc76a634823fecd

Comment 31

[David Flanagan [:djf]]

Uplifted to v1.4: https://github.com/mozilla-b2g/gaia/commit/41cdaa3e3783a1045329c0841222973e0a791e1a

Again, leaving bug open because of ongoing investigation into a fix for 1.3T

Comment 32

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

Still debug for attachment 8405208 [details] issue.
I dump the client and b2g side layer tree data. I don't get any special finding.
I will try to dump the layer buffer data.

Comment 33

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

Attachment: 256.jpg

test image in attachment 8405208 [details]

Comment 34

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

Attachment: test_black_preview.patch

test black preview issue.
1.dump graphic buffer
2.recreate EGLImage from graphic buffer

Comment 35

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

Attachment: 1.3T black gallery preview issue on tarako

video

Comment 36

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

The black preview issue(attachment 8409432 [details]) is very similar to the bug 994590.
https://bugzilla.mozilla.org/show_bug.cgi?id=994590#c18

If I turn on the screen and see the preview becomes black, I can let the preview back by re-creating a EGLImage from the original graphic buffer.
Furthermore, if I dump the graphic buffer when the preview is black, I can see the raw data is correct.

----
Testing step:
device: tarako
gecko: 1.3T commit 20474756dff9026cf37dfd2daea96df672292391
gaia: 1.3T commit 643f3e6676cbb89c62708a9f7cbef2edc795a552

1. apply the gaia gallery patch attachment 8402216 [details] [review]
2. apply the gecko patch attachment 8409431 [details] [diff] [review]
3. put test image attachment 8409430 [details] to device /sdcard folder
	a.adb push 256.jpg /sdcard
4. launch gallery and enter editor mode for the 256.jpg 
3. turn off the screen
4. wait for 4 second
5. turn on the screen
6. if the gallery is not killed by OOM, you might see the preview suddenly becomes black.
7. dump the raw data in graphic buffer
	a. adb shell "mkdir /sdcard/debug"	#create a folder to save the raw file
	b. adb shell "setprop test.dump_buf 1"	#dump raw file to /sdcard/debug folder
	c. adb pull /sdcard/debug		#pull all raw file back
8. re-create EGLImage
	a. adb shell "setprop test.upload 1"	#re-create a EGLImage from the original graphic buffer
9. after re-creating EGLImage, the preview is back.

Comment 37

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

James, could you check this black screen problem? If I re-create a EGLImage, the preview is back.

Comment 38

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

I think that the gallery editor preview issue(video:attachment 8405208 [details]) is the gecko's issue.
I test this 1.3T gaia patch on other device(helix). The gallery preview works after screen on.
Should we land attachment 8402216 [details] [review] first?

Comment 39

[Jason Smith [:jsmith]]

(In reply to Jerry Shih[:jerry] from comment #38)
> I think that the gallery editor preview issue(video:attachment 8405208 [details]
> [details]) is the gecko's issue.
> I test this 1.3T gaia patch on other device(helix). The gallery preview
> works after screen on.
> Should we land attachment 8402216 [details] [review] first?

Sure.

Comment 40

[James Zhang (Spreadtrum)]

(In reply to Jerry Shih[:jerry] from comment #37)
> James, could you check this black screen problem? If I re-create a EGLImage,
> the preview is back.

Loop Ying.Xu

Maybe no memory issue. GPU driver alloc memory fail when low memory.
Jerry, can you attach log?

Comment 41

[James Zhang (Spreadtrum)]

And please check spreadtrum black screen bug, is it the same issue?
http://bugzilla.spreadtrum.com/bugzilla/show_bug.cgi?id=294938
http://bugzilla.spreadtrum.com/bugzilla/show_bug.cgi?id=298081
http://bugzilla.spreadtrum.com/bugzilla/show_bug.cgi?id=298182

Comment 42

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

Attachment: test_black_preview.patch v2

If I just use the original EGLImage and bind to gl texture again, the preview is also recovered.

---
simplify the texture re-bind step

Comment 43

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

(In reply to James Zhang from comment #40)
> (In reply to Jerry Shih[:jerry] from comment #37)
> > James, could you check this black screen problem? If I re-create a EGLImage,
> > the preview is back.
> 
> Loop Ying.Xu
> 
> Maybe no memory issue. GPU driver alloc memory fail when low memory.
> Jerry, can you attach log?
adb log or dmesg?
I don't find graphic or gpu related message in these log.

I think that it just uses few memory in this case(we use eglImage here).
And I don't get any gl error code in these steps.

Comment 44

[James Zhang (Spreadtrum)]

(In reply to Jerry Shih[:jerry] from comment #37)
> James, could you check this black screen problem? If I re-create a EGLImage,
> the preview is back.

I have filed a spreadtrum internal bug to track it.

Comment 45

[James Zhang (Spreadtrum)]

see http://bugzilla.spreadtrum.com/bugzilla/show_bug.cgi?id=303701
You can use your mozilla email account to register spreadtrum bugzilla and discuss with our GPU driver team.

Comment 46

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

see comment 39, please land the attachment 8402216 [details] [review] for 1.3T

Comment 47

[Ryan VanderMeulen [:RyanVM]]

(In reply to Jerry Shih[:jerry] from comment #46)
> see comment 39, please land the attachment 8402216 [details] [review] for 1.3T

Comment 48

[[:fabrice] Fabrice Desré]

We'll land on 1.3T once it's on master.

Comment 49

[[:fabrice] Fabrice Desré]

1.3T:
https://github.com/mozilla-b2g/gaia/commit/3de37f050000b206c6bf8962e1582d93fde742c4

Comment 50

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

Move the gallery preview problem to bug 999264