Bug 1652884 - Make disabled tracks, that haven't seen a frame, black. r=jib
☠☠ backed out by e8054b77177a ☠ ☠
authorAndreas Pehrson <apehrson@mozilla.com>
Wed, 19 Aug 2020 12:48:09 +0000
changeset 545348 658ba8f39abe4553e0c06706b867fc86f9239625
parent 545347 8e67fe040e4a789fb23f8f7fd1efbff781b49b4a
child 545349 28c4e8c373f08bceba73d69cdefa24193a79b05b
push id37713
push userabutkovits@mozilla.com
push dateThu, 20 Aug 2020 09:32:09 +0000
treeherdermozilla-central@8cb700c12bd3 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjib
bugs1652884
milestone81.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1652884 - Make disabled tracks, that haven't seen a frame, black. r=jib This affects HTMLMediaElement and MediaRecorder, which are the only implementors of NotifyEnabledStateChanged. This use case is going to become more common as users will be able to globally mute cameras before requesting one. While muted, the camera is off and no frames will flow. The old logic for showing disabled video tracks as black relied on a frame appearing that could be turned black. With this patch in this case we will create a frame if none has been seen yet, and it will have a hardcoded size. Differential Revision: https://phabricator.services.mozilla.com/D87128
dom/media/VideoFrameConverter.h
dom/media/VideoOutput.h
dom/media/gtest/TestVideoTrackEncoder.cpp
--- a/dom/media/VideoFrameConverter.h
+++ b/dom/media/VideoFrameConverter.h
@@ -134,24 +134,32 @@ class VideoFrameConverter {
         [self = RefPtr<VideoFrameConverter>(this), this, aTrackEnabled] {
           if (mTrackEnabled == aTrackEnabled) {
             return;
           }
           MOZ_LOG(gVideoFrameConverterLog, LogLevel::Debug,
                   ("VideoFrameConverter Track is now %s",
                    aTrackEnabled ? "enabled" : "disabled"));
           mTrackEnabled = aTrackEnabled;
-          if (!aTrackEnabled && mLastFrameConverted) {
-            // After disabling, we re-send the last frame as black in case the
-            // source had already stopped and no frame is coming soon.
-            ProcessVideoFrame(
-                FrameToProcess{nullptr, TimeStamp::Now(),
-                               gfx::IntSize(mLastFrameConverted->width(),
-                                            mLastFrameConverted->height()),
-                               true});
+          if (!aTrackEnabled) {
+            // After disabling we immediately send a frame as black, so it can
+            // be seen quickly, even if no frames are flowing.
+            if (mLastFrameConverted) {
+              // This track has already seen frames so we re-send the last one
+              // as black.
+              ProcessVideoFrame(
+                  FrameToProcess{nullptr, TimeStamp::Now(),
+                                 gfx::IntSize(mLastFrameConverted->width(),
+                                              mLastFrameConverted->height()),
+                                 true});
+            } else {
+              // This track has not yet seen any frame. We make one up.
+              ProcessVideoFrame(FrameToProcess{nullptr, TimeStamp::Now(),
+                                               gfx::IntSize(640, 480), true});
+            }
           }
         }));
     MOZ_DIAGNOSTIC_ASSERT(NS_SUCCEEDED(rv));
     Unused << rv;
   }
 
   void AddListener(const RefPtr<VideoConverterListener>& aListener) {
     nsresult rv = mTaskQueue->Dispatch(NS_NewRunnableFunction(
--- a/dom/media/VideoOutput.h
+++ b/dom/media/VideoOutput.h
@@ -184,24 +184,33 @@ class VideoOutput : public DirectMediaTr
   void NotifyEnabledStateChanged(MediaTrackGraph* aGraph,
                                  bool aEnabled) override {
     MutexAutoLock lock(mMutex);
     mEnabled = aEnabled;
     DropPastFrames();
     if (!mEnabled || mFrames.Length() > 1) {
       // Re-send frames when disabling, as new frames may not arrive. When
       // enabling we keep them black until new frames arrive, or re-send if we
-      // already have frames in the future.
+      // already have frames in the future. If we're disabling and there are no
+      // frames available yet, we invent one. Unfortunately with a hardcoded
+      // size.
       //
       // Since mEnabled will affect whether
       // frames are real, or black, we assign new FrameIDs whenever we re-send
       // frames after an mEnabled change.
       for (auto& idChunkPair : mFrames) {
         idChunkPair.first = mVideoFrameContainer->NewFrameID();
       }
+      if (mFrames.IsEmpty()) {
+        VideoSegment v;
+        v.AppendFrame(nullptr, gfx::IntSize(640, 480), PRINCIPAL_HANDLE_NONE,
+                      true, TimeStamp::Now());
+        mFrames.AppendElement(std::make_pair(mVideoFrameContainer->NewFrameID(),
+                                             *v.GetLastChunk()));
+      }
       SendFramesEnsureLocked();
     }
   }
 
   Mutex mMutex;
   TimeStamp mLastFrameTime;
   // Once the frame is forced to black, we initialize mBlackImage for use in any
   // following forced-black frames.
--- a/dom/media/gtest/TestVideoTrackEncoder.cpp
+++ b/dom/media/gtest/TestVideoTrackEncoder.cpp
@@ -1274,16 +1274,56 @@ TEST(VP8VideoTrackEncoder, DisableBetwee
 
   // [50ms, 100ms)
   EXPECT_EQ(PR_USEC_PER_SEC / 1000 * 50UL, frames[1]->mDuration);
 
   // [100ms, 200ms)
   EXPECT_EQ(PR_USEC_PER_SEC / 1000 * 100UL, frames[2]->mDuration);
 }
 
+// Test that an encoding which is disabled before the first frame becomes black
+// immediately.
+TEST(VP8VideoTrackEncoder, DisableBeforeFirstFrame)
+{
+  TestVP8TrackEncoder encoder;
+  YUVBufferGenerator generator;
+  generator.Init(mozilla::gfx::IntSize(640, 480));
+  nsTArray<RefPtr<EncodedFrame>> frames;
+  TimeStamp now = TimeStamp::Now();
+
+  // Disable the track at t=0.
+  // Pass a frame in at t=50ms.
+  // Enable the track at t=100ms.
+  // Stop encoding at t=200ms.
+  // Should yield 2 frames, 1 black [0, 100); 1 real [100, 200).
+
+  VideoSegment segment;
+  segment.AppendFrame(generator.GenerateI420Image(), generator.GetSize(),
+                      PRINCIPAL_HANDLE_NONE, false,
+                      now + TimeDuration::FromMilliseconds(50));
+
+  encoder.SetStartOffset(now);
+  encoder.Disable(now);
+  encoder.AppendVideoSegment(std::move(segment));
+
+  encoder.Enable(now + TimeDuration::FromMilliseconds(100));
+  encoder.AdvanceCurrentTime(now + TimeDuration::FromMilliseconds(200));
+  encoder.NotifyEndOfStream();
+  ASSERT_TRUE(NS_SUCCEEDED(encoder.GetEncodedTrack(frames)));
+  EXPECT_TRUE(encoder.IsEncodingComplete());
+
+  ASSERT_EQ(2UL, frames.Length());
+
+  // [0, 100ms)
+  EXPECT_EQ(PR_USEC_PER_SEC / 1000 * 100UL, frames[0]->mDuration);
+
+  // [100ms, 200ms)
+  EXPECT_EQ(PR_USEC_PER_SEC / 1000 * 100UL, frames[1]->mDuration);
+}
+
 // Test that an encoding which is enabled on a frame timestamp encodes
 // frames as expected.
 TEST(VP8VideoTrackEncoder, EnableOnFrameTime)
 {
   TestVP8TrackEncoder encoder;
   YUVBufferGenerator generator;
   generator.Init(mozilla::gfx::IntSize(640, 480));
   nsTArray<RefPtr<EncodedFrame>> frames;