WebXR Anchors - update implementation to match the spec draft

Latest spec: https://immersive-web.github.io/anchors/

- remove last changed time from anchor object
- anchor space is not nullable on anchor object now
- anchor objects are not created in zombie state anymore - when anchor
  creation promise resolves, it should receive a fully built object
- XRAnchor got simplified due to the above
- XRFrameProvider needs to make sure that JS promises can run between
  presentation frame update and rAF callbacks so that the app has a chance
  to see newly created anchors before they are delivered to it via
  XRFrame.trackedAnchors

Changes: 
Change-Id: If582552907274d472097f7d7b119bb7ee216736e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2085701
Commit-Queue: Piotr Bialecki <bialpio@chromium.org>
Reviewed-by: Klaus Weidner <klausw@chromium.org>
Cr-Commit-Position: refs/heads/master@{#747901}

third_party/blink/renderer/modules/xr/xr_anchor.cc

@@ -10,26 +10,19 @@
 
 namespace blink {
 
-XRAnchor::XRAnchor(uint64_t id, XRSession* session)
-    : id_(id), session_(session), anchor_data_(base::nullopt) {}
-
 XRAnchor::XRAnchor(uint64_t id,
                    XRSession* session,
-                   const device::mojom::blink::XRAnchorDataPtr& anchor_data,
-                   double timestamp)
+                   const device::mojom::blink::XRAnchorDataPtr& anchor_data)
     : id_(id),
       session_(session),
-      anchor_data_(base::in_place, anchor_data, timestamp) {}
-
-void XRAnchor::Update(const device::mojom::blink::XRAnchorDataPtr& anchor_data,
-                      double timestamp) {
-  if (!anchor_data_) {
-    anchor_data_ = AnchorData(anchor_data, timestamp);
-  } else {
-    *anchor_data_->pose_matrix_ =
-        mojo::ConvertTo<blink::TransformationMatrix>(anchor_data->pose);
-    anchor_data_->last_changed_time_ = timestamp;
-  }
+      mojo_from_anchor_(std::make_unique<TransformationMatrix>(
+          mojo::ConvertTo<blink::TransformationMatrix>(anchor_data->pose))) {}
+
+void XRAnchor::Update(
+    const device::mojom::blink::XRAnchorDataPtr& anchor_data) {
+  // Just copy, already allocated.
+  *mojo_from_anchor_ =
+      mojo::ConvertTo<blink::TransformationMatrix>(anchor_data->pose);
 }
 
 uint64_t XRAnchor::id() const {
@@ -37,9 +30,7 @@ uint64_t XRAnchor::id() const {
 }
 
 XRSpace* XRAnchor::anchorSpace() const {
-  if (!anchor_data_) {
-    return nullptr;
-  }
+  DCHECK(mojo_from_anchor_);
 
   if (!anchor_space_) {
     anchor_space_ =
@@ -50,25 +41,9 @@ XRSpace* XRAnchor::anchorSpace() const {
 }
 
 TransformationMatrix XRAnchor::MojoFromObject() const {
-  if (anchor_data_) {
-    return *anchor_data_->pose_matrix_;
-  }
+  DCHECK(mojo_from_anchor_);
 
-  // |poseMatrix()| shouldn't be called by anyone except XRObjectSpace and if
-  // XRObjectSpace already exists for this anchor, then anchor_data_ should also
-  // exist for this anchor.
-  NOTREACHED();
-  return {};
-}
-
-double XRAnchor::lastChangedTime(bool& is_null) const {
-  if (!anchor_data_) {
-    is_null = true;
-    return 0;
-  }
-
-  is_null = false;
-  return anchor_data_->last_changed_time_;
+  return *mojo_from_anchor_;
 }
 
 void XRAnchor::detach() {
@@ -81,11 +56,4 @@ void XRAnchor::Trace(Visitor* visitor) {
   ScriptWrappable::Trace(visitor);
 }
 
-XRAnchor::AnchorData::AnchorData(
-    const device::mojom::blink::XRAnchorDataPtr& anchor_data,
-    double timestamp)
-    : pose_matrix_(std::make_unique<TransformationMatrix>(
-          mojo::ConvertTo<blink::TransformationMatrix>(anchor_data->pose))),
-      last_changed_time_(timestamp) {}
-
 }  // namespace blink

third_party/blink/renderer/modules/xr/xr_anchor.h

@@ -21,12 +21,9 @@ class XRAnchor : public ScriptWrappable {
   DEFINE_WRAPPERTYPEINFO();
 
  public:
-  XRAnchor(uint64_t id, XRSession* session);
-
   XRAnchor(uint64_t id,
            XRSession* session,
-           const device::mojom::blink::XRAnchorDataPtr& anchor_data,
-           double timestamp);
+           const device::mojom::blink::XRAnchorDataPtr& anchor_data);
 
   uint64_t id() const;
 
@@ -34,33 +31,24 @@ class XRAnchor : public ScriptWrappable {
 
   TransformationMatrix MojoFromObject() const;
 
-  double lastChangedTime(bool& is_null) const;
-
   void detach();
 
-  void Update(const device::mojom::blink::XRAnchorDataPtr& anchor_data,
-              double timestamp);
+  void Update(const device::mojom::blink::XRAnchorDataPtr& anchor_data);
 
   void Trace(Visitor* visitor) override;
 
  private:
-  // AnchorData will only be present in an XRAnchor after the anchor was updated
-  // for the first time (CreateAnchor returns a promise that will resolve to an
-  // XRAnchor prior to first update of the anchor).
-  struct AnchorData {
-    // Anchor's pose in device (mojo) space.
-    std::unique_ptr<TransformationMatrix> pose_matrix_;
-    double last_changed_time_;
-
-    AnchorData(const device::mojom::blink::XRAnchorDataPtr& anchor_data,
-               double timestamp);
-  };
-
   const uint64_t id_;
 
   Member<XRSession> session_;
 
-  base::Optional<AnchorData> anchor_data_;
+  // |mojo_from_anchor_| will be non-null in an XRAnchor after the anchor was
+  // updated for the first time - this *must* happen in the same frame in which
+  // the anchor was created for the anchor to be fully usable. It is currently
+  // ensured by XRSession - anchors that got created prior to receiving the
+  // result from mojo call to GetFrameData are not returned to the application
+  // until their poses are known.
+  std::unique_ptr<TransformationMatrix> mojo_from_anchor_;
 
   // Cached anchor space - it will be created by `anchorSpace()` if it's not
   // set.

third_party/blink/renderer/modules/xr/xr_anchor.idl

@@ -8,8 +8,7 @@
     RuntimeEnabled=WebXRIncubations
 ]
 interface XRAnchor {
-    readonly attribute XRSpace? anchorSpace;
-    readonly attribute DOMHighResTimeStamp? lastChangedTime;
+    readonly attribute XRSpace anchorSpace;
 
     void detach();
 };

third_party/blink/renderer/modules/xr/xr_frame_provider.cc

@@ -370,6 +370,11 @@ void XRFrameProvider::ProcessScheduledFrame(
   TRACE_EVENT2("gpu", "XRFrameProvider::ProcessScheduledFrame", "frame",
                frame_id_, "timestamp", high_res_now_ms);
 
+  LocalFrame* frame = xr_->GetFrame();
+  if (!frame) {
+    return;
+  }
+
   if (!xr_->IsFrameFocused() && !immersive_session_) {
     return;  // Not currently focused, so we won't expose poses (except to
              // immersive sessions).
@@ -429,7 +434,14 @@ void XRFrameProvider::ProcessScheduledFrame(
       immersive_session_->UpdateStageParameters(frame_data->stage_parameters);
     }
 
-    immersive_session_->OnFrame(high_res_now_ms, buffer_mailbox_holder_);
+    // Run immersive_session_->OnFrame() in a posted task to ensure that
+    // createAnchor promises get a chance to run - the presentation frame state
+    // is already updated.
+    frame->GetTaskRunner(blink::TaskType::kInternalMedia)
+        ->PostTask(FROM_HERE,
+                   WTF::Bind(&XRSession::OnFrame,
+                             WrapWeakPersistent(immersive_session_.Get()),
+                             high_res_now_ms, buffer_mailbox_holder_));
   } else {
     // In the process of fulfilling the frame requests for each session they are
     // extremely likely to request another frame. Work off of a separate list
@@ -470,7 +482,13 @@ void XRFrameProvider::ProcessScheduledFrame(
       if (session->ended())
         continue;
 
-      session->OnFrame(high_res_now_ms, base::nullopt);
+      // Run session->OnFrame() in a posted task to ensure that createAnchor
+      // promises get a chance to run - the presentation frame state is already
+      // updated.
+      frame->GetTaskRunner(blink::TaskType::kInternalMedia)
+          ->PostTask(FROM_HERE,
+                     WTF::Bind(&XRSession::OnFrame, WrapWeakPersistent(session),
+                               high_res_now_ms, base::nullopt));
     }
   }
 }

third_party/blink/renderer/modules/xr/xr_session.cc

@@ -842,9 +842,9 @@ void XRSession::OnCreateAnchorResult(ScriptPromiseResolver* resolver,
   create_anchor_promises_.erase(resolver);
 
   if (result == device::mojom::CreateAnchorResult::SUCCESS) {
-    XRAnchor* anchor = MakeGarbageCollected<XRAnchor>(id, this);
-    anchor_ids_to_anchors_.insert(id, anchor);
-    resolver->Resolve(anchor);
+    // Anchor was created successfully on the device. Subsequent frame update
+    // must contain newly created anchor data.
+    newly_created_anchor_ids_to_resolvers_.insert(id, resolver);
   } else {
     resolver->Reject();
   }
@@ -910,25 +910,36 @@ void XRSession::ProcessAnchorsData(
 
   HeapHashMap<uint64_t, Member<XRAnchor>> updated_anchors;
 
-  // First, process all planes that had their information updated (new planes
+  // First, process all anchors that had their information updated (new anchors
   // are also processed here).
   for (const auto& anchor : tracked_anchors_data->updated_anchors_data) {
     auto it = anchor_ids_to_anchors_.find(anchor->id);
     if (it != anchor_ids_to_anchors_.end()) {
       updated_anchors.insert(anchor->id, it->value);
-      it->value->Update(anchor, timestamp);
+      it->value->Update(anchor);
     } else {
-      updated_anchors.insert(
-          anchor->id,
-          MakeGarbageCollected<XRAnchor>(anchor->id, this, anchor, timestamp));
+      auto resolver_it =
+          newly_created_anchor_ids_to_resolvers_.find(anchor->id);
+      if (resolver_it == newly_created_anchor_ids_to_resolvers_.end()) {
+        DCHECK(false)
+            << "Newly created anchor must have a corresponding resolver!";
+        continue;
+      }
+
+      XRAnchor* xr_anchor =
+          MakeGarbageCollected<XRAnchor>(anchor->id, this, anchor);
+      resolver_it->value->Resolve(xr_anchor);
+      newly_created_anchor_ids_to_resolvers_.erase(resolver_it);
+
+      updated_anchors.insert(anchor->id, xr_anchor);
     }
   }
 
-  // Then, copy over the planes that were not updated but are still present.
+  // Then, copy over the anchors that were not updated but are still present.
   for (const auto& anchor_id : tracked_anchors_data->all_anchors_ids) {
     auto it_updated = updated_anchors.find(anchor_id);
 
-    // If the plane was already updated, there is nothing to do as it was
+    // If the anchor was already updated, there is nothing to do as it was
     // already moved to |updated_anchors|. Otherwise just copy it over as-is.
     if (it_updated == updated_anchors.end()) {
       auto it = anchor_ids_to_anchors_.find(anchor_id);
@@ -938,6 +949,11 @@ void XRSession::ProcessAnchorsData(
   }
 
   anchor_ids_to_anchors_.swap(updated_anchors);
+
+  DCHECK(newly_created_anchor_ids_to_resolvers_.IsEmpty())
+      << "All newly created anchors should be updated in subsequent frame, got "
+      << newly_created_anchor_ids_to_resolvers_.size()
+      << " anchors that have not been updated";
 }
 
 void XRSession::CleanUpUnusedHitTestSources() {
@@ -1834,6 +1850,7 @@ void XRSession::Trace(Visitor* visitor) {
   visitor->Trace(request_hit_test_source_promises_);
   visitor->Trace(reference_spaces_);
   visitor->Trace(anchor_ids_to_anchors_);
+  visitor->Trace(newly_created_anchor_ids_to_resolvers_);
   visitor->Trace(prev_base_layer_);
   visitor->Trace(hit_test_source_ids_to_hit_test_sources_);
   visitor->Trace(hit_test_source_ids_to_transient_input_hit_test_sources_);

third_party/blink/renderer/modules/xr/xr_session.h

@@ -396,6 +396,17 @@ class XRSession final
   bool is_tracked_anchors_null_ = true;
   HeapHashMap<uint64_t, Member<XRAnchor>> anchor_ids_to_anchors_;
 
+  // Set of promises returned from CreateAnchor that are still in-flight to the
+  // device. Once the device calls us back with the newly created anchor id, the
+  // resolver will be moved to |newly_created_anchor_ids_to_resolvers_|.
+  HeapHashSet<Member<ScriptPromiseResolver>> create_anchor_promises_;
+  // Promises for which anchors have already been created on the device side but
+  // have not yet been resolved as their data is not yet available to blink.
+  // Next frame update should contain the necessary data - the promise will be
+  // resolved then.
+  HeapHashMap<uint64_t, Member<ScriptPromiseResolver>>
+      newly_created_anchor_ids_to_resolvers_;
+
   // Mapping of hit test source ids (aka hit test subscription ids) to hit test
   // sources. Hit test source has to be stored via weak member - JavaScript side
   // will communicate that it's no longer interested in the subscription by
@@ -415,8 +426,6 @@ class XRSession final
   Member<Element> overlay_element_;
   Member<XRDOMOverlayState> dom_overlay_state_;
   bool environment_error_handler_subscribed_ = false;
-  // Set of promises returned from CreateAnchor that are still in-flight.
-  HeapHashSet<Member<ScriptPromiseResolver>> create_anchor_promises_;
   // Set of promises returned from requestHitTestSource and
   // requestHitTestSourceForTransientInput that are still in-flight.
   HeapHashSet<Member<ScriptPromiseResolver>> request_hit_test_source_promises_;

third_party/blink/web_tests/webexposed/global-interface-listing-expected.txt

@@ -11168,7 +11168,6 @@ interface XPathResult
 interface XRAnchor
     attribute @@toStringTag
     getter anchorSpace
-    getter lastChangedTime
     method constructor
     method detach
 interface XRAnchorSet

third_party/webxr_test_pages/webxr-samples/proposals/phone-ar-hit-test.html

@@ -431,10 +431,8 @@ CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.
             });
 
             tracked_anchors.forEach(anchor => {
-              if (t == anchor.lastChangedTime) {
-                anchor.context.sceneObject.matrix = frame.getPose(anchor.anchorSpace, xrRefSpace).transform.matrix;
-                anchor.context.sceneObject.visible = true;
-              }
+              anchor.context.sceneObject.matrix = frame.getPose(anchor.anchorSpace, xrRefSpace).transform.matrix;
+              anchor.context.sceneObject.visible = true;
             });
 
             all_previous_anchors = tracked_anchors;

third_party/webxr_test_pages/webxr-samples/proposals/phone-ar-plane-detection-anchors.html

@@ -526,10 +526,8 @@ CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.
           });
 
           tracked_anchors.forEach(anchor => {
-            if(t == anchor.lastChangedTime) {
-              anchor.context.sceneObject.matrix = frame.getPose(anchor.anchorSpace, xrRefSpace).transform.matrix;
-              anchor.context.sceneObject.visible = true;
-            }
+            anchor.context.sceneObject.matrix = frame.getPose(anchor.anchorSpace, xrRefSpace).transform.matrix;
+            anchor.context.sceneObject.visible = true;
           });
 
           all_previous_anchors = tracked_anchors;