Bug 210992 - REGRESSION (r259589): Google Maps background layer fades out and back in
Summary: REGRESSION (r259589): Google Maps background layer fades out and back in
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebGL (show other bugs)
Version: WebKit Nightly Build
Hardware: iPhone / iPad Unspecified
: P2 Normal
Assignee: James Darpinian
URL:
Keywords: InRadar
Depends on: 208331 209689
Blocks: 212228 214945
  Show dependency treegraph
 
Reported: 2020-04-24 14:00 PDT by Dean Jackson
Modified: 2020-07-29 16:30 PDT (History)
11 users (show)

See Also:


Attachments
Screen recording (deleted)
2020-04-24 15:39 PDT, Dean Jackson
no flags Details
Screen recording (15.24 MB, video/mp4)
2020-04-24 15:40 PDT, Dean Jackson
no flags Details
Patch (3.25 KB, patch)
2020-05-14 17:56 PDT, James Darpinian
dino: review+
Details | Formatted Diff | Diff
EWS Test (7.96 MB, patch)
2020-05-14 19:41 PDT, Dean Jackson
no flags Details | Formatted Diff | Diff
EWS Test 2 (7.97 MB, patch)
2020-05-15 03:49 PDT, Dean Jackson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dean Jackson 2020-04-24 14:00:55 PDT
* SUMMARY
- Google Maps background layer fades out and back in

* STEPS TO REPRODUCE
1. Navigate to https://maps.google.com/
2. Review the loading experience

(this was an internal report so I'm trying to get a public screen recording)
Comment 1 Dean Jackson 2020-04-24 14:01:22 PDT
rdar://61907344
Comment 2 Dean Jackson 2020-04-24 14:04:15 PDT
Note this is with iOS hardware. I'm not sure it reproduces in the simulator yet.
Comment 3 Dean Jackson 2020-04-24 15:39:15 PDT
Created attachment 397520 [details]
Screen recording
Comment 4 Dean Jackson 2020-04-24 15:40:15 PDT
Created attachment 397521 [details]
Screen recording
Comment 5 David Kilzer (:ddkilzer) 2020-04-24 15:43:26 PDT
The content of attachment 397520 [details] has been deleted for the following reason:

Wrong file uploaded.
Comment 6 Kenneth Russell 2020-04-24 15:44:15 PDT
Trac link is hard to click in description:
http://trac.webkit.org/r259589
Comment 7 Kenneth Russell 2020-04-24 15:46:25 PDT
Apparent regression from Bug 209689.
Comment 8 Dean Jackson 2020-04-24 15:54:54 PDT
I can confirm that maps is getting WebGL, and the page works fine after it has loaded. There is just a black flash at the beginning.

Nothing in the console either.
Comment 9 Kenneth Russell 2020-04-24 15:57:44 PDT
Note: the most recent ANGLE roll into WebKit should have picked up the fix for this ANGLE request for enhancement:
https://bugs.chromium.org/p/angleproject/issues/detail?id=4451

so the ANGLE_depth_texture extension should be being advertised correctly by ANGLE and picked up in WebKit's WebGL implementation. This was the cause of the most recent Maps rendering bug with the ANGLE backend, Bug 208331.
Comment 10 Dean Jackson 2020-04-24 16:24:07 PDT
ANGLE initialised Major: 1 Minor: 4
Extensions: EGL_KHR_create_context EGL_EXT_device_query EGL_KHR_get_all_proc_addresses EGL_ANGLE_create_context_webgl_compatibility EGL_CHROMIUM_create_context_bind_generates_resource EGL_EXT_pixel_format_float EGL_KHR_surfaceless_context EGL_ANGLE_display_texture_share_group EGL_ANGLE_create_context_client_arrays EGL_ANGLE_program_cache_control EGL_ANGLE_robust_resource_initialization EGL_ANGLE_iosurface_client_buffer EGL_ANGLE_create_context_extensions_enabled EGL_ANDROID_blob_cache EGL_ANDROID_recordable EGL_ANGLE_create_context_backwards_compatible EGL_KHR_create_context_no_error
Got EGLConfig
Got EGLContext
Created a GraphicsContextGLOpenGL (0x15730c3e0).
GraphicsContextGLOpenGL::allocateIOSurfaceBackingStore at 300 x 150. (0x15730c3e0)
GraphicsContextGLOpenGL::updateFramebufferTextureBackingStoreFromLayer(). (0x15730c3e0)
GraphicsContextGLOpenGL::allocateIOSurfaceBackingStore at 2048 x 150. (0x15730c3e0)
GraphicsContextGLOpenGL::updateFramebufferTextureBackingStoreFromLayer(). (0x15730c3e0)
GraphicsContextGLOpenGL::allocateIOSurfaceBackingStore at 2048 x 1396. (0x15730c3e0)
GraphicsContextGLOpenGL::updateFramebufferTextureBackingStoreFromLayer(). (0x15730c3e0)
Comment 11 Kenneth Russell 2020-04-24 16:30:48 PDT
Those are the EGL extensions; could you also print out the GL extensions on the device? (The result of glGetString(GL_EXTENSIONS).
Comment 12 Dean Jackson 2020-04-24 16:40:13 PDT
WebGLRenderingContextBase::setupFlags() had the following
#if USE(ANGLE)
    m_isGLES2NPOTStrict = true;
    m_isDepthStencilSupported = true;
#endif

I've replaced that by 

m_isDepthStencilSupported = m_context->getExtensions().isEnabled("GL_OES_packed_depth_stencil");

and confirmed the extension is enabled.

The actual list of extensions (of the GraphicsContextGL) is:

GL_ANGLE_client_arrays GL_ANGLE_program_cache_control GL_ANGLE_request_extension GL_ANGLE_robust_client_memory GL_ANGLE_robust_resource_initialization GL_ANGLE_translated_shader_source GL_ANGLE_webgl_compatibility GL_CHROMIUM_bind_generates_resource GL_CHROMIUM_bind_uniform_location GL_CHROMIUM_copy_texture GL_EXT_debug_marker GL_EXT_discard_framebuffer GL_KHR_debug GL_NV_fence GL_OES_depth24 GL_OES_packed_depth_stencil GL_OES_surfaceless_context

Checking the ANGLE context now.
Comment 13 Dean Jackson 2020-04-24 16:42:48 PDT
OpenGL Extensions: GL_ANGLE_client_arrays GL_ANGLE_program_cache_control GL_ANGLE_request_extension GL_ANGLE_robust_client_memory GL_ANGLE_robust_resource_initialization GL_ANGLE_translated_shader_source GL_ANGLE_webgl_compatibility GL_CHROMIUM_bind_generates_resource GL_CHROMIUM_bind_uniform_location GL_CHROMIUM_copy_texture GL_EXT_debug_marker GL_EXT_discard_framebuffer GL_KHR_debug GL_NV_fence GL_OES_depth24 GL_OES_packed_depth_stencil GL_OES_surfaceless_context
Comment 14 Dean Jackson 2020-04-24 17:42:03 PDT
This took longer than I hoped, but GenerateCaps in ANGLE sees this:

GL_DEPTH_COMPONENT16 yes
GL_DEPTH_COMPONENT32_OES no
GL_DEPTH24_STENCIL8_OES yes
Comment 15 Dean Jackson 2020-04-24 18:00:18 PDT
If I remove the requirement for GL_DEPTH_COMPONENT32_OES then i do get the depth textures, but it doesn't fix the maps issue.
Comment 16 Kenneth Russell 2020-04-27 13:52:25 PDT
One thought: looking at the specific conformance failures for:
webgl/1.0.3/conformance/textures/tex-image-and-sub-image-2d-with-canvas.html

from Bug 207858, it looks like flip-Y canvas-to-WebGL-texture uploads might be broken in the ANGLE path for some reason. That could possibly explain why the initial background is black.
Comment 17 James Darpinian 2020-05-04 16:35:06 PDT
I'm looking at this now. First I'm fixing bug 211398.

The reason why the map appears at first and fades to black is that the map is loaded as img tags before WebGL is initialized, and there is a fade transition when switching to WebGL. I'm pretty sure that the black map is related to the progressive antialiasing feature, which renders the map to a offscreen render target before copying it to the screen. The symptoms are consistent with the first frame not being rendered to the offscreen render target. Maybe a bindFramebuffer call is failing for some reason.
Comment 18 Dean Jackson 2020-05-06 19:41:51 PDT
ANGLE produces an error: Feedback loop formed between Framebuffer and active Texture.

I confirmed the issue goes away when commenting out ANGLE's feedback detection.
Comment 19 Dean Jackson 2020-05-06 19:54:14 PDT
(In reply to Dean Jackson from comment #18)
> ANGLE produces an error: Feedback loop formed between Framebuffer and active
> Texture.
> 
> I confirmed the issue goes away when commenting out ANGLE's feedback
> detection.

There are two places where this can be triggered in ANGLE. This one in in ValidateDrawStates.

Comes from FramebufferState::updateHasRenderingFeedbackLoop()

Happens when trying to draw what appears to be a quad. The incoming code is obfuscated, but the shaders are:

precision highp float;
varying vec2 a;
uniform vec2 b,d,e;
uniform float c,f;
attribute vec2 g;
void main(){
  vec2 h,i;
  h=g*b*c+e;
  i=2.*(h/d)-1.;
  i.y*=f;
  gl_Position=vec4(i,1,1);
  a=g;
}

precision highp float;
varying vec2 a;
uniform sampler2D h;
uniform float i,j;
void main(){
  gl_FragColor = texture2D(h,a);
  gl_FragColor.a = gl_FragColor.a*i+j;
}
Comment 20 Dean Jackson 2020-05-07 17:09:53 PDT
Feedback loop detected in
[WebGLLayer display]
at
gl::BindTexture(WebCore::GraphicsContextGL::IOSurfaceTextureTarget, texture)

because it thinks texture is already bound as the framebuffer texture
Comment 21 Kenneth Russell 2020-05-08 10:31:59 PDT
Great find Dean.

James tried saving/restoring the WebCore::GraphicsContextGL::IOSurfaceTextureTarget binding point - would that help?

--------
diff --git a/Source/WebCore/platform/graphics/GraphicsContextGL.h b/Source/WebCore/platform/graphics/GraphicsContextGL.h
index ee0de95b96f..96f6d6e0fcb 100644
--- a/Source/WebCore/platform/graphics/GraphicsContextGL.h
+++ b/Source/WebCore/platform/graphics/GraphicsContextGL.h
@@ -666,7 +666,8 @@ public:
         MAX_CLIENT_WAIT_TIMEOUT_WEBGL = 0x9247,
 
         // Necessary desktop OpenGL constants.
-        TEXTURE_RECTANGLE_ARB = 0x84F5
+        TEXTURE_RECTANGLE_ARB = 0x84F5,
+        TEXTURE_BINDING_RECTANGLE_ARB  0x84F6
     };
 
     // Attempt to enumerate all possible native image formats to
@@ -787,9 +788,11 @@ public:
         // reference them.
 #if PLATFORM(MAC)
         IOSurfaceTextureTarget = TEXTURE_RECTANGLE_ARB, // also GL_TEXTURE_RECTANGLE_ANGLE
+        IOSurfaceTextureBinding = TEXTURE_BINDING_RECTANGLE_ARB, // also GL_TEXTURE_BINDING_RECTANGLE_ANGLE
         EGLIOSurfaceTextureTarget = 0x345B, // EGL_TEXTURE_RECTANGLE_ANGLE
 #else
         IOSurfaceTextureTarget = TEXTURE_2D,
+        IOSurfaceTextureBinding = TEXTURE_BINDING_2D,
         EGLIOSurfaceTextureTarget = 0x305F, // EGL_TEXTURE_2D
 #endif // PLATFORM(MAC)
     };
--------

and then in WebGLLayer.mm, something like:

GLint lastTexture = 0;
glGetIntegerv(WebCore::GraphicsContextGL::IOSurfaceTextureBinding, &lastTexture);
glBindTexture(...)
...

glBindTexture(WebCore::GraphicsContextGL::IOSurfaceTextureTarget, lastTexture);

?

One other thought is that we could try temporarily unbinding the framebuffer, but that's problematic because the code needs to know whether it's running on GLES2 or GLES3, and whether to save/restore the GL_FRAMEBUFFER or both the GL_DRAW_FRAMEBUFFER and GL_READ_FRAMEBUFFER binding points.
Comment 22 Dean Jackson 2020-05-08 17:21:03 PDT
James has confirmed that restoring the texture state didn't avoid the feedback loop detection. 

Unfortunately this is a bit tricky to debug since, as far as I can tell, there are "valid" feedback loops that temporarily exist. So setting a breakpoint when they are detected is too noisy.
Comment 23 Kenneth Russell 2020-05-08 18:24:14 PDT
Should we try teaching WebGLLayer about whether it's for WebGL 1.0 or 2.0, and then try clearing/restoring the GL_FRAMEBUFFER or GL_DRAW_FRAMEBUFFER / GL_READ_FRAMEBUFFER binding points?
Comment 24 James Darpinian 2020-05-14 17:56:07 PDT
Created attachment 399432 [details]
Patch
Comment 25 EWS Watchlist 2020-05-14 17:56:56 PDT
Note that there are important steps to take when updating ANGLE. See http://trac.webkit.org/wiki/UpdatingANGLE
Comment 26 James Darpinian 2020-05-14 17:59:54 PDT
It was a bug in ANGLE's rendering feedback loop detection. The cached feedback loop state was not updated properly when sampler uniforms were set. I don't know if this patch will apply now since the ANGLE roll was reverted, but we should be able to roll forward with it, or just do a new ANGLE roll once the patch lands in ANGLE: https://chromium-review.googlesource.com/c/angle/angle/+/2203431
Comment 27 Dean Jackson 2020-05-14 18:19:58 PDT
This fails to compile because we reverted the version of ANGLE that caused it in r261262.

I'll land this manually along with re-applying r259589.
Comment 28 Dean Jackson 2020-05-14 19:41:54 PDT
Created attachment 399445 [details]
EWS Test

Uploading a patch to test EWS.
Comment 29 Dean Jackson 2020-05-15 03:49:52 PDT
Created attachment 399469 [details]
EWS Test 2
Comment 30 Dean Jackson 2020-05-15 11:35:06 PDT
Committed r261751: <https://trac.webkit.org/changeset/261751>