Convert VideoTrackReader to use callbacks

This CL replaces VideoTrackReader's ReadableStream with an output
callback, to be passed in at construction time.

Switching VTR to use a callback aligns it with the general direction of
WebCodecs. It also works because there isn't the need for backpressure
when using getUserMedia(). Additionally, a change in constraints to the
MediaStreamTrack would mean that we might have to flush the unread frames
from ReadableStream. This shouldn't happen with a callback (but we
don't invalidate pending frames when constraints change, for now).

Bug: 1080834
Change-Id: I5ff424f9440660dadc52ba6d4d76d14b5840a6d7
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2191204
Commit-Queue: Thomas Guilbert <tguilbert@chromium.org>
Auto-Submit: Thomas Guilbert <tguilbert@chromium.org>
Reviewed-by: Dan Sanders <sandersd@chromium.org>
Cr-Commit-Position: refs/heads/master@{#770841}

third_party/blink/renderer/modules/webcodecs/video_track_reader.cc

@@ -4,12 +4,10 @@
 
 #include "third_party/blink/renderer/modules/webcodecs/video_track_reader.h"
 
+#include "base/threading/thread_task_runner_handle.h"
 #include "media/base/video_frame.h"
-#include "third_party/blink/public/web/modules/mediastream/media_stream_video_sink.h"
 #include "third_party/blink/public/web/modules/mediastream/media_stream_video_track.h"
-#include "third_party/blink/renderer/core/streams/readable_stream.h"
-#include "third_party/blink/renderer/core/streams/readable_stream_default_controller_with_script_scope.h"
-#include "third_party/blink/renderer/core/streams/underlying_source_base.h"
+#include "third_party/blink/renderer/core/execution_context/execution_context.h"
 #include "third_party/blink/renderer/modules/webcodecs/video_frame.h"
 #include "third_party/blink/renderer/platform/bindings/exception_state.h"
 #include "third_party/blink/renderer/platform/bindings/script_state.h"
@@ -18,70 +16,93 @@
 
 namespace blink {
 
-class VideoTrackReadableStreamSource final : public UnderlyingSourceBase,
-                                             public MediaStreamVideoSink {
- public:
-  VideoTrackReadableStreamSource(ScriptState* script_state,
-                                 MediaStreamTrack* track)
-      : UnderlyingSourceBase(script_state),
-        task_runner_(ExecutionContext::From(script_state)
-                         ->GetTaskRunner(TaskType::kInternalMedia)) {
-    ConnectToTrack(track->Component(),
-                   ConvertToBaseRepeatingCallback(CrossThreadBindRepeating(
-                       &VideoTrackReadableStreamSource::OnFrameFromVideoTrack,
-                       WrapCrossThreadPersistent(this))),
-                   false /* is_sink_secure */);
+VideoTrackReader::VideoTrackReader(ScriptState* script_state,
+                                   MediaStreamTrack* track)
+    : ExecutionContextLifecycleObserver(ExecutionContext::From(script_state)),
+      started_(false),
+      real_time_media_task_runner_(
+          ExecutionContext::From(script_state)
+              ->GetTaskRunner(TaskType::kInternalMediaRealTime)),
+      track_(track) {}
+
+void VideoTrackReader::start(V8VideoDecoderOutputCallback* callback,
+                             ExceptionState& exception_state) {
+  DCHECK(real_time_media_task_runner_->BelongsToCurrentThread());
+
+  if (started_) {
+    exception_state.ThrowDOMException(
+        DOMExceptionCode::kInvalidStateError,
+        "The VideoTrackReader has already been started.");
+    return;
   }
 
-  // Callback of MediaStreamVideoSink::ConnectToTrack
-  void OnFrameFromVideoTrack(scoped_refptr<media::VideoFrame> media_frame,
-                             base::TimeTicks estimated_capture_time) {
-    // The value of estimated_capture_time here seems to almost always be the
-    // system clock and most implementations of this callback ignore it.
-    // So, we will also ignore it.
-    DCHECK(media_frame);
-    PostCrossThreadTask(
-        *task_runner_.get(), FROM_HERE,
-        CrossThreadBindOnce(
-            &VideoTrackReadableStreamSource::OnFrameFromVideoTrackOnTaskRunner,
-            WrapCrossThreadPersistent(this), std::move(media_frame)));
-  }
+  started_ = true;
+  callback_ = callback;
+  ConnectToTrack(track_->Component(),
+                 ConvertToBaseRepeatingCallback(CrossThreadBindRepeating(
+                     &VideoTrackReader::OnFrameFromVideoTrack,
+                     WrapCrossThreadPersistent(this))),
+                 false /* is_sink_secure */);
+}
 
-  void OnFrameFromVideoTrackOnTaskRunner(
-      scoped_refptr<media::VideoFrame> media_frame) {
-    Controller()->Enqueue(
-        MakeGarbageCollected<VideoFrame>(std::move(media_frame)));
-  }
+void VideoTrackReader::stop(ExceptionState& exception_state) {
+  DCHECK(real_time_media_task_runner_->BelongsToCurrentThread());
 
-  // MediaStreamVideoSink override
-  void OnReadyStateChanged(WebMediaStreamSource::ReadyState state) override {
-    if (state == WebMediaStreamSource::kReadyStateEnded)
-      Close();
+  if (!started_) {
+    exception_state.ThrowDOMException(
+        DOMExceptionCode::kInvalidStateError,
+        "The VideoTrackReader has already been stopped.");
+    return;
   }
 
-  // UnderlyingSourceBase overrides.
-  ScriptPromise pull(ScriptState* script_state) override {
-    // Since video tracks are all push-based with no back pressure, there's
-    // nothing to do here.
-    return ScriptPromise::CastUndefined(script_state);
-  }
+  StopInternal();
+}
 
-  ScriptPromise Cancel(ScriptState* script_state, ScriptValue reason) override {
-    Close();
-    return ScriptPromise::CastUndefined(script_state);
-  }
+void VideoTrackReader::StopInternal() {
+  DCHECK(real_time_media_task_runner_->BelongsToCurrentThread());
+  started_ = false;
+  callback_ = nullptr;
+  DisconnectFromTrack();
+}
+
+void VideoTrackReader::OnFrameFromVideoTrack(
+    scoped_refptr<media::VideoFrame> media_frame,
+    base::TimeTicks estimated_capture_time) {
+  // The value of estimated_capture_time here seems to almost always be the
+  // system clock and most implementations of this callback ignore it.
+  // So, we will also ignore it.
+  DCHECK(media_frame);
+  PostCrossThreadTask(
+      *real_time_media_task_runner_.get(), FROM_HERE,
+      CrossThreadBindOnce(&VideoTrackReader::ExecuteCallbackOnMainThread,
+                          WrapCrossThreadPersistent(this),
+                          std::move(media_frame)));
+}
 
-  void ContextDestroyed() override { DisconnectFromTrack(); }
+void VideoTrackReader::ExecuteCallbackOnMainThread(
+    scoped_refptr<media::VideoFrame> media_frame) {
+  DCHECK(real_time_media_task_runner_->BelongsToCurrentThread());
 
-  void Close() {
-    if (Controller())
-      Controller()->Close();
-    DisconnectFromTrack();
+  if (!callback_) {
+    // We may have already been stopped.
+    return;
   }
 
-  // VideoFrames will be queue on this task runner.
-  const scoped_refptr<base::SingleThreadTaskRunner> task_runner_;
-};
+  // If |track_|'s constraints changed (e.g. the resolution changed from a call
+  // to MediaStreamTrack.applyConstraints() in JS), this |media_frame| might
+  // still have the old constraints, due to the thread hop.
+  // We may want to invalidate |media_frames| when constraints change, but it's
+  // unclear whether this is a problem for now.
+
+  callback_->InvokeAndReportException(
+      nullptr, MakeGarbageCollected<VideoFrame>(std::move(media_frame)));
+}
+
+void VideoTrackReader::OnReadyStateChanged(
+    WebMediaStreamSource::ReadyState state) {
+  if (state == WebMediaStreamSource::kReadyStateEnded)
+    StopInternal();
+}
 
 VideoTrackReader* VideoTrackReader::Create(ScriptState* script_state,
                                            MediaStreamTrack* track,
@@ -99,23 +120,14 @@ VideoTrackReader* VideoTrackReader::Create(ScriptState* script_state,
     return nullptr;
   }
 
-  auto* source =
-      MakeGarbageCollected<VideoTrackReadableStreamSource>(script_state, track);
-  auto* readable =
-      ReadableStream::CreateWithCountQueueingStrategy(script_state, source, 0);
-  return MakeGarbageCollected<VideoTrackReader>(readable);
-}
-
-VideoTrackReader::VideoTrackReader(ReadableStream* readable)
-    : readable_(readable) {}
-
-ReadableStream* VideoTrackReader::readable() const {
-  return readable_;
+  return MakeGarbageCollected<VideoTrackReader>(script_state, track);
 }
 
 void VideoTrackReader::Trace(Visitor* visitor) const {
-  visitor->Trace(readable_);
+  visitor->Trace(track_);
+  visitor->Trace(callback_);
   ScriptWrappable::Trace(visitor);
+  ExecutionContextLifecycleObserver::Trace(visitor);
 }
 
 }  // namespace blink

third_party/blink/renderer/modules/webcodecs/video_track_reader.h

@@ -5,24 +5,35 @@
 #ifndef THIRD_PARTY_BLINK_RENDERER_MODULES_WEBCODECS_VIDEO_TRACK_READER_H_
 #define THIRD_PARTY_BLINK_RENDERER_MODULES_WEBCODECS_VIDEO_TRACK_READER_H_
 
+#include "third_party/blink/public/web/modules/mediastream/media_stream_video_sink.h"
+#include "third_party/blink/renderer/bindings/modules/v8/v8_video_decoder_output_callback.h"
+#include "third_party/blink/renderer/core/execution_context/execution_context_lifecycle_observer.h"
 #include "third_party/blink/renderer/modules/mediastream/media_stream_track.h"
 #include "third_party/blink/renderer/modules/modules_export.h"
 #include "third_party/blink/renderer/platform/bindings/script_wrappable.h"
 
 namespace blink {
 
-class ReadableStream;
 class ScriptState;
 
-class MODULES_EXPORT VideoTrackReader final : public ScriptWrappable {
+class MODULES_EXPORT VideoTrackReader final
+    : public ScriptWrappable,
+      public ExecutionContextLifecycleObserver,
+      public MediaStreamVideoSink {
   DEFINE_WRAPPERTYPEINFO();
+  USING_GARBAGE_COLLECTED_MIXIN(VideoTrackReader);
 
  public:
-  static VideoTrackReader* Create(ScriptState* script_state,
-                                  MediaStreamTrack* track,
-                                  ExceptionState& exception_state);
-  explicit VideoTrackReader(ReadableStream* readable);
-  ReadableStream* readable() const;
+  static VideoTrackReader* Create(ScriptState*,
+                                  MediaStreamTrack*,
+                                  ExceptionState&);
+  VideoTrackReader(ScriptState*, MediaStreamTrack*);
+
+  // Connects |this| to |track_| and starts delivering frames via |callback_|.
+  void start(V8VideoDecoderOutputCallback*, ExceptionState&);
+
+  // Disconnects from |track_| and clears |callback_|.
+  void stop(ExceptionState&);
 
   void Trace(Visitor* visitor) const override;
 
@@ -30,7 +41,29 @@ class MODULES_EXPORT VideoTrackReader final : public ScriptWrappable {
   VideoTrackReader(const VideoTrackReader&) = delete;
   VideoTrackReader& operator=(const VideoTrackReader&) = delete;
 
-  Member<ReadableStream> readable_;
+  // ExecutionContextLifecycleObserver implementation.
+  void ContextDestroyed() override { DisconnectFromTrack(); }
+
+  // MediaStreamVideoSink implementation.
+  void OnReadyStateChanged(WebMediaStreamSource::ReadyState) override;
+
+  // Callback For MediaStreamVideoSink::ConnectToTrack.
+  void OnFrameFromVideoTrack(scoped_refptr<media::VideoFrame> media_frame,
+                             base::TimeTicks estimated_capture_time);
+
+  void StopInternal();
+
+  void ExecuteCallbackOnMainThread(
+      scoped_refptr<media::VideoFrame> media_frame);
+
+  // Whether we are connected to |track_| and using |callback_| to deliver
+  // frames.
+  bool started_;
+
+  const scoped_refptr<base::SingleThreadTaskRunner>
+      real_time_media_task_runner_;
+  Member<V8VideoDecoderOutputCallback> callback_;
+  Member<MediaStreamTrack> track_;
 };
 
 }  // namespace blink

third_party/blink/renderer/modules/webcodecs/video_track_reader.idl

@@ -10,5 +10,10 @@
 ] interface VideoTrackReader {
   [CallWith=ScriptState, RaisesException]
   constructor(MediaStreamTrack track);
-  readonly attribute ReadableStream readable; // of VideoFrame
+
+  [RaisesException]
+  void start(VideoDecoderOutputCallback callback);
+
+  [RaisesException]
+  void stop();
 };

third_party/blink/renderer/modules/webcodecs/video_track_reader_writer_test.cc

@@ -3,7 +3,9 @@
 // found in the LICENSE file.
 
 #include "base/run_loop.h"
+#include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
+#include "third_party/blink/renderer/bindings/core/v8/script_function.h"
 #include "third_party/blink/renderer/bindings/core/v8/v8_binding_for_testing.h"
 #include "third_party/blink/renderer/bindings/modules/v8/v8_video_frame.h"
 #include "third_party/blink/renderer/bindings/modules/v8/v8_video_track_writer_parameters.h"
@@ -15,10 +17,27 @@
 #include "third_party/blink/renderer/modules/webcodecs/video_track_writer.h"
 #include "third_party/blink/renderer/platform/bindings/exception_state.h"
 #include "third_party/blink/renderer/platform/bindings/script_state.h"
+#include "third_party/blink/renderer/platform/bindings/to_v8.h"
 #include "third_party/blink/renderer/platform/testing/io_task_runner_testing_platform_support.h"
 
 namespace blink {
 
+class MockFunction : public ScriptFunction {
+ public:
+  static testing::StrictMock<MockFunction>* Create(ScriptState* script_state) {
+    return MakeGarbageCollected<testing::StrictMock<MockFunction>>(
+        script_state);
+  }
+
+  v8::Local<v8::Function> Bind() { return BindToV8Function(); }
+
+  MOCK_METHOD1(Call, ScriptValue(ScriptValue));
+
+ protected:
+  explicit MockFunction(ScriptState* script_state)
+      : ScriptFunction(script_state) {}
+};
+
 class VideoTrackReaderWriterTest : public testing::Test {
  public:
   void TearDown() override {
@@ -42,6 +61,10 @@ class VideoTrackReaderWriterTest : public testing::Test {
     base::RunLoop().RunUntilIdle();
   }
 
+  V8VideoDecoderOutputCallback* GetCallback(MockFunction* function) {
+    return V8VideoDecoderOutputCallback::Create(function->Bind());
+  }
+
  private:
   ScopedTestingPlatformSupport<IOTaskRunnerTestingPlatformSupport> platform_;
 };
@@ -54,9 +77,13 @@ TEST_F(VideoTrackReaderWriterTest, WriteAndRead) {
   params.setReleaseFrames(false);
   auto* writer =
       VideoTrackWriter::Create(script_state, &params, ASSERT_NO_EXCEPTION);
+
+  auto* read_output_function = MockFunction::Create(script_state);
   auto* reader = VideoTrackReader::Create(script_state, writer->track(),
                                           ASSERT_NO_EXCEPTION);
 
+  reader->start(GetCallback(read_output_function), ASSERT_NO_EXCEPTION);
+
   auto* frame = CreateBlackVideoFrame();
   writer->writable()
       ->getWriter(script_state, ASSERT_NO_EXCEPTION)
@@ -64,24 +91,23 @@ TEST_F(VideoTrackReaderWriterTest, WriteAndRead) {
               ScriptValue(scope.GetIsolate(), ToV8(frame, script_state)),
               ASSERT_NO_EXCEPTION);
 
-  auto read_promise = reader->readable()
-                          ->getReader(script_state, ASSERT_NO_EXCEPTION)
-                          ->read(script_state, ASSERT_NO_EXCEPTION);
-  auto v8_read_promise = read_promise.V8Value().As<v8::Promise>();
-
-  EXPECT_EQ(v8::Promise::kPending, v8_read_promise->State());
+  ScriptValue v8_frame;
+  // We don't care about Call()'s return value, so we use undefined.
+  ScriptValue undefined_value =
+      ScriptValue::From(script_state, ToV8UndefinedGenerator());
+  EXPECT_CALL(*read_output_function, Call(testing::_))
+      .WillOnce(
+          testing::DoAll(testing::SaveArg<0>(&v8_frame),
+                         testing::Return(testing::ByMove(undefined_value))));
 
   RunIOUntilIdle();
 
-  EXPECT_EQ(v8::Promise::kFulfilled, v8_read_promise->State());
+  testing::Mock::VerifyAndClear(read_output_function);
+
+  auto* read_frame =
+      V8VideoFrame::ToImplWithTypeCheck(scope.GetIsolate(), v8_frame.V8Value());
 
-  auto* read_frame = V8VideoFrame::ToImplWithTypeCheck(
-      scope.GetIsolate(),
-      v8_read_promise->Result()
-          ->ToObject(scope.GetContext())
-          .ToLocalChecked()
-          ->Get(scope.GetContext(), V8String(scope.GetIsolate(), "value"))
-          .ToLocalChecked());
+  reader->stop(ASSERT_NO_EXCEPTION);
 
   ASSERT_TRUE(frame);
   EXPECT_EQ(frame->frame(), read_frame->frame());

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

@@ -8831,8 +8831,9 @@ interface VideoTrackList : EventTarget
     setter onremovetrack
 interface VideoTrackReader
     attribute @@toStringTag
-    getter readable
     method constructor
+    method start
+    method stop
 interface VideoTrackWriter
     attribute @@toStringTag
     getter track