[Otoro] Lock screen unlock handle flashes with graphics defect

RESOLVED FIXED

Status

Firefox OS
General
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: timdream, Assigned: kanru)

Tracking

unspecified
x86
Mac OS X
Dependency tree / graph

Firefox Tracking Flags

(blocking-basecamp:+)

Details

(Whiteboard: [LOE:M])

Attachments

(2 attachments, 1 obsolete attachment)

STR:

1. Boot up the phone, press the unlock handle
2. Swipe left or right but don't trigger the unlock/camera icon.
3. Release the handle

Expected:

1. The handle bounce back in 0.1 sec

Actual:

1. The handle bounce back in 0.1 sec but with flashes and graphics defect

Note: Doesn't happen on B2G/Desktop
Seems like we need to fix it.
blocking-basecamp: ? → +
Assignee: nobody → jones.chris.g
(Assignee)

Comment 2

6 years ago
https://mxr.mozilla.org/mozilla-central/source/gfx/layers/opengl/LayerManagerOGL.cpp#1297

A call to glCopyTexSubImage2D failed so the buffer is not initialized properly.

 E/Adreno200-ES20( 9025): <qgl2DrvAPI_glCopyTexSubImage2D:321>: GL_INVALID_OPERATION
Assignee: jones.chris.g → kchen
Oh wow.  We should not be hitting that code.

Thanks for stealing this bug!
(Assignee)

Comment 4

6 years ago
It is called from ContainerLayerOGL::ContainerRender with InitModeCopy. I'm wondering why we are copying the background in this case :-/
Yeah, this is a bad slow path.  We shouldn't be hitting it for this dirt-simple page.  If you can, try to see why we're using an intermediate fbo, and why it needs copy-up.
(Assignee)

Comment 6

6 years ago
Change the opacity of the unlock & camera icon makes us hit the

  opacity != 1.0f && HasMultipleChildren()

check.
Hm, that's also not expected on the lockscreen.  A layer tree dump would be really useful.
(Assignee)

Comment 8

6 years ago
OGLLayerManager (0x497708a0)
  OGLShadowContainerLayer (0x4b527090) [shadow-visible=< (x=0, y=0, w=320, h=480); >] [visible=< (x=0, y=0, w=320, h=480); >] [opaqueContent] [metrics={ viewport=(x=0, y=0, w=320, h=480) viewportScroll=(x=0.000000, y=0.000000) displayport=(x=0, y=0, w=0, h=0) scrollId=0 }]
    OGLShadowThebesLayer (0x4b527c90) [shadow-clip=(x=0, y=0, w=0, h=0)] [clip=(x=0, y=0, w=0, h=0)]
    OGLShadowColorLayer (0x4417d020) [shadow-clip=(x=0, y=0, w=0, h=0)] [clip=(x=0, y=0, w=0, h=0)] [opaqueContent] [color=rgba(0, 0, 0, 1)]
    OGLShadowContainerLayer (0x4b528c90) [shadow-clip=(x=0, y=0, w=320, h=480)] [shadow-visible=< (x=0, y=0, w=320, h=480); >] [clip=(x=0, y=0, w=320, h=480)] [visible=< (x=0, y=0, w=320, h=480); >] [opaqueContent]
      OGLShadowThebesLayer (0x4b5db490) [shadow-visible=< (x=0, y=0, w=320, h=480); >] [visible=< (x=0, y=0, w=320, h=480); >] [opaqueContent] [valid=< (x=0, y=0, w=320, h=480); >]
      OGLShadowContainerLayer (0x487e4490) [shadow-clip=(x=0, y=20, w=320, h=460)] [shadow-visible=< (x=15, y=409, w=117, h=42); >] [clip=(x=0, y=20, w=320, h=460)] [visible=< (x=15, y=409, w=117, h=42); >] [opacity=0.7125] [usesTmpSurf]
        OGLShadowThebesLayer (0x48afcc90) [shadow-visible=< (x=15, y=409, w=42, h=42); >] [visible=< (x=15, y=409, w=42, h=42); >] [valid=< (x=15, y=409, w=42, h=42); >]
        OGLShadowContainerLayer (0x49fe2490) [shadow-transform=[ 1 0; 0 1; -37.8436 0; ]] [shadow-visible=< (x=129, y=419, w=12, h=22); >] [transform=[ 1 0; 0 1; -9.43481 0; ]] [visible=< (x=129, y=419, w=12, h=22); >]
          OGLShadowContainerLayer (0x49fe2890) [shadow-visible=< (x=129, y=419, w=12, h=22); >] [visible=< (x=129, y=419, w=12, h=22); >] [opacity=0.865217]
            OGLShadowThebesLayer (0x49fe2c90) [shadow-visible=< (x=129, y=419, w=12, h=22); >] [visible=< (x=129, y=419, w=12, h=22); >] [valid=< (x=129, y=419, w=12, h=22); >]
      OGLShadowThebesLayer (0x495e1090) [shadow-visible=< (x=57, y=429, w=126, h=2); >] [visible=< (x=57, y=429, w=126, h=2); >] [valid=< (x=57, y=429, w=126, h=2); >]
      OGLShadowContainerLayer (0x4592e090) [shadow-clip=(x=0, y=20, w=320, h=460)] [shadow-transform=[ 1 0; 0 1; 23 0; ]] [shadow-visible=< (x=126, y=396, w=68, h=68); >] [clip=(x=0, y=20, w=320, h=460)] [transform=[ 1 0; 0 1; 23 0; ]] [visible=< (x=126, y=396, w=68, h=68); >]
        OGLShadowContainerLayer (0x4592e890) [shadow-visible=< (x=126, y=396, w=68, h=68); >] [visible=< (x=126, y=396, w=68, h=68); >]
          OGLShadowThebesLayer (0x4592f090) [shadow-visible=< (x=126, y=396, w=68, h=68); >] [visible=< (x=126, y=396, w=68, h=68); >] [valid=< (x=126, y=396, w=68, h=68); >]
        OGLShadowThebesLayer (0x4592f490) [shadow-visible=< (x=135, y=405, w=50, h=50); >] [visible=< (x=135, y=405, w=50, h=50); >] [valid=< (x=135, y=405, w=50, h=50); >]
(Assignee)

Comment 9

6 years ago
OGLShadowContainerLayer (0x487e4490) [shadow-clip=(x=0, y=20, w=320, h=460)]
                                     [shadow-visible=< (x=15, y=409, w=117, h=42); >]
                                     [clip=(x=0, y=20, w=320, h=460)]
                                     [visible=< (x=15, y=409, w=117, h=42); >]
                                     [opacity=0.7125] [usesTmpSurf]
  OGLShadowThebesLayer (0x48afcc90) [shadow-visible=< (x=15, y=409, w=42, h=42); >]
                                    [visible=< (x=15, y=409, w=42, h=42); >]
                                    [valid=< (x=15, y=409, w=42, h=42); >]
  OGLShadowContainerLayer (0x49fe2490) [shadow-transform=[ 1 0; 0 1; -37.8436 0; ]]
                                       [shadow-visible=< (x=129, y=419, w=12, h=22); >]
                                       [transform=[ 1 0; 0 1; -9.43481 0; ]]
                                       [visible=< (x=129, y=419, w=12, h=22); >]
    OGLShadowContainerLayer (0x49fe2890) [shadow-visible=< (x=129, y=419, w=12, h=22); >]
                                         [visible=< (x=129, y=419, w=12, h=22); >]
                                         [opacity=0.865217]
      OGLShadowThebesLayer (0x49fe2c90) [shadow-visible=< (x=129, y=419, w=12, h=22); >]
                                        [visible=< (x=129, y=419, w=12, h=22); >]
                                        [valid=< (x=129, y=419, w=12, h=22); >]

corresponds to the lockscreen handle:

 <div id="lockscreen-area-camera" class="lockscreen-icon lockscreen-icon-left">
   <div id="lockscreen-left-arrow" class="lockscreen-arrow"></div>
 </div>
(Assignee)

Updated

6 years ago
Whiteboard: [LOE:M]
(Assignee)

Comment 10

6 years ago
Created attachment 666456 [details] [diff] [review]
fReadPixels should also respect mFlipped.

Like fCopyTexImage2D and fCopyTexSubImage2D, fReadPixels should also respect the flipping state.
Attachment #666456 - Flags: review?(matt.woodrow)
(Assignee)

Comment 11

6 years ago
Created attachment 666457 [details] [diff] [review]
Use glReadPixels to read back when back buffer isn't RGBA compatible.
Attachment #666457 - Flags: review?(jgilbert)
Attachment #666456 - Flags: review?(matt.woodrow) → review+
Comment on attachment 666457 [details] [diff] [review]
Use glReadPixels to read back when back buffer isn't RGBA compatible.

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

R+, but please fix the nits, especially 1 and 2.

::: gfx/layers/opengl/LayerManagerOGL.cpp
@@ +1275,5 @@
>                                    aRect.width, aRect.height,
>                                    0);
>      } else {
>        // Curses, incompatible formats.  Take a slow path.
> +      unsigned char* buf = new unsigned char[aRect.width * aRect.height * 4 /* RGBA */];

Use |uint8_t| instead of |unsigned char|.
Prefer nsAutoArrayPtr to doing delete[] ourselves. 
Don't inline the RGBA comment.
Consider pulling the math out into something like 'size_t bufferSize', and tagging the comment on that line.
Attachment #666457 - Flags: review?(jgilbert) → review+
https://hg.mozilla.org/mozilla-central/rev/167652a31865
https://hg.mozilla.org/mozilla-central/rev/1ca8706715d0
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Don't put anything in GLContext::raw_f~ calls. raw_ calls should only do exactly what they say they do. Any logic should be put in the fReadPixels func, not raw_fReadPixels.
Actually, I do not believe this is the correct way to do what you want it to do, in regards to respecting mFlipped. In particular, mFlipped should only apply when reading from FB 0. Also, I do not think it should be GLContext's responsibility to respect clients' coordinate systems.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment on attachment 666456 [details] [diff] [review]
fReadPixels should also respect mFlipped.

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

::: gfx/gl/GLContext.h
@@ +2630,5 @@
>      }
>  
>      void raw_fReadPixels(GLint x, GLint y, GLsizei width, GLsizei height, GLenum format, GLenum type, GLvoid *pixels) {
>          BEFORE_GL_CALL;
> +        mSymbols.fReadPixels(x, FixYValue(y, height), width, height, format, type, pixels);

This is not the correct place for this. Also, this should only apply when reading from FB 0. Can this logic not live in the client API (I presume Layers?) which needs this functionality?
Attachment #666456 - Flags: review-
I am also hesitant to add this to ReadPixels but not CopyTex*Image.
Created attachment 667719 [details] [diff] [review]
Respect mFlipped for non-offscreen CopyTex(Sub)Image and ReadPixels

Would this work?
Attachment #667719 - Flags: review?(matt.woodrow)
Attachment #667719 - Flags: review?(kchen)
(Assignee)

Comment 20

6 years ago
(In reply to Jeff Gilbert [:jgilbert] from comment #18)
> I am also hesitant to add this to ReadPixels but not CopyTex*Image.

Note that raw_fScissor, raw_fCopyTexImage2D and raw_fCopyTexSubImage2D also has this fix already.
Bizarre. This stuff shouldn't be in the raw_ funcs.
I suppose this can be dealt with in a separate bug, then.
Status: REOPENED → RESOLVED
Last Resolved: 6 years ago6 years ago
Resolution: --- → FIXED
Attachment #667719 - Attachment is obsolete: true
Attachment #667719 - Flags: review?(matt.woodrow)
Attachment #667719 - Flags: review?(kchen)
Blocks: 797664
You need to log in before you can comment on or make changes to this bug.