Bug 1652884 - Make disabled tracks, that haven't seen a frame, black. r=jib
☠☠ backed out by 0ba7afbc4eee ☠ ☠
authorAndreas Pehrson <apehrson@mozilla.com>
Wed, 19 Aug 2020 20:35:36 +0000
changeset 545386 fc697986d538f5ae4e7d19217bfd2ab3b845d2aa
parent 545385 20a10128b5e165f3fb3b777aaf188916f4845c1e
child 545387 f83951453e94df4d4bf9930127a06bc8f878242a
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, MediaRecorder and RTCPeerConnection, which are the 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. Depends on D87127 Differential Revision: https://phabricator.services.mozilla.com/D87128
dom/media/VideoFrameConverter.h
dom/media/VideoOutput.h
dom/media/gtest/TestVideoFrameConverter.cpp
dom/media/gtest/TestVideoTrackEncoder.cpp
--- a/dom/media/VideoFrameConverter.h
+++ b/dom/media/VideoFrameConverter.h
@@ -134,24 +134,30 @@ 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 (mLastFrameQueuedForProcessing.Serial() != -2) {
+              // This track has already seen a frame so we re-send the last one
+              // queued as black.
+              FrameToProcess f = mLastFrameQueuedForProcessing;
+              f.mTime = TimeStamp::Now();
+              ProcessVideoFrame(f);
+            } else {
+              // This track has not yet seen any frame. We make one up.
+              QueueForProcessing(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/TestVideoFrameConverter.cpp
+++ b/dom/media/gtest/TestVideoFrameConverter.cpp
@@ -170,25 +170,26 @@ TEST_F(VideoFrameConverterTest, BlackOnD
   TimeStamp future2 = now + TimeDuration::FromMilliseconds(200);
   TimeStamp future3 = now + TimeDuration::FromMilliseconds(400);
   mConverter->SetActive(true);
   mConverter->SetTrackEnabled(false);
   mConverter->QueueVideoChunk(GenerateChunk(640, 480, future1), false);
   mConverter->QueueVideoChunk(GenerateChunk(640, 480, future2), false);
   mConverter->QueueVideoChunk(GenerateChunk(640, 480, future3), false);
   auto frames = WaitForNConverted(2);
-  EXPECT_GT(TimeStamp::Now() - now, TimeDuration::FromMilliseconds(1100));
+  EXPECT_GT(TimeStamp::Now() - now, TimeDuration::FromSeconds(1));
   ASSERT_EQ(frames.size(), 2U);
+  // The first frame was created instantly by SetTrackEnabled().
   EXPECT_EQ(frames[0].first.width(), 640);
   EXPECT_EQ(frames[0].first.height(), 480);
-  EXPECT_GT(frames[0].second - now, future1 - now);
+  EXPECT_GT(frames[0].second - now, TimeDuration::FromSeconds(0));
+  // The second frame was created by the same-frame timer (after 1s).
   EXPECT_EQ(frames[1].first.width(), 640);
   EXPECT_EQ(frames[1].first.height(), 480);
-  EXPECT_GT(frames[1].second - now,
-            future1 - now + TimeDuration::FromSeconds(1));
+  EXPECT_GT(frames[1].second - now, TimeDuration::FromSeconds(1));
   // Check that the second frame comes between 1s and 2s after the first.
   EXPECT_NEAR(frames[1].first.timestamp_us(),
               frames[0].first.timestamp_us() + ((PR_USEC_PER_SEC * 3) / 2),
               PR_USEC_PER_SEC / 2);
 }
 
 TEST_F(VideoFrameConverterTest, ClearFutureFramesOnJumpingBack) {
   TimeStamp start = TimeStamp::Now();
--- 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;