Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Lift restriction that embedders may not trample the render thread OpenGL context in composition callbacks. #16653

Conversation

chinmaygarde
Copy link
Member

@chinmaygarde chinmaygarde commented Feb 17, 2020

During the implementation of custom compositor integration, the embedder gets
callbacks on the render thread to prepare render targets (framebuffers,
textures, etc) for the engine to render into, callbacks to present these render
targets along with platform managed contents, and, callbacks to collect render
targets once they can no longer be recycled by the engine in subsequent frames.
During these callbacks, the engine mandates the OpenGL state on the render
thread be preserved. This restriction has been the source of hard to isolate
issues where the embedder trampled on the OpenGL bindings state in the callback
but failed to restore state before control went back to the engine. Due to the
nature of the OpenGL API, such errors are easy to make and overlook. This patch
lifts the restriction from the embedder. Embedders may now freely work with the
OpenGL state in custom compositor callbacks and the engine will make sure to
disregard OpenGL bindings when control flows back to it.

Disregarding current OpenGL state has a certain performance penalty and the
majority of this patch handles refactoring various engine embedder components
such that this happens only once per frame. The most trivial version of this
patch would reset context bindings on every transition of control flow from the
embedder to the engine. However, that naive approach would have necessitated
more than 50 binding resets in existing unit-test cases (depending on the number
of platform view interleaving levels and render target recycling hit rates). In
this implementation, bindings will be reset only once per frame and this does
not depend on the number of platform views in the scene.

The majority of this patch is a refactoring of engine subsystems used in
ExternalViewEmbedder::SubmitFrame which is thoroughly documented with each
opportunity for the embedder to invalidate OpenGL state tagged.

The refactoring also enables the implementation of the following optimizations
to engine behavior which should aid in reducing the memory needed for the
creation of render targets. These optimization include:

  • The engine will only ask the embedder for render targets in which it expects
    to render into. This was a quirk in the way in which root and non-root render
    targets were handled. The engine could require the embedder to create a render
    target but then realize it didn’t have anything to render into it. In the
    presentation callback, it would skip that render target. But the embedder
    still had to allocate that extra render target. This will no longer be the
    case and should reduce memory use.
  • The engine may now skip always realizing (via the embedder render target
    creation callback) and presenting the root render target. This was also a side
    effect of the same quirk. Previously, the engine would always ask the embedder
    to present the root render target even if it was empty. Since this is no
    longer the case, fewer render targets should be allocated which will reduce
    memory consumption.
  • The engine will now ask the embedder to collect unused render targets before
    it asks it to create new ones. The previous behavior was to ask the embedder
    for new targets and then collect old ones. This would cause spikes in memory
    use when the size of the render targets would change. These memory use spikes
    should now be troughs.
  • The previous render target cache also considered the platform view ID in cache
    viability considerations (instead of just the size of the render target). This
    was a bug which has been fixed. This should lead to better cache utilization
    in some situations.

These optimizations are now codified in unit-tests and the updated test
expectations are a result of these optimizations now being in place.

Fixes flutter/flutter#50751
Fixes flutter/flutter#46911
Fixes flutter/flutter#43778
Fixes b/146142979

…nGL context in composition callbacks.

During the implementation of custom compositor integration, the embedder gets
callbacks on the render thread to prepare render targets (framebuffers,
textures, etc) for the engine to render into, callbacks to present these render
targets along with platform managed contents, and, callbacks to collect render
targets once they can no longer be recycled by the engine in subsequent frames.
During these callbacks, the engine mandates the OpenGL state on the render
thread be preserved. This restriction has been the source of hard to isolate
issues where the embedder trampled on the OpenGL bindings state in the callback
but failed to restore state before control went back to the engine. Due to the
nature of the OpenGL API, such errors are easy to make and overlook. This patch
lifts the restriction from the embedder. Embedders may now freely work with the
OpenGL state in custom compositor callbacks and the engine will make sure to
disregard OpenGL bindings when control flows back to it.

Disregarding current OpenGL state has a certain performance penalty and the
majority of this patch handles refactoring various engine embedder components
such that this happens only once per frame. The most trivial version of this
patch would reset context bindings on every transition of control flow from the
embedder to the engine. However, that naive approach would have necessitated
more than 50 binding resets in existing unit-test cases (depending on the number
of platform view interleaving levels and render target recycling hit rates). In
this implementation, bindings will be reset only once per frame and this does
not depend on the number of platform views in the scene.

The majority of this patch is a refactoring of engine subsystems used in
`ExternalViewEmbedder::SubmitFrame` which is thoroughly documented with each
opportunity for the embedder to invalidate OpenGL state tagged.

The refactoring also enables the implementation of the following optimizations
to engine behavior which should aid in reducing the memory needed for the
creation of render targets. These optimization include:
* The engine will only ask the embedder for render targets in which it expects
  to render into. This was a quirk in the way in which root and non-root render
  targets were handled. The engine could require the embedder to create a render
  target but then realize it didn’t have anything to render into it. In the
  presentation callback, it would skip that render target. But the embedder
  still had to allocate that extra render target. This will no longer be the
  case and should reduce memory use.
* The engine may now skip always realizing (via the embedder render target
  creation callback) and presenting the root render target. This was also a side
  effect of the same quirk. Previously, the engine would always ask the embedder
  to present the root render target even if it was empty. Since this is no
  longer the case, few render targets should be allocated which will reduce
  memory consumption.
* The engine will now ask the embedder to collect unused render targets before
  it asks it to create new ones. The previous behavior was to ask the embedder
  for new targets and then collect old ones. This would cause spikes in memory
  use when the size of the render targets would change. These memory use spikes
  should now be troughs.
* The previous render target cache also considered the platform view ID in cache
  viability considerations (instead of just the size of the render target). This
  was a bug which has been fixed. This should lead to better cache utilization
  in some situations.

These optimizations are now codified in unit-tests and the updated test
expectations are a result of these optimizations now being in place.

Fixes flutter/flutter#50751 Fixes
flutter/flutter#46911 Fixes
flutter/flutter#43778 Fixes
https://buganizer.corp.google.com/issues/146142979
Copy link
Member

@jason-simmons jason-simmons left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

TRACE_EVENT0("flutter", "EmbedderExternalView::Render");

FML_DCHECK(HasEngineRenderedContents())
<< "Unnecessary asked to render into a render target when there was "
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo: should to "unnecessarily"

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

FML_DLOG(WARNING)
<< "No root canvas could be found. This is extremely unlikely and "
"indicates that the external view embedder did not receive the "
"notification to being the frame.";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo: "begin"

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@chinmaygarde chinmaygarde merged commit 110c1c9 into flutter:master Feb 18, 2020
@chinmaygarde chinmaygarde deleted the grcontext_reset_after_embedder_context branch February 18, 2020 20:51
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Feb 18, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Feb 19, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Feb 19, 2020
chinmaygarde added a commit that referenced this pull request Feb 19, 2020
…read OpenGL context in composition callbacks. (#16653)"

This reverts commit 110c1c9.
chinmaygarde added a commit that referenced this pull request Feb 19, 2020
…read OpenGL context in composition callbacks. (#16653)" (#16674)

This reverts commit 110c1c9.
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Feb 19, 2020
iskakaushik pushed a commit to flutter/flutter that referenced this pull request Feb 20, 2020
* 48d64c1 update hash code logic on the web (flutter/engine#16624)

* 110c1c9 Lift restriction that embedders may not trample the render thread OpenGL context in composition callbacks. (flutter/engine#16653)

* ca6165f Roll src/third_party/skia c1bb9cba16be..7281a8623799 (9 commits) (flutter/engine#16665)

* c264e1c Roll src/third_party/dart 999eeea5a3ff..3883c6070942 (5 commits) (flutter/engine#16666)

* 1cd1304 Control test timeouts based on debugger status or command line flags. (flutter/engine#16375)

* 9309ff5 Roll src/third_party/skia 7281a8623799..fe6fe6c5a8a8 (7 commits) (flutter/engine#16667)

* d03582d URL-encode asset URLs so assets are properly loaded (flutter/engine#16630)

* 40e3ab1 Roll fuchsia/sdk/core/mac-amd64 from 06MUz... to _jvYk... (flutter/engine#16668)

* ef9e7b1 Revert "Lift restriction that embedders may not trample the render thread OpenGL context in composition callbacks. (#16653)" (flutter/engine#16674)

* 16eeac5 Roll src/third_party/skia fe6fe6c5a8a8..799a23cf0602 (1 commits) (flutter/engine#16669)

* c796205 Roll fuchsia/sdk/core/linux-amd64 from 2W9Xr... to VHyDa... (flutter/engine#16670)

* 4f4a1d5 Roll src/third_party/dart 3883c6070942..c11c0ae3fdca (9 commits) (flutter/engine#16673)

* 80f73ee Roll src/third_party/skia 799a23cf0602..d0d033a12556 (3 commits) (flutter/engine#16675)

* 151688c Roll src/third_party/skia d0d033a12556..a037445e07a7 (4 commits) (flutter/engine#16676)

* 8e82311 Roll src/third_party/skia a037445e07a7..c5ff41f2976e (1 commits) (flutter/engine#16677)

* 84dc383 Roll src/third_party/dart c11c0ae3fdca..707ecda05e14 (1 commits) (flutter/engine#16678)
NoamDev pushed a commit to NoamDev/engine that referenced this pull request Feb 27, 2020
…nGL context in composition callbacks. (flutter#16653)

During the implementation of custom compositor integration, the embedder gets
callbacks on the render thread to prepare render targets (framebuffers,
textures, etc) for the engine to render into, callbacks to present these render
targets along with platform managed contents, and, callbacks to collect render
targets once they can no longer be recycled by the engine in subsequent frames.
During these callbacks, the engine mandates the OpenGL state on the render
thread be preserved. This restriction has been the source of hard to isolate
issues where the embedder trampled on the OpenGL bindings state in the callback
but failed to restore state before control went back to the engine. Due to the
nature of the OpenGL API, such errors are easy to make and overlook. This patch
lifts the restriction from the embedder. Embedders may now freely work with the
OpenGL state in custom compositor callbacks and the engine will make sure to
disregard OpenGL bindings when control flows back to it.

Disregarding current OpenGL state has a certain performance penalty and the
majority of this patch handles refactoring various engine embedder components
such that this happens only once per frame. The most trivial version of this
patch would reset context bindings on every transition of control flow from the
embedder to the engine. However, that naive approach would have necessitated
more than 50 binding resets in existing unit-test cases (depending on the number
of platform view interleaving levels and render target recycling hit rates). In
this implementation, bindings will be reset only once per frame and this does
not depend on the number of platform views in the scene.

The majority of this patch is a refactoring of engine subsystems used in
`ExternalViewEmbedder::SubmitFrame` which is thoroughly documented with each
opportunity for the embedder to invalidate OpenGL state tagged.

The refactoring also enables the implementation of the following optimizations
to engine behavior which should aid in reducing the memory needed for the
creation of render targets. These optimization include:
* The engine will only ask the embedder for render targets in which it expects
  to render into. This was a quirk in the way in which root and non-root render
  targets were handled. The engine could require the embedder to create a render
  target but then realize it didn’t have anything to render into it. In the
  presentation callback, it would skip that render target. But the embedder
  still had to allocate that extra render target. This will no longer be the
  case and should reduce memory use.
* The engine may now skip always realizing (via the embedder render target
  creation callback) and presenting the root render target. This was also a side
  effect of the same quirk. Previously, the engine would always ask the embedder
  to present the root render target even if it was empty. Since this is no
  longer the case, few render targets should be allocated which will reduce
  memory consumption.
* The engine will now ask the embedder to collect unused render targets before
  it asks it to create new ones. The previous behavior was to ask the embedder
  for new targets and then collect old ones. This would cause spikes in memory
  use when the size of the render targets would change. These memory use spikes
  should now be troughs.
* The previous render target cache also considered the platform view ID in cache
  viability considerations (instead of just the size of the render target). This
  was a bug which has been fixed. This should lead to better cache utilization
  in some situations.

These optimizations are now codified in unit-tests and the updated test
expectations are a result of these optimizations now being in place.

* Fixes flutter/flutter#50751
* Fixes flutter/flutter#46911
* Fixes flutter/flutter#43778
* Fixes b/146142979
NoamDev pushed a commit to NoamDev/engine that referenced this pull request Feb 27, 2020
…read OpenGL context in composition callbacks. (flutter#16653)" (flutter#16674)

This reverts commit 110c1c9.
NoamDev added a commit to NoamDev/engine that referenced this pull request Feb 27, 2020
…ender thread OpenGL context in composition callbacks. (flutter#16653)" (flutter#16674)"

This reverts commit 361b82f.
NoamDev added a commit to NoamDev/engine that referenced this pull request Feb 27, 2020
…read OpenGL context in composition callbacks. (flutter#16653)"

This reverts commit e38afca.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
3 participants