[Cast Remoting] Fix Timestamps Set In Sender

Currently, the timestamps set by the Remoting sender are "best guesses" rather than the actual timestamp from the original DecoderBuffer. This CL updates the timestamps to instead use the "real" values as defined by the media pipeline. Bug: 263496701 Change-Id: I6479773b2978a325b7d03345644783988456f4b8 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4160022 Reviewed-by: Jordan Bayles <jophba@chromium.org> Reviewed-by: Dale Curtis <dalecurtis@chromium.org> Commit-Queue: Ryan Keane <rwkeane@google.com> Reviewed-by: Daniel Cheng <dcheng@chromium.org> Cr-Commit-Position: refs/heads/main@{#1095872}
chromium · Jan 23, 2023 · 7edbf9c · 7edbf9c
1 parent 6a34df7
commit 7edbf9c
Show file tree

Hide file tree

Showing 19 changed files with 484 additions and 746 deletions.
diff --git a/components/cast_streaming/public/BUILD.gn b/components/cast_streaming/public/BUILD.gn
@@ -4,12 +4,9 @@
 
 import("//testing/test.gni")
 
-visibility = [ ":*" ]
-
 # TODO(crbug.com/1357085): Determine whether we need all these separate targets.
 
 source_set("public") {
-  visibility += [ "*" ]
   public = [
     "app_ids.h",
     "cast_streaming_url.h",
@@ -29,12 +26,10 @@ source_set("public") {
 }
 
 source_set("demuxer_stream_traits") {
-  visibility += [ "*" ]
   public = [ "demuxer_stream_traits.h" ]
 }
 
 source_set("config_conversions") {
-  visibility += [ "*" ]
   public = [ "config_conversions.h" ]
   sources = [ "config_conversions.cc" ]
   public_deps = [
@@ -47,8 +42,22 @@ source_set("config_conversions") {
   ]
 }
 
+source_set("decoder_buffer_reader") {
+  public = [ "decoder_buffer_reader.h" ]
+  sources = [ "decoder_buffer_reader.cc" ]
+  public_deps = [
+    "//base",
+    "//media/mojo/mojom",
+    "//mojo/public/cpp/system",
+  ]
+  deps = [
+    "//media",
+    "//media/mojo/common",
+    "//mojo/public/cpp/system",
+  ]
+}
+
 source_set("remoting_utils") {
-  visibility += [ "*" ]
   public = [
     "remoting_message_factories.h",
     "remoting_proto_enum_utils.h",
@@ -74,19 +83,22 @@ source_set("remoting_utils") {
 
 source_set("unit_tests") {
   testonly = true
-  visibility += [ "//components/cast_streaming:unit_tests" ]
   public = []
   sources = [
     "config_conversions_unittest.cc",
+    "decoder_buffer_reader_unittest.cc",
     "remoting_message_factories_unittest.cc",
     "remoting_proto_utils_unittest.cc",
     "rpc_call_message_handler_unittest.cc",
   ]
   deps = [
     ":config_conversions",
+    ":decoder_buffer_reader",
     ":remoting_utils",
+    "//base/test:test_support",
     "//media",
     "//media:test_support",
+    "//media/mojo/common",
     "//testing/gmock",
     "//testing/gtest",
     "//third_party/openscreen/src/cast/streaming:remoting_proto",

diff --git a/components/cast_streaming/public/DEPS b/components/cast_streaming/public/DEPS
@@ -1,5 +1,7 @@
 include_rules = [
   "+media/base",
+  "+media/mojo",
+  "+mojo/public",
   "+third_party/openscreen/src/cast/common",
   "+third_party/openscreen/src/cast/streaming",
   "+ui/gfx/geometry",

diff --git a/...reaming/renderer/decoder_buffer_reader.cc → ...streaming/public/decoder_buffer_reader.cc b/...reaming/renderer/decoder_buffer_reader.cc → ...streaming/public/decoder_buffer_reader.cc
@@ -2,7 +2,7 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#include "components/cast_streaming/renderer/decoder_buffer_reader.h"
+#include "components/cast_streaming/public/decoder_buffer_reader.h"
 
 #include "base/functional/bind.h"
 
@@ -50,22 +50,25 @@ void DecoderBufferReader::ReadBufferAsync() {
 void DecoderBufferReader::CompletePendingRead() {
   DVLOG(3) << __func__;
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-  if (!is_read_pending_ || !current_buffer_)
+  if (!is_read_pending_ || !current_buffer_) {
     return;
+  }
 
   is_read_pending_ = false;
   const bool is_eos = current_buffer_->end_of_stream();
   new_buffer_cb_.Run(std::move(current_buffer_));
-  if (!is_eos)
+  if (!is_eos) {
     TryGetNextBuffer();
+  }
 }
 
 void DecoderBufferReader::TryGetNextBuffer() {
   DVLOG(3) << __func__;
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
 
-  if (current_buffer_ || pending_buffer_metadata_.empty())
+  if (current_buffer_ || pending_buffer_metadata_.empty()) {
     return;
+  }
 
   media::mojom::DecoderBufferPtr buffer =
       std::move(pending_buffer_metadata_.front());

diff --git a/...treaming/renderer/decoder_buffer_reader.h → ..._streaming/public/decoder_buffer_reader.h b/...treaming/renderer/decoder_buffer_reader.h → ..._streaming/public/decoder_buffer_reader.h
@@ -2,8 +2,8 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#ifndef COMPONENTS_CAST_STREAMING_RENDERER_DECODER_BUFFER_READER_H_
-#define COMPONENTS_CAST_STREAMING_RENDERER_DECODER_BUFFER_READER_H_
+#ifndef COMPONENTS_CAST_STREAMING_PUBLIC_DECODER_BUFFER_READER_H_
+#define COMPONENTS_CAST_STREAMING_PUBLIC_DECODER_BUFFER_READER_H_
 
 #include "base/containers/circular_deque.h"
 #include "base/functional/callback.h"
@@ -17,7 +17,13 @@
 namespace cast_streaming {
 
 // This class wraps functionality around reading a media::DecoderBuffer from
-// a mojo pipe.
+// a mojo pipe, while providing synchronization around the following three
+// operations:
+// - Providing the media::mojom::DecoderBufferPtr for a pending DecoderBuffer
+//   read.
+// - Reading the data for a DecoderBuffer from a |data_pipe| provided to the
+//   ctor.
+// - Requesting a new buffer callback by the embedding class.
 class DecoderBufferReader {
  public:
   using NewBufferCb =
@@ -41,7 +47,8 @@ class DecoderBufferReader {
   // call to |new_buffer_cb_| is expected.
   void ClearReadPending();
 
-  bool is_queue_empty() { return pending_buffer_metadata_.empty(); }
+  bool is_queue_empty() const { return pending_buffer_metadata_.empty(); }
+  bool is_read_pending() const { return is_read_pending_; }
 
  private:
   void TryGetNextBuffer();
@@ -63,4 +70,4 @@ class DecoderBufferReader {
 
 }  // namespace cast_streaming
 
-#endif  // COMPONENTS_CAST_STREAMING_RENDERER_DECODER_BUFFER_READER_H_
+#endif  // COMPONENTS_CAST_STREAMING_PUBLIC_DECODER_BUFFER_READER_H_
diff --git a/...enderer/decoder_buffer_reader_unittest.cc → .../public/decoder_buffer_reader_unittest.cc b/...enderer/decoder_buffer_reader_unittest.cc → .../public/decoder_buffer_reader_unittest.cc
@@ -2,7 +2,7 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#include "components/cast_streaming/renderer/decoder_buffer_reader.h"
+#include "components/cast_streaming/public/decoder_buffer_reader.h"
 
 #include "base/test/bind.h"
 #include "base/test/task_environment.h"

diff --git a/components/cast_streaming/renderer/BUILD.gn b/components/cast_streaming/renderer/BUILD.gn
@@ -85,11 +85,7 @@ source_set("buffer_requester") {
     "buffer_requester.h",
     "public/decoder_buffer_provider.h",
   ]
-  sources = [
-    "decoder_buffer_provider_impl.h",
-    "decoder_buffer_reader.cc",
-    "decoder_buffer_reader.h",
-  ]
+  sources = [ "decoder_buffer_provider_impl.h" ]
   public_deps = [
     "//base",
     "//components/cast_streaming/public:demuxer_stream_traits",
@@ -100,6 +96,7 @@ source_set("buffer_requester") {
   ]
   deps = [
     "//base",
+    "//components/cast_streaming/public:decoder_buffer_reader",
     "//media",
     "//media/mojo/common",
     "//mojo/public/cpp/system",
@@ -134,7 +131,6 @@ source_set("unit_tests") {
   sources = [
     "buffer_requester_unittest.cc",
     "decoder_buffer_provider_impl_unittest.cc",
-    "decoder_buffer_reader_unittest.cc",
     "playback_command_forwarding_renderer_factory_unittest.cc",
     "playback_command_forwarding_renderer_unittest.cc",
     "web_codecs/delegating_decoder_buffer_provider_unittest.cc",

diff --git a/components/cast_streaming/renderer/decoder_buffer_provider_impl.h b/components/cast_streaming/renderer/decoder_buffer_provider_impl.h
@@ -12,7 +12,7 @@
 #include "base/memory/scoped_refptr.h"
 #include "base/memory/weak_ptr.h"
 #include "base/task/sequenced_task_runner.h"
-#include "components/cast_streaming/renderer/decoder_buffer_reader.h"
+#include "components/cast_streaming/public/decoder_buffer_reader.h"
 #include "components/cast_streaming/renderer/public/decoder_buffer_provider.h"
 #include "media/base/decoder_buffer.h"
 #include "media/mojo/mojom/media_types.mojom-forward.h"

diff --git a/components/mirroring/service/BUILD.gn b/components/mirroring/service/BUILD.gn
@@ -49,6 +49,7 @@ component("mirroring_service") {
   deps = [
     "//build:chromeos_buildflags",
     "//components/cast_streaming/public:config_conversions",
+    "//components/cast_streaming/public:decoder_buffer_reader",
     "//components/mirroring/mojom:service",
     "//components/openscreen_platform",
     "//components/version_info",
@@ -117,6 +118,7 @@ source_set("unittests") {
     "//media/cast:net",
     "//media/cast:sender",
     "//media/cast:test_support",
+    "//media/mojo/common",
     "//media/mojo/mojom",
     "//media/mojo/mojom:remoting",
     "//mojo/public/cpp/bindings",