Closed Bug 1012551 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: Graphics, defect)

Other
Other
defect
Not set

Tracking

()

RESOLVED FIXED
blocking-b2g 1.3T+
Tracking Status
b2g-v1.3 --- affected
b2g-v1.3T --- fixed
b2g-v1.4 --- unaffected
b2g-v2.0 --- unaffected

People

(Reporter: angelc04, Assigned: jerry)

References

Details

(Whiteboard: [sprd311871][partner-blocker])

Attachments

(3 files, 1 obsolete file)

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
blocking-b2g: --- → 1.3T?
Flags: needinfo?(pchang)
Whiteboard: [sprd311871][partner-blocker]
jerry, please help to check the black screen during gallery edit mode.
Flags: needinfo?(pchang) → needinfo?(hshih)
I can reproduce this issue as comment 0.
checking...
Assignee: nobody → hshih
Flags: needinfo?(hshih)
1.3t+ please.
Flags: needinfo?(ttsai)
Flags: needinfo?(styang)
Flags: needinfo?(kli)
Flags: needinfo?(kkuo)
triage: 1.3T+ for potential graphics issue
blocking-b2g: 1.3T? → 1.3T+
Flags: needinfo?(ttsai)
Flags: needinfo?(styang)
Flags: needinfo?(kli)
Flags: needinfo?(kkuo)
triage: put this back to 1.3T? for now and wait for more input from graphics team
blocking-b2g: 1.3T+ → 1.3T?
(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.
Assignee: hshih → nobody
Flags: needinfo?(jcheng)
Component: Gaia::Gallery → Graphics
Product: Firefox OS → Core
Moving to graphics based on comment 6
Flags: needinfo?(jcheng) → needinfo?(james.zhang)
Andrew will fix bug 994590 first, then fix this issue.
Assignee: nobody → andrew.wu
Depends on: 994590
Flags: needinfo?(james.zhang)
triage: 1.3T+, [POVB]
blocking-b2g: 1.3T? → 1.3T+
Whiteboard: [sprd311871][partner-blocker] → [sprd311871][partner-blocker][POVB]
Whiteboard: [sprd311871][partner-blocker][POVB] → [sprd311871][partner-blocker]
Flags: needinfo?(sotaro.ikeda.g)
Duplicate of this bug: 999264
Bug 994590 patch can't fix this issue.
Jerry, please take a look.
Flags: needinfo?(hshih)
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?
Flags: needinfo?(hshih)
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.
Flags: needinfo?(james.zhang)
Andrew, can you help to review Jerry's patch for gallery editor.
Flags: needinfo?(james.zhang) → needinfo?(andrew.wu)
Assignee: andrew.wu → hshih
Attached file 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.
Flags: needinfo?(andrew.wu)
Jerry, please see Andrew's log.
Andrew said the patch is workaround, we should find the root cause why texture be deleted.
Flags: needinfo?(hshih)
(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.
Flags: needinfo?(james.zhang)
Flags: needinfo?(hshih)
Flags: needinfo?(andrew.wu)
(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.
(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.
Flags: needinfo?(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.
Please let AE come here and let Andrew show debugging method to you.
Flags: needinfo?(ttsai)
Flags: needinfo?(styang)
Flags: needinfo?(kkuo)
Flags: needinfo?(james.zhang)
(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.
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);
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
reset texture id to 0 in DeleteTextureIfPresent()
Flags: needinfo?(kkuo)
James, please apply this patch and test again.
Flags: needinfo?(ttsai)
Flags: needinfo?(styang)
Flags: needinfo?(sotaro.ikeda.g)
Flags: needinfo?(james.zhang)
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;
Flags: needinfo?(sotaro.ikeda.g)
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.
Attachment #8434839 - Flags: review?(sotaro.ikeda.g)
(In reply to Jerry Shih[:jerry] from comment #27)
> James, please apply this patch and test again.

Verified, please land it!
Flags: needinfo?(james.zhang)
Attachment #8434839 - Flags: review?(sotaro.ikeda.g) → review+
Flags: needinfo?(sotaro.ikeda.g)
(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!
This patch can also fix bug 994590 without Sotaro's patch!
Great job!
Flags: needinfo?(sotaro.ikeda.g)
Flags: needinfo?(pchang)
Flags: needinfo?(hshih)
Flags: needinfo?(andrew.wu)
Duplicate of this bug: 994590
Please check all delete texture case(keywords is 'fDeleteTextures') and reset data member to 0, thank you.
Flags: needinfo?(ttsai)
Flags: needinfo?(kli)
Flags: needinfo?(kkuo)
https://hg.mozilla.org/releases/mozilla-b2g28_v1_3t/rev/ed7f4a5804df
Status: NEW → RESOLVED
Closed: 6 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Flags: needinfo?(pchang)
Flags: needinfo?(ttsai)
Flags: needinfo?(sotaro.ikeda.g)
Flags: needinfo?(kli)
Flags: needinfo?(kkuo)
Flags: needinfo?(hshih)
Flags: needinfo?(andrew.wu)
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?
Flags: needinfo?(ryanvm)
Keywords: checkin-needed
Nope, doesn't have blocking status or approval. You need to discuss with B2G release management, which I am not.
Flags: needinfo?(ryanvm)
Keywords: checkin-needed
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.
Flags: needinfo?(sotaro.ikeda.g)
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
Attachment #8435493 - Flags: approval-mozilla-b2g28?
Flags: needinfo?(sotaro.ikeda.g)
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
Attachment #8435493 - Flags: approval-mozilla-b2g28? → approval-mozilla-b2g28-
Duplicate of this bug: 1020442
You need to log in before you can comment on or make changes to this bug.