A track source should be unmuted whenever reenabled after setDirection changes
authoryouenn@apple.com <youenn@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 18 Jan 2019 17:19:26 +0000 (17:19 +0000)
committeryouenn@apple.com <youenn@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 18 Jan 2019 17:19:26 +0000 (17:19 +0000)
https://bugs.webkit.org/show_bug.cgi?id=193554
<rdar://problem/47366196>

Reviewed by Eric Carlson.

Source/WebCore:

Ensure that track gets unmuted after being fired as part of track event.
Test is triggering some existing issues with MediaPlayerPrivateMediaStreamAVFObjC.
Given the enqueuing of samples happens in a different frame than the thread used to update media stream and the active video track,
some enqueued samples might not be from the right active video track or there might be no active video track.

Test: webrtc/video-setDirection.html

* Modules/mediastream/libwebrtc/LibWebRTCMediaEndpoint.cpp:
(WebCore::LibWebRTCMediaEndpoint::fireTrackEvent):
* Modules/mediastream/libwebrtc/LibWebRTCMediaEndpoint.h:
* platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.mm:
(WebCore::MediaPlayerPrivateMediaStreamAVFObjC::enqueueVideoSample):
(WebCore::MediaPlayerPrivateMediaStreamAVFObjC::requestNotificationWhenReadyForVideoData):

LayoutTests:

* webrtc/video-setDirection-expected.txt: Added.
* webrtc/video-setDirection.html: Added.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@240152 268f45cc-cd09-0410-ab3c-d52691b4dbfc

LayoutTests/ChangeLog
LayoutTests/webrtc/video-setDirection-expected.txt [new file with mode: 0644]
LayoutTests/webrtc/video-setDirection.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/Modules/mediastream/libwebrtc/LibWebRTCMediaEndpoint.cpp
Source/WebCore/Modules/mediastream/libwebrtc/LibWebRTCMediaEndpoint.h
Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.mm

index 4bc8f13..0229fd9 100644 (file)
@@ -1,3 +1,14 @@
+2019-01-18  Youenn Fablet  <youenn@apple.com>
+
+        A track source should be unmuted whenever reenabled after setDirection changes
+        https://bugs.webkit.org/show_bug.cgi?id=193554
+        <rdar://problem/47366196>
+
+        Reviewed by Eric Carlson.
+
+        * webrtc/video-setDirection-expected.txt: Added.
+        * webrtc/video-setDirection.html: Added.
+
 2019-01-18  Jonathan Bedard  <jbedard@apple.com>
 
         webkitpy: Implement device type specific expected results (Part 2)
diff --git a/LayoutTests/webrtc/video-setDirection-expected.txt b/LayoutTests/webrtc/video-setDirection-expected.txt
new file mode 100644 (file)
index 0000000..4571b84
--- /dev/null
@@ -0,0 +1,5 @@
+
+
+PASS Going from sendrecv to inactive and back to sendrecv 
+FAIL The MediaStream should remain the same assert_equals: expected object "[object MediaStream]" but got object "[object MediaStream]"
+
diff --git a/LayoutTests/webrtc/video-setDirection.html b/LayoutTests/webrtc/video-setDirection.html
new file mode 100644 (file)
index 0000000..f21c1c3
--- /dev/null
@@ -0,0 +1,108 @@
+<!doctype html>
+<html>
+    <head>
+        <meta charset="utf-8">
+        <title>Testing video exchange using setDirection with renegotiation from offerer to receiver</title>
+        <script src="../resources/testharness.js"></script>
+        <script src="../resources/testharnessreport.js"></script>
+    </head>
+    <body>
+        <video id="video" autoplay=""></video>
+        <canvas id="canvas" width="640" height="480"></canvas>
+        <script src ="routines.js"></script>
+        <script>
+video = document.getElementById("video");
+canvas = document.getElementById("canvas");
+
+function grabFrameData(x, y, w, h)
+{
+    canvas.width = video.videoWidth;
+    canvas.height = video.videoHeight;
+
+    canvas.getContext('2d').drawImage(video, x, y, w, h, x, y, w, h);
+    return canvas.getContext('2d').getImageData(x, y, w, h).data;
+}
+
+function testImage()
+{
+    const data = grabFrameData(10, 325, 250, 1);
+
+    var index = 20;
+    assert_true(data[index] < 100);
+    assert_true(data[index + 1] < 100);
+    assert_true(data[index + 2] < 100);
+
+    index = 80;
+    assert_true(data[index] > 200);
+    assert_true(data[index + 1] > 200);
+    assert_true(data[index + 2] > 200);
+
+    index += 80;
+    assert_true(data[index] > 200);
+    assert_true(data[index + 1] > 200);
+    assert_true(data[index + 2] < 100);
+}
+
+var pc1, pc2;
+async function renegotiate()
+{
+    let d = await pc1.createOffer();
+    await pc1.setLocalDescription(d);
+    await pc2.setRemoteDescription(d);
+    d = await pc2.createAnswer();
+    await pc1.setRemoteDescription(d);
+    await pc2.setLocalDescription(d);
+}
+
+promise_test(async (t) => {
+    if (window.testRunner)
+        testRunner.setUserMediaPermission(true);
+
+    const localStream = await navigator.mediaDevices.getUserMedia({video: true});
+    const stream = await new Promise((resolve, reject) => {
+        createConnections((firstConnection) => {
+            pc1 = firstConnection;
+            firstConnection.addTrack(localStream.getVideoTracks()[0], localStream);
+        }, (secondConnection) => {
+            pc2 = secondConnection;
+            secondConnection.ontrack = (trackEvent) => { resolve(trackEvent.streams[0]); };
+        });
+        setTimeout(() => reject("Test timed out"), 5000);
+    });
+
+    video.srcObject = stream;
+    await video.play();
+
+    testImage();
+
+    let promise = new Promise((resolve) => {
+        pc2.getReceivers()[0].track.onmute = resolve;
+    });
+
+    pc1.getTransceivers()[0].direction = "inactive";
+    await renegotiate();
+    await promise;
+
+    promise = new Promise((resolve) => {
+        pc2.getReceivers()[0].track.onunmute = resolve;
+    });
+
+    pc1.getTransceivers()[0].direction = "sendrecv";
+    const streamPromise = new Promise(resolve => {
+        pc2.ontrack = (trackEvent) => { resolve (trackEvent.streams[0]); };
+    });
+
+    await renegotiate();
+    video.srcObject = await streamPromise;
+    await promise;
+
+    test(() => {
+        assert_equals(stream, video.srcObject);
+    }, "The MediaStream should remain the same");
+    await video.play();
+
+    testImage();
+}, "Going from sendrecv to inactive and back to sendrecv");
+        </script>
+    </body>
+</html>
index d87b9c0..ae955ea 100644 (file)
@@ -1,3 +1,25 @@
+2019-01-18  Youenn Fablet  <youenn@apple.com>
+
+        A track source should be unmuted whenever reenabled after setDirection changes
+        https://bugs.webkit.org/show_bug.cgi?id=193554
+        <rdar://problem/47366196>
+
+        Reviewed by Eric Carlson.
+
+        Ensure that track gets unmuted after being fired as part of track event.
+        Test is triggering some existing issues with MediaPlayerPrivateMediaStreamAVFObjC.
+        Given the enqueuing of samples happens in a different frame than the thread used to update media stream and the active video track,
+        some enqueued samples might not be from the right active video track or there might be no active video track.
+
+        Test: webrtc/video-setDirection.html
+
+        * Modules/mediastream/libwebrtc/LibWebRTCMediaEndpoint.cpp:
+        (WebCore::LibWebRTCMediaEndpoint::fireTrackEvent):
+        * Modules/mediastream/libwebrtc/LibWebRTCMediaEndpoint.h:
+        * platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.mm:
+        (WebCore::MediaPlayerPrivateMediaStreamAVFObjC::enqueueVideoSample):
+        (WebCore::MediaPlayerPrivateMediaStreamAVFObjC::requestNotificationWhenReadyForVideoData):
+
 2019-01-18  Charlie Turner  <cturner@igalia.com>
 
         [GStreamer][EME][ClearKey] Request keys from CDMInstance rather than passing via bus messages
index 3db42ae..51ad321 100644 (file)
@@ -413,21 +413,24 @@ void LibWebRTCMediaEndpoint::addRemoteTrack(rtc::scoped_refptr<webrtc::RtpReceiv
     fireTrackEvent(receiver.releaseNonNull(), track, rtcStreams, nullptr);
 }
 
-void LibWebRTCMediaEndpoint::fireTrackEvent(Ref<RTCRtpReceiver>&& receiver, Ref<MediaStreamTrack>&& track, const std::vector<rtc::scoped_refptr<webrtc::MediaStreamInterface>>& rtcStreams, RefPtr<RTCRtpTransceiver>&& transceiver)
+void LibWebRTCMediaEndpoint::fireTrackEvent(Ref<RTCRtpReceiver>&& receiver, MediaStreamTrack& track, const std::vector<rtc::scoped_refptr<webrtc::MediaStreamInterface>>& rtcStreams, RefPtr<RTCRtpTransceiver>&& transceiver)
 {
     Vector<RefPtr<MediaStream>> streams;
     for (auto& rtcStream : rtcStreams) {
         auto& mediaStream = mediaStreamFromRTCStream(*rtcStream.get());
         streams.append(&mediaStream);
-        mediaStream.addTrackFromPlatform(track.get());
+        mediaStream.addTrackFromPlatform(track);
     }
     auto streamIds = WTF::map(streams, [](auto& stream) -> String {
         return stream->id();
     });
-    m_remoteStreamsFromRemoteTrack.add(track.ptr(), WTFMove(streamIds));
+    m_remoteStreamsFromRemoteTrack.add(&track, WTFMove(streamIds));
 
     m_peerConnectionBackend.connection().fireEvent(RTCTrackEvent::create(eventNames().trackEvent,
-        Event::CanBubble::No, Event::IsCancelable::No, WTFMove(receiver), WTFMove(track), WTFMove(streams), WTFMove(transceiver)));
+        Event::CanBubble::No, Event::IsCancelable::No, WTFMove(receiver), &track, WTFMove(streams), WTFMove(transceiver)));
+
+    // FIXME: As per spec, we should set muted to 'false' when starting to receive the content from network.
+    track.source().setMuted(false);
 }
 
 static inline void setExistingReceiverSourceTrack(RealtimeMediaSource& existingSource, webrtc::RtpReceiverInterface& rtcReceiver)
index f8cb8b9..74db236 100644 (file)
@@ -143,7 +143,7 @@ private:
     void newTransceiver(rtc::scoped_refptr<webrtc::RtpTransceiverInterface>&&);
     void removeRemoteTrack(rtc::scoped_refptr<webrtc::RtpReceiverInterface>&&);
 
-    void fireTrackEvent(Ref<RTCRtpReceiver>&&, Ref<MediaStreamTrack>&&, const std::vector<rtc::scoped_refptr<webrtc::MediaStreamInterface>>&, RefPtr<RTCRtpTransceiver>&&);
+    void fireTrackEvent(Ref<RTCRtpReceiver>&&, MediaStreamTrack&, const std::vector<rtc::scoped_refptr<webrtc::MediaStreamInterface>>&, RefPtr<RTCRtpTransceiver>&&);
 
     template<typename T>
     Optional<Backends> createTransceiverBackends(T&&, const RTCRtpTransceiverInit&, LibWebRTCRtpSenderBackend::Source&&);
index 238a2ea..935b1bf 100644 (file)
@@ -359,8 +359,6 @@ void MediaPlayerPrivateMediaStreamAVFObjC::enqueueCorrectedVideoSample(MediaSamp
 
 void MediaPlayerPrivateMediaStreamAVFObjC::enqueueVideoSample(MediaStreamTrackPrivate& track, MediaSample& sample)
 {
-    ASSERT(m_videoTrackMap.contains(track.id()));
-
     if (&track != m_mediaStreamPrivate->activeVideoTrack())
         return;
 
@@ -414,6 +412,11 @@ void MediaPlayerPrivateMediaStreamAVFObjC::requestNotificationWhenReadyForVideoD
 
         [m_sampleBufferDisplayLayer stopRequestingMediaData];
 
+        if (!m_activeVideoTrack) {
+            m_pendingVideoSampleQueue.clear();
+            return;
+        }
+
         while (!m_pendingVideoSampleQueue.isEmpty()) {
             if (![m_sampleBufferDisplayLayer isReadyForMoreMediaData]) {
                 requestNotificationWhenReadyForVideoData();