Step to Reproduce: 1. set layers.mlgpu.dev-enabled true. 2. Open https://game.nicovideo.jp/atsumaru/ 3. See slide animation in banner. Expected result: All animations are smooth and no image corruption. Actual result: Some slide animation is sluggish and sometimes right side of　image isn't rendered. When layers.mlgpu.dev-enabled is false, animation is normal.

Graphic section in about:support Graphics -------- Features Compositing: Direct3D 11 (Advanced Layers) Asynchronous Pan/Zoom: wheel input enabled; scrollbar drag enabled; keyboard enabled WebGL 1 Driver WSI Info: EGL_VENDOR: Google Inc. (adapter LUID: 000000000000646c) EGL_VERSION: 1.4 (ANGLE 2.1.0.dec065540d5f) EGL_EXTENSIONS: EGL_EXT_create_context_robustness EGL_ANGLE_d3d_share_handle_client_buffer EGL_ANGLE_d3d_texture_client_buffer EGL_ANGLE_surface_d3d_texture_2d_share_handle EGL_ANGLE_query_surface_pointer EGL_ANGLE_window_fixed_size EGL_ANGLE_keyed_mutex EGL_ANGLE_surface_orientation EGL_ANGLE_direct_composition EGL_NV_post_sub_buffer EGL_KHR_create_context EGL_EXT_device_query EGL_KHR_image EGL_KHR_image_base EGL_KHR_gl_texture_2D_image EGL_KHR_gl_texture_cubemap_image EGL_KHR_gl_renderbuffer_image EGL_KHR_get_all_proc_addresses EGL_KHR_stream EGL_KHR_stream_consumer_gltexture EGL_NV_stream_consumer_gltexture_yuv EGL_ANGLE_flexible_surface_compatibility EGL_ANGLE_stream_producer_d3d_texture_nv12 EGL_ANGLE_create_context_webgl_compatibility EGL_CHROMIUM_create_context_bind_generates_resource EGL_EXTENSIONS(nullptr): EGL_EXT_client_extensions EGL_EXT_platform_base EGL_EXT_platform_device EGL_ANGLE_platform_angle EGL_ANGLE_platform_angle_d3d EGL_ANGLE_device_creation EGL_ANGLE_device_creation_d3d11 EGL_ANGLE_experimental_present_path EGL_KHR_client_get_all_proc_addresses WebGL 1 Driver Renderer: Google Inc. -- ANGLE (AMD Radeon HD 7700 Series Direct3D11 vs_5_0 ps_5_0) WebGL 1 Driver Version: OpenGL ES 2.0 (ANGLE 2.1.0.dec065540d5f) WebGL 1 Driver Extensions: GL_ANGLE_depth_texture GL_ANGLE_framebuffer_blit GL_ANGLE_framebuffer_multisample GL_ANGLE_instanced_arrays GL_ANGLE_lossy_etc_decode GL_ANGLE_pack_reverse_row_order GL_ANGLE_request_extension GL_ANGLE_robust_client_memory GL_ANGLE_texture_compression_dxt3 GL_ANGLE_texture_compression_dxt5 GL_ANGLE_texture_usage GL_ANGLE_translated_shader_source GL_CHROMIUM_bind_generates_resource GL_CHROMIUM_bind_uniform_location GL_CHROMIUM_copy_compressed_texture GL_CHROMIUM_copy_texture GL_CHROMIUM_sync_query GL_EXT_blend_minmax GL_EXT_color_buffer_half_float GL_EXT_debug_marker GL_EXT_discard_framebuffer GL_EXT_disjoint_timer_query GL_EXT_draw_buffers GL_EXT_frag_depth GL_EXT_map_buffer_range GL_EXT_occlusion_query_boolean GL_EXT_read_format_bgra GL_EXT_robustness GL_EXT_sRGB GL_EXT_shader_texture_lod GL_EXT_texture_compression_dxt1 GL_EXT_texture_filter_anisotropic GL_EXT_texture_format_BGRA8888 GL_EXT_texture_rg GL_EXT_texture_storage GL_EXT_unpack_subimage GL_KHR_debug GL_NV_EGL_stream_consumer_external GL_NV_fence GL_NV_pack_subimage GL_NV_pixel_buffer_object GL_OES_EGL_image GL_OES_EGL_image_external GL_OES_compressed_ETC1_RGB8_texture GL_OES_depth32 GL_OES_element_index_uint GL_OES_get_program_binary GL_OES_mapbuffer GL_OES_packed_depth_stencil GL_OES_rgb8_rgba8 GL_OES_standard_derivatives GL_OES_texture_float GL_OES_texture_float_linear GL_OES_texture_half_float GL_OES_texture_half_float_linear GL_OES_texture_npot GL_OES_vertex_array_object WebGL 1 Extensions: ANGLE_instanced_arrays EXT_blend_minmax EXT_color_buffer_half_float EXT_frag_depth EXT_shader_texture_lod EXT_texture_filter_anisotropic EXT_disjoint_timer_query MOZ_debug OES_element_index_uint OES_standard_derivatives OES_texture_float OES_texture_float_linear OES_texture_half_float OES_texture_half_float_linear OES_vertex_array_object WEBGL_color_buffer_float WEBGL_compressed_texture_s3tc WEBGL_debug_renderer_info WEBGL_debug_shaders WEBGL_depth_texture WEBGL_draw_buffers WEBGL_lose_context MOZ_WEBGL_lose_context MOZ_WEBGL_compressed_texture_s3tc MOZ_WEBGL_depth_texture WebGL 2 Driver WSI Info: EGL_VENDOR: Google Inc. (adapter LUID: 000000000000646c) EGL_VERSION: 1.4 (ANGLE 2.1.0.dec065540d5f) EGL_EXTENSIONS: EGL_EXT_create_context_robustness EGL_ANGLE_d3d_share_handle_client_buffer EGL_ANGLE_d3d_texture_client_buffer EGL_ANGLE_surface_d3d_texture_2d_share_handle EGL_ANGLE_query_surface_pointer EGL_ANGLE_window_fixed_size EGL_ANGLE_keyed_mutex EGL_ANGLE_surface_orientation EGL_ANGLE_direct_composition EGL_NV_post_sub_buffer EGL_KHR_create_context EGL_EXT_device_query EGL_KHR_image EGL_KHR_image_base EGL_KHR_gl_texture_2D_image EGL_KHR_gl_texture_cubemap_image EGL_KHR_gl_renderbuffer_image EGL_KHR_get_all_proc_addresses EGL_KHR_stream EGL_KHR_stream_consumer_gltexture EGL_NV_stream_consumer_gltexture_yuv EGL_ANGLE_flexible_surface_compatibility EGL_ANGLE_stream_producer_d3d_texture_nv12 EGL_ANGLE_create_context_webgl_compatibility EGL_CHROMIUM_create_context_bind_generates_resource EGL_EXTENSIONS(nullptr): EGL_EXT_client_extensions EGL_EXT_platform_base EGL_EXT_platform_device EGL_ANGLE_platform_angle EGL_ANGLE_platform_angle_d3d EGL_ANGLE_device_creation EGL_ANGLE_device_creation_d3d11 EGL_ANGLE_experimental_present_path EGL_KHR_client_get_all_proc_addresses WebGL 2 Driver Renderer: Google Inc. -- ANGLE (AMD Radeon HD 7700 Series Direct3D11 vs_5_0 ps_5_0) WebGL 2 Driver Version: OpenGL ES 3.0 (ANGLE 2.1.0.dec065540d5f) WebGL 2 Driver Extensions: GL_ANGLE_depth_texture GL_ANGLE_framebuffer_blit GL_ANGLE_framebuffer_multisample GL_ANGLE_instanced_arrays GL_ANGLE_lossy_etc_decode GL_ANGLE_pack_reverse_row_order GL_ANGLE_request_extension GL_ANGLE_robust_client_memory GL_ANGLE_texture_compression_dxt3 GL_ANGLE_texture_compression_dxt5 GL_ANGLE_texture_usage GL_ANGLE_translated_shader_source GL_CHROMIUM_bind_generates_resource GL_CHROMIUM_bind_uniform_location GL_CHROMIUM_copy_compressed_texture GL_CHROMIUM_copy_texture GL_CHROMIUM_sync_query GL_EXT_blend_minmax GL_EXT_color_buffer_float GL_EXT_color_buffer_half_float GL_EXT_debug_marker GL_EXT_discard_framebuffer GL_EXT_disjoint_timer_query GL_EXT_draw_buffers GL_EXT_frag_depth GL_EXT_map_buffer_range GL_EXT_occlusion_query_boolean GL_EXT_read_format_bgra GL_EXT_robustness GL_EXT_sRGB GL_EXT_shader_texture_lod GL_EXT_texture_compression_dxt1 GL_EXT_texture_filter_anisotropic GL_EXT_texture_format_BGRA8888 GL_EXT_texture_norm16 GL_EXT_texture_rg GL_EXT_texture_storage GL_EXT_unpack_subimage GL_KHR_debug GL_NV_EGL_stream_consumer_external GL_NV_fence GL_NV_pack_subimage GL_NV_pixel_buffer_object GL_OES_EGL_image GL_OES_EGL_image_external GL_OES_EGL_image_external_essl3 GL_OES_compressed_ETC1_RGB8_texture GL_OES_depth32 GL_OES_element_index_uint GL_OES_get_program_binary GL_OES_mapbuffer GL_OES_packed_depth_stencil GL_OES_rgb8_rgba8 GL_OES_standard_derivatives GL_OES_texture_float GL_OES_texture_float_linear GL_OES_texture_half_float GL_OES_texture_half_float_linear GL_OES_texture_npot GL_OES_vertex_array_object WebGL 2 Extensions: EXT_color_buffer_float EXT_texture_filter_anisotropic EXT_disjoint_timer_query MOZ_debug OES_texture_float_linear WEBGL_compressed_texture_s3tc WEBGL_debug_renderer_info WEBGL_debug_shaders WEBGL_lose_context MOZ_WEBGL_lose_context MOZ_WEBGL_compressed_texture_s3tc Direct2D: true DirectWrite: true (10.0.15063.483) GPU #1 Active: Yes Description: AMD Radeon HD 7700 Series Vendor ID: 0x1002 Device ID: 0x683d Driver Version: 22.19.662.4 Driver Date: 7-20-2017 Drivers: aticfx64 aticfx64 aticfx64 amdxc64 aticfx32 aticfx32 aticfx32 amdxc32 atiumd64 atidxx64 atidxx64 atiumdag atidxx32 atidxx32 atiumdva atiumd6a atitmm64 Subsys ID: 00000000 RAM: 1024 Diagnostics ClearType Parameters: Gamma: 2.2 Pixel Structure: RGB ClearType Level: 100 Enhanced Contrast: 100 AzureCanvasAccelerated: 0 AzureCanvasBackend: Direct2D 1.1 AzureCanvasBackend (UI Process): skia AzureContentBackend: Direct2D 1.1 AzureContentBackend (UI Process): skia AzureFallbackCanvasBackend (UI Process): cairo GPUProcessPid: 3976 GPUProcess: Terminate GPU Process Device Reset: Trigger Device Reset ClearType Parameters: Gamma: 2.2 Pixel Structure: RGB ClearType Level: 100 Enhanced Contrast: 100 Decision Log WEBRENDER: opt-in by default: WebRender is an opt-in feature

I can reproduce the issue. Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:57.0) Gecko/20100101 Firefox/57.0 Build ID 20170816100153

Bas, is the "slide on the right shows up only when animation finishes" a clipping issue on our side, perhaps with OMTA? Can you take a look at this?

I have a machine I can reproduce this on.

I've been able to capture this in the graphics debugger. It turns out as far as I can tell there's nothing wrong with the drawing commands we're issuing. Rather, this area of the banner is actually black in the uploaded texture, this could either be partial uploading going wrong, or some problem on the drawing side, but the latter seems unlikely since disabling advanced layers shouldn't help then. My best guess is we've performed the upload for another frame, and while an async animation is running failed to upload the part of a thebes layer that has just come into view.

There is a bunch of other weirdness going on here, it seems we're creating/uploading a whole bunch of '0' height mask textures during this animation. I don't know why yet, but I suspect that could be related to the sluggish performance.

Further investigation shows that the texture mentioned in comment 6 is actually a temporary render target. It appears we are drawing the wrong portion of the temporary render target. I'm guessing we are forgetting to incorporate the APZ transform in there somewhere.

When I unconditionally set the invalid rect to the viewport in ContainerLayerMLGPU::OnPrepareToRender this fixes the issue, suggesting we render a certain portion of the container, probably mark the entire container valid, but fail to mark new parts of the container invalid as they come into view.

This invalidates the composited area when the transform changes. This patch fixes the issue but we should be able to do something better, investigating further.

Hrm, I suppose something like this may be the only solution, I don't really see how if a container layer's transform is change we can easily know whether its content is still valid.

Comment on attachment 8899346 [details] [diff] [review] WIP - Always invalidate composited area when the transform changes David not being here, what do you think, Matt?

So is the main problem here that we change the transform on a ContainerLayer (with an intermediate surface), but we don't update the visible region of the layer? Since the visible region hasn't changed, LTI assumes that the retained contents of the intermediate surface are still valid.

(In reply to Matt Woodrow (:mattwoodrow) from comment #13) > So is the main problem here that we change the transform on a ContainerLayer > (with an intermediate surface), but we don't update the visible region of > the layer? > > Since the visible region hasn't changed, LTI assumes that the retained > contents of the intermediate surface are still valid. The visible region here is the 'whole' container layer (which is 2 banners wide), but that is not at all completely visible. When the async animation happens though, that visible region doesn't change. However when we composite, only the 'actually visible region' for that frame is painted with valid content, a subsequent frame with a different 'actually visible area' needs to have that redrawn.

(In reply to Bas Schouten (:bas.schouten) from comment #14) > (In reply to Matt Woodrow (:mattwoodrow) from comment #13) > > So is the main problem here that we change the transform on a ContainerLayer > > (with an intermediate surface), but we don't update the visible region of > > the layer? > > > > Since the visible region hasn't changed, LTI assumes that the retained > > contents of the intermediate surface are still valid. > > The visible region here is the 'whole' container layer (which is 2 banners > wide), but that is not at all completely visible. When the async animation > happens though, that visible region doesn't change. However when we > composite, only the 'actually visible region' for that frame is painted with > valid content, a subsequent frame with a different 'actually visible area' > needs to have that redrawn. When you say the visible region, do you mean the normal one or the shadow one? The general idea is that the shadow one should take async animation into account. Basically, if Layers is reporting that the entire Layer is visible, but Advanced Layers is choosing to ignore that and only render a subset (based on what it thinks is really visible), then we can't use LayerTreeInvalidation to tell us what changed, since that uses the Layers API, and not whatever AL is using. With LayerManagerComposite/Compositor, we use LayerManagerComposite::PostProcessLayers to recompute minimal shadow visible regions on Layers (after async animations and APZ have been applied), and then LayerTreeInvalidation can detect changes to these. Advanced Layers isn't using this pass (in the hope that it can be fast enough without it, and thus save the CPU cycles).

(In reply to Matt Woodrow (:mattwoodrow) from comment #15) > (In reply to Bas Schouten (:bas.schouten) from comment #14) > > (In reply to Matt Woodrow (:mattwoodrow) from comment #13) > > > So is the main problem here that we change the transform on a ContainerLayer > > > (with an intermediate surface), but we don't update the visible region of > > > the layer? > > > > > > Since the visible region hasn't changed, LTI assumes that the retained > > > contents of the intermediate surface are still valid. > > > > The visible region here is the 'whole' container layer (which is 2 banners > > wide), but that is not at all completely visible. When the async animation > > happens though, that visible region doesn't change. However when we > > composite, only the 'actually visible region' for that frame is painted with > > valid content, a subsequent frame with a different 'actually visible area' > > needs to have that redrawn. > > When you say the visible region, do you mean the normal one or the shadow > one? > > The general idea is that the shadow one should take async animation into > account. > > Basically, if Layers is reporting that the entire Layer is visible, but > Advanced Layers is choosing to ignore that and only render a subset (based > on what it thinks is really visible), then we can't use > LayerTreeInvalidation to tell us what changed, since that uses the Layers > API, and not whatever AL is using. > > With LayerManagerComposite/Compositor, we use > LayerManagerComposite::PostProcessLayers to recompute minimal shadow visible > regions on Layers (after async animations and APZ have been applied), and > then LayerTreeInvalidation can detect changes to these. > Advanced Layers isn't using this pass (in the hope that it can be fast > enough without it, and thus save the CPU cycles). How would I get to the 'shadow' one? The containerlayer I'm looking at only seems to have one visible region...

(In reply to Bas Schouten (:bas.schouten) from comment #16) > (In reply to Matt Woodrow (:mattwoodrow) from comment #15) > > (In reply to Bas Schouten (:bas.schouten) from comment #14) > > > (In reply to Matt Woodrow (:mattwoodrow) from comment #13) > > > > So is the main problem here that we change the transform on a ContainerLayer > > > > (with an intermediate surface), but we don't update the visible region of > > > > the layer? > > > > > > > > Since the visible region hasn't changed, LTI assumes that the retained > > > > contents of the intermediate surface are still valid. > > > > > > The visible region here is the 'whole' container layer (which is 2 banners > > > wide), but that is not at all completely visible. When the async animation > > > happens though, that visible region doesn't change. However when we > > > composite, only the 'actually visible region' for that frame is painted with > > > valid content, a subsequent frame with a different 'actually visible area' > > > needs to have that redrawn. > > > > When you say the visible region, do you mean the normal one or the shadow > > one? > > > > The general idea is that the shadow one should take async animation into > > account. > > > > Basically, if Layers is reporting that the entire Layer is visible, but > > Advanced Layers is choosing to ignore that and only render a subset (based > > on what it thinks is really visible), then we can't use > > LayerTreeInvalidation to tell us what changed, since that uses the Layers > > API, and not whatever AL is using. > > > > With LayerManagerComposite/Compositor, we use > > LayerManagerComposite::PostProcessLayers to recompute minimal shadow visible > > regions on Layers (after async animations and APZ have been applied), and > > then LayerTreeInvalidation can detect changes to these. > > Advanced Layers isn't using this pass (in the hope that it can be fast > > enough without it, and thus save the CPU cycles). > > How would I get to the 'shadow' one? The containerlayer I'm looking at only > seems to have one visible region... Alright, I found this, the ShadowVisibleRegion also doesn't seem to change here, it appears to be an over-estimation for the container layer in the same way as the LocalVisibleRegion is. So it doesn't appear like I can use that to detect when to invalidate composited areas. I'm also hesitant about this patch though, as this would cause us to also recomposite when there isn't any need. I'd like to find a better way to figure out if we have to redraw, but I don't understand the code well enough for that yet.

I think this patch is more correct, and it does indeed fix the issue. I also suspect this might be done a little more efficient by somehow integrating it to the code above here, but I'd be interested to here what you think Matt.

Comment on attachment 8900431 [details] [diff] [review] Invalidate areas of the container where child areas got new visible regions Review of attachment 8900431 [details] [diff] [review]: ----------------------------------------------------------------- Don't we do this already? The main loop in this function should be iterating over mChildren already, looking for changes in them, and accumulating into 'result'. What type is the changed child in this case? And why doesn't calling into ComputeChange for that child already add this same area?

(In reply to Matt Woodrow (:mattwoodrow) from comment #19) > Comment on attachment 8900431 [details] [diff] [review] > Invalidate areas of the container where child areas got new visible regions > > Review of attachment 8900431 [details] [diff] [review]: > ----------------------------------------------------------------- > > Don't we do this already? > > The main loop in this function should be iterating over mChildren already, > looking for changes in them, and accumulating into 'result'. > > What type is the changed child in this case? And why doesn't calling into > ComputeChange for that child already add this same area? I don't really understand what the rest of this function that well. The child in this case is a PaintedLayer, its visible region simply changes, and I stepped through the function and it's not clear to me which part should find out that a single, constantly present painted layer with a moving visible region (but no invalid area!) has changed. Also, as hard evidence: The patch works :-).

(In reply to Bas Schouten (:bas.schouten) from comment #20) > I don't really understand what the rest of this function that well. The > child in this case is a PaintedLayer, its visible region simply changes, and > I stepped through the function and it's not clear to me which part should > find out that a single, constantly present painted layer with a moving > visible region (but no invalid area!) has changed. It is indeed rather complex. It's iterating over both mChildren (the set of the children last composite) as well as the current set of children and looking for new layers, removed layers, reordered layers and changes within layers. It's a tough problem. Ok, so it looks like we only looks for visible region changes in some of the layer-specific comparison functions (ComputeChangeInternal impls). That seems wrong for this, since it seems that any layer could have an async visible region change that needs to be detected. It looks like ContainerLayerProperties really wants the visible region check to be within ComputeChangeInternal so that it can contribute to the SetInvalidCompositeRect call (and we want to make sure we exclude changes like transform that only affect the compositing of the intermediate surface). How about ensuring that all ComputeChangeInternal impls do a visible region comparison, including the default one (which is what gets used for PaintedLayer)? > > Also, as hard evidence: The patch works :-). Hard to argue with that!

(In reply to Matt Woodrow (:mattwoodrow) from comment #21) > (In reply to Bas Schouten (:bas.schouten) from comment #20) > > > I don't really understand what the rest of this function that well. The > > child in this case is a PaintedLayer, its visible region simply changes, and > > I stepped through the function and it's not clear to me which part should > > find out that a single, constantly present painted layer with a moving > > visible region (but no invalid area!) has changed. > > It is indeed rather complex. It's iterating over both mChildren (the set of > the children last composite) as well as the current set of children and > looking for new layers, removed layers, reordered layers and changes within > layers. It's a tough problem. > > Ok, so it looks like we only looks for visible region changes in some of the > layer-specific comparison functions (ComputeChangeInternal impls). That > seems wrong for this, since it seems that any layer could have an async > visible region change that needs to be detected. > > It looks like ContainerLayerProperties really wants the visible region check > to be within ComputeChangeInternal so that it can contribute to the > SetInvalidCompositeRect call (and we want to make sure we exclude changes > like transform that only affect the compositing of the intermediate surface). > > How about ensuring that all ComputeChangeInternal impls do a visible region > comparison, including the default one (which is what gets used for > PaintedLayer)? This could potentially be expensive even though this would only be needed when we actually have an intermediate surface, which is at least moderately rare. How much more of these checks would we be doing?

We already compare visible regions for most Layer types, just not PaintedLayer. I'm not sure it is limited to intermediate surfaces, it seems like we could potentially have the same bug computing the invalid rectangle for the root layer (used for partial present rects). I think we can probably just accept the extra cost here, and look into optimizing this whole pass if it starts showing up as a big factor.

I can confirm this patch does solve the problem, having said that it seems to very significantly reduce the performance of the animation.. Could this change somehow be interfering with async animations? Or possibly in some other place cause significant additional drawing.

That's a bit worrying! We do use LayerTreeInvalidation for computing invalid regions of inactive layer trees, is there anything noticeably different when you enable paint flashing?

There is! Actually, the banner on the page, the part being animated now, if you will, is continuously redrawn during the animation with the patch, without the patch, only the portions animating into view are being drawn.

So I guess that's why we didn't check visible regions for PaintedLayers in the past :) I think the issue is that we don't always need to invalidate within a PaintedLayer (what is happening when we compute layer tree differences for an inactive layer tree) when things move, because we have rotated buffers, and the relative distance from the top-left is actually unchanged. This is sort of annoying, since clearly visible region changes for active layers on the compositor side are 'real', yet on the client side not so much. We could add a shadow layers specific check for visible region changes?

(In reply to Matt Woodrow (:mattwoodrow) from comment #27) > So I guess that's why we didn't check visible regions for PaintedLayers in > the past :) > > I think the issue is that we don't always need to invalidate within a > PaintedLayer (what is happening when we compute layer tree differences for > an inactive layer tree) when things move, because we have rotated buffers, > and the relative distance from the top-left is actually unchanged. > > This is sort of annoying, since clearly visible region changes for active > layers on the compositor side are 'real', yet on the client side not so much. > > We could add a shadow layers specific check for visible region changes? I'm fine with that, I suppose that will also solve problems related to partial present rects and such, I'll make a patch.

Pushed by bschouten@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/86a0af16631d Recomposite areas of a container where child layers became visible. r=mattwoodrow