Auto merge of #23904 - paulrouget:loop2, r=gterzian

Better Hololens loop Depends on #23900 Fix #23823 r? @gterzian  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/23904)
servo · Aug 7, 2019 · 371f514 · 371f514
2 parents b6cdf93 + 20c7692
commit 371f514
Show file tree

Hide file tree

Showing 4 changed files with 166 additions and 59 deletions.
diff --git a/ports/libsimpleservo/api/src/lib.rs b/ports/libsimpleservo/api/src/lib.rs
@@ -490,7 +490,8 @@ impl ServoGlue {
                             .host_callbacks
                             .on_allow_navigation(url.to_string());
                         let window_event = WindowEvent::AllowNavigationResponse(pipeline_id, data);
-                        let _ = self.process_event(window_event);
+                        self.events.push(window_event);
+                        let _ = self.perform_updates();
                     }
                 },
                 EmbedderMsg::HistoryChanged(entries, current) => {

diff --git a/support/hololens/.clang-format b/support/hololens/.clang-format
@@ -0,0 +1,115 @@
+---
+Language:        Cpp
+BasedOnStyle:    Mozilla
+AccessModifierOffset: -2
+AlignAfterOpenBracket: Align
+AlignConsecutiveAssignments: false
+AlignConsecutiveDeclarations: false
+AlignEscapedNewlines: Right
+AlignOperands:   true
+AlignTrailingComments: true
+AllowAllParametersOfDeclarationOnNextLine: true
+AllowShortBlocksOnASingleLine: false
+AllowShortCaseLabelsOnASingleLine: false
+AllowShortFunctionsOnASingleLine: All
+AllowShortIfStatementsOnASingleLine: false
+AllowShortLoopsOnASingleLine: false
+AlwaysBreakAfterDefinitionReturnType: None
+AlwaysBreakAfterReturnType: None
+AlwaysBreakBeforeMultilineStrings: false
+AlwaysBreakTemplateDeclarations: false
+BinPackArguments: true
+BinPackParameters: true
+BraceWrapping:   
+  AfterClass:      false
+  AfterControlStatement: false
+  AfterEnum:       false
+  AfterFunction:   false
+  AfterNamespace:  false
+  AfterObjCDeclaration: false
+  AfterStruct:     false
+  AfterUnion:      false
+  AfterExternBlock: false
+  BeforeCatch:     false
+  BeforeElse:      false
+  IndentBraces:    false
+  SplitEmptyFunction: true
+  SplitEmptyRecord: true
+  SplitEmptyNamespace: true
+BreakBeforeBinaryOperators: None
+BreakBeforeBraces: Attach
+BreakBeforeInheritanceComma: false
+BreakBeforeTernaryOperators: true
+BreakConstructorInitializersBeforeComma: false
+BreakConstructorInitializers: BeforeColon
+BreakAfterJavaFieldAnnotations: false
+BreakStringLiterals: true
+ColumnLimit:     80
+CommentPragmas:  '^ IWYU pragma:'
+CompactNamespaces: false
+ConstructorInitializerAllOnOneLineOrOnePerLine: false
+ConstructorInitializerIndentWidth: 4
+ContinuationIndentWidth: 4
+Cpp11BracedListStyle: true
+DerivePointerAlignment: false
+DisableFormat:   false
+ExperimentalAutoDetectBinPacking: false
+FixNamespaceComments: true
+ForEachMacros:   
+  - foreach
+  - Q_FOREACH
+  - BOOST_FOREACH
+IncludeBlocks:   Preserve
+IncludeCategories: 
+  - Regex:           '^"(llvm|llvm-c|clang|clang-c)/'
+    Priority:        2
+  - Regex:           '^(<|"(gtest|gmock|isl|json)/)'
+    Priority:        3
+  - Regex:           '.*'
+    Priority:        1
+IncludeIsMainRegex: '(Test)?$'
+IndentCaseLabels: false
+IndentPPDirectives: None
+IndentWidth:     2
+IndentWrappedFunctionNames: false
+JavaScriptQuotes: Leave
+JavaScriptWrapImports: true
+KeepEmptyLinesAtTheStartOfBlocks: true
+MacroBlockBegin: ''
+MacroBlockEnd:   ''
+MaxEmptyLinesToKeep: 1
+NamespaceIndentation: None
+ObjCBlockIndentWidth: 2
+ObjCSpaceAfterProperty: false
+ObjCSpaceBeforeProtocolList: true
+PenaltyBreakAssignment: 2
+PenaltyBreakBeforeFirstCallParameter: 19
+PenaltyBreakComment: 300
+PenaltyBreakFirstLessLess: 120
+PenaltyBreakString: 1000
+PenaltyExcessCharacter: 1000000
+PenaltyReturnTypeOnItsOwnLine: 60
+PointerAlignment: Right
+RawStringFormats: 
+  - Delimiter:       pb
+    Language:        TextProto
+    BasedOnStyle:    google
+ReflowComments:  true
+SortIncludes:    false
+SortUsingDeclarations: true
+SpaceAfterCStyleCast: false
+SpaceAfterTemplateKeyword: true
+SpaceBeforeAssignmentOperators: true
+SpaceBeforeParens: ControlStatements
+SpaceInEmptyParentheses: false
+SpacesBeforeTrailingComments: 1
+SpacesInAngles:  false
+SpacesInContainerLiterals: true
+SpacesInCStyleCastParentheses: false
+SpacesInParentheses: false
+SpacesInSquareBrackets: false
+Standard:        Cpp11
+TabWidth:        8
+UseTab:          Never
+...
+
diff --git a/support/hololens/ServoApp/BrowserPage.cpp b/support/hololens/ServoApp/BrowserPage.cpp
@@ -22,6 +22,8 @@ namespace winrt::ServoApp::implementation {
 BrowserPage::BrowserPage() {
   log("BrowserPage::BrowserPage()");
   InitializeComponent();
+  InitializeConditionVariable(&mGLCondVar);
+  InitializeCriticalSection(&mGLLock);
   Loaded(std::bind(&BrowserPage::OnPageLoaded, this, _1, _2));
   Window::Current().CoreWindow().VisibilityChanged(
       std::bind(&BrowserPage::OnVisibilityChanged, this, _1, _2));
@@ -31,15 +33,13 @@ void BrowserPage::Shutdown() {
   log("BrowserPage::Shutdown()");
 
   if (mServo != nullptr) {
-    if (!IsLoopRunning()) {
+    if (!mLooping) {
       // FIXME: this should not happen. In that case, we can't send the
       // shutdown event to Servo.
     } else {
-      HANDLE hEvent = ::CreateEventA(nullptr, FALSE, FALSE, sShutdownEvent);
-      RunOnGLThread([=] {mServo->RequestShutdown();});
-      log("Waiting for Servo to shutdown");
-      ::WaitForSingleObject(hEvent, INFINITE);
-      StopRenderLoop();
+      RunOnGLThread([=] { mServo->RequestShutdown(); });
+      mLoopTask->wait();
+      mLoopTask.reset();
       mServo.reset();
     }
   }
@@ -63,10 +63,10 @@ void BrowserPage::OnVisibilityChanged(CoreWindow const &,
   // stopping the event loop, which we can't recover from yet (see comment in
   // Loop())
 
-  // if (visible && !IsLoopRunning()) {
+  // if (visible && !mLooping) {
   //  StartRenderLoop();
   //}
-  // if (!visible && IsLoopRunning()) {
+  // if (!visible && mLooping) {
   //  StopRenderLoop();
   //}
 }
@@ -92,15 +92,9 @@ void BrowserPage::RecoverFromLostDevice() {
 
 /**** GL THREAD LOOP ****/
 
-bool BrowserPage::IsLoopRunning() {
-  return mLoopTask != nullptr && !mLoopTask->is_done();
-}
-
-void BrowserPage::Loop(cancellation_token cancel) {
+void BrowserPage::Loop() {
   log("BrowserPage::Loop(). GL thread: %i", GetCurrentThreadId());
 
-  HANDLE hEvent = ::CreateEventA(nullptr, FALSE, FALSE, sWakeupEvent);
-
   mOpenGLES.MakeCurrent(mRenderSurface);
 
   EGLint panelWidth = 0;
@@ -118,51 +112,48 @@ void BrowserPage::Loop(cancellation_token cancel) {
     throw winrt::hresult_error(E_FAIL, L"Recovering loop unimplemented");
   }
 
-  // mServo->SetBatchMode(true);
-  // FIXME: ^ this should be necessary as we call perform_update
-  // ourself. But enabling batch mode will make clicking a link
-  // not working because during the click, this thread is not
-  // waiting on the hEvent object. See the "wakeup" comment.
-
-  log("Entering loop");
-  while (!cancel.is_canceled()) {
-    // Block until wakeup is called.
-    // Or run full speed if animating (see OnAnimatingChanged),
-    // it will endup blocking on Flush to limit rendering to 60FPS
-    if (!mAnimating) {
-      ::WaitForSingleObject(hEvent, INFINITE);
+  mServo->SetBatchMode(true);
+
+  while (true) {
+    EnterCriticalSection(&mGLLock);
+    while (mTasks.size() == 0 && !mAnimating && mLooping) {
+      SleepConditionVariableCS(&mGLCondVar, &mGLLock, INFINITE);
+    }
+    if (!mLooping) {
+      LeaveCriticalSection(&mGLLock);
+      break;
     }
-    mTasksMutex.lock();
     for (auto &&task : mTasks) {
       task();
     }
     mTasks.clear();
-    mTasksMutex.unlock();
+    LeaveCriticalSection(&mGLLock);
     mServo->PerformUpdates();
   }
-  log("Leaving loop");
   mServo->DeInit();
   cancel_current_task();
 } // namespace winrt::ServoApp::implementation
 
 void BrowserPage::StartRenderLoop() {
-  if (IsLoopRunning()) {
+  if (mLooping) {
 #if defined _DEBUG
     throw winrt::hresult_error(E_FAIL, L"GL thread is already looping");
 #else
     return;
 #endif
   }
+  mLooping = true;
   log("BrowserPage::StartRenderLoop(). UI thread: %i", GetCurrentThreadId());
-  auto token = mLoopCancel.get_token();
-  mLoopTask = std::make_unique<Concurrency::task<void>>(
-      Concurrency::create_task([=] { Loop(token); }, token));
+  auto task = Concurrency::create_task([=] { Loop(); });
+  mLoopTask = std::make_unique<Concurrency::task<void>>(task);
 }
 
 void BrowserPage::StopRenderLoop() {
-  if (IsLoopRunning()) {
-    mLoopCancel.cancel();
-    WakeUp();
+  if (mLooping) {
+    EnterCriticalSection(&mGLLock);
+    mLooping = false;
+    LeaveCriticalSection(&mGLLock);
+    WakeConditionVariable(&mGLCondVar);
     mLoopTask->wait();
     mLoopTask.reset();
   }
@@ -192,9 +183,9 @@ void BrowserPage::OnHistoryChanged(bool back, bool forward) {
 }
 
 void BrowserPage::OnShutdownComplete() {
-  log("Servo notified ShutdownComplete");
-  HANDLE hEvent = ::OpenEventA(EVENT_ALL_ACCESS, FALSE, sShutdownEvent);
-  ::SetEvent(hEvent);
+  EnterCriticalSection(&mGLLock);
+  mLooping = false;
+  LeaveCriticalSection(&mGLLock);
 }
 
 void BrowserPage::OnAlert(std::wstring message) {
@@ -225,15 +216,17 @@ void BrowserPage::Flush() {
 void BrowserPage::MakeCurrent() { mOpenGLES.MakeCurrent(mRenderSurface); }
 
 void BrowserPage::WakeUp() {
-  // FIXME: this won't work if it's triggered while the thread is not
-  // waiting. We need a better looping logic.
-  HANDLE hEvent = ::OpenEventA(EVENT_ALL_ACCESS, FALSE, sWakeupEvent);
-  ::SetEvent(hEvent);
+  RunOnGLThread([=] { });
 }
 
 bool BrowserPage::OnAllowNavigation(std::wstring) { return true; }
 
-void BrowserPage::OnAnimatingChanged(bool animating) { mAnimating = animating; }
+void BrowserPage::OnAnimatingChanged(bool animating) {
+  EnterCriticalSection(&mGLLock);
+  mAnimating = animating;
+  LeaveCriticalSection(&mGLLock);
+  WakeConditionVariable(&mGLCondVar);
+}
 
 template <typename Callable> void BrowserPage::RunOnUIThread(Callable cb) {
   swapChainPanel().Dispatcher().RunAsync(
@@ -300,10 +293,10 @@ void BrowserPage::OnSurfaceClicked(IInspectable const &,
 }
 
 void BrowserPage::RunOnGLThread(std::function<void()> task) {
-  mTasksMutex.lock();
+  EnterCriticalSection(&mGLLock);
   mTasks.push_back(task);
-  mTasksMutex.unlock();
-  WakeUp();
+  LeaveCriticalSection(&mGLLock);
+  WakeConditionVariable(&mGLCondVar);
 }
 
 } // namespace winrt::ServoApp::implementation
diff --git a/support/hololens/ServoApp/BrowserPage.h b/support/hololens/ServoApp/BrowserPage.h
@@ -11,9 +11,6 @@
 
 namespace winrt::ServoApp::implementation {
 
-static char sWakeupEvent[] = "SIGNAL_WAKEUP";
-static char sShutdownEvent[] = "SIGNAL_SHUTDOWN";
-
 struct BrowserPage : BrowserPageT<BrowserPage>,
                       public servo::ServoDelegate {
 public:
@@ -66,21 +63,22 @@ struct BrowserPage : BrowserPageT<BrowserPage>,
 
   void StartRenderLoop();
   void StopRenderLoop();
-  void Loop(Concurrency::cancellation_token);
-  bool IsLoopRunning();
+  void Loop();
 
-  Concurrency::cancellation_token_source mLoopCancel;
   std::unique_ptr<Concurrency::task<void>> mLoopTask;
   winrt::ServoApp::ImmersiveViewSource mImmersiveViewSource;
   EGLSurface mRenderSurface{EGL_NO_SURFACE};
   std::unique_ptr<servo::Servo> mServo;
 
   std::vector<std::function<void()>> mTasks;
-  std::mutex mTasksMutex;
 
-  OpenGLES mOpenGLES; // FIXME: shared pointer
+  CRITICAL_SECTION mGLLock;
+  CONDITION_VARIABLE mGLCondVar;
 
-  bool mAnimating;
+  bool mAnimating = false;
+  bool mLooping = false;
+
+  OpenGLES mOpenGLES; // FIXME: shared pointer
 };
 } // namespace winrt::ServoApp::implementation