Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Started providing the GPU sync switch to Rasterizer.DrawToSurface() #28383

Merged
gaaclarke merged 14 commits into from
Sep 1, 2021
Merged

Started providing the GPU sync switch to Rasterizer.DrawToSurface() #28383

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
4fd366f
Started providing the GPU sync switch to Rasterizer.Draw()
ColdPaleLight Aug 31, 2021
b85350e
Return early when raster status is failed
ColdPaleLight Aug 31, 2021
d1875bd
Make SyncSwitch recursive
ColdPaleLight Aug 31, 2021
4816654
Replace IsAllowDrawingToSurfaceWhenGpuDisabled with AllowsDrawingWhen…
ColdPaleLight Aug 31, 2021
01ee198
Remove early return in DoDraw
ColdPaleLight Aug 31, 2021
290836f
Move sync switch to DrawToSurface
ColdPaleLight Sep 1, 2021
4d1b9b5
Revert "started providing the GPU sync switch to external view embedd…
ColdPaleLight Sep 1, 2021
93eaa37
Add rasterizer unit tests
ColdPaleLight Sep 1, 2021
314e22f
Update FlatlandExternalViewEmbedder
ColdPaleLight Sep 1, 2021
cfd563f
Format rasterizer_unittests.cc
ColdPaleLight Sep 1, 2021
7c1f729
Revert "Make SyncSwitch recursive"
ColdPaleLight Sep 1, 2021
d4b500c
Let GetIsGpuDisabledSyncSwitch will return is_gpu_disabled_sync_switc…
ColdPaleLight Sep 1, 2021
3422430
Delete unused code
ColdPaleLight Sep 1, 2021
60260aa
Tweaked the tests to assert the frame is discarded and added some doc…
gaaclarke Sep 1, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions flow/surface.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,4 +18,8 @@ bool Surface::ClearRenderContext() {
return false;
}

bool Surface::IsAllowDrawingToSurfaceWhenGpuDisabled() const {
return true;
}

} // namespace flutter
2 changes: 2 additions & 0 deletions flow/surface.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,8 @@ class Surface {

virtual bool ClearRenderContext();

virtual bool IsAllowDrawingToSurfaceWhenGpuDisabled() const;

private:
FML_DISALLOW_COPY_AND_ASSIGN(Surface);
};
Expand Down
2 changes: 1 addition & 1 deletion fml/synchronization/sync_switch.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ class SyncSwitch {
void SetSwitch(bool value);

private:
mutable std::mutex mutex_;
mutable std::recursive_mutex mutex_;
Copy link
Member

@gaaclarke gaaclarke Aug 31, 2021

Choose a reason for hiding this comment

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

Is there a reason this is recursive? I'd rather not make it recursive. I don't think anyone under DoDraw should be executing the sync switch, no?

Copy link
Member Author

@ColdPaleLight ColdPaleLight Aug 31, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Replaced here with std::recursive_mutex is to let Scenario App Integration Tests pass，This is the test result before the replacement：https://logs.chromium.org/logs/flutter/buildbucket/cr-buildbucket/8837385340239046353/+/u/Scenario_App_Integration_Tests/stdout?format=raw.
The reason is that external view embedders also use SyncSwitch. #22302
So there are two options.

  1. Use std::recursive_mutex instead of std::mutex. This solution has a small workload but may have poor performance
  2. Remove the SyncSwitch used by external view embedders. This solution may have some workload

Please let me know if you prefer the second option, my original plan was to land this PR first, and then see if we can remove the SyncSwitch in the external view embedders.

Copy link
Member

@gaaclarke gaaclarke Aug 31, 2021

Choose a reason for hiding this comment

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

I think we should do the second thing, remove the sync switch from the external view embedders, this lock supersedes that one.

Rasterizer::Draw
  -> Rasterizer::DoDraw
    -> Rasterizer::DrawToSurface
      -> FlutterPlatformViewsController::SubmitFrame
        -> SyncSwitch::Execute

However look at Rasterizer::DrawLastLayerTree, potentially that is using the sync switch since that calls Rasterizer::DrawToSurface outside of Rasterizer::Draw

Rasterizer::DrawLastLayerTree
  -> Rasterizer::DrawToSurface
    -> FlutterPlatformViewsController::SubmitFrame
      -> SyncSwitch::Execute

That means we should use the sync switch in Rasterizer::DrawToSurface instead of Rasterizer::Draw. Instead of removing or adding a sync switch, we can think of this as just moving the existing one in FlutterPlatformViewsController up one more layer. From the stack traces we've seen this should be a good place to have it still.

Copy link
Member Author

@ColdPaleLight ColdPaleLight Aug 31, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Currently, if we are in the background, the RasterStatus we return is RasterStatus::kDiscarded.I can be sure that there is no problem, because the following code also returns RasterStatus::kDiscarded in Draw.

 if (discardCallback(*layer_tree.get())) {
   raster_status = RasterStatus::kDiscarded;
 }

But if we move sync switch to Rasterizer::DrawToSurface, and in the background in iOS we retuns RasterStatus::kDiscarded from Rasterizer::DrawToSurface, how should Rasterizer::DoDraw handle it?
https://github.com/flutter/engine/blob/master/shell/common/rasterizer.cc#L380-L388

Copy link
Member Author

@ColdPaleLight ColdPaleLight Aug 31, 2021

Choose a reason for hiding this comment

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

RasterStatus::kDiscarded is Added by #21179.

Copy link
Member

@gaaclarke gaaclarke Aug 31, 2021

Choose a reason for hiding this comment

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

I'd return kDiscarded from DrawToSurface and in DoDraw expand the branch that checks return codes to early out like so:

  RasterStatus raster_status =
      DrawToSurface(*frame_timings_recorder, *layer_tree);
  if (raster_status == RasterStatus::kSuccess) {
    last_layer_tree_ = std::move(layer_tree);
  } else if (raster_status == RasterStatus::kResubmit ||
             raster_status == RasterStatus::kSkipAndRetry) {
    resubmitted_layer_tree_ = std::move(layer_tree);
    return raster_status;
  } else if (raster_status = RasterStatus::kDiscarded) {
    return raster_status;
  }

I think that should be good.

Copy link
Member Author

@ColdPaleLight ColdPaleLight Aug 31, 2021

Choose a reason for hiding this comment

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

I got it. Another problem is that we need to wrap a method outside DrawToSurface to handle sync switch (such as DrawToSurfaceWrapper), DoDraw and DrawLastLayerTree should call DrawToSurfaceWrapper instead of DrawToSurface . Here I need a good method name because I am not a native speaker.

Copy link
Member

@gaaclarke gaaclarke Aug 31, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

"Wrapper" isn't that good since it doesn't tell you anything interesting about the code's semantics, just how it is structured.

How about his:

RasterStatus DrawToSurface() {
  RasterStatus result;
  ...
  IsGpuDisabledSyncSwitch().execute(Handlers()
    .SetIfTrue([]{result = kDiscarded;})
    .SetIfFalse([]{result = DrawToSurfaceUnSafe()});
  ...
}

We use the "UnSafe" postfix in other code in the engine to denote the context with which it should be used is enforced higher in the call stack.

Is that was you were asking about?

Copy link
Member Author

@ColdPaleLight ColdPaleLight Aug 31, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Is that was you were asking about?

Yes, I got it

bool value_;

FML_DISALLOW_COPY_AND_ASSIGN(SyncSwitch);
Expand Down
32 changes: 32 additions & 0 deletions fml/synchronization/sync_switch_unittest.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,38 @@ TEST(SyncSwitchTest, Basic) {
EXPECT_TRUE(switchValue);
}

TEST(SyncSwitchTest, Recursive) {
SyncSwitch syncSwitch;
bool switchValue = false;
syncSwitch.Execute(
SyncSwitch::Handlers()
.SetIfTrue([&] {
syncSwitch.Execute(SyncSwitch::Handlers()
.SetIfTrue([&] { switchValue = true; })
.SetIfFalse([&] { switchValue = false; }));
})
.SetIfFalse([&] {
syncSwitch.Execute(SyncSwitch::Handlers()
.SetIfTrue([&] { switchValue = true; })
.SetIfFalse([&] { switchValue = false; }));
}));
EXPECT_FALSE(switchValue);
syncSwitch.SetSwitch(true);
syncSwitch.Execute(
SyncSwitch::Handlers()
.SetIfTrue([&] {
syncSwitch.Execute(SyncSwitch::Handlers()
.SetIfTrue([&] { switchValue = true; })
.SetIfFalse([&] { switchValue = false; }));
})
.SetIfFalse([&] {
syncSwitch.Execute(SyncSwitch::Handlers()
.SetIfTrue([&] { switchValue = true; })
.SetIfFalse([&] { switchValue = false; }));
}));
EXPECT_TRUE(switchValue);
}

TEST(SyncSwitchTest, NoopIfUndefined) {
SyncSwitch syncSwitch;
bool switchValue = false;
Expand Down
20 changes: 18 additions & 2 deletions shell/common/rasterizer.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -172,8 +172,24 @@ void Rasterizer::Draw(
if (discardCallback(*layer_tree.get())) {
raster_status = RasterStatus::kDiscarded;
} else {
raster_status =
DoDraw(std::move(frame_timings_recorder), std::move(layer_tree));
if (!layer_tree || !surface_) {
Copy link
Member

@gaaclarke gaaclarke Aug 31, 2021

Choose a reason for hiding this comment

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

Leave this to DoDraw, this creates an early return that didn't exist previously.

Copy link
Member Author

@ColdPaleLight ColdPaleLight Aug 31, 2021

Choose a reason for hiding this comment

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

The early return here is used to ensure that surface_ is not null pointer, because we need to use it right away

if (surface_->AllowsDrawingWhenGpuDisabled()) {
  raster_status = DoDraw(std::move(frame_timings_recorder),
                         std::move(layer_tree));

Copy link
Member

@gaaclarke gaaclarke Aug 31, 2021

Choose a reason for hiding this comment

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

Good call, lets change it to:

if (!layer_tree || !surface_) {
  raster_status = RasterStatus::kFailed;
} else if (surface_->AllowsDrawingWhenGpuDisabled()) {
  raster_status = DoDraw(...);
} else {
  ...
}

This removes the new early out. You can remove the check inside of DoDraw too now that it is here (replace it with asserts is probably better).

Copy link
Member Author

@ColdPaleLight ColdPaleLight Aug 31, 2021

Choose a reason for hiding this comment

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

Done. I’m not sure if assertions can be used here, so I’ll just keep the original logic unchanged.

Copy link
Member

@gaaclarke gaaclarke Aug 31, 2021

Choose a reason for hiding this comment

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

(although this becomes moot if we make the change I just suggested in the other comment, moving the sync switch to DrawToSurface)

raster_status = RasterStatus::kFailed;
return;
}

if (surface_->IsAllowDrawingToSurfaceWhenGpuDisabled()) {
raster_status = DoDraw(std::move(frame_timings_recorder),
std::move(layer_tree));
} else {
delegate_.GetIsGpuDisabledSyncSwitch()->Execute(
fml::SyncSwitch::Handlers()
.SetIfTrue(
[&] { raster_status = RasterStatus::kDiscarded; })
.SetIfFalse([&] {
raster_status = DoDraw(std::move(frame_timings_recorder),
std::move(layer_tree));
}));
}
}
};

Expand Down
5 changes: 5 additions & 0 deletions shell/gpu/gpu_surface_gl.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -318,4 +318,9 @@ bool GPUSurfaceGL::ClearRenderContext() {
return delegate_->GLContextClearCurrent();
}

// |Surface|
bool GPUSurfaceGL::IsAllowDrawingToSurfaceWhenGpuDisabled() const {
return delegate_->IsAllowDrawingToSurfaceWhenGpuDisabled();
}

} // namespace flutter
3 changes: 3 additions & 0 deletions shell/gpu/gpu_surface_gl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,9 @@ class GPUSurfaceGL : public Surface {
// |Surface|
bool ClearRenderContext() override;

// |Surface|
bool IsAllowDrawingToSurfaceWhenGpuDisabled() const override;

private:
GPUSurfaceGLDelegate* delegate_;
sk_sp<GrDirectContext> context_;
Expand Down
4 changes: 4 additions & 0 deletions shell/gpu/gpu_surface_gl_delegate.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,4 +99,8 @@ GPUSurfaceGLDelegate::GetDefaultPlatformGLInterface() {
return CreateGLInterface(nullptr);
}

bool GPUSurfaceGLDelegate::IsAllowDrawingToSurfaceWhenGpuDisabled() const {
return true;
}

} // namespace flutter
3 changes: 3 additions & 0 deletions shell/gpu/gpu_surface_gl_delegate.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,9 @@ class GPUSurfaceGLDelegate {
// instrumentation to specific GL calls can specify custom GL functions
// here.
virtual GLProcResolver GetGLProcResolver() const;

// Whether to allow drawing to the surface when the GPU is disabled
virtual bool IsAllowDrawingToSurfaceWhenGpuDisabled() const;
};

} // namespace flutter
Expand Down
3 changes: 3 additions & 0 deletions shell/gpu/gpu_surface_metal.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,9 @@ class SK_API_AVAILABLE_CA_METAL_LAYER GPUSurfaceMetal : public Surface {
// |Surface|
std::unique_ptr<GLContextResult> MakeRenderContextCurrent() override;

// |Surface|
bool IsAllowDrawingToSurfaceWhenGpuDisabled() const override;

std::unique_ptr<SurfaceFrame> AcquireFrameFromCAMetalLayer(
const SkISize& frame_info);

Expand Down
4 changes: 4 additions & 0 deletions shell/gpu/gpu_surface_metal.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -184,6 +184,10 @@
return std::make_unique<GLContextDefaultResult>(true);
}

bool GPUSurfaceMetal::IsAllowDrawingToSurfaceWhenGpuDisabled() const {
return delegate_->IsAllowDrawingToSurfaceWhenGpuDisabled();
}

void GPUSurfaceMetal::ReleaseUnusedDrawableIfNecessary() {
// If the previous surface frame was not submitted before a new one is acquired, the old drawable
// needs to be released. An RAII wrapper may not be used because this needs to interoperate with
Expand Down
4 changes: 4 additions & 0 deletions shell/gpu/gpu_surface_metal_delegate.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,4 +16,8 @@ MTLRenderTargetType GPUSurfaceMetalDelegate::GetRenderTargetType() {
return render_target_type_;
}

bool GPUSurfaceMetalDelegate::IsAllowDrawingToSurfaceWhenGpuDisabled() const {
return true;
}

} // namespace flutter
5 changes: 5 additions & 0 deletions shell/gpu/gpu_surface_metal_delegate.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,11 @@ class GPUSurfaceMetalDelegate {
///
virtual bool PresentTexture(GPUMTLTextureInfo texture) const = 0;

//------------------------------------------------------------------------------
/// @brief Whether to allow drawing to the surface when the GPU is disabled
///
virtual bool IsAllowDrawingToSurfaceWhenGpuDisabled() const;

MTLRenderTargetType GetRenderTargetType();

private:
Expand Down
3 changes: 3 additions & 0 deletions shell/platform/darwin/ios/ios_surface_gl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,9 @@ class IOSSurfaceGL final : public IOSSurface, public GPUSurfaceGLDelegate {
// |GPUSurfaceGLDelegate|
bool SurfaceSupportsReadback() const override;

// |GPUSurfaceGLDelegate|
bool IsAllowDrawingToSurfaceWhenGpuDisabled() const override;

private:
std::unique_ptr<IOSRenderTargetGL> render_target_;

Expand Down
5 changes: 5 additions & 0 deletions shell/platform/darwin/ios/ios_surface_gl.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,4 +89,9 @@
return IsValid() && render_target_->PresentRenderBuffer();
}

// |GPUSurfaceGLDelegate|
bool IOSSurfaceGL::IsAllowDrawingToSurfaceWhenGpuDisabled() const {
return false;
}

} // namespace flutter
3 changes: 3 additions & 0 deletions shell/platform/darwin/ios/ios_surface_metal.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,9 @@ class SK_API_AVAILABLE_CA_METAL_LAYER IOSSurfaceMetal final : public IOSSurface,
// |GPUSurfaceMetalDelegate|
bool PresentTexture(GPUMTLTextureInfo texture) const override;

// |GPUSurfaceMetalDelegate|
bool IsAllowDrawingToSurfaceWhenGpuDisabled() const override;

FML_DISALLOW_COPY_AND_ASSIGN(IOSSurfaceMetal);
};

Expand Down
5 changes: 5 additions & 0 deletions shell/platform/darwin/ios/ios_surface_metal.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,4 +98,9 @@
return false;
}

// |GPUSurfaceMetalDelegate|
bool IOSSurfaceMetal::IsAllowDrawingToSurfaceWhenGpuDisabled() const {
return false;
}

} // namespace flutter