Skip to content
This repository was archived by the owner on Jun 24, 2022. It is now read-only.

Commit 1c3243a

Browse files
author
[email protected]
committed
MediaRecorder stopRecorder() returns empty Blob after first use
https://bugs.webkit.org/show_bug.cgi?id=212274 <rdar://problem/63601298> Reviewed by Eric Carlson. Source/WebCore: Refactor code to create/destroy MediaRecorderPrivate on MediaRecorder start/stop. This allows reusing a MediaRecorder after a stop and restarting with a clean state. We introduce MediaRecorderPrivate::startRecording to do the initialization, which allows to fix a potential ref cycle as part of the error callback handling. Make some improvements to the platform implementation, in particular add default initialization to all fields. Align the code using AudioConverterRef to what is done in AudioSampleDataSource. Also call VTCompressionSessionInvalidate when destroying the VideoSampleBufferCompressor. Test: http/wpt/mediarecorder/MediaRecorder-multiple-start-stop.html * Modules/mediarecorder/MediaRecorder.cpp: (WebCore::MediaRecorder::create): (WebCore::MediaRecorder::MediaRecorder): (WebCore::MediaRecorder::startRecording): (WebCore::MediaRecorder::stopRecording): (WebCore::MediaRecorder::requestData): * Modules/mediarecorder/MediaRecorder.h: * platform/mediarecorder/MediaRecorderPrivateMock.cpp: (WebCore::MediaRecorderPrivateMock::fetchData): * platform/mediarecorder/MediaRecorderPrivate.h: (WebCore::MediaRecorderPrivate::startRecording): * platform/mediarecorder/cocoa/AudioSampleBufferCompressor.h: * platform/mediarecorder/cocoa/AudioSampleBufferCompressor.mm: (WebCore::AudioSampleBufferCompressor::~AudioSampleBufferCompressor): (WebCore::AudioSampleBufferCompressor::initAudioConverterForSourceFormatDescription): (WebCore::AudioSampleBufferCompressor::attachPrimingTrimsIfNeeded): (WebCore::AudioSampleBufferCompressor::gradualDecoderRefreshCount): (WebCore::AudioSampleBufferCompressor::sampleBufferWithNumPackets): (WebCore::AudioSampleBufferCompressor::processSampleBuffersUntilLowWaterTime): * platform/mediarecorder/cocoa/MediaRecorderPrivateWriterCocoa.h: * platform/mediarecorder/cocoa/MediaRecorderPrivateWriterCocoa.mm: (WebCore::MediaRecorderPrivateWriter::stopRecording): * platform/mediarecorder/cocoa/VideoSampleBufferCompressor.mm: (WebCore::VideoSampleBufferCompressor::~VideoSampleBufferCompressor): Source/WebKit: Update implementation to do initialization as part of startRecording. * GPUProcess/webrtc/RemoteMediaRecorderManager.cpp: (WebKit::RemoteMediaRecorderManager::releaseRecorder): Remove ASSERT as recorder creation in WebProcess is always ok while creation in GPUProcess may fail and m_recorders may not be populated. * WebProcess/GPU/webrtc/MediaRecorderPrivate.cpp: (WebKit::MediaRecorderPrivate::MediaRecorderPrivate): (WebKit::MediaRecorderPrivate::startRecording): * WebProcess/GPU/webrtc/MediaRecorderPrivate.h: LayoutTests: * http/wpt/mediarecorder/MediaRecorder-multiple-start-stop-expected.txt: Added. * http/wpt/mediarecorder/MediaRecorder-multiple-start-stop.html: Added. git-svn-id: http://svn.webkit.org/repository/webkit/trunk@263633 268f45cc-cd09-0410-ab3c-d52691b4dbfc
1 parent 084b3c8 commit 1c3243a

18 files changed

+226
-72
lines changed

‎LayoutTests/ChangeLog

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,14 @@
1+
2020-06-28 Youenn Fablet <[email protected]>
2+
3+
MediaRecorder stopRecorder() returns empty Blob after first use
4+
https://bugs.webkit.org/show_bug.cgi?id=212274
5+
<rdar://problem/63601298>
6+
7+
Reviewed by Eric Carlson.
8+
9+
* http/wpt/mediarecorder/MediaRecorder-multiple-start-stop-expected.txt: Added.
10+
* http/wpt/mediarecorder/MediaRecorder-multiple-start-stop.html: Added.
11+
112
2020-06-27 Zalan Bujtas <[email protected]>
213

314
[LFC][IFC] Replaced inline boxes sit on the baseline with their margins

‎LayoutTests/http/wpt/mediarecorder/MediaRecorder-multiple-start-stop-expected.txt

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,3 @@
1+
2+
PASS Make sure that MediaRecorder can be used after stopping it once
3+

‎LayoutTests/http/wpt/mediarecorder/MediaRecorder-multiple-start-stop.html

Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,49 @@
1+
<!doctype html>
2+
<html>
3+
<head>
4+
<title>MediaRecorder Dataavailable</title>
5+
<link rel="help" href="https://w3c.github.io/mediacapture-record/MediaRecorder.html#mediarecorder">
6+
<script src="/resources/testharness.js"></script>
7+
<script src="/resources/testharnessreport.js"></script>
8+
</head>
9+
<body>
10+
<canvas id="canvas" width="200" height="200">
11+
</canvas>
12+
<script>
13+
promise_test(async t => {
14+
const video = await navigator.mediaDevices.getUserMedia({ audio : true, video : true });
15+
const recorder = new MediaRecorder(video);
16+
17+
assert_equals(recorder.stream, video);
18+
19+
let promise = new Promise(resolve => {
20+
recorder.ondataavailable = t.step_func(blobEvent => {
21+
if (blobEvent.data.size)
22+
resolve();
23+
});
24+
});
25+
26+
recorder.start();
27+
let timer = setInterval(() => recorder.requestData(), 10);
28+
await promise;
29+
clearInterval(timer);
30+
31+
recorder.stop();
32+
33+
promise = new Promise(resolve => {
34+
recorder.ondataavailable = t.step_func(blobEvent => {
35+
if (blobEvent.data.size)
36+
resolve();
37+
});
38+
});
39+
40+
recorder.start();
41+
timer = setInterval(() => recorder.requestData(), 10);
42+
await promise;
43+
clearInterval(timer);
44+
45+
recorder.stop();
46+
}, 'Make sure that MediaRecorder can be used after stopping it once');
47+
</script>
48+
</body>
49+
</html>

‎Source/WebCore/ChangeLog

Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,48 @@
1+
2020-06-28 Youenn Fablet <[email protected]>
2+
3+
MediaRecorder stopRecorder() returns empty Blob after first use
4+
https://bugs.webkit.org/show_bug.cgi?id=212274
5+
<rdar://problem/63601298>
6+
7+
Reviewed by Eric Carlson.
8+
9+
Refactor code to create/destroy MediaRecorderPrivate on MediaRecorder start/stop.
10+
This allows reusing a MediaRecorder after a stop and restarting with a clean state.
11+
12+
We introduce MediaRecorderPrivate::startRecording to do the initialization,
13+
which allows to fix a potential ref cycle as part of the error callback handling.
14+
15+
Make some improvements to the platform implementation, in particular add default initialization to all fields.
16+
Align the code using AudioConverterRef to what is done in AudioSampleDataSource.
17+
Also call VTCompressionSessionInvalidate when destroying the VideoSampleBufferCompressor.
18+
19+
Test: http/wpt/mediarecorder/MediaRecorder-multiple-start-stop.html
20+
21+
* Modules/mediarecorder/MediaRecorder.cpp:
22+
(WebCore::MediaRecorder::create):
23+
(WebCore::MediaRecorder::MediaRecorder):
24+
(WebCore::MediaRecorder::startRecording):
25+
(WebCore::MediaRecorder::stopRecording):
26+
(WebCore::MediaRecorder::requestData):
27+
* Modules/mediarecorder/MediaRecorder.h:
28+
* platform/mediarecorder/MediaRecorderPrivateMock.cpp:
29+
(WebCore::MediaRecorderPrivateMock::fetchData):
30+
* platform/mediarecorder/MediaRecorderPrivate.h:
31+
(WebCore::MediaRecorderPrivate::startRecording):
32+
* platform/mediarecorder/cocoa/AudioSampleBufferCompressor.h:
33+
* platform/mediarecorder/cocoa/AudioSampleBufferCompressor.mm:
34+
(WebCore::AudioSampleBufferCompressor::~AudioSampleBufferCompressor):
35+
(WebCore::AudioSampleBufferCompressor::initAudioConverterForSourceFormatDescription):
36+
(WebCore::AudioSampleBufferCompressor::attachPrimingTrimsIfNeeded):
37+
(WebCore::AudioSampleBufferCompressor::gradualDecoderRefreshCount):
38+
(WebCore::AudioSampleBufferCompressor::sampleBufferWithNumPackets):
39+
(WebCore::AudioSampleBufferCompressor::processSampleBuffersUntilLowWaterTime):
40+
* platform/mediarecorder/cocoa/MediaRecorderPrivateWriterCocoa.h:
41+
* platform/mediarecorder/cocoa/MediaRecorderPrivateWriterCocoa.mm:
42+
(WebCore::MediaRecorderPrivateWriter::stopRecording):
43+
* platform/mediarecorder/cocoa/VideoSampleBufferCompressor.mm:
44+
(WebCore::VideoSampleBufferCompressor::~VideoSampleBufferCompressor):
45+
146
2020-06-27 Zalan Bujtas <[email protected]>
247

348
[LFC][IFC] Replaced inline boxes sit on the baseline with their margins

‎Source/WebCore/Modules/mediarecorder/MediaRecorder.cpp

Lines changed: 29 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -51,11 +51,8 @@ ExceptionOr<Ref<MediaRecorder>> MediaRecorder::create(Document& document, Ref<Me
5151
auto privateInstance = MediaRecorder::createMediaRecorderPrivate(document, stream->privateStream());
5252
if (!privateInstance)
5353
return Exception { NotSupportedError, "The MediaRecorder is unsupported on this platform"_s };
54-
auto recorder = adoptRef(*new MediaRecorder(document, WTFMove(stream), WTFMove(privateInstance), WTFMove(options)));
54+
auto recorder = adoptRef(*new MediaRecorder(document, WTFMove(stream), WTFMove(options)));
5555
recorder->suspendIfNeeded();
56-
recorder->m_private->setErrorCallback([recorder](auto&& exception) mutable {
57-
recorder->dispatchError(WTFMove(*exception));
58-
});
5956
return recorder;
6057
}
6158

@@ -82,11 +79,10 @@ std::unique_ptr<MediaRecorderPrivate> MediaRecorder::createMediaRecorderPrivate(
8279
#endif
8380
}
8481

85-
MediaRecorder::MediaRecorder(Document& document, Ref<MediaStream>&& stream, std::unique_ptr<MediaRecorderPrivate>&& privateImpl, Options&& option)
82+
MediaRecorder::MediaRecorder(Document& document, Ref<MediaStream>&& stream, Options&& option)
8683
: ActiveDOMObject(document)
8784
, m_options(WTFMove(option))
8885
, m_stream(WTFMove(stream))
89-
, m_private(WTFMove(privateImpl))
9086
{
9187
m_tracks = WTF::map(m_stream->getTracks(), [] (auto&& track) -> Ref<MediaStreamTrackPrivate> {
9288
return track->privateTrack();
@@ -132,9 +128,26 @@ const char* MediaRecorder::activeDOMObjectName() const
132128
ExceptionOr<void> MediaRecorder::startRecording(Optional<int> timeslice)
133129
{
134130
UNUSED_PARAM(timeslice);
131+
if (!m_isActive)
132+
return Exception { InvalidStateError, "The MediaRecorder is not active"_s };
133+
135134
if (state() != RecordingState::Inactive)
136135
return Exception { InvalidStateError, "The MediaRecorder's state must be inactive in order to start recording"_s };
137-
136+
137+
ASSERT(!m_private);
138+
m_private = createMediaRecorderPrivate(*document(), m_stream->privateStream());
139+
140+
if (!m_private)
141+
return Exception { NotSupportedError, "The MediaRecorder is unsupported on this platform"_s };
142+
143+
m_private->startRecording([this, pendingActivity = makePendingActivity(*this)](auto&& exception) mutable {
144+
if (!m_isActive || !exception)
145+
return;
146+
147+
stopRecordingInternal();
148+
dispatchError(WTFMove(*exception));
149+
});
150+
138151
for (auto& track : m_tracks)
139152
track->addObserver(*this);
140153

@@ -146,22 +159,17 @@ ExceptionOr<void> MediaRecorder::stopRecording()
146159
{
147160
if (state() == RecordingState::Inactive)
148161
return Exception { InvalidStateError, "The MediaRecorder's state cannot be inactive"_s };
149-
150-
queueTaskKeepingObjectAlive(*this, TaskSource::Networking, [this] {
151-
if (!m_isActive || state() == RecordingState::Inactive)
152-
return;
153162

154-
stopRecordingInternal();
155-
ASSERT(m_state == RecordingState::Inactive);
156-
m_private->fetchData([this, pendingActivity = makePendingActivity(*this)](auto&& buffer, auto& mimeType) {
163+
stopRecordingInternal();
164+
auto& privateRecorder = *m_private;
165+
privateRecorder.fetchData([this, pendingActivity = makePendingActivity(*this), privateRecorder = WTFMove(m_private)](auto&& buffer, auto& mimeType) {
166+
queueTaskKeepingObjectAlive(*this, TaskSource::Networking, [this, buffer = WTFMove(buffer), mimeType]() mutable {
157167
if (!m_isActive)
158168
return;
159-
160169
dispatchEvent(BlobEvent::create(eventNames().dataavailableEvent, Event::CanBubble::No, Event::IsCancelable::No, buffer ? Blob::create(buffer.releaseNonNull(), mimeType) : Blob::create()));
161170

162171
if (!m_isActive)
163172
return;
164-
165173
dispatchEvent(Event::create(eventNames().stopEvent, Event::CanBubble::No, Event::IsCancelable::No));
166174
});
167175
});
@@ -174,10 +182,12 @@ ExceptionOr<void> MediaRecorder::requestData()
174182
return Exception { InvalidStateError, "The MediaRecorder's state cannot be inactive"_s };
175183

176184
m_private->fetchData([this, pendingActivity = makePendingActivity(*this)](auto&& buffer, auto& mimeType) {
177-
if (!m_isActive)
178-
return;
185+
queueTaskKeepingObjectAlive(*this, TaskSource::Networking, [this, buffer = WTFMove(buffer), mimeType]() mutable {
186+
if (!m_isActive)
187+
return;
179188

180-
dispatchEvent(BlobEvent::create(eventNames().dataavailableEvent, Event::CanBubble::No, Event::IsCancelable::No, buffer ? Blob::create(buffer.releaseNonNull(), mimeType) : Blob::create()));
189+
dispatchEvent(BlobEvent::create(eventNames().dataavailableEvent, Event::CanBubble::No, Event::IsCancelable::No, buffer ? Blob::create(buffer.releaseNonNull(), mimeType) : Blob::create()));
190+
});
181191
});
182192
return { };
183193
}

‎Source/WebCore/Modules/mediarecorder/MediaRecorder.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -75,7 +75,7 @@ class MediaRecorder final
7575
MediaStream& stream() { return m_stream.get(); }
7676

7777
private:
78-
MediaRecorder(Document&, Ref<MediaStream>&&, std::unique_ptr<MediaRecorderPrivate>&&, Options&& = { });
78+
MediaRecorder(Document&, Ref<MediaStream>&&, Options&& = { });
7979

8080
static std::unique_ptr<MediaRecorderPrivate> createMediaRecorderPrivate(Document&, MediaStreamPrivate&);
8181

‎Source/WebCore/platform/mediarecorder/MediaRecorderPrivate.h

Lines changed: 2 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -60,16 +60,13 @@ class MediaRecorderPrivate
6060
virtual void fetchData(FetchDataCallback&&) = 0;
6161
virtual void stopRecording() = 0;
6262

63-
using ErrorCallback = Function<void(Optional<Exception>&&)>;
64-
void setErrorCallback(ErrorCallback&& errorCallback) { m_errorCallback = WTFMove(errorCallback); }
63+
using ErrorCallback = CompletionHandler<void(Optional<Exception>&&)>;
64+
virtual void startRecording(ErrorCallback&& callback) { callback({ }); }
6565

6666
protected:
6767
void setAudioSource(RefPtr<RealtimeMediaSource>&&);
6868
void setVideoSource(RefPtr<RealtimeMediaSource>&&);
6969

70-
protected:
71-
ErrorCallback m_errorCallback;
72-
7370
private:
7471
RefPtr<RealtimeMediaSource> m_audioSource;
7572
RefPtr<RealtimeMediaSource> m_videoSource;

‎Source/WebCore/platform/mediarecorder/MediaRecorderPrivateMock.cpp

Lines changed: 9 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -83,11 +83,15 @@ void MediaRecorderPrivateMock::generateMockCounterString()
8383

8484
void MediaRecorderPrivateMock::fetchData(FetchDataCallback&& completionHandler)
8585
{
86-
auto locker = holdLock(m_bufferLock);
87-
Vector<uint8_t> value(m_buffer.length());
88-
memcpy(value.data(), m_buffer.characters8(), m_buffer.length());
89-
m_buffer.clear();
90-
completionHandler(SharedBuffer::create(WTFMove(value)), mimeType());
86+
RefPtr<SharedBuffer> buffer;
87+
{
88+
auto locker = holdLock(m_bufferLock);
89+
Vector<uint8_t> value(m_buffer.length());
90+
memcpy(value.data(), m_buffer.characters8(), m_buffer.length());
91+
m_buffer.clear();
92+
buffer = SharedBuffer::create(WTFMove(value));
93+
}
94+
completionHandler(WTFMove(buffer), mimeType());
9195
}
9296

9397
const String& MediaRecorderPrivateMock::mimeType()

‎Source/WebCore/platform/mediarecorder/cocoa/AudioSampleBufferCompressor.h

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -60,23 +60,23 @@ class AudioSampleBufferCompressor {
6060
OSStatus provideSourceDataNumOutputPackets(UInt32*, AudioBufferList*, AudioStreamPacketDescription**);
6161

6262
dispatch_queue_t m_serialDispatchQueue;
63-
CMTime m_lowWaterTime;
63+
CMTime m_lowWaterTime { kCMTimeInvalid };
6464

6565
RetainPtr<CMBufferQueueRef> m_outputBufferQueue;
6666
RetainPtr<CMBufferQueueRef> m_inputBufferQueue;
6767
bool m_isEncoding { false };
6868

69-
RetainPtr<AudioConverterRef> m_converter;
69+
AudioConverterRef m_converter { nullptr };
7070
AudioStreamBasicDescription m_sourceFormat;
7171
AudioStreamBasicDescription m_destinationFormat;
7272
RetainPtr<CMFormatDescriptionRef> m_destinationFormatDescription;
7373
RetainPtr<NSNumber> m_gdrCountNum;
7474
UInt32 m_maxOutputPacketSize { 0 };
7575
Vector<AudioStreamPacketDescription> m_destinationPacketDescriptions;
7676

77-
CMTime m_currentNativePresentationTimeStamp;
78-
CMTime m_currentOutputPresentationTimeStamp;
79-
CMTime m_remainingPrimeDuration;
77+
CMTime m_currentNativePresentationTimeStamp { kCMTimeInvalid };
78+
CMTime m_currentOutputPresentationTimeStamp { kCMTimeInvalid };
79+
CMTime m_remainingPrimeDuration { kCMTimeInvalid };
8080

8181
Vector<char> m_sourceBuffer;
8282
Vector<char> m_destinationBuffer;

0 commit comments

Comments
 (0)