New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 732522 link

Starred by 2 users

Issue metadata

Status: Started
Owner:
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 2
Type: Bug

Blocked on:
issue 655722

Blocking:
issue 691102



Sign in to add a comment

Wrong logic in GLSurfaceFormat::IsCompatible method

Reported by artyo...@gmail.com, Jun 12 2017

Issue description

Steps to reproduce the problem:
Call GLSurfaceFormat::IsCompatible(other) when:
other.depth_bits_ == 24
other.stencil_bits_ == 8
and while
this->depth_bits_ == -1
this->stencil_bits_ == -1

and it will return TRUE (meaning, these are compatible surfaces)

What is the expected behavior?
Must return FALSE, since 'this' surface doesn't have depth or stencil, while the 'other' does.

What went wrong?
The root of the issue lies in following lines:

GetValue(stencil_bits_, 8) == GetValue(other.stencil_bits_, 8) &&
GetValue(depth_bits_, 24) == GetValue(other.depth_bits_, 24) &&

where constants 24 and 8 are used as 'default' values. This is wrong, since by default EGL surface doesn't have depth or stencil and those default values must be all '0'. 

Did this work before? No 

Does this work in other browsers? N/A

Chrome version: M61/M60/M59/M58/M57....  Channel: dev
OS Version: 
Flash Version: 

For me, this issue causes wrong EGL surface to be used in GpuCommandBufferStub::Initialize in  the case of 'OwnOffscreenSurface path'. I know, you guys don't use it, but this issue may hit you differently. In any case, the IsCompatible is broken and should be fixed.
 

Comment 1 by kbr@chromium.org, Jun 14 2017

Blockedon: 655722
Cc: klausw@chromium.org aelias@chromium.org bajones@chromium.org
Components: -Blink>WebGL Internals>GPU>Internals
Owner: klausw@chromium.org
Status: Assigned (was: Unconfirmed)
Submitter, while your description is clear, could you please explain on which hardware this is doing the wrong thing, and what the failure mode is? Is it possible for you to provide the contents of about:gpu (plaintext preferred) there?

This logic was introduced in https://codereview.chromium.org/2616723002 from  Issue 655722  . It's not obvious to me whether it was intended as a pure refactoring. Klaus, could you please take this bug?

Removed the Blink>WebGL label since this is deeper in the GPU internals.

Comment 2 by artyo...@gmail.com, Jun 14 2017

Graphics Feature Status
Canvas: Hardware accelerated
Flash: Hardware accelerated
Flash Stage3D: Hardware accelerated
Flash Stage3D Baseline profile: Hardware accelerated
Compositing: Hardware accelerated
Multiple Raster Threads: Disabled
Native GpuMemoryBuffers: Software only. Hardware acceleration disabled
Rasterization: Hardware accelerated
Video Decode: Hardware accelerated
Video Encode: Software only, hardware acceleration unavailable
VPx Video Decode: Hardware accelerated
WebGL: Hardware accelerated
WebGL2: Unavailable
Driver Bug Workarounds
clear_uniforms_before_first_program_use
disable_framebuffer_cmaa
max_texture_size_limit_4096
scalarize_vec_and_mat_constructor_args
use_client_side_arrays_for_stream_buffers
use_virtualized_gl_contexts
must_use_virtualized_gl_context_for_offscreen
Problems Detected
MediaCodec is still too buggy to use for encoding (b/11536167): 615108
Disabled Features: accelerated_video_encode
WebGL 2 is not yet ready on Android: 295792, 641635
Disabled Features: webgl2
ARM driver doesn't like uploading lots of buffer data constantly
Applied Workarounds: use_client_side_arrays_for_stream_buffers
The Mali-Txxx driver does not guarantee flush ordering: 154715, 10068, 269829, 294779, 285292
Applied Workarounds: use_virtualized_gl_contexts
Clear uniforms before first program use on all platforms: 124764, 349137
Applied Workarounds: clear_uniforms_before_first_program_use
Always rewrite vec/mat constructors to be consistent: 398694
Applied Workarounds: scalarize_vec_and_mat_constructor_args
Limit max texure size to 4096 on all of Android
Applied Workarounds: max_texture_size_limit_4096
Limited enabling of Chromium GL_INTEL_framebuffer_CMAA: 535198
Applied Workarounds: disable_framebuffer_cmaa
Disable KHR_blend_equation_advanced until cc shaders are updated: 661715
The Mali-Txxx driver does not work correctly with non-virt contexts for offscreen surfaces: 0
Applied Workarounds: must_use_virtualized_gl_context_for_offscreen
Raster is using a single thread.
Disabled Features: multiple_raster_threads
Native GpuMemoryBuffers have been disabled, either via about:flags or command line.
Disabled Features: native_gpu_memory_buffers
Version Information
Data exported	6/15/2017, 9:21:04 PM
Chrome version	Chrome/57.0.2987.146
Operating system	Android 7.0.0
Software rendering list version	12.13
Driver bug list version	9.29
ANGLE commit id	c1a5d16e964a
2D graphics backend	Skia/57 ae9cc5d3588d52f4b371b55845704b25d88cf06d
Command Line Args	--use-mobile-user-agent --top-controls-show-threshold=0.5 --top-controls-hide-threshold=0.5 --enable-logging --v=1 --vmodule=*/VR*/*=2,*/vr/*=2,*/oculus/*=2,gearvr_*=2,*/VR*=2,vr_*=2,gles2_cmd_decoder*=1,WebGL*=1 --use-mobile-user-agent --enable-pinch --enable-viewport --enable-overlay-scrollbar --validate-input-event-stream --enable-longpress-drag-selection --touch-selection-strategy=direction --disable-gpu-process-crash-limit --single-process --enable-webvr --main-frame-resizes-are-orientation-changes --disable-composited-antialiasing --ui-prioritize-in-gpu-process --profiler-timing=0 --prerender-from-omnibox=enabled --enable-dom-distiller --flag-switches-begin --flag-switches-end --disable-gpu-watchdog --supports-dual-gpus=false --gpu-driver-bug-workarounds=7,23,58,74,88,93,97 --disable-gl-extensions=GL_KHR_blend_equation_advanced GL_KHR_blend_equation_advanced_coherent --gpu-vendor-id=0x0000 --gpu-device-id=0x0000 --gpu-driver-vendor --gpu-driver-version=1. --gpu-driver-date --lang=en-US --num-raster-threads=1 --enable-gpu-rasterization --gpu-rasterization-msaa-sample-count=4 --enable-main-frame-before-activation --content-image-texture-target=0,0,3553;0,1,3553;0,2,3553;0,3,3553;0,4,3553;0,5,3553;0,6,3553;0,7,3553;0,8,3553;0,9,3553;0,10,3553;0,11,3553;0,12,3553;0,13,3553;0,14,3553;0,15,3553;1,0,3553;1,1,3553;1,2,3553;1,3,3553;1,4,3553;1,5,3553;1,6,3553;1,7,3553;1,8,3553;1,9,3553;1,10,3553;1,11,3553;1,12,3553;1,13,3553;1,14,3553;1,15,3553;2,0,3553;2,1,3553;2,2,3553;2,3,3553;2,4,3553;2,5,3553;2,6,3553;2,7,3553;2,8,3553;2,9,3553;2,10,3553;2,11,3553;2,12,3553;2,13,3553;2,14,3553;2,15,3553;3,0,3553;3,1,3553;3,2,3553;3,3,3553;3,4,3553;3,5,3553;3,6,3553;3,7,3553;3,8,3553;3,9,3553;3,10,3553;3,11,3553;3,12,3553;3,13,3553;3,14,3553;3,15,3553 --disable-webrtc-hw-vp8-encoding
Driver Information
Initialization time	229
In-process GPU	true
Passthrough Command Decoder	false
Sandboxed	false
GPU0	VENDOR = 0x0000 [ARM], DEVICE= 0x0000 [Mali-T760]
Optimus	false
AMD switchable	false
Driver vendor	
Driver version	1.
Driver date	
Pixel shader version	3.20
Vertex shader version	3.20
Max. MSAA samples	8
Machine model name	SM-G920F
Machine model version	
GL_VENDOR	ARM
GL_RENDERER	Mali-T760
GL_VERSION	OpenGL ES 3.2 v1.r15p0-00rel0.68b65ac7cf15907aeb95fa944f39eef2
GL_EXTENSIONS	GL_EXT_debug_marker GL_ARM_rgba8 GL_ARM_mali_shader_binary GL_OES_depth24 GL_OES_depth_texture GL_OES_depth_texture_cube_map GL_OES_packed_depth_stencil GL_OES_rgb8_rgba8 GL_EXT_read_format_bgra GL_OES_compressed_paletted_texture GL_OES_compressed_ETC1_RGB8_texture GL_OES_standard_derivatives GL_OES_EGL_image GL_OES_EGL_image_external GL_OES_EGL_image_external_essl3 GL_OES_EGL_sync GL_OES_texture_npot GL_OES_vertex_half_float GL_OES_required_internalformat GL_OES_vertex_array_object GL_OES_mapbuffer GL_EXT_texture_format_BGRA8888 GL_EXT_texture_rg GL_EXT_texture_type_2_10_10_10_REV GL_OES_fbo_render_mipmap GL_OES_element_index_uint GL_EXT_shadow_samplers GL_OES_texture_compression_astc GL_KHR_texture_compression_astc_ldr GL_KHR_texture_compression_astc_hdr GL_KHR_texture_compression_astc_sliced_3d GL_KHR_debug GL_EXT_occlusion_query_boolean GL_EXT_disjoint_timer_query GL_EXT_blend_minmax GL_EXT_discard_framebuffer GL_OES_get_program_binary GL_OES_texture_3D GL_EXT_texture_storage GL_EXT_multisampled_render_to_texture GL_OES_surfaceless_context GL_OES_texture_stencil8 GL_EXT_shader_pixel_local_storage GL_ARM_shader_framebuffer_fetch GL_ARM_shader_framebuffer_fetch_depth_stencil GL_ARM_mali_program_binary GL_EXT_sRGB GL_EXT_sRGB_write_control GL_EXT_texture_sRGB_decode GL_EXT_texture_sRGB_R8 GL_EXT_texture_sRGB_RG8 GL_KHR_blend_equation_advanced GL_KHR_blend_equation_advanced_coherent GL_OES_texture_storage_multisample_2d_array GL_OES_shader_image_atomic GL_EXT_robustness GL_EXT_draw_buffers_indexed GL_OES_draw_buffers_indexed GL_EXT_texture_border_clamp GL_OES_texture_border_clamp GL_EXT_texture_cube_map_array GL_OES_texture_cube_map_array GL_OES_sample_variables GL_OES_sample_shading GL_OES_shader_multisample_interpolation GL_EXT_shader_io_blocks GL_OES_shader_io_blocks GL_EXT_tessellation_shader GL_OES_tessellation_shader GL_EXT_primitive_bounding_box GL_OES_primitive_bounding_box GL_EXT_geometry_shader GL_OES_geometry_shader GL_ANDROID_extension_pack_es31a GL_EXT_gpu_shader5 GL_OES_gpu_shader5 GL_EXT_texture_buffer GL_OES_texture_buffer GL_EXT_copy_image GL_OES_copy_image GL_EXT_color_buffer_half_float GL_EXT_color_buffer_float GL_EXT_YUV_target GL_OVR_multiview GL_OVR_multiview2 GL_OVR_multiview_multisampled_render_to_texture GL_KHR_robustness GL_KHR_robust_buffer_access_behavior GL_EXT_draw_elements_base_vertex GL_OES_draw_elements_base_vertex GL_EXT_protected_textures
Disabled Extensions	GL_KHR_blend_equation_advanced GL_KHR_blend_equation_advanced_coherent
Window system binding vendor	
Window system binding version	
Window system binding extensions	
Direct rendering	Yes
Reset notification strategy	0x8252
GPU process crash count	0
Compositor Information
Tile Update Mode	One-copy
Partial Raster	Enabled
GpuMemoryBuffers Status
ATC	Software only
ATCIA	Software only
DXT1	Software only
DXT5	Software only
ETC1	Software only
R_8	Software only
RG_88	Software only
BGR_565	Software only
RGBA_4444	Software only
RGBX_8888	Software only
RGBA_8888	Software only
BGRX_8888	Software only
BGRA_8888	Software only
YVU_420	Software only
YUV_420_BIPLANAR	Software only
UYVY_422	Software only

Comment 3 by klausw@chromium.org, Jun 14 2017

Status: Started (was: Assigned)
Summary: I think this is indeed a logic error, though harmless due to nonzero depth not currently being used, and fixing it seems both appropriate and safe. Longer explanation following.

kbr@, my original change was intended to be a refactor, but there was indeed a logic error where the format class's depth default was out of sync with the default surface's properties.

In current usage this didn't have any visible effect, since to the best of my knowledge nobody is specifying depth bits for an offscreen context. The default depth=-1 (EGL_DONTCARE) is considered compatible with itself, so it continues using a virtualized context.

The only non-test call to SurfaceFormat's SetDepthBits() is here: https://cs.chromium.org/chromium/src/gpu/ipc/service/gpu_command_buffer_stub.cc?type=cs&l=655

Note that this only modifies the setting away from "don't care" for nonzero depth, so depth 0 remains as "don't care". The bug here is that depth 24 is erroneously considered compatible to the default surface, even though the default offscreen surface is created with zero depth bits:

That section is conditional on init_params.attribs.own_offscreen_surface being set, specifically to avoid incompatibilities such as an unexpected Ozone usage. (See http://crrev.com/2761153002 .)

own_offscreen_surface is currently only being activated by a WebVR runtime experiment that's off by default due to fundamental synchronization issues with non-virtualized contexts:
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp?gsn=support_depth&l=628


Comment 4 by klausw@chromium.org, Jun 14 2017

For the synchronization issue with non-virtualized contexts, see http://crbug.com/691102#c10 where I tried to summarize my (tentative) interpretation of what's going on.

Comment 5 by artyo...@gmail.com, Jun 14 2017

In Oculus we use this own offscreen surface path, since we do not experience those sync issues Klaus is talking about on our HW. Please, fix it :) Or, we would need to fix it every time we sync with Chromium, which is a big PITA. 

Comment 6 by kbr@chromium.org, Jun 19 2017

Blocking: 691102

Sign in to add a comment