Changeset 274154 in webkit


Ignore:
Timestamp:
Mar 9, 2021 9:38:47 AM (17 months ago)
Author:
youenn@apple.com
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:
Location:
trunk
Files:
6 edited

Legend:

Unmodified
Added
Removed

  • trunk/LayoutTests/ChangeLog

    r274150 r274154  
     12021-03-09  Youenn Fablet  <youenn@apple.com>
     2
     3        MediaRecorder.requestData() not returning all captured media after a pause
     4        https://bugs.webkit.org/show_bug.cgi?id=222285
     5        <rdar://problem/74884561>
     6
     7        Reviewed by Eric Carlson.
     8
     9        * http/wpt/mediarecorder/pause-recording-expected.txt:
     10        * http/wpt/mediarecorder/pause-recording.html:
     11
    1122021-03-09  Philippe Normand  <pnormand@igalia.com>
    213

  • trunk/LayoutTests/http/wpt/mediarecorder/pause-recording-expected.txt

    r268130 r274154  
    22
    33PASS Pausing and resuming the recording should impact the video duration
     4PASS Calling requestData once after pausing should lead to more than header data
    45

  • trunk/LayoutTests/http/wpt/mediarecorder/pause-recording.html

    r272722 r274154  
    4141}, "Pausing and resuming the recording should impact the video duration");
    4242
     43promise_test(async (test) => {
     44    const stream = await navigator.mediaDevices.getUserMedia({ audio: true, video: true });
     45
     46    const recorder = new MediaRecorder(stream);
     47    const dataPromise = new Promise(resolve => recorder.ondataavailable = (e) => resolve(e.data));
     48
     49    const startPromise = new Promise(resolve => recorder.onstart = resolve);
     50    recorder.start();
     51    await startPromise;
     52
     53    await waitFor(1000);
     54    recorder.pause();
     55    recorder.requestData();
     56
     57    const blob = await dataPromise;
     58    assert_greater_than(blob.size, 2000);
     59}, "Calling requestData once after pausing should lead to more than header data");
     60
     61
    4362    </script>
    4463</body>

  • trunk/Source/WebCore/ChangeLog

    r274150 r274154  
     12021-03-09  Youenn Fablet  <youenn@apple.com>
     2
     3        MediaRecorder.requestData() not returning all captured media after a pause
     4        https://bugs.webkit.org/show_bug.cgi?id=222285
     5        <rdar://problem/74884561>
     6
     7        Reviewed by Eric Carlson.
     8
     9        Previously, when flushing, we are called on a background thread and we hop to the main thread to append data.
     10        In some cases, we were resolving the completion handlers before appending all data.
     11        To prevent this, we now append data from a background thread.
     12        To do so, we lock when accessing m_data, either to append or take the data.
     13        In addition, we cancel writing when clearing the writer.
     14        This allows to clean the writer delegate without fearing that write operations are happening.
     15
     16        Covered by updated test.
     17
     18        * platform/mediarecorder/cocoa/MediaRecorderPrivateWriterCocoa.h:
     19        * platform/mediarecorder/cocoa/MediaRecorderPrivateWriterCocoa.mm:
     20        (-[WebAVAssetWriterDelegate initWithWriter:]):
     21        (-[WebAVAssetWriterDelegate assetWriter:didProduceFragmentedHeaderData:]):
     22        (-[WebAVAssetWriterDelegate assetWriter:didProduceFragmentedMediaData:fragmentedMediaDataReport:]):
     23        (WebCore::MediaRecorderPrivateWriter::initialize):
     24        (WebCore::MediaRecorderPrivateWriter::clear):
     25        (WebCore::MediaRecorderPrivateWriter::stopRecording):
     26        (WebCore::MediaRecorderPrivateWriter::completeFetchData):
     27        (WebCore::MediaRecorderPrivateWriter::appendData):
     28        (WebCore::MediaRecorderPrivateWriter::takeData):
     29
    1302021-03-09  Philippe Normand  <pnormand@igalia.com>
    231

  • trunk/Source/WebCore/platform/mediarecorder/cocoa/MediaRecorderPrivateWriterCocoa.h

    r268363 r274154  
    7777
    7878    void appendData(const char*, size_t);
    79     void appendData(Ref<SharedBuffer>&&);
    8079
    8180    const String& mimeType() const;
    … …  
    109108    void finishedFlushingSamples();
    110109    void completeFetchData();
     110    RefPtr<SharedBuffer> takeData();
    111111
    112112    bool m_hasAudio { false };
    … …  
    119119    RetainPtr<AVAssetWriter> m_writer;
    120120
     121    Lock m_dataLock;
    121122    RefPtr<SharedBuffer> m_data;
    122123    CompletionHandler<void(RefPtr<SharedBuffer>&&, double)> m_fetchDataCompletionHandler;

  • trunk/Source/WebCore/platform/mediarecorder/cocoa/MediaRecorderPrivateWriterCocoa.mm

    r270158 r274154  
    5151
    5252@interface WebAVAssetWriterDelegate : NSObject <AVAssetWriterDelegate> {
    53     WeakPtr<WebCore::MediaRecorderPrivateWriter> m_writer;
    54 }
    55 
    56 - (instancetype)initWithWriter:(WebCore::MediaRecorderPrivateWriter*)writer;
     53    WebCore::MediaRecorderPrivateWriter* m_writer;
     54}
     55
     56- (instancetype)initWithWriter:(WebCore::MediaRecorderPrivateWriter&)writer;
    5757- (void)close;
    5858
    … …  
    6262};
    6363
    64 - (instancetype)initWithWriter:(WebCore::MediaRecorderPrivateWriter*)writer
     64- (instancetype)initWithWriter:(WebCore::MediaRecorderPrivateWriter&)writer
    6565{
    6666    ASSERT(isMainThread());
    6767    self = [super init];
    6868    if (self)
    69         self->m_writer = makeWeakPtr(writer);
     69        self->m_writer = &writer;
    7070
    7171    return self;
    … …  
    7575{
    7676    UNUSED_PARAM(assetWriter);
    77     if (!isMainThread()) {
    78         if (auto size = [fragmentedHeaderData length]) {
    79             callOnMainThread([protectedSelf = RetainPtr<WebAVAssetWriterDelegate>(self), buffer = WebCore::SharedBuffer::create(static_cast<const char*>([fragmentedHeaderData bytes]), size)]() mutable {
    80                 if (protectedSelf->m_writer)
    81                     protectedSelf->m_writer->appendData(WTFMove(buffer));
    82             });
    83         }
    84         return;
    85     }
    86 
    87     if (m_writer)
    88         m_writer->appendData(static_cast<const char*>([fragmentedHeaderData bytes]), [fragmentedHeaderData length]);
     77    m_writer->appendData(static_cast<const char*>([fragmentedHeaderData bytes]), [fragmentedHeaderData length]);
    8978}
    9079
    … …  
    9382    UNUSED_PARAM(assetWriter);
    9483    UNUSED_PARAM(fragmentedMediaDataReport);
    95     if (!isMainThread()) {
    96         if (auto size = [fragmentedMediaData length]) {
    97             callOnMainThread([protectedSelf = RetainPtr<WebAVAssetWriterDelegate>(self), buffer = WebCore::SharedBuffer::create(static_cast<const char*>([fragmentedMediaData bytes]), size)]() mutable {
    98                 if (protectedSelf->m_writer)
    99                     protectedSelf->m_writer->appendData(WTFMove(buffer));
    100             });
    101         }
    102         return;
    103     }
    104 
    105     if (m_writer)
    106         m_writer->appendData(static_cast<const char*>([fragmentedMediaData bytes]), [fragmentedMediaData length]);
     84    m_writer->appendData(static_cast<const char*>([fragmentedMediaData bytes]), [fragmentedMediaData length]);
    10785}
    10886
    … …  
    165143    }
    166144
    167     m_writerDelegate = adoptNS([[WebAVAssetWriterDelegate alloc] initWithWriter: this]);
     145    m_writerDelegate = adoptNS([[WebAVAssetWriterDelegate alloc] initWithWriter: *this]);
    168146    [m_writer.get() setDelegate:m_writerDelegate.get()];
    169147
    … …  
    397375    m_pendingAudioSampleQueue.clear();
    398376    m_pendingVideoSampleQueue.clear();
    399     if (m_writer)
     377    if (m_writer) {
     378        [m_writer cancelWriting];
    400379        m_writer.clear();
    401 
     380    }
     381
     382    // At this pointer, we should no longer be writing any data, so it should be safe to close and nullify m_data without locking.
     383    if (m_writerDelegate)
     384        [m_writerDelegate close];
    402385    m_data = nullptr;
     386
    403387    if (auto completionHandler = WTFMove(m_fetchDataCompletionHandler))
    404388        completionHandler(nullptr, 0);
    … …  
    489473            m_hasStartedWriting = false;
    490474
    491             if (m_writer)
     475            if (m_writer) {
     476                [m_writer cancelWriting];
    492477                m_writer.clear();
     478            }
     479
    493480            if (m_fetchDataCompletionHandler)
    494                 m_fetchDataCompletionHandler(std::exchange(m_data, nullptr), 0);
     481                m_fetchDataCompletionHandler(takeData(), 0);
    495482        };
    496483
    … …  
    552539        m_timeCode = CMTimeGetSeconds(CMTimeAdd(sampleTime, m_currentVideoDuration));
    553540    }
    554     m_fetchDataCompletionHandler(std::exchange(m_data, nullptr), currentTimeCode);
     541    m_fetchDataCompletionHandler(takeData(), currentTimeCode);
    555542}
    556543
    557544void MediaRecorderPrivateWriter::appendData(const char* data, size_t size)
    558545{
     546    auto locker = holdLock(m_dataLock);
    559547    if (!m_data) {
    560548        m_data = SharedBuffer::create(data, size);
    … …  
    564552}
    565553
    566 void MediaRecorderPrivateWriter::appendData(Ref<SharedBuffer>&& buffer)
    567 {
    568     if (!m_data) {
    569         m_data = WTFMove(buffer);
    570         return;
    571     }
    572     m_data->append(WTFMove(buffer));
     554RefPtr<SharedBuffer> MediaRecorderPrivateWriter::takeData()
     555{
     556    auto locker = holdLock(m_dataLock);
     557    auto data = WTFMove(m_data);
     558    return data;
    573559}
    574560
Note: See TracChangeset for help on using the changeset viewer.