Changeset 249574 in webkit

Timestamp:

Sep 6, 2019 8:49:39 AM (5 years ago)

Author:

youenn@apple.com

Message:

Remove MediaStreamPrivate::scheduleDeferredTask
​https://bugs.webkit.org/show_bug.cgi?id=200975

Reviewed by Eric Carlson.

LayoutTests/imported/w3c:

• web-platform-tests/mediacapture-streams/MediaStream-finished-add.https-expected.txt:

Source/WebCore:

All calls to scheduleDeferredTask are done on the main thread.
This was initially done to trigger less reconfiguration.
But this makes the implementation significantly more complex.

For instance, we have to wait for the document to update its media state
and send it to UIProcess before calling the allow completion handler.

Covered by existing tests.

• Modules/mediastream/MediaStream.cpp:

(WebCore::MediaStream::MediaStream):
Make sure to update the document media state once the tracks have been added, similarly to the other constructor.
This ensures the document media state is computed with the new MediaStreamTrack.

• Modules/mediastream/UserMediaRequest.cpp:

(WebCore::isMediaStreamCorrectlyStarted):
(WebCore::UserMediaRequest::allow):
(WebCore::UserMediaRequest::stop):
(WebCore::UserMediaRequest::mediaStreamDidFail):

• Modules/mediastream/UserMediaRequest.h:
• page/MediaProducer.h:

(WebCore::MediaProducer::isCapturing):
Make sure to include getDisplayMedia as part of capture check.

• platform/mediastream/MediaStreamPrivate.cpp:

(WebCore::MediaStreamPrivate::trackMutedChanged):
(WebCore::MediaStreamPrivate::trackEnabledChanged):
(WebCore::MediaStreamPrivate::trackStarted):
(WebCore::MediaStreamPrivate::trackEnded):

• platform/mediastream/MediaStreamPrivate.h:

Location:

trunk

Files:

• LayoutTests/imported/w3c/ChangeLog (modified) (1 diff)
• LayoutTests/imported/w3c/web-platform-tests/mediacapture-streams/MediaStream-finished-add.https-expected.txt (modified) (1 diff)
• Source/WebCore/ChangeLog (modified) (1 diff)
• Source/WebCore/Modules/mediastream/MediaStream.cpp (modified) (2 diffs)
• Source/WebCore/Modules/mediastream/UserMediaRequest.cpp (modified) (6 diffs)
• Source/WebCore/Modules/mediastream/UserMediaRequest.h (modified) (2 diffs)
• Source/WebCore/page/MediaProducer.h (modified) (2 diffs)
• Source/WebCore/platform/mediastream/MediaStreamPrivate.cpp (modified) (4 diffs)
• Source/WebCore/platform/mediastream/MediaStreamPrivate.h (modified) (3 diffs)

trunk/LayoutTests/imported/w3c/ChangeLog

@@ +1 @@
+2019-09-06  Youenn Fablet  <youenn@apple.com>
+
+        Remove MediaStreamPrivate::scheduleDeferredTask
+        https://bugs.webkit.org/show_bug.cgi?id=200975
+
+        Reviewed by Eric Carlson.
+
+        * web-platform-tests/mediacapture-streams/MediaStream-finished-add.https-expected.txt:
+
 2019-09-06  Rob Buis  <rbuis@igalia.com>
 

trunk/LayoutTests/imported/w3c/web-platform-tests/mediacapture-streams/MediaStream-finished-add.https-expected.txt

@@ -6 +6 @@
 
 
-FAIL Tests that adding a track to an inactive MediaStream is allowed assert_false: audio stream is inactive after stopping its only audio track expected false got true
+PASS Tests that adding a track to an inactive MediaStream is allowed 
 

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2019-09-06  Youenn Fablet  <youenn@apple.com>
+
+        Remove MediaStreamPrivate::scheduleDeferredTask
+        https://bugs.webkit.org/show_bug.cgi?id=200975
+
+        Reviewed by Eric Carlson.
+
+        All calls to scheduleDeferredTask are done on the main thread.
+        This was initially done to trigger less reconfiguration.
+        But this makes the implementation significantly more complex.
+
+        For instance, we have to wait for the document to update its media state
+        and send it to UIProcess before calling the allow completion handler.
+
+        Covered by existing tests.
+
+        * Modules/mediastream/MediaStream.cpp:
+        (WebCore::MediaStream::MediaStream):
+        Make sure to update the document media state once the tracks have been added, similarly to the other constructor.
+        This ensures the document media state is computed with the new MediaStreamTrack.
+        * Modules/mediastream/UserMediaRequest.cpp:
+        (WebCore::isMediaStreamCorrectlyStarted):
+        (WebCore::UserMediaRequest::allow):
+        (WebCore::UserMediaRequest::stop):
+        (WebCore::UserMediaRequest::mediaStreamDidFail):
+        * Modules/mediastream/UserMediaRequest.h:
+        * page/MediaProducer.h:
+        (WebCore::MediaProducer::isCapturing):
+        Make sure to include getDisplayMedia as part of capture check.
+        * platform/mediastream/MediaStreamPrivate.cpp:
+        (WebCore::MediaStreamPrivate::trackMutedChanged):
+        (WebCore::MediaStreamPrivate::trackEnabledChanged):
+        (WebCore::MediaStreamPrivate::trackStarted):
+        (WebCore::MediaStreamPrivate::trackEnded):
+        * platform/mediastream/MediaStreamPrivate.h:
+
 2019-09-06  Rob Buis  <rbuis@igalia.com>
 

trunk/Source/WebCore/Modules/mediastream/MediaStream.cpp

@@ -102 +102 @@
     ALWAYS_LOG(LOGIDENTIFIER);
 
-    setIsActive(m_private->active());
-    m_private->addObserver(*this);
-
     for (auto& trackPrivate : m_private->tracks()) {
         auto track = MediaStreamTrack::create(document, *trackPrivate);
@@ -110 +107 @@
         m_trackSet.add(track->id(), WTFMove(track));
     }
+
+    setIsActive(m_private->active());
+    m_private->addObserver(*this);
     suspendIfNeeded();
 }

trunk/Source/WebCore/Modules/mediastream/UserMediaRequest.cpp

@@ -216 +216 @@
 }
 
+static inline bool isMediaStreamCorrectlyStarted(const MediaStream& stream)
+{
+    if (stream.getTracks().isEmpty())
+        return false;
+
+    return WTF::allOf(stream.getTracks(), [](auto& track) {
+        return !track->source().captureDidFail();
+    });
+}
+
 void UserMediaRequest::allow(CaptureDevice&& audioDevice, CaptureDevice&& videoDevice, String&& deviceIdentifierHashSalt, CompletionHandler<void()>&& completionHandler)
 {
@@ -221 +231 @@
 
     auto callback = [this, protector = makePendingActivity(*this), completionHandler = WTFMove(completionHandler)](RefPtr<MediaStreamPrivate>&& privateStream) mutable {
-        auto scopeExit = makeScopeExit([&] {
+        auto scopeExit = makeScopeExit([completionHandler = WTFMove(completionHandler)]() mutable {
             completionHandler();
         });
-        if (!m_scriptExecutionContext)
+        if (isContextStopped())
             return;
 
@@ -232 +242 @@
             return;
         }
-        privateStream->monitorOrientation(downcast<Document>(m_scriptExecutionContext)->orientationNotifier());
-
-        auto stream = MediaStream::create(*downcast<Document>(m_scriptExecutionContext), privateStream.releaseNonNull());
-        if (stream->getTracks().isEmpty()) {
+
+        auto& document = downcast<Document>(*m_scriptExecutionContext);
+        privateStream->monitorOrientation(document.orientationNotifier());
+
+        auto stream = MediaStream::create(document, privateStream.releaseNonNull());
+        stream->startProducingData();
+
+        if (!isMediaStreamCorrectlyStarted(stream)) {
             deny(MediaAccessDenialReason::HardwareError);
             return;
         }
 
-        scopeExit.release();
-        m_pendingActivationMediaStream = makeUnique<PendingActivationMediaStream>(WTFMove(protector), *this, WTFMove(stream), WTFMove(completionHandler));
+        ASSERT(document.isCapturing());
+        stream->document()->setHasCaptureMediaStreamTrack();
+        m_promise.resolve(WTFMove(stream));
     };
 
@@ -311 +326 @@
 void UserMediaRequest::stop()
 {
-    // Protecting 'this' since nulling m_pendingActivationMediaStream might destroy it.
-    Ref<UserMediaRequest> protectedThis(*this);
-
-    m_pendingActivationMediaStream = nullptr;
-
     auto& document = downcast<Document>(*m_scriptExecutionContext);
     if (auto* controller = UserMediaController::from(document.page()))
@@ -334 +344 @@
 {
     return downcast<Document>(m_scriptExecutionContext);
-}
-
-UserMediaRequest::PendingActivationMediaStream::PendingActivationMediaStream(Ref<PendingActivity<UserMediaRequest>>&& protectingUserMediaRequest, UserMediaRequest& userMediaRequest, Ref<MediaStream>&& stream, CompletionHandler<void()>&& completionHandler)
-    : m_protectingUserMediaRequest(WTFMove(protectingUserMediaRequest))
-    , m_userMediaRequest(userMediaRequest)
-    , m_mediaStream(WTFMove(stream))
-    , m_completionHandler(WTFMove(completionHandler))
-{
-    m_mediaStream->privateStream().addObserver(*this);
-    m_mediaStream->startProducingData();
-}
-
-UserMediaRequest::PendingActivationMediaStream::~PendingActivationMediaStream()
-{
-    m_mediaStream->privateStream().removeObserver(*this);
-    m_completionHandler();
-    if (auto* document = m_mediaStream->document())
-        document->updateIsPlayingMedia();
-}
-
-void UserMediaRequest::PendingActivationMediaStream::characteristicsChanged()
-{
-    if (!m_userMediaRequest.m_pendingActivationMediaStream)
-        return;
-
-    for (auto& track : m_mediaStream->privateStream().tracks()) {
-        if (track->source().captureDidFail()) {
-            m_userMediaRequest.mediaStreamDidFail(track->source().type());
-            return;
-        }
-    }
-
-    if (m_mediaStream->privateStream().hasVideo() || m_mediaStream->privateStream().hasAudio()) {
-        m_userMediaRequest.mediaStreamIsReady(WTFMove(m_mediaStream));
-        return;
-    }
-}
-
-void UserMediaRequest::mediaStreamIsReady(Ref<MediaStream>&& stream)
-{
-    RELEASE_LOG(MediaStream, "UserMediaRequest::mediaStreamIsReady");
-    stream->document()->setHasCaptureMediaStreamTrack();
-    m_promise.resolve(WTFMove(stream));
-    m_pendingActivationMediaStream = nullptr;
 }
 
@@ -396 +362 @@
     }
     m_promise.reject(NotReadableError, makeString("Failed starting capture of a "_s, typeDescription, " track"_s));
-    m_pendingActivationMediaStream = nullptr;
 }
 

trunk/Source/WebCore/Modules/mediastream/UserMediaRequest.h

@@ -80 +80 @@
     bool canSuspendForDocumentSuspension() const final;
 
-    void mediaStreamIsReady(Ref<MediaStream>&&);
     void mediaStreamDidFail(RealtimeMediaSource::Type);
-
-    class PendingActivationMediaStream : private MediaStreamPrivate::Observer {
-        WTF_MAKE_FAST_ALLOCATED;
-    public:
-        PendingActivationMediaStream(Ref<PendingActivity<UserMediaRequest>>&&, UserMediaRequest&, Ref<MediaStream>&&, CompletionHandler<void()>&&);
-        ~PendingActivationMediaStream();
-
-    private:
-        void characteristicsChanged() final;
-
-        Ref<PendingActivity<UserMediaRequest>> m_protectingUserMediaRequest;
-        UserMediaRequest& m_userMediaRequest;
-        Ref<MediaStream> m_mediaStream;
-        CompletionHandler<void()> m_completionHandler;
-    };
 
     Vector<String> m_videoDeviceUIDs;
@@ -102 +86 @@
 
     DOMPromiseDeferred<IDLInterface<MediaStream>> m_promise;
-    std::unique_ptr<PendingActivationMediaStream> m_pendingActivationMediaStream;
     MediaStreamRequest m_request;
 };

trunk/Source/WebCore/page/MediaProducer.h

@@ -59 +59 @@
         VideoCaptureMask = HasActiveVideoCaptureDevice | HasMutedVideoCaptureDevice | HasInterruptedVideoCaptureDevice,
         DisplayCaptureMask = HasActiveDisplayCaptureDevice | HasMutedDisplayCaptureDevice | HasInterruptedDisplayCaptureDevice,
+        ActiveCaptureMask = HasActiveAudioCaptureDevice | HasActiveVideoCaptureDevice | HasActiveDisplayCaptureDevice,
         MutedCaptureMask =  HasMutedAudioCaptureDevice | HasMutedVideoCaptureDevice | HasMutedDisplayCaptureDevice,
         MediaCaptureMask = AudioCaptureMask | VideoCaptureMask | DisplayCaptureMask,
@@ -64 +65 @@
     typedef unsigned MediaStateFlags;
 
-    static bool isCapturing(MediaStateFlags state) { return (state & HasActiveAudioCaptureDevice) || (state & HasActiveVideoCaptureDevice) || (state & HasMutedAudioCaptureDevice) || (state & HasMutedVideoCaptureDevice); }
+    static bool isCapturing(MediaStateFlags state) { return (state & ActiveCaptureMask) || (state & MutedCaptureMask); }
 
     virtual MediaStateFlags mediaState() const = 0;

trunk/Source/WebCore/platform/mediastream/MediaStreamPrivate.cpp

@@ -283 +283 @@
 
     ALWAYS_LOG(LOGIDENTIFIER, track.logIdentifier(), " ", track.muted());
-    scheduleDeferredTask([this] {
-        characteristicsChanged();
-    });
+    characteristicsChanged();
 }
 
@@ -302 +300 @@
     updateActiveVideoTrack();
 
-    scheduleDeferredTask([this] {
-        characteristicsChanged();
-    });
+    characteristicsChanged();
 }
 
@@ -314 +310 @@
 
     ALWAYS_LOG(LOGIDENTIFIER, track.logIdentifier());
-    scheduleDeferredTask([this] {
-        characteristicsChanged();
-    });
+    characteristicsChanged();
 }
 
@@ -326 +320 @@
 
     ALWAYS_LOG(LOGIDENTIFIER, track.logIdentifier());
-    scheduleDeferredTask([this] {
-        updateActiveState(NotifyClientOption::Notify);
-        characteristicsChanged();
-    });
-}
-
-void MediaStreamPrivate::scheduleDeferredTask(Function<void ()>&& function)
-{
-    ASSERT(function);
-    callOnMainThread([weakThis = makeWeakPtr(*this), function = WTFMove(function)] {
-        if (!weakThis)
-            return;
-
-        function();
-    });
+    updateActiveState(NotifyClientOption::Notify);
+    characteristicsChanged();
 }
 

trunk/Source/WebCore/platform/mediastream/MediaStreamPrivate.h

@@ -48 +48 @@
 #include <wtf/UUID.h>
 #include <wtf/Vector.h>
-#include <wtf/WeakPtr.h>
 
 namespace WebCore {
@@ -58 +57 @@
     : public MediaStreamTrackPrivate::Observer
     , public RefCounted<MediaStreamPrivate>
-    , public CanMakeWeakPtr<MediaStreamPrivate>
 #if !RELEASE_LOG_DISABLED
     , private LoggerHelper
@@ -129 +127 @@
     void updateActiveVideoTrack();
 
-    void scheduleDeferredTask(Function<void ()>&&);
     void forEachObserver(const WTF::Function<void(Observer&)>&) const;