1012551 - [tarako] picture preview in edit mode sometimes appear abnormal after unlock screen

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Peipei Cheng (needinfo if you need my action)]

Steps to reproduce
---------------------------------------------------------------------------
1. Launch Gallery
2. Select a picture and preview
3. Enter Edit mode
4. Lock screen --> wait for several seconds --> unlock screen
5. repeat 4 for several times
   --> You will see the picture preview in edit mode show black. Sometimes it show the image of "lockscreen", sometimes it shows the "Lock icon", but after a while it shows a black image.

Please see the STR video here: https://www.dropbox.com/s/6qaj5rqolsdeu8m/IMG_0870.MOV

Test build
--------------------------------------------------------------------
Gaia      917174ee3812a43758bf43f7ba5f9416dcdb2ca8
Gecko     https://hg.mozilla.org/releases/mozilla-b2g28_v1_3t/rev/7296746ab908
BuildID   20140518164003
Version   28.1
ro.build.version.incremental=eng.cltbld.20140518.200243
ro.build.date=Sun May 18 20:02:50 EDT 2014

Comment 1

[Peter Chang[:pchang]]

jerry, please help to check the black screen during gallery edit mode.

Comment 2

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

I can reproduce this issue as comment 0.
checking...

Comment 3

[James Zhang (Spreadtrum)]

1.3t+ please.

Comment 4

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

triage: 1.3T+ for potential graphics issue

Comment 5

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

triage: put this back to 1.3T? for now and wait for more input from graphics team

Comment 6

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

(In reply to Joe Cheng [:jcheng] from comment #5)
> triage: put this back to 1.3T? for now and wait for more input from graphics
> team

It is the same problem as Bug 999264, and it is also related to Bug 994590.
I will update some comments at Bug 999264.

Joe, you might need to find someone to check the Bug 999264.
It is maybe related to GPU driver.

Comment 7

[Hema Koka [:hema]]

Moving to graphics based on comment 6

Comment 8

[James Zhang (Spreadtrum)]

Andrew will fix bug 994590 first, then fix this issue.

Comment 9

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

triage: 1.3T+, [POVB]

Comment 11

[James Zhang (Spreadtrum)]

Bug 994590 patch can't fix this issue.
Jerry, please take a look.

Comment 12

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

Bug 994590 use a workaround, but we still don't know why we have problem for these gl call.

Please check
https://bugzilla.mozilla.org/show_bug.cgi?id=999264#c5 and 
https://bugzilla.mozilla.org/show_bug.cgi?id=999264#c6

Why we have problem for mTextureTarget?
I check gl error code, the mTextureTarget and mEGLImage, and I don't see bad value and error code.
If we call EGLImageTargetTexture2D() for every frame, we will not see the black preview.
But this does not make sense.

Should we check whether this is a driver issue?

Comment 13

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

Attachment: gallery_workaround.patch

bind eglimage at every frame update.

This is just a workaround, and maybe it has performance penalty.
I suggest to find someone to check the gpu driver's behavior.

Comment 14

[James Zhang (Spreadtrum)]

Andrew, can you help to review Jerry's patch for gallery editor.

Comment 15

[Andrew Wu]

Attachment: ff_gal.txt

Here is the log with empty image in edit mode.

From line 4571, the gallery render the edit mode a few frames. The texture 6 is the texture for the image, but it is deleted in line 4508 and never get anything uploaded later. So the empty image shows because there is not data for it.

Comment 16

[James Zhang (Spreadtrum)]

Jerry, please see Andrew's log.

Comment 17

[James Zhang (Spreadtrum)]

Andrew said the patch is workaround, we should find the root cause why texture be deleted.

Comment 18

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

(In reply to James Zhang from comment #17)
> Andrew said the patch is workaround, we should find the root cause why
> texture be deleted.

Yes, it's just a workaround. I don't think we can land this.
Could you find someone to check gpu driver or add some log in gpu driver?


(In reply to Andrew Wu from comment #15)
> Created attachment 8434087 [details]
> ff_gal.txt
> 
> Here is the log with empty image in edit mode.
> 
> From line 4571, the gallery render the edit mode a few frames. The texture 6
> is the texture for the image, but it is deleted in line 4508 and never get
> anything uploaded later. So the empty image shows because there is not data
> for it.

line 4508
D/libEGL  (   85): glDeleteTextures(1, [6]);
D/libEGL  (   85): glDeleteTextures(1, [6]);
D/libEGL  (   85): eglDestroyImageKHR(0x1, 0x10000006)
D/libEGL  (   85): glDeleteTextures(1, [6]);
D/libEGL  (   85): glDeleteTextures(1, [6]);
D/libEGL  (   85): glDeleteTextures(1, [7]);
D/libEGL  (   85): eglDestroyImageKHR(0x1, 0x10000008)
D/libEGL  (   85): glDeleteTextures(1, [1]);
D/libEGL  (   85): glDeleteTextures(1, [1]);
D/libEGL  (   85): glDeleteTextures(1, [1]);
D/libEGL  (   85): glDeleteTextures(1, [1]);

line 4571
D/libEGL  (   85): eglDestroyImageKHR(0x1, 0x10000003)
D/libEGL  (   85): glActiveTexture(GL_TEXTURE0);
D/libEGL  (   85): glBindTexture(GL_TEXTURE_2D, 2);   <= use texture id 2
D/libEGL  (   85): eglCreateImageKHR(0x1, 0x0, 0x3140, 0x446df984, 0x459ff788) = 0x10000003
D/libEGL  (   85): glEGLImageTargetTexture2DOES(GL_TEXTURE_2D, (GLeglImageOES) 0x10000003);
D/libEGL  (   85): glBindFramebuffer(GL_FRAMEBUFFER_OES, 0);
D/libEGL  (   85): glCheckFramebufferStatus(GL_FRAMEBUFFER_OES);
D/libEGL  (   85): glViewport(0, 0, 320, 480);
D/libEGL  (   85): glBlendFuncSeparate(GL_LINES, GL_ONE_MINUS_SRC_ALPHA, GL_LINES, GL_LINES);
D/libEGL  (   85): glEnable(GL_BLEND);
D/libEGL  (   85): glEnable(GL_SCISSOR_TEST);
D/libEGL  (   85): glScissor(0, 0, 320, 480);
D/libEGL  (   85): glClearColor(0, 0, 0, 0);


Line 4508 delete the texture 6, but we don't use the texture id 6. We use texture id 2.
See: D/libEGL  (   85): glBindTexture(GL_TEXTURE_2D, 2);

And we use eglimage, we don't have to call glteximage2d() to upload image data.
So, I don't know what you talk about.

Comment 19

[Peter Chang[:pchang]]

(In reply to Jerry Shih[:jerry] from comment #18)
> (In reply to James Zhang from comment #17)
> > Andrew said the patch is workaround, we should find the root cause why
> > texture be deleted.
> 
> Yes, it's just a workaround. I don't think we can land this.
> Could you find someone to check gpu driver or add some log in gpu driver?
> 
> 
> (In reply to Andrew Wu from comment #15)
> > Created attachment 8434087 [details]
> > ff_gal.txt
> > 
> > Here is the log with empty image in edit mode.
> > 
> > From line 4571, the gallery render the edit mode a few frames. The texture 6
> > is the texture for the image, but it is deleted in line 4508 and never get
> > anything uploaded later. So the empty image shows because there is not data
> > for it.
> 
> line 4508
> D/libEGL  (   85): glDeleteTextures(1, [6]);
> D/libEGL  (   85): glDeleteTextures(1, [6]);
> D/libEGL  (   85): eglDestroyImageKHR(0x1, 0x10000006)
> D/libEGL  (   85): glDeleteTextures(1, [6]);
> D/libEGL  (   85): glDeleteTextures(1, [6]);
> D/libEGL  (   85): glDeleteTextures(1, [7]);
> D/libEGL  (   85): eglDestroyImageKHR(0x1, 0x10000008)
> D/libEGL  (   85): glDeleteTextures(1, [1]);
> D/libEGL  (   85): glDeleteTextures(1, [1]);
> D/libEGL  (   85): glDeleteTextures(1, [1]);
> D/libEGL  (   85): glDeleteTextures(1, [1]);
> 
> line 4571
> D/libEGL  (   85): eglDestroyImageKHR(0x1, 0x10000003)
> D/libEGL  (   85): glActiveTexture(GL_TEXTURE0);
> D/libEGL  (   85): glBindTexture(GL_TEXTURE_2D, 2);   <= use texture id 2
> D/libEGL  (   85): eglCreateImageKHR(0x1, 0x0, 0x3140, 0x446df984,
> 0x459ff788) = 0x10000003
> D/libEGL  (   85): glEGLImageTargetTexture2DOES(GL_TEXTURE_2D,
> (GLeglImageOES) 0x10000003);
> D/libEGL  (   85): glBindFramebuffer(GL_FRAMEBUFFER_OES, 0);
> D/libEGL  (   85): glCheckFramebufferStatus(GL_FRAMEBUFFER_OES);
> D/libEGL  (   85): glViewport(0, 0, 320, 480);
> D/libEGL  (   85): glBlendFuncSeparate(GL_LINES, GL_ONE_MINUS_SRC_ALPHA,
> GL_LINES, GL_LINES);
> D/libEGL  (   85): glEnable(GL_BLEND);
> D/libEGL  (   85): glEnable(GL_SCISSOR_TEST);
> D/libEGL  (   85): glScissor(0, 0, 320, 480);
> D/libEGL  (   85): glClearColor(0, 0, 0, 0);
> 
> 
> Line 4508 delete the texture 6, but we don't use the texture id 6. We use
> texture id 2.
> See: D/libEGL  (   85): glBindTexture(GL_TEXTURE_2D, 2);
> 
> And we use eglimage, we don't have to call glteximage2d() to upload image
> data.
> So, I don't know what you talk about.

Andrew, I think this issue is easier to reproduce. Could you get more detail information from GPU tool or driver side instead of checking gl log? Because we already waste lots of time for log analysis.

Comment 20

[Andrew Wu]

(In reply to peter chang[:pchang][:peter] from comment #19)
> Andrew, I think this issue is easier to reproduce. Could you get more detail
> information from GPU tool or driver side instead of checking gl log? Because
> we already waste lots of time for log analysis.

Peter, I am checking it with driver code. And always found empty texture object while debugging.
And always found it's B2G's fault to use an empty texture object after delete it.
I have pasted a lot of log to make it clear that it is your fault but you always does not admit it.

Comment 21

[Andrew Wu]

(In reply to Jerry Shih[:jerry] from comment #18)
> Line 4508 delete the texture 6, but we don't use the texture id 6. We use
> texture id 2.
> See: D/libEGL  (   85): glBindTexture(GL_TEXTURE_2D, 2);
> 
> And we use eglimage, we don't have to call glteximage2d() to upload image
> data.
> So, I don't know what you talk about.


In my analysis, the texture 6 is the texture of image about to edit. Texture 2 is the whole background.
You can see the end of the log separated by a few blank lines. Each of them is a single frame. Each frame have 2 glDrawArrays. First draw use texture 2, second draw use texture 6. Texture 6 is an empty texture representing the image about to edit.

Comment 22

[James Zhang (Spreadtrum)]

Please let AE come here and let Andrew show debugging method to you.

Comment 23

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

(In reply to Andrew Wu from comment #21)
> (In reply to Jerry Shih[:jerry] from comment #18)
> > Line 4508 delete the texture 6, but we don't use the texture id 6. We use
> > texture id 2.
> > See: D/libEGL  (   85): glBindTexture(GL_TEXTURE_2D, 2);
> > 
> > And we use eglimage, we don't have to call glteximage2d() to upload image
> > data.
> > So, I don't know what you talk about.
> 
> 
> In my analysis, the texture 6 is the texture of image about to edit. Texture
> 2 is the whole background.
> You can see the end of the log separated by a few blank lines. Each of them
> is a single frame. Each frame have 2 glDrawArrays. First draw use texture 2,
> second draw use texture 6. Texture 6 is an empty texture representing the
> image about to edit.

ok, I will print the image size info with gl texture id to make sure each texture standing for.

Comment 24

[u459114]

Jerry:
<Generate>
2235 D/libEGL  (   85): glGenTextures(1, (GLuint *) 0x453083dc); 
<Delete>
4508 D/libEGL  (   85): glDeleteTextures(1, [6]);
<Bind>
4668 D/libEGL  (   85): glBindTexture(GL_TEXTURE_2D, 6);

Comment 25

[u459114]

Peter,
Not direct relative to this bug. 

In this log, I notice many duplicate gl calls.
  1611 D/libEGL  (   85): glBindTexture(GL_TEXTURE_2D, 4);
  1618 D/libEGL  (   85): glBindTexture(GL_TEXTURE_2D, 4);
  1643 D/libEGL  (   85): glBindTexture(GL_TEXTURE_2D, 4);
  
  1737 D/libEGL  (   85): glDeleteTextures(1, [2]);
  1738 D/libEGL  (   85): glDeleteTextures(1, [2]);
  1740 D/libEGL  (   85): glDeleteTextures(1, [2]);
  1741 D/libEGL  (   85): glDeleteTextures(1, [2]);

  4508 D/libEGL  (   85): glDeleteTextures(1, [6]);
  4509 D/libEGL  (   85): glDeleteTextures(1, [6]);
  4511 D/libEGL  (   85): glDeleteTextures(1, [6]);
  4512 D/libEGL  (   85): glDeleteTextures(1, [6]);

We may do more analysis later

Comment 26

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

Attachment: reset texture id to 0 in delete fuction

reset texture id to 0 in DeleteTextureIfPresent()

Comment 27

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

James, please apply this patch and test again.

Comment 28

[u459114]

Hi Sotaro,
with this patch(attachment 8434839 [details] [diff] [review]), I think you may rollback the change in bug 994590.
> +mImageClientTypeContainer = autoLock.GetImage() ?
> +                                  CompositableType::BUFFER_IMAGE_BUFFERED : 
> +                                  CompositableType::BUFFER_UNKNOWN;

Comment 29

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

Comment on attachment 8434839 [details] [diff] [review]
reset texture id to 0 in delete fuction

Hi Sotaro,
This patch reset the texture id to zero after delete.

Comment 30

[James Zhang (Spreadtrum)]

(In reply to Jerry Shih[:jerry] from comment #27)
> James, please apply this patch and test again.

Verified, please land it!

Comment 31

[Sotaro Ikeda [:sotaro]]

(In reply to Jerry Shih[:jerry] from comment #29)
> Comment on attachment 8434839 [details] [diff] [review]
> reset texture id to 0 in delete fuction
> 
> Hi Sotaro,
> This patch reset the texture id to zero after delete.

Nice to find it!

Comment 32

[James Zhang (Spreadtrum)]

This patch can also fix bug 994590 without Sotaro's patch!
Great job!

Comment 34

[James Zhang (Spreadtrum)]

Please check all delete texture case(keywords is 'fDeleteTextures') and reset data member to 0, thank you.

Comment 35

[Gregor Wagner [:gwagner]]

https://hg.mozilla.org/releases/mozilla-b2g28_v1_3t/rev/ed7f4a5804df

Comment 36

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

Attachment: Reset texture id to zero after delete. r=sotaro. a=1.3t+

Comment 37

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

I think that the v1.3 also needs this fix to prevent the gl texture id error problem.

Ryan, could you please land the attachment 8435493 [details] [diff] [review] to v1.3 branch?

Comment 38

[Ryan VanderMeulen [:RyanVM]]

Nope, doesn't have blocking status or approval. You need to discuss with B2G release management, which I am not.

Comment 39

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

Hi Sotaro,
Could you comment for approval to uplift to 1.3 with attachment 8435493 [details] [diff] [review]?
We still have this problem at 1.3 brahch.

Comment 40

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

Comment on attachment 8435493 [details] [diff] [review]
Reset texture id to zero after delete. r=sotaro. a=1.3t+

[Approval Request Comment]
Bug caused by (feature/regressing bug #):  Unsure
User impact if declined: CompositorOGL maybe will use a deleted texture, and see some black area at screen.
Testing completed (on m-c, etc.): test at v1.3t
Risk to taking this patch (and alternatives if risky): Low risk, just reset the texture id to initial value 0 after delete.
String or IDL/UUID changes made by this patch: none

Comment 41

[bhavana bajaj [:bajaj]]

Comment on attachment 8435493 [details] [diff] [review]
Reset texture id to zero after delete. r=sotaro. a=1.3t+

Given this is needed only on 1.3T and it is already blocking that branch, you can directly land on the 1.3T branch. Check landing procedure here : https://wiki.mozilla.org/Release_Management/B2G_Landing#v1.3T