Details
Diff Detail
- Repository
- rMOZILLACENTRAL mozilla-central
- Lint
Automatic diff as part of commit; lint not applicable. - Unit
Automatic diff as part of commit; unit tests not applicable.
Event Timeline
In our previous code coverage analysis run, we found some files which had no coverage and are being modified in this patch:
- gfx/webrender_bindings/WebRenderAPI.cpp
- gfx/webrender_bindings/RendererOGL.cpp
- gfx/webrender_bindings/RenderCompositorANGLE.cpp
- gfx/webrender_bindings/RenderCompositor.cpp
- gfx/webrender_bindings/DCLayerTree.cpp
- gfx/layers/wr/WebRenderBridgeParent.cpp
Should they have tests, or are they dead code ?
- You can file a bug blocking Bug 1415824 for untested files that should be tested.
- You can file a bug blocking Bug 1415819 for untested files that should be removed.
If you see a problem in this automated review, please report it here.
In our previous code coverage analysis run, we found some files which had no coverage and are being modified in this patch:
- gfx/webrender_bindings/WebRenderAPI.cpp
- gfx/webrender_bindings/RendererOGL.cpp
- gfx/webrender_bindings/RenderCompositorANGLE.cpp
- gfx/webrender_bindings/RenderCompositor.cpp
- gfx/webrender_bindings/DCLayerTree.cpp
- gfx/layers/wr/WebRenderBridgeParent.cpp
Should they have tests, or are they dead code ?
- You can file a bug blocking Bug 1415824 for untested files that should be tested.
- You can file a bug blocking Bug 1415819 for untested files that should be removed.
If you see a problem in this automated review, please report it here.
In our previous code coverage analysis run, we found some files which had no coverage and are being modified in this patch:
- gfx/webrender_bindings/WebRenderAPI.cpp
- gfx/webrender_bindings/RendererScreenshotGrabber.cpp
- gfx/webrender_bindings/RendererOGL.cpp
- gfx/webrender_bindings/RenderCompositorANGLE.cpp
- gfx/webrender_bindings/RenderCompositor.cpp
- gfx/webrender_bindings/DCLayerTree.cpp
- gfx/layers/wr/WebRenderBridgeParent.cpp
Should they have tests, or are they dead code ?
- You can file a bug blocking Bug 1415824 for untested files that should be tested.
- You can file a bug blocking Bug 1415819 for untested files that should be removed.
If you see a problem in this automated review, please report it here.
Is there a way to re-enable the native compositor after screenshots are complete? Perhaps if we stick with this method, it could be EnableNativeCompositor(bool) in the message to allow enable / disable?
The existing patch does seem a bit error prone, I think (but maybe it's the only way!).
I wonder if we can change it so that when handling the DisableNativeCompositor message, it only updates the compositor mode in the config structures, and then handling of destruction of native compositor surfaces is handled by the picture caching code next time the frame is generated?
Does that sound possible? I could try to write a proof of concept patch for this today / tomorrow, to see if it works? Or you could try that out and see if it works?
Thinking about this a bit more - what we'd probably do is:
In the picture cache code that checks if a tile needs to be invalidated - we would add a condition where if the compositor mode is different to the current surface type of a tile, the entire tile gets invalidated. This should work for both enabling and disabling native compositor mode in the same way, I think.
In the picture cache code that checks if a tile needs to be invalidated
In this case, render backend side worked well. But I saw a problem in Renderer. In Renderer, there are several places that changes how it works depends on CompositorConfig. Current patch updates CompositorConfig in Renderer. It seems simple, if all previous tasks are done before updating CompositorConfig in Renderer.
https://searchfox.org/mozilla-central/rev/803a42f24c8714631ed81cb824ea1c1a803cb7b8/gfx/wr/webrender/src/renderer.rs#5104
https://searchfox.org/mozilla-central/rev/803a42f24c8714631ed81cb824ea1c1a803cb7b8/gfx/wr/webrender/src/renderer.rs#5298
https://searchfox.org/mozilla-central/rev/803a42f24c8714631ed81cb824ea1c1a803cb7b8/gfx/wr/webrender/src/renderer.rs#3281
https://searchfox.org/mozilla-central/rev/803a42f24c8714631ed81cb824ea1c1a803cb7b8/gfx/wr/webrender/src/renderer.rs#5215
gw, can you advice how to change Renderer? There are several code like the following.
if let CompositorConfig::Native { ref mut compositor, .. } = self.compositor_config {
match self.compositor_config {
I have an idea for it, I am working for it.
I only had a quick look this morning, but this looks great! I'll do a proper review later this morning, but this looks like a good approach to me :)
This mostly looks good to me! I have a couple of concerns below - just ping me in matrix if they don't make sense and we can discuss :)
| gfx/wr/webrender/src/composite.rs | ||
|---|---|---|
| 209 | I _think_ we can completely remove this variable - details in the comments below. | |
| 524 | nit: let's call this enable_native_compositor | |
| gfx/wr/webrender/src/picture.rs | ||
| 2546 | Instead of having compositor_kind_updated, I think we could so something like:
In this way, we shouldn't need to maintain an extra state bool that might get out of sync in some cases. | |
| 2558 | This shouldn't be necessary if we move it to Tile::pre_update since by that time, all the old_tiles have been handled. | |
| gfx/wr/webrender/src/renderer.rs | ||
| 2877 | I think this could cause race conditions if we handle it here (since the DebugCommand might arrive out of order with the changes to the scenes coming from render backend). I think we could remove this whole match in the renderer, and instead update current_compositor_kind at the start of render, based on the composite state that is passed in the Frame. | |
In our previous code coverage analysis run, we found some files which had no coverage and are being modified in this patch:
- gfx/webrender_bindings/WebRenderAPI.cpp
- gfx/webrender_bindings/RendererScreenshotGrabber.cpp
- gfx/webrender_bindings/RendererOGL.cpp
- gfx/webrender_bindings/RenderCompositorANGLE.cpp
- gfx/webrender_bindings/RenderCompositor.cpp
- gfx/webrender_bindings/DCLayerTree.cpp
- gfx/layers/wr/WebRenderBridgeParent.cpp
Should they have tests, or are they dead code ?
- You can file a bug blocking Bug 1415824 for untested files that should be tested.
- You can file a bug blocking Bug 1415819 for untested files that should be removed.
If you see a problem in this automated review, please report it here.
In our previous code coverage analysis run, we found some files which had no coverage and are being modified in this patch:
- gfx/webrender_bindings/WebRenderAPI.cpp
- gfx/webrender_bindings/RendererScreenshotGrabber.cpp
- gfx/webrender_bindings/RendererOGL.cpp
- gfx/webrender_bindings/RenderCompositorANGLE.cpp
- gfx/webrender_bindings/RenderCompositor.cpp
- gfx/webrender_bindings/DCLayerTree.cpp
- gfx/layers/wr/WebRenderBridgeParent.cpp
Should they have tests, or are they dead code ?
- You can file a bug blocking Bug 1415824 for untested files that should be tested.
- You can file a bug blocking Bug 1415819 for untested files that should be removed.
If you see a problem in this automated review, please report it here.
I added one more suggestion, as I think compositor surfaces will get leaked at the moment. Marked as accepted though, since it should be good to go once that question is resolved! Nice work!
| gfx/wr/webrender/src/picture.rs | ||
|---|---|---|
| 844 | See comment below - I think the code to free the compositor surface if it exists needs to be handled here? | |
| 3950 | Does this ever get hit? I would think that the compositor surfaces would leak, since surface is set to None above? I think we need to move this block of code to be run during the invalidation block above? | |
| gfx/wr/webrender/src/picture.rs | ||
|---|---|---|
| 3950 | Yes, it was hit. Tile::pre_update() does not have a reference to TileCacheInstance::native_surface_id. Is it ok to handle releasing the native_surface_id in TileCacheInstance::pre_update()? | |
| gfx/wr/webrender/src/picture.rs | ||
|---|---|---|
| 844 | Ah, I was wrong - sorry. But I think we can completely remove this piece of code (more detail in other comments). | |
| 2568 | I think what we want to do is add a piece of code here, that is very similar to this opacity check. It should replace the other two pieces of code. It can check if compositor mode changed, and then go through and invalidate all tiles + destroy the native surface. The difference from the opacity change is that we also need to set the tile.surface to None in the non-compositor case. | |
| 3950 | Ah, I was wrong - sorry. But I think we can completely remove this piece of code (more detail in other comments). | |
| gfx/wr/webrender/src/picture.rs | ||
|---|---|---|
| 2568 | TileCacheInstance does not have previous CompositorKind information. Then we need to add it to somewhere. | |
| gfx/wr/webrender/src/picture.rs | ||
|---|---|---|
| 2568 | When "tile.surface = None;" is added here, it cause a crash in the following. It is OK to call it in TileCacheInstance::pre_update()?
| |
The analysis task source-test-coverity-coverity failed, but we could not detect any issue.
Please check this task manually.
If you see a problem in this automated review, please report it here.
In our previous code coverage analysis run, we found some files which had no coverage and are being modified in this patch:
- gfx/webrender_bindings/WebRenderAPI.cpp
- gfx/webrender_bindings/RendererScreenshotGrabber.cpp
- gfx/webrender_bindings/RendererOGL.cpp
- gfx/webrender_bindings/RenderCompositorANGLE.cpp
- gfx/webrender_bindings/RenderCompositor.cpp
- gfx/webrender_bindings/DCLayerTree.cpp
- gfx/layers/wr/WebRenderBridgeParent.cpp
Should they have tests, or are they dead code ?
- You can file a bug blocking Bug 1415824 for untested files that should be tested.
- You can file a bug blocking Bug 1415819 for untested files that should be removed.
If you see a problem in this automated review, please report it here.
In our previous code coverage analysis run, we found some files which had no coverage and are being modified in this patch:
- gfx/webrender_bindings/WebRenderAPI.cpp
- gfx/webrender_bindings/RendererScreenshotGrabber.cpp
- gfx/webrender_bindings/RendererOGL.cpp
- gfx/webrender_bindings/RenderCompositorANGLE.cpp
- gfx/webrender_bindings/RenderCompositor.cpp
- gfx/webrender_bindings/DCLayerTree.cpp
- gfx/layers/wr/WebRenderBridgeParent.cpp
Should they have tests, or are they dead code ?
- You can file a bug blocking Bug 1415824 for untested files that should be tested.
- You can file a bug blocking Bug 1415819 for untested files that should be removed.
If you see a problem in this automated review, please report it here.