- Revision
- 274154
- Author
- [email protected]
- Date
- 2021-03-09 09:38:47 -0800 (Tue, 09 Mar 2021)
Log Message
MediaRecorder.requestData() not returning all captured media after a pause
https://bugs.webkit.org/show_bug.cgi?id=222285
<rdar://problem/74884561>
Reviewed by Eric Carlson.
Source/WebCore:
Previously, when flushing, we are called on a background thread and we hop to the main thread to append data.
In some cases, we were resolving the completion handlers before appending all data.
To prevent this, we now append data from a background thread.
To do so, we lock when accessing m_data, either to append or take the data.
In addition, we cancel writing when clearing the writer.
This allows to clean the writer delegate without fearing that write operations are happening.
Covered by updated test.
* platform/mediarecorder/cocoa/MediaRecorderPrivateWriterCocoa.h:
* platform/mediarecorder/cocoa/MediaRecorderPrivateWriterCocoa.mm:
(-[WebAVAssetWriterDelegate initWithWriter:]):
(-[WebAVAssetWriterDelegate assetWriter:didProduceFragmentedHeaderData:]):
(-[WebAVAssetWriterDelegate assetWriter:didProduceFragmentedMediaData:fragmentedMediaDataReport:]):
(WebCore::MediaRecorderPrivateWriter::initialize):
(WebCore::MediaRecorderPrivateWriter::clear):
(WebCore::MediaRecorderPrivateWriter::stopRecording):
(WebCore::MediaRecorderPrivateWriter::completeFetchData):
(WebCore::MediaRecorderPrivateWriter::appendData):
(WebCore::MediaRecorderPrivateWriter::takeData):
LayoutTests:
* http/wpt/mediarecorder/pause-recording-expected.txt:
* http/wpt/mediarecorder/pause-recording.html:
Modified Paths
Diff
Modified: trunk/LayoutTests/ChangeLog (274153 => 274154)
--- trunk/LayoutTests/ChangeLog 2021-03-09 17:37:44 UTC (rev 274153)
+++ trunk/LayoutTests/ChangeLog 2021-03-09 17:38:47 UTC (rev 274154)
@@ -1,3 +1,14 @@
+2021-03-09 Youenn Fablet <[email protected]>
+
+ MediaRecorder.requestData() not returning all captured media after a pause
+ https://bugs.webkit.org/show_bug.cgi?id=222285
+ <rdar://problem/74884561>
+
+ Reviewed by Eric Carlson.
+
+ * http/wpt/mediarecorder/pause-recording-expected.txt:
+ * http/wpt/mediarecorder/pause-recording.html:
+
2021-03-09 Philippe Normand <[email protected]>
[WebRTC][GStreamer] webrtc/multi-video.html crashes
Modified: trunk/LayoutTests/http/wpt/mediarecorder/pause-recording-expected.txt (274153 => 274154)
--- trunk/LayoutTests/http/wpt/mediarecorder/pause-recording-expected.txt 2021-03-09 17:37:44 UTC (rev 274153)
+++ trunk/LayoutTests/http/wpt/mediarecorder/pause-recording-expected.txt 2021-03-09 17:38:47 UTC (rev 274154)
@@ -1,4 +1,5 @@
PASS Pausing and resuming the recording should impact the video duration
+PASS Calling requestData once after pausing should lead to more than header data
Modified: trunk/LayoutTests/http/wpt/mediarecorder/pause-recording.html (274153 => 274154)
--- trunk/LayoutTests/http/wpt/mediarecorder/pause-recording.html 2021-03-09 17:37:44 UTC (rev 274153)
+++ trunk/LayoutTests/http/wpt/mediarecorder/pause-recording.html 2021-03-09 17:38:47 UTC (rev 274154)
@@ -40,6 +40,25 @@
URL.revokeObjectURL(url);
}, "Pausing and resuming the recording should impact the video duration");
+promise_test(async (test) => {
+ const stream = await navigator.mediaDevices.getUserMedia({ audio: true, video: true });
+
+ const recorder = new MediaRecorder(stream);
+ const dataPromise = new Promise(resolve => recorder._ondataavailable_ = (e) => resolve(e.data));
+
+ const startPromise = new Promise(resolve => recorder._onstart_ = resolve);
+ recorder.start();
+ await startPromise;
+
+ await waitFor(1000);
+ recorder.pause();
+ recorder.requestData();
+
+ const blob = await dataPromise;
+ assert_greater_than(blob.size, 2000);
+}, "Calling requestData once after pausing should lead to more than header data");
+
+
</script>
</body>
</html>
Modified: trunk/Source/WebCore/ChangeLog (274153 => 274154)
--- trunk/Source/WebCore/ChangeLog 2021-03-09 17:37:44 UTC (rev 274153)
+++ trunk/Source/WebCore/ChangeLog 2021-03-09 17:38:47 UTC (rev 274154)
@@ -1,3 +1,32 @@
+2021-03-09 Youenn Fablet <[email protected]>
+
+ MediaRecorder.requestData() not returning all captured media after a pause
+ https://bugs.webkit.org/show_bug.cgi?id=222285
+ <rdar://problem/74884561>
+
+ Reviewed by Eric Carlson.
+
+ Previously, when flushing, we are called on a background thread and we hop to the main thread to append data.
+ In some cases, we were resolving the completion handlers before appending all data.
+ To prevent this, we now append data from a background thread.
+ To do so, we lock when accessing m_data, either to append or take the data.
+ In addition, we cancel writing when clearing the writer.
+ This allows to clean the writer delegate without fearing that write operations are happening.
+
+ Covered by updated test.
+
+ * platform/mediarecorder/cocoa/MediaRecorderPrivateWriterCocoa.h:
+ * platform/mediarecorder/cocoa/MediaRecorderPrivateWriterCocoa.mm:
+ (-[WebAVAssetWriterDelegate initWithWriter:]):
+ (-[WebAVAssetWriterDelegate assetWriter:didProduceFragmentedHeaderData:]):
+ (-[WebAVAssetWriterDelegate assetWriter:didProduceFragmentedMediaData:fragmentedMediaDataReport:]):
+ (WebCore::MediaRecorderPrivateWriter::initialize):
+ (WebCore::MediaRecorderPrivateWriter::clear):
+ (WebCore::MediaRecorderPrivateWriter::stopRecording):
+ (WebCore::MediaRecorderPrivateWriter::completeFetchData):
+ (WebCore::MediaRecorderPrivateWriter::appendData):
+ (WebCore::MediaRecorderPrivateWriter::takeData):
+
2021-03-09 Philippe Normand <[email protected]>
[WebRTC][GStreamer] webrtc/multi-video.html crashes
Modified: trunk/Source/WebCore/platform/mediarecorder/cocoa/MediaRecorderPrivateWriterCocoa.h (274153 => 274154)
--- trunk/Source/WebCore/platform/mediarecorder/cocoa/MediaRecorderPrivateWriterCocoa.h 2021-03-09 17:37:44 UTC (rev 274153)
+++ trunk/Source/WebCore/platform/mediarecorder/cocoa/MediaRecorderPrivateWriterCocoa.h 2021-03-09 17:38:47 UTC (rev 274154)
@@ -76,7 +76,6 @@
void resume();
void appendData(const char*, size_t);
- void appendData(Ref<SharedBuffer>&&);
const String& mimeType() const;
unsigned audioBitRate() const;
@@ -108,6 +107,7 @@
void finishedFlushingSamples();
void completeFetchData();
+ RefPtr<SharedBuffer> takeData();
bool m_hasAudio { false };
bool m_hasVideo { false };
@@ -118,6 +118,7 @@
RetainPtr<AVAssetWriter> m_writer;
+ Lock m_dataLock;
RefPtr<SharedBuffer> m_data;
CompletionHandler<void(RefPtr<SharedBuffer>&&, double)> m_fetchDataCompletionHandler;
Modified: trunk/Source/WebCore/platform/mediarecorder/cocoa/MediaRecorderPrivateWriterCocoa.mm (274153 => 274154)
--- trunk/Source/WebCore/platform/mediarecorder/cocoa/MediaRecorderPrivateWriterCocoa.mm 2021-03-09 17:37:44 UTC (rev 274153)
+++ trunk/Source/WebCore/platform/mediarecorder/cocoa/MediaRecorderPrivateWriterCocoa.mm 2021-03-09 17:38:47 UTC (rev 274154)
@@ -50,10 +50,10 @@
#include <pal/cocoa/AVFoundationSoftLink.h>
@interface WebAVAssetWriterDelegate : NSObject <AVAssetWriterDelegate> {
- WeakPtr<WebCore::MediaRecorderPrivateWriter> m_writer;
+ WebCore::MediaRecorderPrivateWriter* m_writer;
}
-- (instancetype)initWithWriter:(WebCore::MediaRecorderPrivateWriter*)writer;
+- (instancetype)initWithWriter:(WebCore::MediaRecorderPrivateWriter&)writer;
- (void)close;
@end
@@ -61,12 +61,12 @@
@implementation WebAVAssetWriterDelegate {
};
-- (instancetype)initWithWriter:(WebCore::MediaRecorderPrivateWriter*)writer
+- (instancetype)initWithWriter:(WebCore::MediaRecorderPrivateWriter&)writer
{
ASSERT(isMainThread());
self = [super init];
if (self)
- self->m_writer = makeWeakPtr(writer);
+ self->m_writer = &writer;
return self;
}
@@ -74,18 +74,7 @@
- (void)assetWriter:(AVAssetWriter *)assetWriter didProduceFragmentedHeaderData:(NSData *)fragmentedHeaderData
{
UNUSED_PARAM(assetWriter);
- if (!isMainThread()) {
- if (auto size = [fragmentedHeaderData length]) {
- callOnMainThread([protectedSelf = RetainPtr<WebAVAssetWriterDelegate>(self), buffer = WebCore::SharedBuffer::create(static_cast<const char*>([fragmentedHeaderData bytes]), size)]() mutable {
- if (protectedSelf->m_writer)
- protectedSelf->m_writer->appendData(WTFMove(buffer));
- });
- }
- return;
- }
-
- if (m_writer)
- m_writer->appendData(static_cast<const char*>([fragmentedHeaderData bytes]), [fragmentedHeaderData length]);
+ m_writer->appendData(static_cast<const char*>([fragmentedHeaderData bytes]), [fragmentedHeaderData length]);
}
- (void)assetWriter:(AVAssetWriter *)assetWriter didProduceFragmentedMediaData:(NSData *)fragmentedMediaData fragmentedMediaDataReport:(AVFragmentedMediaDataReport *)fragmentedMediaDataReport
@@ -92,18 +81,7 @@
{
UNUSED_PARAM(assetWriter);
UNUSED_PARAM(fragmentedMediaDataReport);
- if (!isMainThread()) {
- if (auto size = [fragmentedMediaData length]) {
- callOnMainThread([protectedSelf = RetainPtr<WebAVAssetWriterDelegate>(self), buffer = WebCore::SharedBuffer::create(static_cast<const char*>([fragmentedMediaData bytes]), size)]() mutable {
- if (protectedSelf->m_writer)
- protectedSelf->m_writer->appendData(WTFMove(buffer));
- });
- }
- return;
- }
-
- if (m_writer)
- m_writer->appendData(static_cast<const char*>([fragmentedMediaData bytes]), [fragmentedMediaData length]);
+ m_writer->appendData(static_cast<const char*>([fragmentedMediaData bytes]), [fragmentedMediaData length]);
}
- (void)close
@@ -164,7 +142,7 @@
return false;
}
- m_writerDelegate = adoptNS([[WebAVAssetWriterDelegate alloc] initWithWriter: this]);
+ m_writerDelegate = adoptNS([[WebAVAssetWriterDelegate alloc] initWithWriter: *this]);
[m_writer.get() setDelegate:m_writerDelegate.get()];
if (m_hasAudio) {
@@ -396,10 +374,16 @@
{
m_pendingAudioSampleQueue.clear();
m_pendingVideoSampleQueue.clear();
- if (m_writer)
+ if (m_writer) {
+ [m_writer cancelWriting];
m_writer.clear();
+ }
+ // At this pointer, we should no longer be writing any data, so it should be safe to close and nullify m_data without locking.
+ if (m_writerDelegate)
+ [m_writerDelegate close];
m_data = nullptr;
+
if (auto completionHandler = WTFMove(m_fetchDataCompletionHandler))
completionHandler(nullptr, 0);
}
@@ -488,10 +472,13 @@
m_isStopped = false;
m_hasStartedWriting = false;
- if (m_writer)
+ if (m_writer) {
+ [m_writer cancelWriting];
m_writer.clear();
+ }
+
if (m_fetchDataCompletionHandler)
- m_fetchDataCompletionHandler(std::exchange(m_data, nullptr), 0);
+ m_fetchDataCompletionHandler(takeData(), 0);
};
if (!m_hasStartedWriting) {
@@ -551,11 +538,12 @@
auto sampleTime = CMTimeSubtract(CMClockGetTime(CMClockGetHostTimeClock()), m_resumedVideoTime);
m_timeCode = CMTimeGetSeconds(CMTimeAdd(sampleTime, m_currentVideoDuration));
}
- m_fetchDataCompletionHandler(std::exchange(m_data, nullptr), currentTimeCode);
+ m_fetchDataCompletionHandler(takeData(), currentTimeCode);
}
void MediaRecorderPrivateWriter::appendData(const char* data, size_t size)
{
+ auto locker = holdLock(m_dataLock);
if (!m_data) {
m_data = SharedBuffer::create(data, size);
return;
@@ -563,13 +551,11 @@
m_data->append(data, size);
}
-void MediaRecorderPrivateWriter::appendData(Ref<SharedBuffer>&& buffer)
+RefPtr<SharedBuffer> MediaRecorderPrivateWriter::takeData()
{
- if (!m_data) {
- m_data = WTFMove(buffer);
- return;
- }
- m_data->append(WTFMove(buffer));
+ auto locker = holdLock(m_dataLock);
+ auto data = ""
+ return data;
}
void MediaRecorderPrivateWriter::pause()
