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

[android] set presentation time via eglPresentationTimeANDROID #33881

Merged
merged 3 commits into from
Jun 8, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
5 changes: 5 additions & 0 deletions flow/surface_frame.h
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
#include "flutter/common/graphics/gl_context_switch.h"
#include "flutter/display_list/display_list_canvas_recorder.h"
#include "flutter/fml/macros.h"
#include "flutter/fml/time/time_point.h"
#include "third_party/skia/include/core/SkCanvas.h"
#include "third_party/skia/include/core/SkSurface.h"

Expand Down Expand Up @@ -72,6 +73,10 @@ class SurfaceFrame {
//
// Corresponds to EGL_KHR_partial_update
std::optional<SkIRect> buffer_damage;

// Time at which this frame is scheduled to be presented. This is a hint
// that can be passed to the platform to drop queued frames.
std::optional<fml::TimePoint> presentation_time;
};

bool Submit();
Expand Down
8 changes: 8 additions & 0 deletions shell/common/rasterizer.cc
Original file line number Diff line number Diff line change
Expand Up @@ -604,6 +604,14 @@ RasterStatus Rasterizer::DrawToSurfaceUnsafe(
}

SurfaceFrame::SubmitInfo submit_info;
// TODO (https://github.com/flutter/flutter/issues/105596): this can be in
// the past and might need to get snapped to future as this frame could have
// been resubmitted. `presentation_time` on `submit_info` is not set in this
// case.
const auto presentation_time = frame_timings_recorder.GetVsyncTargetTime();
if (presentation_time > fml::TimePoint::Now()) {
submit_info.presentation_time = presentation_time;
}
if (damage) {
submit_info.frame_damage = damage->GetFrameDamage();
submit_info.buffer_damage = damage->GetBufferDamage();
Expand Down
151 changes: 150 additions & 1 deletion shell/common/rasterizer_unittests.cc
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
#include "flutter/shell/common/rasterizer.h"

#include <memory>
#include <optional>

#include "flutter/flow/frame_timings.h"
#include "flutter/fml/synchronization/count_down_latch.h"
Expand Down Expand Up @@ -777,7 +778,7 @@ TEST(RasterizerTest,
framebuffer_info.supports_readback = true;
return std::make_unique<SurfaceFrame>(
/*surface=*/nullptr, framebuffer_info,
/*submit_callback=*/[](const SurfaceFrame&, SkCanvas*) {
/*submit_callback=*/[](const SurfaceFrame& frame, SkCanvas*) {
return true;
});
}));
Expand Down Expand Up @@ -830,4 +831,152 @@ TEST(RasterizerTest,
latch.Wait();
}

TEST(RasterizerTest, presentationTimeSetWhenVsyncTargetInFuture) {
std::string test_name =
::testing::UnitTest::GetInstance()->current_test_info()->name();
ThreadHost thread_host("io.flutter.test." + test_name + ".",
ThreadHost::Type::Platform | ThreadHost::Type::RASTER |
ThreadHost::Type::IO | ThreadHost::Type::UI);
TaskRunners task_runners("test", thread_host.platform_thread->GetTaskRunner(),
thread_host.raster_thread->GetTaskRunner(),
thread_host.ui_thread->GetTaskRunner(),
thread_host.io_thread->GetTaskRunner());
MockDelegate delegate;
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: use NiceMock<MockDelegate> (using ::testing::NiceMock) so this doesn't spam output about unused calls, here and elsewhere.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nice! flutter/flutter#105631, i think we want to use it everywhere in this file.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm updating the rest of the file in #33890 which is currently stuck on Fuchsia weirdness.

ON_CALL(delegate, GetTaskRunners()).WillByDefault(ReturnRef(task_runners));

fml::AutoResetWaitableEvent latch;
std::unique_ptr<Rasterizer> rasterizer;
thread_host.raster_thread->GetTaskRunner()->PostTask([&] {
rasterizer = std::make_unique<Rasterizer>(delegate);
latch.Signal();
});
latch.Wait();

const auto millis_16 = fml::TimeDelta::FromMilliseconds(16);
const auto first_timestamp = fml::TimePoint::Now() + millis_16;
auto second_timestamp = first_timestamp + millis_16;
std::vector<fml::TimePoint> timestamps = {first_timestamp, second_timestamp};

int frames_submitted = 0;
fml::CountDownLatch submit_latch(2);
auto surface = std::make_unique<MockSurface>();
ON_CALL(*surface, AllowsDrawingWhenGpuDisabled()).WillByDefault(Return(true));
ON_CALL(*surface, AcquireFrame(SkISize()))
.WillByDefault(::testing::Invoke([&] {
SurfaceFrame::FramebufferInfo framebuffer_info;
framebuffer_info.supports_readback = true;
return std::make_unique<SurfaceFrame>(
/*surface=*/nullptr, framebuffer_info,
/*submit_callback=*/[&](const SurfaceFrame& frame, SkCanvas*) {
const auto pres_time = *frame.submit_info().presentation_time;
const auto diff = pres_time - first_timestamp;
int num_frames_submitted = frames_submitted++;
EXPECT_EQ(diff.ToMilliseconds(),
num_frames_submitted * millis_16.ToMilliseconds());
submit_latch.CountDown();
return true;
});
}));

ON_CALL(*surface, MakeRenderContextCurrent())
.WillByDefault(::testing::Invoke(
[] { return std::make_unique<GLContextDefaultResult>(true); }));

thread_host.raster_thread->GetTaskRunner()->PostTask([&] {
rasterizer->Setup(std::move(surface));
auto pipeline = std::make_shared<LayerTreePipeline>(/*depth=*/10);
for (int i = 0; i < 2; i++) {
auto layer_tree =
std::make_unique<LayerTree>(/*frame_size=*/SkISize(),
/*device_pixel_ratio=*/2.0f);
auto layer_tree_item = std::make_unique<LayerTreeItem>(
std::move(layer_tree), CreateFinishedBuildRecorder(timestamps[i]));
PipelineProduceResult result =
pipeline->Produce().Complete(std::move(layer_tree_item));
EXPECT_TRUE(result.success);
EXPECT_EQ(result.is_first_item, i == 0);
}
auto no_discard = [](LayerTree&) { return false; };
// Although we only call 'Rasterizer::Draw' once, it will be called twice
// finally because there are two items in the pipeline.
rasterizer->Draw(pipeline, no_discard);
});

submit_latch.Wait();
thread_host.raster_thread->GetTaskRunner()->PostTask([&] {
rasterizer.reset();
latch.Signal();
});
latch.Wait();
}

TEST(RasterizerTest, presentationTimeNotSetWhenVsyncTargetInPast) {
std::string test_name =
::testing::UnitTest::GetInstance()->current_test_info()->name();
ThreadHost thread_host("io.flutter.test." + test_name + ".",
ThreadHost::Type::Platform | ThreadHost::Type::RASTER |
ThreadHost::Type::IO | ThreadHost::Type::UI);
TaskRunners task_runners("test", thread_host.platform_thread->GetTaskRunner(),
thread_host.raster_thread->GetTaskRunner(),
thread_host.ui_thread->GetTaskRunner(),
thread_host.io_thread->GetTaskRunner());
MockDelegate delegate;
ON_CALL(delegate, GetTaskRunners()).WillByDefault(ReturnRef(task_runners));

fml::AutoResetWaitableEvent latch;
std::unique_ptr<Rasterizer> rasterizer;
thread_host.raster_thread->GetTaskRunner()->PostTask([&] {
rasterizer = std::make_unique<Rasterizer>(delegate);
latch.Signal();
});
latch.Wait();

const auto millis_16 = fml::TimeDelta::FromMilliseconds(16);
const auto first_timestamp = fml::TimePoint::Now() - millis_16;

fml::CountDownLatch submit_latch(1);
auto surface = std::make_unique<MockSurface>();
ON_CALL(*surface, AllowsDrawingWhenGpuDisabled()).WillByDefault(Return(true));
ON_CALL(*surface, AcquireFrame(SkISize()))
.WillByDefault(::testing::Invoke([&] {
SurfaceFrame::FramebufferInfo framebuffer_info;
framebuffer_info.supports_readback = true;
return std::make_unique<SurfaceFrame>(
/*surface=*/nullptr, framebuffer_info,
/*submit_callback=*/[&](const SurfaceFrame& frame, SkCanvas*) {
const std::optional<fml::TimePoint> pres_time =
frame.submit_info().presentation_time;
EXPECT_EQ(pres_time, std::nullopt);
submit_latch.CountDown();
return true;
});
}));

ON_CALL(*surface, MakeRenderContextCurrent())
.WillByDefault(::testing::Invoke(
[] { return std::make_unique<GLContextDefaultResult>(true); }));

thread_host.raster_thread->GetTaskRunner()->PostTask([&] {
rasterizer->Setup(std::move(surface));
auto pipeline = std::make_shared<LayerTreePipeline>(/*depth=*/10);
auto layer_tree = std::make_unique<LayerTree>(/*frame_size=*/SkISize(),
/*device_pixel_ratio=*/2.0f);
auto layer_tree_item = std::make_unique<LayerTreeItem>(
std::move(layer_tree), CreateFinishedBuildRecorder(first_timestamp));
PipelineProduceResult result =
pipeline->Produce().Complete(std::move(layer_tree_item));
EXPECT_TRUE(result.success);
EXPECT_EQ(result.is_first_item, true);
auto no_discard = [](LayerTree&) { return false; };
rasterizer->Draw(pipeline, no_discard);
});

submit_latch.Wait();
thread_host.raster_thread->GetTaskRunner()->PostTask([&] {
rasterizer.reset();
latch.Signal();
});
latch.Wait();
}

} // namespace flutter
4 changes: 4 additions & 0 deletions shell/gpu/gpu_surface_gl_delegate.h
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,10 @@ struct GLPresentInfo {
// Damage is a hint to compositor telling it which parts of front buffer
// need to be updated
const std::optional<SkIRect>& damage;

// Time at which this frame is scheduled to be presented. This is a hint
// that can be passed to the platform to drop queued frames.
std::optional<fml::TimePoint> presentation_time = std::nullopt;
};

class GPUSurfaceGLDelegate {
Expand Down
3 changes: 3 additions & 0 deletions shell/gpu/gpu_surface_gl_impeller.cc
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,9 @@ std::unique_ptr<SurfaceFrame> GPUSurfaceGLImpeller::AcquireFrame(
GLPresentInfo present_info = {
.fbo_id = 0,
.damage = std::nullopt,
// TODO (https://github.com/flutter/flutter/issues/105597): wire-up
// presentation time to impeller backend.
.presentation_time = std::nullopt,
};
delegate->GLContextPresent(present_info);
}
Expand Down
6 changes: 5 additions & 1 deletion shell/gpu/gpu_surface_gl_skia.cc
Original file line number Diff line number Diff line change
Expand Up @@ -266,7 +266,11 @@ bool GPUSurfaceGLSkia::PresentSurface(const SurfaceFrame& frame,
onscreen_surface_->getCanvas()->flush();
}

GLPresentInfo present_info = {fbo_id_, frame.submit_info().frame_damage};
GLPresentInfo present_info = {
.fbo_id = fbo_id_,
.damage = frame.submit_info().frame_damage,
.presentation_time = frame.submit_info().presentation_time,
};
if (!delegate_->GLContextPresent(present_info)) {
return false;
}
Expand Down
36 changes: 29 additions & 7 deletions shell/platform/android/android_egl_surface.cc
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,17 @@ void LogLastEGLError() {
FML_LOG(ERROR) << "Unknown EGL Error";
}

namespace {

static bool HasExtension(const char* extensions, const char* name) {
Copy link
Contributor

Choose a reason for hiding this comment

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

We have this method in like 3 or 4 places now. Or maybe we will with the swiftshader patch. We should consider moving it somewhere common

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dnfield i think this is out of scope for this PR. Will refactor this later if that is ok.

Copy link
Contributor

Choose a reason for hiding this comment

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

SGTM

const char* r = strstr(extensions, name);
auto len = strlen(name);
// check that the extension name is terminated by space or null terminator
return r != nullptr && (r[len] == ' ' || r[len] == 0);
}

} // namespace

class AndroidEGLSurfaceDamage {
public:
void init(EGLDisplay display, EGLContext context) {
Expand Down Expand Up @@ -170,13 +181,6 @@ class AndroidEGLSurfaceDamage {

bool partial_redraw_supported_;

bool HasExtension(const char* extensions, const char* name) {
const char* r = strstr(extensions, name);
auto len = strlen(name);
// check that the extension name is terminated by space or null terminator
return r != nullptr && (r[len] == ' ' || r[len] == 0);
}

std::list<SkIRect> damage_history_;
};

Expand All @@ -188,6 +192,14 @@ AndroidEGLSurface::AndroidEGLSurface(EGLSurface surface,
context_(context),
damage_(std::make_unique<AndroidEGLSurfaceDamage>()) {
damage_->init(display_, context);

const char* extensions = eglQueryString(display, EGL_EXTENSIONS);

if (HasExtension(extensions, "EGL_ANDROID_presentation_time")) {
presentation_time_proc_ =
reinterpret_cast<PFNEGLPRESENTATIONTIMEANDROIDPROC>(
eglGetProcAddress("eglPresentationTimeANDROID"));
Copy link
Contributor

Choose a reason for hiding this comment

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

For future us: this was the big part of what was missing last time.

When I tested this locally, I hacked in a defintiion of this method and saw improvements. I then refactored things to actually do the proc lookup and put the extension name instead of the method name here, which resulted in no improvements (and slight regressions!).

}
}

AndroidEGLSurface::~AndroidEGLSurface() {
Expand Down Expand Up @@ -240,6 +252,16 @@ void AndroidEGLSurface::SetDamageRegion(
damage_->SetDamageRegion(display_, surface_, buffer_damage);
}

bool AndroidEGLSurface::SetPresentationTime(
const fml::TimePoint& presentation_time) {
if (presentation_time_proc_) {
const auto time_ns = presentation_time.ToEpochDelta().ToNanoseconds();
return presentation_time_proc_(display_, surface_, time_ns);
} else {
return false;
}
}

bool AndroidEGLSurface::SwapBuffers(
const std::optional<SkIRect>& surface_damage) {
TRACE_EVENT0("flutter", "AndroidContextGL::SwapBuffers");
Expand Down
11 changes: 11 additions & 0 deletions shell/platform/android/android_egl_surface.h
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,13 @@
#ifndef FLUTTER_SHELL_PLATFORM_ANDROID_ANDROID_EGL_SURFACE_H_
#define FLUTTER_SHELL_PLATFORM_ANDROID_ANDROID_EGL_SURFACE_H_

#include <EGL/egl.h>
#include <EGL/eglext.h>
#include <KHR/khrplatform.h>
#include <optional>

#include "flutter/fml/macros.h"
#include "flutter/fml/time/time_point.h"
#include "flutter/shell/platform/android/android_environment_gl.h"
#include "third_party/skia/include/core/SkRect.h"

Expand Down Expand Up @@ -77,6 +81,12 @@ class AndroidEGLSurface {
// eglSetDamageRegionKHR
void SetDamageRegion(const std::optional<SkIRect>& buffer_damage);

//----------------------------------------------------------------------------
/// @brief Sets the presentation time for the current surface. This
// corresponds to calling eglPresentationTimeAndroid when
// available.
bool SetPresentationTime(const fml::TimePoint& presentation_time);

//----------------------------------------------------------------------------
/// @brief This only applies to on-screen surfaces such as those created
/// by `AndroidContextGL::CreateOnscreenSurface`.
Expand All @@ -98,6 +108,7 @@ class AndroidEGLSurface {
const EGLDisplay display_;
const EGLContext context_;
std::unique_ptr<AndroidEGLSurfaceDamage> damage_;
PFNEGLPRESENTATIONTIMEANDROIDPROC presentation_time_proc_;
};

} // namespace flutter
Expand Down
3 changes: 3 additions & 0 deletions shell/platform/android/android_surface_gl_skia.cc
Original file line number Diff line number Diff line change
Expand Up @@ -156,6 +156,9 @@ void AndroidSurfaceGLSkia::GLContextSetDamageRegion(
bool AndroidSurfaceGLSkia::GLContextPresent(const GLPresentInfo& present_info) {
FML_DCHECK(IsValid());
FML_DCHECK(onscreen_surface_);
if (present_info.presentation_time) {
onscreen_surface_->SetPresentationTime(*present_info.presentation_time);
}
return onscreen_surface_->SwapBuffers(present_info.damage);
}

Expand Down