Bug 190778

Created attachment 353057 [details] Patch

Created attachment 353058 [details] Patch

Created attachment 353060 [details] Patch

Comment on attachment 353060 [details] Patch Attachment 353060 [details] did not pass mac-ews (mac): Output: https://webkit-queues.webkit.org/results/9718730 New failing tests: imported/w3c/web-platform-tests/mediacapture-record/BlobEvent-constructor.html imported/w3c/web-platform-tests/mediacapture-record/MediaRecorder-dataavailable.html

Created attachment 353064 [details] Archive of layout-test-results from ews100 for mac-sierra The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews100 Port: mac-sierra Platform: Mac OS X 10.12.6

Comment on attachment 353060 [details] Patch Attachment 353060 [details] did not pass mac-wk2-ews (mac-wk2): Output: https://webkit-queues.webkit.org/results/9718778 New failing tests: imported/w3c/web-platform-tests/mediacapture-record/BlobEvent-constructor.html

Created attachment 353065 [details] Archive of layout-test-results from ews106 for mac-sierra-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews106 Port: mac-sierra-wk2 Platform: Mac OS X 10.12.6

Comment on attachment 353060 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=353060&action=review Some early comments, did not have time to do a full pass yet. > Source/WebCore/Modules/mediarecorder/BlobEvent.cpp:37 > + auto blob = init.data; This local is not needed. > Source/WebCore/Modules/mediarecorder/BlobEvent.cpp:38 > + return adoptRef(*new BlobEvent(type, WTFMove(init), WTFMove(blob), isTrusted)); Just don't pass the blob. > Source/WebCore/Modules/mediarecorder/BlobEvent.cpp:47 > + : Event(type, WTFMove(init), isTrusted) You do not need to WTFMove() init here, you can just pass init here. > Source/WebCore/Modules/mediarecorder/BlobEvent.cpp:48 > + , m_blob(WTFMove(data)) and WTFMove(init.data) here. > Source/WebCore/Modules/mediarecorder/BlobEvent.cpp:50 > + m_timecode = init.timecode; This should be in the initializer list rather than the constructor body. Also note that you're using init after it's be WTFMoved out. > Source/WebCore/Modules/mediarecorder/BlobEvent.h:45 > + Blob* data() const { return m_blob.get(); } Should probably return a Blob& as it cannot return null. > Source/WebCore/Modules/mediarecorder/BlobEvent.h:55 > + RefPtr<Blob> m_blob; Should probably be a Ref<Blob> as it cannot be null. > Source/WebCore/Modules/mediarecorder/BlobEvent.idl:35 > // readonly attribute DOMHighResTimeStamp timecode; Seems like you could uncomment this one too since you implemented it? > Source/WebCore/Modules/mediarecorder/MediaRecorder.cpp:104 > + dispatchEvent(Event::create(eventNames().stopEvent, Event::CanBubble::No, Event::IsCancelable::No)); The line above just ran script. Therefore, you cannot assume |this| is still alive and in an active state at this point. Simply calling dispatchEvent() here could be a use-after-free. > Source/WebCore/Modules/mediarecorder/MediaRecorder.cpp:132 > + dispatchEvent(Event::create(eventNames().stopEvent, Event::CanBubble::No, Event::IsCancelable::No)); Same comment as above with regards to being still alive and active. > Source/WebCore/Modules/mediarecorder/MediaRecorder.h:98 > + std::unique_ptr<MediaRecorderPrivate> m_private; Can this ever be null? If not, it should probably use UniqueRef<> instead of std::unique_ptr<>.

Comment on attachment 353060 [details] Patch Attachment 353060 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: https://webkit-queues.webkit.org/results/9718976 New failing tests: imported/w3c/web-platform-tests/mediacapture-record/BlobEvent-constructor.html imported/w3c/web-platform-tests/mediacapture-record/MediaRecorder-dataavailable.html

Created attachment 353067 [details] Archive of layout-test-results from ews122 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews122 Port: ios-simulator-wk2 Platform: Mac OS X 10.13.6

Comment on attachment 353060 [details] Patch Attachment 353060 [details] did not pass mac-debug-ews (mac): Output: https://webkit-queues.webkit.org/results/9718979 New failing tests: imported/w3c/web-platform-tests/mediacapture-record/BlobEvent-constructor.html

Created attachment 353068 [details] Archive of layout-test-results from ews113 for mac-sierra The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews113 Port: mac-sierra Platform: Mac OS X 10.12.6

Comment on attachment 353060 [details] Patch Attachment 353060 [details] did not pass mac-debug-ews (mac): Output: https://webkit-queues.webkit.org/results/9719781 New failing tests: imported/w3c/web-platform-tests/mediacapture-record/BlobEvent-constructor.html

Created attachment 353072 [details] Archive of layout-test-results from ews112 for mac-sierra The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews112 Port: mac-sierra Platform: Mac OS X 10.12.6

Created attachment 353077 [details] Patch

Comment on attachment 353077 [details] Patch I think we should make MediaRecorderPrivate an interface. Then there would be a MediaRecorderPrivateMock that would implement the interface as is currently done. And we would add in the future a MediaRecorderPrivateMac (better name maybe) that would implement the interface for real. In WTR, we could use the mock or use the real private depending on the test. See for instance how this is done for PeerConnection. In this patch, it is fine to introduce an interface and the MediaRecorderPrivateMock. A follow-up patch will implement the switch once we implement the real MediaRecorderPrivate. I also think we should add non WPT tests that would check the values of the fetch data somehow. Non WPT as it would use the mock behavior which is not shared. The test would basically show that we receive both audio and video callbacks. Some improvements below. View in context: https://bugs.webkit.org/attachment.cgi?id=353077&action=review > Source/WebCore/Modules/mediarecorder/MediaRecorder.cpp:96 > + return Exception { InvalidStateError, "The MediaRecorder's state cannot be inactive"_s }; Shouldn't we try to stop observing tracks here? > Source/WebCore/Modules/mediarecorder/MediaRecorder.cpp:143 > + return; I am not sure we need these since m_isActive and state() are modified in main thread while this callback will be in background threads. > Source/WebCore/Modules/mediarecorder/MediaRecorder.cpp:150 > + return; Ditto. > Source/WebCore/platform/mediarecorder/MediaRecorderPrivate.cpp:31 > +#include "AudioStreamDescription.h" No need to have this include. > Source/WebCore/platform/mediarecorder/MediaRecorderPrivate.cpp:33 > +#include "MediaSample.h" Ditto for this one. > Source/WebCore/platform/mediarecorder/MediaRecorderPrivate.cpp:35 > +#include "PlatformAudioData.h" Ditto for this one. > Source/WebCore/platform/mediarecorder/MediaRecorderPrivate.cpp:41 > + m_counter = 0; Can be set in the header. > Source/WebCore/platform/mediarecorder/MediaRecorderPrivate.cpp:51 > + const String mockString("Video Track ID: " + track.id() + " Counter: " + String::number(++m_counter) + "\r\n---------\r\n"); Use makeString instead. > Source/WebCore/platform/mediarecorder/MediaRecorderPrivate.cpp:88 > + m_buffer->clear(); Might be better to create a Vector<uint8_t> here. That could mean that m_buffer could be a StringBuilder for convenience. > Source/WebCore/platform/mediarecorder/MediaRecorderPrivate.h:58 > + int m_counter; // FIXME: Used to output mock string in order to test s/int/unsigned/ > LayoutTests/imported/w3c/ChangeLog:10 > + * web-platform-tests/mediacapture-record/MediaRecorder-dataavailable.html: Added. Can you upstream this to WPT?

Created attachment 353157 [details] Patch

Created attachment 353158 [details] Patch

Comment on attachment 353158 [details] Patch Attachment 353158 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/9735153 New failing tests: fast/mediarecorder/MediaRecorder-mock-dataavailable.html

Created attachment 353162 [details] Archive of layout-test-results from ews205 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews205 Port: win-future Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

Comment on attachment 353158 [details] Patch Looks good, some improvements below. View in context: https://bugs.webkit.org/attachment.cgi?id=353158&action=review > Source/WebCore/Modules/mediarecorder/MediaRecorder.cpp:69 > m_isActive = false; maybe we should do if (!m_isActive) return; > Source/WebCore/Modules/mediarecorder/MediaRecorder.cpp:91 > + track->addObserver(*this); I wonder whether we should be more strict about adding/removing observers. Current observers in track private is a HashSet, so this is ok, but MediaStreamTrack is using Vector (should change that I guess). Maybe we should add a struct TracksObserver that in constructor does the addObserver and in the destructor does the removeObserver. Then we can add in the MediaRecorder class: std::optional<TracksObserver> m_tracksObserver; In MediaRecorder::start, we will do: m_tracksObserver = TracksObserver { *this }; And in stopRecording: m_tracksObserver = { }; or maybe std::nullopt will be clearer. > Source/WebCore/platform/mediarecorder/MediaRecorderPrivateMock.cpp:40 > + const String mockString = makeString("Video Track ID: ", track.id(), " Counter: ", String::number(++m_counter), "\r\n---------\r\n"); It might be better to call append on the buffer instead of creating this string. > Source/WebCore/platform/mediarecorder/MediaRecorderPrivateMock.cpp:50 > + UNUSED_PARAM(sampleCount); Instead of using UNUSED, you can just remove the name in the prototype. > Source/WebCore/platform/mediarecorder/MediaRecorderPrivateMock.cpp:55 > + const String mockString = makeString("Audio Track ID: ", track.id(), " Counter: ", String::number(++m_counter), "\r\n---------\r\n"); It might be better to call append on the buffer instead of creating this string.

Comment on attachment 353158 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=353158&action=review > LayoutTests/fast/mediarecorder/MediaRecorder-mock-dataavailable.html:4 > + <script src="../../resources/js-test-pre.js"></script> Can you use testharness.js instead of js-test-pre.js, we should try to move this test anyway. We might not share it as WPT but it might be reused/copied for other tests that might be useful to export to WPT. > LayoutTests/imported/w3c/web-platform-tests/mediacapture-record/MediaRecorder-dataavailable.html:10 > +<canvas id="canvas" width="200" height="200"> Is this one upstreamable to WPT?

Comment on attachment 353158 [details] Patch Attachment 353158 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/9735910 New failing tests: fast/mediarecorder/MediaRecorder-mock-dataavailable.html

Created attachment 353165 [details] Archive of layout-test-results from ews202 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews202 Port: win-future Platform: CYGWIN_NT-6.1-2.10.0-0.325-5-3-x86_64-64bit

Created attachment 353167 [details] Patch

Created attachment 353171 [details] Patch

Comment on attachment 353171 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=353171&action=review > Source/WebCore/Modules/mediarecorder/MediaRecorder.cpp:71 > + track->removeObserver(*this); Please see my comment on a previous patch. > Source/WebCore/Modules/mediarecorder/MediaRecorder.cpp:112 > + return; Do we have a test for this check? > Source/WebCore/Modules/mediarecorder/MediaRecorder.cpp:142 > + return; Ditto. > Source/WebCore/platform/mediarecorder/MediaRecorderPrivateMock.cpp:35 > + UNUSED_PARAM(mediaSample); Not needed, just remove mediaSample. > Source/WebCore/platform/mediarecorder/MediaRecorderPrivateMock.cpp:40 > + const String mockString = makeString("Video Track ID: ", track.id(), " Counter: ", String::number(++m_counter), "\r\n---------\r\n"); See comment in previous patch here and below. > Source/WebCore/platform/mediarecorder/MediaRecorderPrivateMock.h:37 > + ~MediaRecorderPrivateMock() { } Do we need to declare these? > Source/WebCore/platform/mediarecorder/MediaRecorderPrivateMock.h:42 > + Ref<Blob> fetchData() final; All 3 should be private. > Source/WebCore/platform/mediarecorder/MediaRecorderPrivateMock.h:46 > + unsigned m_counter = 0; We usually do: unsigned m_counter { 0 }; > LayoutTests/imported/w3c/web-platform-tests/mediacapture-record/MediaRecorder-dataavailable.html:35 > + let recorder = new MediaRecorder(video); could be const here and below. > LayoutTests/imported/w3c/web-platform-tests/mediacapture-record/MediaRecorder-dataavailable.html:38 > + dataAvailableAssertions(blobEvent); It might be good to verify that the addition of blob size is not zero once done. > LayoutTests/imported/w3c/web-platform-tests/mediacapture-record/MediaRecorder-dataavailable.html:47 > + }, 'MediaRecorder will fire a dataavailable event which only contains video buffers for a video-only stream when stop() is called', { timeout: 10000 }); The title should be updated since we are not checking video buffers. > LayoutTests/imported/w3c/web-platform-tests/mediacapture-record/MediaRecorder-stop.html:32 > }, "MediaRecorder will stop recording and fire a stop event when all tracks are ended", { timeout: 10000 }); I am not sure we need { timeout: 10000 } here and below. If this is needed, it might be best to split each test in its own file. > LayoutTests/imported/w3c/web-platform-tests/mediacapture-record/MediaRecorder-stop.html:45 > + recorder.stop(); Can we assert the state here? > LayoutTests/platform/win/TestExpectations:4228 > +http/wpt/mediarecorder/MediaRecorder-mock-dataavailable.html [ Skip ] We should probably skip http/wpt/mediarecorder

Comment on attachment 353171 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=353171&action=review >> Source/WebCore/Modules/mediarecorder/MediaRecorder.cpp:71 >> + track->removeObserver(*this); > > Please see my comment on a previous patch. I asked Wendy to add this, I believe we need this in case the MediaRecorder's scriptExecutionContext goes away while it is recording. In the new model, we register the observer when we start recording and we unregister the observer when we stop the recording, either because: 1. stopRecording() has been called 2. stop() has been called (i.e. the script execution context is going away) 3. The MediaRecorder object gets destroyed. Note that with iframes, it is very easy to keep another frame's MediaRecorder alive, after that frame is detached (and thus its script execution context is done). I think we would want to stop the recording as soon as the iframe is detached, no?

(In reply to Chris Dumez from comment #28) > Comment on attachment 353171 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=353171&action=review > > >> Source/WebCore/Modules/mediarecorder/MediaRecorder.cpp:71 > >> + track->removeObserver(*this); > > > > Please see my comment on a previous patch. > > I asked Wendy to add this, I believe we need this in case the > MediaRecorder's scriptExecutionContext goes away while it is recording. In > the new model, we register the observer when we start recording and we > unregister the observer when we stop the recording, either because: > 1. stopRecording() has been called > 2. stop() has been called (i.e. the script execution context is going away) > 3. The MediaRecorder object gets destroyed. > > Note that with iframes, it is very easy to keep another frame's > MediaRecorder alive, after that frame is detached (and thus its script > execution context is done). I think we would want to stop the recording as > soon as the iframe is detached, no? I mean, what would be the point in keeping the observer after the script execution context is gone and we stop firing the events. The MediaRecoder's destructor may get called a lot later if another frame keeps the MediaRecorder alive after its iframe is gone. Would seem bad to keep the register and populate a buffer during all this time.

> > LayoutTests/imported/w3c/web-platform-tests/mediacapture-record/MediaRecorder-stop.html:45 > > + recorder.stop(); > > Can we assert the state here? Do you mean assert the inactive state here? I am not sure if my understanding is correct, but the spec seems show stop() will change the state asynchronously while the start() will change the state synchronously.

(In reply to Wendy from comment #30) > > > LayoutTests/imported/w3c/web-platform-tests/mediacapture-record/MediaRecorder-stop.html:45 > > > + recorder.stop(); > > > > Can we assert the state here? > > Do you mean assert the inactive state here? > I am not sure if my understanding is correct, but the spec seems show stop() > will change the state asynchronously while the start() will change the state > synchronously. we should assert that state remains the same after calling stop().

(In reply to Chris Dumez from comment #29) > (In reply to Chris Dumez from comment #28) > > Comment on attachment 353171 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=353171&action=review > > > > >> Source/WebCore/Modules/mediarecorder/MediaRecorder.cpp:71 > > >> + track->removeObserver(*this); > > > > > > Please see my comment on a previous patch. > > > > I asked Wendy to add this, I believe we need this in case the > > MediaRecorder's scriptExecutionContext goes away while it is recording. In > > the new model, we register the observer when we start recording and we > > unregister the observer when we stop the recording, either because: > > 1. stopRecording() has been called > > 2. stop() has been called (i.e. the script execution context is going away) > > 3. The MediaRecorder object gets destroyed. > > > > Note that with iframes, it is very easy to keep another frame's > > MediaRecorder alive, after that frame is detached (and thus its script > > execution context is done). I think we would want to stop the recording as > > soon as the iframe is detached, no? > > I mean, what would be the point in keeping the observer after the script > execution context is gone and we stop firing the events. The MediaRecoder's > destructor may get called a lot later if another frame keeps the > MediaRecorder alive after its iframe is gone. Would seem bad to keep the > register and populate a buffer during all this time. My comment was indeed not cleat at all. I agree with this added code. In previous bug, I believe I said we should treat a/v frame callbacks differently from other callbacks given how frequently they are called. What I mean is that we will be trying to remove from observers several times. Given it is a HashSet, it is fine but we could try to iterate over the tracks to remove/add only if not already removed/added. A boolean is ok, TracksObserver as described in a previous comment is fine too.

Comment on attachment 353171 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=353171&action=review > Source/WebCore/Modules/mediarecorder/MediaRecorder.cpp:102 > + for (auto& track : m_tracks) Based on the spec, I think this should be inside the scheduleDeferredTask(). If you do then, then if other places where you unregister the observer, you could do it only if the state is Recording. E.g. void MediaRecorder::stopRecordingInternal() { if (state() != RecordingState::Recording) return; for (auto& track : m_tracks) track->addObserver(*this); m_state = RecordingState::Inactive; } Then call stopRecordingInternal() in: - The destructor - stop() - stopRecording() inside the deferred task.

Comment on attachment 353171 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=353171&action=review >> Source/WebCore/Modules/mediarecorder/MediaRecorder.cpp:102 >> + for (auto& track : m_tracks) > > Based on the spec, I think this should be inside the scheduleDeferredTask(). > > If you do then, then if other places where you unregister the observer, you could do it only if the state is Recording. > > E.g. > > void MediaRecorder::stopRecordingInternal() > { > if (state() != RecordingState::Recording) > return; > > for (auto& track : m_tracks) > track->addObserver(*this); > > m_state = RecordingState::Inactive; > } > > Then call stopRecordingInternal() in: > - The destructor > - stop() > - stopRecording() inside the deferred task. I meant: void MediaRecorder::stopRecordingInternal() { if (state() != RecordingState::Recording) return; for (auto& track : m_tracks) track->removeObserver(*this); m_state = RecordingState::Inactive; }

Comment on attachment 353171 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=353171&action=review >>> Source/WebCore/Modules/mediarecorder/MediaRecorder.cpp:102 >>> + for (auto& track : m_tracks) >> >> Based on the spec, I think this should be inside the scheduleDeferredTask(). >> >> If you do then, then if other places where you unregister the observer, you could do it only if the state is Recording. >> >> E.g. >> >> void MediaRecorder::stopRecordingInternal() >> { >> if (state() != RecordingState::Recording) >> return; >> >> for (auto& track : m_tracks) >> track->addObserver(*this); >> >> m_state = RecordingState::Inactive; >> } >> >> Then call stopRecordingInternal() in: >> - The destructor >> - stop() >> - stopRecording() inside the deferred task. > > I meant: > > void MediaRecorder::stopRecordingInternal() > { > if (state() != RecordingState::Recording) > return; > > for (auto& track : m_tracks) > track->removeObserver(*this); > > m_state = RecordingState::Inactive; > } Agreed that this aligns better with the spec, even though the spec does not mandate it per se. Also that would resolve my above concern and should work well with pause.

Created attachment 353327 [details] Patch

Comment on attachment 353327 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=353327&action=review I did not look too closely at the tests, will let Youenn do a full pass. Several of my earlier comments have not been taken into account though. > Source/WebCore/Modules/mediarecorder/BlobEvent.cpp:37 > + auto blob = init.data.releaseNonNull(); Not needed as commented several iterations ago. > Source/WebCore/Modules/mediarecorder/BlobEvent.cpp:47 > + : Event(type, WTFMove(init), isTrusted) As commented earlier, you don't need to WTFMove() init here. Then you don't need to separate Blob parameter. > Source/WebCore/Modules/mediarecorder/MediaRecorder.cpp:53 > + m_tracks = WTF::map(m_stream->getTracks(), [] (const auto& track) -> Ref<MediaStreamTrackPrivate> { I don't think you need the "const", we like to omit it in WebKit as much as possible. auto& track should work fine here. > Source/WebCore/Modules/mediarecorder/MediaRecorder.cpp:54 > auto& privateTrack = track->privateTrack(); Unnecessary local variable, just return track->privateTrack(); > Source/WebCore/Modules/mediarecorder/MediaRecorder.cpp:117 > + dispatchEvent(BlobEvent::create(eventNames().dataavailableEvent, Event::CanBubble::No, Event::IsCancelable::No, m_private->fetchData())); This call is safe because scheduleDeferredTask() takes care of capturing a WeakPtr and makes sure not to call this lambda if |this| has been destroyed. > Source/WebCore/Modules/mediarecorder/MediaRecorder.cpp:118 > + if (!m_isActive) However, there is a potential use-after-free here since nothing keeps you alive. The call to dispatchEvent() will run javascript and this script may destroy the MediaRecorder. What I would do is: 1. capture `weakThis = makeWeakPtr(*this)` in your lambda 2. `if (!weakThis || !m_isActive) return` here. > Source/WebCore/Modules/mediarecorder/MediaRecorder.cpp:148 > + if (!m_isActive) Potential use-after-free here. > Source/WebCore/Modules/mediarecorder/MediaRecorder.h:66 > ExceptionOr<void> start(std::optional<int>); nit: Could we call this one startRecording() for consistency with the new one? > Source/WebCore/platform/mediarecorder/MediaRecorderPrivate.h:52 > +protected: I think it is considered bad practice to have protected data members in an interface. You may want to make the data member private and have a protected getter instead. Maybe even better, instead of a mere getter, it could be: Locker<Lock> takeLock() { return holdLock(m_bufferLock); } > Source/WebCore/platform/mediarecorder/MediaRecorderPrivateMock.cpp:36 > + m_buffer.append(makeString("Video Track ID: ", track.id(), " Counter: ", String::number(++m_counter), "\r\n---------\r\n")); No need for String::number() here, just include <wtf/text/StringConcatenateNumbers.h>. > Source/WebCore/platform/mediarecorder/MediaRecorderPrivateMock.cpp:42 > + m_buffer.append(makeString("Audio Track ID: ", track.id(), " Counter: ", String::number(++m_counter), "\r\n---------\r\n")); ditto. > Source/WebCore/platform/mediarecorder/MediaRecorderPrivateMock.h:36 > + MediaRecorderPrivateMock() { } MediaRecorderPrivateMock() = default; > LayoutTests/imported/w3c/web-platform-tests/mediacapture-record/MediaRecorder-destroy-script-execution.html:12 > + document.body.removeChild(document.getElementById('subFrame')); FYI, this is much shorter and should work just as well: subFrame.remove();

Created attachment 353346 [details] Patch

Created attachment 353348 [details] Patch

Created attachment 353350 [details] Patch

Comment on attachment 353350 [details] Patch This patch goes in the right direction. The amount of tests is great. Some suggestions below. View in context: https://bugs.webkit.org/attachment.cgi?id=353350&action=review > Source/WebCore/Modules/mediarecorder/BlobEvent.h:52 > + EventInterface eventInterface() const override; s/override/final/ > Source/WebCore/Modules/mediarecorder/MediaRecorder.cpp:53 > + m_tracks = WTF::map(m_stream->getTracks(), [] (auto track) -> Ref<MediaStreamTrackPrivate> { s/auto/auto&/ > Source/WebCore/Modules/mediarecorder/MediaRecorder.cpp:71 > +void MediaRecorder::stopRecordingInternal() I would move stopRecordingInternal just after MediaRecorder::startRecording. That way, it is clear that add/remove observers is based on m_state. > Source/WebCore/Modules/mediarecorder/MediaRecorder.cpp:110 > + scheduleDeferredTask([weakThis = makeWeakPtr(*this), this] { We now create two weakThis, one for scheduleDeferredTask and one in this callback. It seems simpler and more robust to take a Ref<MediaRecorder> in this lambda and below. scheduleDeferredTask could be renamed in postTask and no longer take a weakThis. Could be done as a follow-up though. > Source/WebCore/Modules/mediarecorder/MediaRecorder.cpp:115 > + m_state = RecordingState::Inactive; I would think that stopRecordingInternal would ensure m_state to be Inactive. Maybe we just need to do ASSERT(m_state == RecordingState::Inactive). > Source/WebCore/Modules/mediarecorder/MediaRecorder.cpp:141 > + scheduleDeferredTask([weakThis = makeWeakPtr(*this), this] { This code is exactly the same code as for stopRecording(). Can we try sharing the code? > Source/WebCore/Modules/mediarecorder/MediaRecorder.cpp:146 > + m_state = RecordingState::Inactive; Ditto here. > Source/WebCore/platform/mediarecorder/MediaRecorderPrivate.h:30 > +#include "MediaStreamTrackPrivate.h" Can we try to forward declare these? > Source/WebCore/platform/mediarecorder/MediaRecorderPrivate.h:31 > +#include <wtf/Lock.h> No need for Lock.h if we move it to private mock. > Source/WebCore/platform/mediarecorder/MediaRecorderPrivate.h:51 > + Locker<Lock> takeLock() { return holdLock(m_bufferLock); } I do not think we should have this there or as public. Instead it should be a method of MediaRecorderPrivateMock. Ditto for m_bufferLock. > Source/WebCore/platform/mediarecorder/MediaRecorderPrivateMock.cpp:38 > + m_buffer.append(makeString("Video Track ID: ", track.id(), " Counter: ", ++m_counter, "\r\n---------\r\n")); As a small improvement, we could call m_buffer.append on each value. This will remove the need to allocate a String for the concatenation. Would the following work: m_buffer.append("Video Track ID: "); m_buffer.append(track.id()) m_buffer.append(" Counter: ") m_buffer.append(++m_counter) m_buffer.append("\r\n---------\r\n") Note that you could share some of the code between sampleBufferUpdated and audioSamplesAvailable. > LayoutTests/http/wpt/mediarecorder/MediaRecorder-mock-dataavailable.html:44 > + }, 500); I wonder whether this will cause some flakiness, hopefully not. We should think of using requestData to timeslice once implemented to make these tests no longer rely on a timer like that. Ditto for other tests. > LayoutTests/imported/w3c/web-platform-tests/mediacapture-record/BlobEvent-constructor-expected.txt:4 > +PASS The BlobEvent instance's data attribute is set. Nice :) > LayoutTests/imported/w3c/web-platform-tests/mediacapture-record/MediaRecorder-dataavailable.html:1 > +<!doctype html> Let's move that test to LayoutTests/http/wpt for now, given the potential for flakiness. Once we have timeslice or requestData, we should be able to make them more robust. > LayoutTests/imported/w3c/web-platform-tests/mediacapture-record/MediaRecorder-destroy-script-execution.html:1 > +<!doctype html> Nice tests! > LayoutTests/imported/w3c/web-platform-tests/mediacapture-record/MediaRecorder-destroy-script-execution.html:21 > + iframeForCallingStop.remove(); I wonder whether calling gc() at that moment would ensure we have coverage of the weakThis check. > LayoutTests/imported/w3c/web-platform-tests/mediacapture-record/MediaRecorder-destroy-script-execution.html:30 > + }, 500); Do we need to wait for 500ms? Maybe we only need to call recorder.stop() here. Even if we do not have data, we will fire both events. > LayoutTests/imported/w3c/web-platform-tests/mediacapture-record/MediaRecorder-destroy-script-execution.html:46 > + }, 500); Ditto here.

Comment on attachment 353350 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=353350&action=review >> Source/WebCore/Modules/mediarecorder/MediaRecorder.cpp:53 >> + m_tracks = WTF::map(m_stream->getTracks(), [] (auto track) -> Ref<MediaStreamTrackPrivate> { > > s/auto/auto&/ actually, I think it is auto&& because m_stream->getTracks() returns a temporary and WTF::map() will give us a RefPtr<>&& instead of a const RefPtr<>& in this case.

Created attachment 353414 [details] Patch

Comment on attachment 353414 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=353414&action=review > Source/WebCore/Modules/mediarecorder/MediaRecorder.cpp:114 > +void MediaRecorder::queueTaskForStopRecording() I guess we can do this to share the code. But is there a reason to not call stopRecording() directly from trackEnded() instead?

Created attachment 353421 [details] Patch

Comment on attachment 353421 [details] Patch LGTM. View in context: https://bugs.webkit.org/attachment.cgi?id=353421&action=review > Source/WebCore/Modules/mediarecorder/MediaRecorder.cpp:169 > return; We probably do not need that check, it seems that the given function will in most case do a check on its state().

Created attachment 353493 [details] Patch

Comment on attachment 353493 [details] Patch Let's land it once more bots are green

Comment on attachment 353493 [details] Patch Clearing flags on attachment: 353493 Committed r237642: <https://trac.webkit.org/changeset/237642>

All reviewed patches have been landed. Closing bug.

<rdar://problem/45703574>

The test http/wpt/mediarecorder/MediaRecorder-mock-dataavailable.html added in https://trac.webkit.org/changeset/237642/webkit is failing across platforms History: https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=http%2Fwpt%2Fmediarecorder%2FMediaRecorder-mock-dataavailable.html Diff: --- /Volumes/Data/slave/sierra-release-tests-wk2/build/layout-test-results/http/wpt/mediarecorder/MediaRecorder-mock-dataavailable-expected.txt +++ /Volumes/Data/slave/sierra-release-tests-wk2/build/layout-test-results/http/wpt/mediarecorder/MediaRecorder-mock-dataavailable-actual.txt @@ -1,5 +1,5 @@ PASS MediaRecorder will fire a dataavailable event which only contains video buffers for a video-only stream when stop() is called -PASS MediaRecorder will fire a dataavailable event which only contains audio buffers for a audio-only stream when stop() is called +FAIL MediaRecorder will fire a dataavailable event which only contains audio buffers for a audio-only stream when stop() is called The object is in an invalid state. PASS MediaRecorder will fire a dataavailable event which only contains both video and audio buffers when stop() is called

Maybe we need to increase the timer to something like 2 seconds until we support timeslice. (In reply to Truitt Savell from comment #52) > The test http/wpt/mediarecorder/MediaRecorder-mock-dataavailable.html > > added in https://trac.webkit.org/changeset/237642/webkit > > is failing across platforms > > History: > https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard. > html#showAllRuns=true&tests=http%2Fwpt%2Fmediarecorder%2FMediaRecorder-mock- > dataavailable.html > > Diff: > --- > /Volumes/Data/slave/sierra-release-tests-wk2/build/layout-test-results/http/ > wpt/mediarecorder/MediaRecorder-mock-dataavailable-expected.txt > +++ > /Volumes/Data/slave/sierra-release-tests-wk2/build/layout-test-results/http/ > wpt/mediarecorder/MediaRecorder-mock-dataavailable-actual.txt > @@ -1,5 +1,5 @@ > > PASS MediaRecorder will fire a dataavailable event which only contains > video buffers for a video-only stream when stop() is called > -PASS MediaRecorder will fire a dataavailable event which only contains > audio buffers for a audio-only stream when stop() is called > +FAIL MediaRecorder will fire a dataavailable event which only contains > audio buffers for a audio-only stream when stop() is called The object is in > an invalid state. > PASS MediaRecorder will fire a dataavailable event which only contains both > video and audio buffers when stop() is called

Reopening to attach new patch.

Created attachment 353521 [details] Patch

Comment on attachment 353521 [details] Patch Clearing flags on attachment: 353521 Committed r237650: <https://trac.webkit.org/changeset/237650>

All reviewed patches have been landed. Closing bug.

(In reply to WebKit Commit Bot from comment #56) > Committed r237650: <https://trac.webkit.org/changeset/237650> http/wpt/mediarecorder/MediaRecorder-mock-dataavailable.html contines to be a flaky failure after this fix attempt: https://build.webkit.org/results/Apple%20Mojave%20Release%20WK1%20(Tests)/r237659%20(641)/results.html

Reopening to attach new patch.

Created attachment 353572 [details] Patch

Comment on attachment 353572 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=353572&action=review > LayoutTests/ChangeLog:9 > + Remove share FileReader object between three asynchronous tests which might cause flaky failure. This kind of issue often happens with asynchronous tests. I recommend not using async_test directly. Instead - Use promise_test in multi test pages (promise_test is just a way to trigger async_test that are executed one after the other). - Use single page test which wraps the asserts in an async_test that testharness.js creates for you.

Comment on attachment 353572 [details] Patch Clearing flags on attachment: 353572 Committed r237675: <https://trac.webkit.org/changeset/237675>

All reviewed patches have been landed. Closing bug.