Screen blinks during the fullscreen transition on Android |
|||||
Issue descriptionChrome Version: content_shell 59 OS: Android What steps will reproduce the problem? (1) Run content_shell (2) Start playing a video (i.e. a video on bbc news) (3) Go fullscreen What is the expected result? Smooth transition to fullscreen mode. What happens instead? The screen blinks 1-2 seconds after going to fullscreen. SurfaceView destroys and recreates the surface when changing the pixel format, hence the screen blinks. It was already fixed for Chrome layer in the following change but it's still reproducible in content_shell. https://codereview.chromium.org/2201483002 A suggestion from the chrome owner to make this fix available in content was: - move CompositorSurfaceManager + Impl into content - modify CVRV to use a CompositorSurfaceManager - create a new, simple CompositorSurfaceManager impl that uses only one SV in the same way that CVRV does now. - allow the embedder to provide the CSM implementation that CVRV uses.
,
May 7 2018
We have a patch for Opera (the suggested change above) that we want to upstream. How does it sound to you?
,
May 7 2018
Sounds reasonable to me but would like to have input from one of the reviewers of the Chrome-side CL.
,
May 8 2018
,
May 8 2018
The suggested change is to move CompositorSurfaceManager interface and provide a simple implementation that uses one SV. This approach doesn't fix the issue in content but allows the embedder to provide the implementation to fix it. How does it sound? What about moving CompositorSurfaceManagerImpl to content as well?
,
May 8 2018
I'm not intimately familiar with this code in src/chrome, but content right now exposes a way to set a surface, that to me is the right API. In terms of layering, ContentViewRenderView shouldn't be in content, it's just no one has done the work to move it out into a component yet. I assume your goal for opera is to make it easier for another content embedder to reuse chrome's surface management code, not to actually make it available in content? In that case, a component would work as well. I don't know the details though, eg which pieces should be moved to the component, should ContentViewRenderView be merged with CompositorView, should chromecast switch to CompositorView too? So my opinions use a component instead of moving things into content
,
May 9 2018
ContentViewRenderView allows to set a SurfaceView once instantiated but it doesn't allow to switch between two different SVs on the fly. I'm not sure if it's possible to to implement a custom SV that manages two SVs without changing anything in ContentViewRenderView. Our goal for opera is to have one less patch to maintain. On the other hand there is obviously a bug in content_shell and we have a fix for it. Is there any plant to move ContentViewRenderView to a component? Perhaps we should wait until it gets moved.
,
May 9 2018
ContentViewRenderView is just a simpler version of CompositorSurfaceManager / CompositorView, missing some features. But I'm seeing a trend where behaviors in CompositorView are duplicated in ContentViewRenderView because chromecast is running into the same issues that chrome ran into in the past. So maybe overall should have a goal to have chromecast and content shell switch to CompositorView as well and just remove ContentViewRenderView. One step at a time though. I don't think anyone is actively moving ContentViewRenderView to a component. So feel free to send a patch.
,
May 9 2018
,
Jun 5 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/1a5f941ab400ac2008dab51327490235fa1b6e47 commit 1a5f941ab400ac2008dab51327490235fa1b6e47 Author: Attila Uygun <auygun@opera.com> Date: Tue Jun 05 09:58:01 2018 Android: Move ContentViewRenderView to the embedder support component Bug: 840360 , 617324 Change-Id: I66a3a35d5fb8df66ee003ae1be48cbb89ba848d2 Reviewed-on: https://chromium-review.googlesource.com/1070159 Reviewed-by: Avi Drissman <avi@chromium.org> Reviewed-by: Bo <boliu@chromium.org> Reviewed-by: Ted Choc <tedchoc@chromium.org> Reviewed-by: danakj <danakj@chromium.org> Reviewed-by: Luke Halliwell <halliwell@chromium.org> Commit-Queue: Attila Uygun <auygun@opera.com> Cr-Commit-Position: refs/heads/master@{#564425} [modify] https://crrev.com/1a5f941ab400ac2008dab51327490235fa1b6e47/chromecast/browser/BUILD.gn [modify] https://crrev.com/1a5f941ab400ac2008dab51327490235fa1b6e47/chromecast/browser/android/BUILD.gn [modify] https://crrev.com/1a5f941ab400ac2008dab51327490235fa1b6e47/chromecast/browser/android/apk/src/org/chromium/chromecast/shell/CastWebContentsView.java [modify] https://crrev.com/1a5f941ab400ac2008dab51327490235fa1b6e47/components/embedder_support/android/BUILD.gn [modify] https://crrev.com/1a5f941ab400ac2008dab51327490235fa1b6e47/components/embedder_support/android/DEPS [rename] https://crrev.com/1a5f941ab400ac2008dab51327490235fa1b6e47/components/embedder_support/android/java/src/org/chromium/components/embedder_support/view/ContentViewRenderView.java [rename] https://crrev.com/1a5f941ab400ac2008dab51327490235fa1b6e47/components/embedder_support/android/view/content_view_render_view.cc [rename] https://crrev.com/1a5f941ab400ac2008dab51327490235fa1b6e47/components/embedder_support/android/view/content_view_render_view.h [modify] https://crrev.com/1a5f941ab400ac2008dab51327490235fa1b6e47/content/browser/BUILD.gn [modify] https://crrev.com/1a5f941ab400ac2008dab51327490235fa1b6e47/content/public/android/BUILD.gn [modify] https://crrev.com/1a5f941ab400ac2008dab51327490235fa1b6e47/content/shell/BUILD.gn [modify] https://crrev.com/1a5f941ab400ac2008dab51327490235fa1b6e47/content/shell/android/BUILD.gn [modify] https://crrev.com/1a5f941ab400ac2008dab51327490235fa1b6e47/content/shell/android/java/src/org/chromium/content_shell/Shell.java [modify] https://crrev.com/1a5f941ab400ac2008dab51327490235fa1b6e47/content/shell/android/java/src/org/chromium/content_shell/ShellManager.java
,
Jun 19 2018
|
|||||
►
Sign in to add a comment |
|||||
Comment 1 by auy...@opera.com
, May 7 2018Labels: OS-Android