New issue
Advanced search Search tips

Issue 649738 link

Starred by 2 users
Status: Verified
Owner:
boliu@chromium.org
Closed: Sep 2016
Cc:
tobiasjs@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 1
Type: Bug



Sign in to add a comment

webview ignores fbo in functor

Project Member Reported by boliu@chromium.org, Sep 23 2016 
from b/31668620

Came from this innocent-looking re-ordering (which I reviewed..) from this larger CL: https://codereview.chromium.org/2036563002/diff/180001/android_webview/browser/render_thread_manager.cc#newcode289

The reorder is required because HardwareRenderer constructor runs GL, so must be moved to after ScopedAllowGL.

Main manifestation of the bug is the first frame of a webview has a non-zero fbo, it's ignored: HardwareRenderer constructor runs GL which triggers virtual context restore, and hat comes before SetBackingFrameBufferObject, so the context restore overwrites the fbo to 0, and for that one frame, webview renders to screen rather than to the fbo.

The more subtle although less likely issue, is that ScopedAllowGL constructor can also run any pending GL as well, which may also cause state restore. Probably should address both if possible.

Comment 1 by boliu@chromium.org, Sep 23 2016 

given the ScopedAllowGL problem, I think I'm gonna write a different fix than what was merged to m53 (https://codereview.chromium.org/2361903003/)

Comment 2 Deleted

Project Member

Comment 3 by bugdroid1@chromium.org, Sep 26 2016 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/cf4731774e0d1110244be09be8cb456f83f5ce44

commit cf4731774e0d1110244be09be8cb456f83f5ce44
Author: boliu <boliu@chromium.org>
Date: Mon Sep 26 17:07:32 2016

aw: Fix FBO restore in webview functor

See bug for problem description.

Have AwGLSurface get the current FBO directly from
ScopedAppGLStateRestore. Conceptually ScopedAppGLStateRestore would be
in the ui/gl layer, so having AwGLSurface access it directly isn't
really a problem.

Seems a lot cleaner than dependency injection which involves crossing
many (conceptual) layers anyway. Global is bad, but any time GL executes
without ScopedAppGLStateRestore would indicate bigger bugs, so in
practice should be ok.

BUG= 649738 

Review-Url: https://codereview.chromium.org/2360423003
Cr-Commit-Position: refs/heads/master@{#420913}

[modify] https://crrev.com/cf4731774e0d1110244be09be8cb456f83f5ce44/android_webview/browser/aw_gl_surface.cc
[modify] https://crrev.com/cf4731774e0d1110244be09be8cb456f83f5ce44/android_webview/browser/aw_gl_surface.h
[modify] https://crrev.com/cf4731774e0d1110244be09be8cb456f83f5ce44/android_webview/browser/hardware_renderer.cc
[modify] https://crrev.com/cf4731774e0d1110244be09be8cb456f83f5ce44/android_webview/browser/hardware_renderer.h
[modify] https://crrev.com/cf4731774e0d1110244be09be8cb456f83f5ce44/android_webview/browser/parent_output_surface.cc
[modify] https://crrev.com/cf4731774e0d1110244be09be8cb456f83f5ce44/android_webview/browser/parent_output_surface.h
[modify] https://crrev.com/cf4731774e0d1110244be09be8cb456f83f5ce44/android_webview/browser/render_thread_manager.cc
[modify] https://crrev.com/cf4731774e0d1110244be09be8cb456f83f5ce44/android_webview/browser/scoped_app_gl_state_restore.cc
[modify] https://crrev.com/cf4731774e0d1110244be09be8cb456f83f5ce44/android_webview/browser/scoped_app_gl_state_restore.h
[modify] https://crrev.com/cf4731774e0d1110244be09be8cb456f83f5ce44/android_webview/browser/surfaces_instance.cc
[modify] https://crrev.com/cf4731774e0d1110244be09be8cb456f83f5ce44/android_webview/browser/surfaces_instance.h

Comment 4 by boliu@chromium.org, Sep 27 2016

Labels: Merge-Request-54

Comment 5 by dimu@chromium.org, Sep 27 2016

Labels: -Merge-Request-54 Merge-Approved-54 Hotlist-Merge-Approved
Your change meets the bar and is auto-approved for M54 (branch: 2840)

Comment 6 by boliu@chromium.org, Sep 27 2016

Labels: -Merge-Approved-54
Status: Fixed (was: Assigned)
merged: https://codereview.chromium.org/2375743002/
Project Member

Comment 7 by bugdroid1@chromium.org, Sep 28 2016

Labels: merge-merged-2840
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/ea2934399b34646b59c0e5dc5793b52847ff69b6

commit ea2934399b34646b59c0e5dc5793b52847ff69b6
Author: Bo Liu <boliu@chromium.org>
Date: Tue Sep 27 20:53:42 2016

[Merge M54] aw: Fix FBO restore in webview functor

See bug for problem description.

Have AwGLSurface get the current FBO directly from
ScopedAppGLStateRestore. Conceptually ScopedAppGLStateRestore would be
in the ui/gl layer, so having AwGLSurface access it directly isn't
really a problem.

Seems a lot cleaner than dependency injection which involves crossing
many (conceptual) layers anyway. Global is bad, but any time GL executes
without ScopedAppGLStateRestore would indicate bigger bugs, so in
practice should be ok.

BUG= 649738 

Review-Url: https://codereview.chromium.org/2360423003
Cr-Commit-Position: refs/heads/master@{#420913}
(cherry picked from commit cf4731774e0d1110244be09be8cb456f83f5ce44)

Review URL: https://codereview.chromium.org/2375743002 .

Cr-Commit-Position: refs/branch-heads/2840@{#553}
Cr-Branched-From: 1ae106dbab4bddd85132d5b75c670794311f4c57-refs/heads/master@{#414607}

[modify] https://crrev.com/ea2934399b34646b59c0e5dc5793b52847ff69b6/android_webview/browser/aw_gl_surface.cc
[modify] https://crrev.com/ea2934399b34646b59c0e5dc5793b52847ff69b6/android_webview/browser/aw_gl_surface.h
[modify] https://crrev.com/ea2934399b34646b59c0e5dc5793b52847ff69b6/android_webview/browser/hardware_renderer.cc
[modify] https://crrev.com/ea2934399b34646b59c0e5dc5793b52847ff69b6/android_webview/browser/hardware_renderer.h
[modify] https://crrev.com/ea2934399b34646b59c0e5dc5793b52847ff69b6/android_webview/browser/parent_output_surface.cc
[modify] https://crrev.com/ea2934399b34646b59c0e5dc5793b52847ff69b6/android_webview/browser/parent_output_surface.h
[modify] https://crrev.com/ea2934399b34646b59c0e5dc5793b52847ff69b6/android_webview/browser/render_thread_manager.cc
[modify] https://crrev.com/ea2934399b34646b59c0e5dc5793b52847ff69b6/android_webview/browser/scoped_app_gl_state_restore.cc
[modify] https://crrev.com/ea2934399b34646b59c0e5dc5793b52847ff69b6/android_webview/browser/scoped_app_gl_state_restore.h
[modify] https://crrev.com/ea2934399b34646b59c0e5dc5793b52847ff69b6/android_webview/browser/surfaces_instance.cc
[modify] https://crrev.com/ea2934399b34646b59c0e5dc5793b52847ff69b6/android_webview/browser/surfaces_instance.h

Comment 8 by aluo@chromium.org, Sep 28 2016

Status: Verified (was: Fixed)
Verified on Marlin in NME90 with MonoChrome version 54.0.2840.42

Comment 9 by ppolise...@chromium.org, Sep 30 2016 

Verified on Sailfish in NME91D, Monochrome: 55.0.2875.4
Project Member

Comment 10 by bugdroid1@chromium.org, Oct 27 2016 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/ea2934399b34646b59c0e5dc5793b52847ff69b6

commit ea2934399b34646b59c0e5dc5793b52847ff69b6
Author: Bo Liu <boliu@chromium.org>
Date: Tue Sep 27 20:53:42 2016

[Merge M54] aw: Fix FBO restore in webview functor

See bug for problem description.

Have AwGLSurface get the current FBO directly from
ScopedAppGLStateRestore. Conceptually ScopedAppGLStateRestore would be
in the ui/gl layer, so having AwGLSurface access it directly isn't
really a problem.

Seems a lot cleaner than dependency injection which involves crossing
many (conceptual) layers anyway. Global is bad, but any time GL executes
without ScopedAppGLStateRestore would indicate bigger bugs, so in
practice should be ok.

BUG= 649738 

Review-Url: https://codereview.chromium.org/2360423003
Cr-Commit-Position: refs/heads/master@{#420913}
(cherry picked from commit cf4731774e0d1110244be09be8cb456f83f5ce44)

Review URL: https://codereview.chromium.org/2375743002 .

Cr-Commit-Position: refs/branch-heads/2840@{#553}
Cr-Branched-From: 1ae106dbab4bddd85132d5b75c670794311f4c57-refs/heads/master@{#414607}

[modify] https://crrev.com/ea2934399b34646b59c0e5dc5793b52847ff69b6/android_webview/browser/aw_gl_surface.cc
[modify] https://crrev.com/ea2934399b34646b59c0e5dc5793b52847ff69b6/android_webview/browser/aw_gl_surface.h
[modify] https://crrev.com/ea2934399b34646b59c0e5dc5793b52847ff69b6/android_webview/browser/hardware_renderer.cc
[modify] https://crrev.com/ea2934399b34646b59c0e5dc5793b52847ff69b6/android_webview/browser/hardware_renderer.h
[modify] https://crrev.com/ea2934399b34646b59c0e5dc5793b52847ff69b6/android_webview/browser/parent_output_surface.cc
[modify] https://crrev.com/ea2934399b34646b59c0e5dc5793b52847ff69b6/android_webview/browser/parent_output_surface.h
[modify] https://crrev.com/ea2934399b34646b59c0e5dc5793b52847ff69b6/android_webview/browser/render_thread_manager.cc
[modify] https://crrev.com/ea2934399b34646b59c0e5dc5793b52847ff69b6/android_webview/browser/scoped_app_gl_state_restore.cc
[modify] https://crrev.com/ea2934399b34646b59c0e5dc5793b52847ff69b6/android_webview/browser/scoped_app_gl_state_restore.h
[modify] https://crrev.com/ea2934399b34646b59c0e5dc5793b52847ff69b6/android_webview/browser/surfaces_instance.cc
[modify] https://crrev.com/ea2934399b34646b59c0e5dc5793b52847ff69b6/android_webview/browser/surfaces_instance.h
►

Sign in to add a comment