Attachment 358146 Details for Bug 193016 – Patch v3

[patch] Patch v3

bug-193016-20181229210735.patch (text/plain), 7.15 KB, created by David Kilzer (:ddkilzer) on 2018-12-29 21:07:35 PST

(hide)

Description:

Filename:

MIME Type:

Creator: David Kilzer (:ddkilzer)

Created: 2018-12-29 21:07:35 PST

Size: 7.15 KB

patch

obsolete

>Subversion Revision: 239558
>diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
>index a5f0ab8651d949041f83481c740849a8417d39a9..43d21d2a1c8051c7bf288694ae2c791da798bda7 100644
>--- a/Source/WebCore/ChangeLog
>+++ b/Source/WebCore/ChangeLog
>@@ -1,3 +1,36 @@
>+2018-12-29  David Kilzer  <ddkilzer@apple.com>
>+
>+        Leak of CMSampleBuffer (752 bytes) in com.apple.WebKit.WebContent running WebKit layout tests
>+        <https://webkit.org/b/193016>
>+        <rdar://problem/46925703>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        * platform/mediarecorder/cocoa/MediaRecorderPrivateWriterCocoa.mm:
>+        (WebCore::copySampleBufferWithCurrentTimeStamp):
>+        - Change to return RetainPtr<CMSampleBufferRef>.
>+        - Check return value of CMSampleBufferCreateCopyWithNewTiming().
>+        (WebCore::MediaRecorderPrivateWriter::appendVideoSampleBuffer):
>+        - Check return value of copySampleBufferWithCurrentTimeStamp().
>+        - Fix leak by using RetainPtr<CMSampleBufferRef> returned from
>+          copySampleBufferWithCurrentTimeStamp() instead of leaking
>+          `bufferWithCurrentTime` by using retainPtr().
>+        (WebCore::createAudioFormatDescription):
>+        - Extract method from appendAudioSampleBuffer() to return
>+          RetainPtr<CMFormatDescriptionRef> after calling
>+          CMAudioFormatDescriptionCreate().
>+        - Check return value of CMAudioFormatDescriptionCreate().
>+        (WebCore::createAudioSampleBufferWithPacketDescriptions):
>+        - Extract method from appendAudioSampleBuffer() to return
>+          RetainPtr<CMSampleBufferRef> after calling
>+          CMAudioSampleBufferCreateWithPacketDescriptions().
>+        (WebCore::MediaRecorderPrivateWriter::appendAudioSampleBuffer):
>+        - Check return values of createAudioFormatDescription() and
>+          createAudioSampleBufferWithPacketDescriptions().
>+        - Fix leaks by extracting code into helper methods that return
>+          RetainPtr<> objects instead of leaking CMFormatDescriptionRef
>+          directly or leaking `sampleBuffer` by using retainPtr().
>+
> 2018-12-29  David Kilzer  <ddkilzer@apple.com>
> 
>         clang-tidy: Save 8 padding bytes on WebCore::BorderEdge
>diff --git a/Source/WebCore/platform/mediarecorder/cocoa/MediaRecorderPrivateWriterCocoa.mm b/Source/WebCore/platform/mediarecorder/cocoa/MediaRecorderPrivateWriterCocoa.mm
>index 57aeec32d4ed2751e69a763529ccf1aaecf0d8b0..1f703bc31e853bf8eb331fe3f98e4914c16224e8 100644
>--- a/Source/WebCore/platform/mediarecorder/cocoa/MediaRecorderPrivateWriterCocoa.mm
>+++ b/Source/WebCore/platform/mediarecorder/cocoa/MediaRecorderPrivateWriterCocoa.mm
>@@ -196,7 +196,7 @@ bool MediaRecorderPrivateWriter::setAudioInput()
>     return true;
> }
> 
>-static inline CMSampleBufferRef copySampleBufferWithCurrentTimeStamp(CMSampleBufferRef originalBuffer)
>+static inline RetainPtr<CMSampleBufferRef> copySampleBufferWithCurrentTimeStamp(CMSampleBufferRef originalBuffer)
> {
>     CMTime startTime = CMClockGetTime(CMClockGetHostTimeClock());
>     CMItemCount count = 0;
>@@ -210,9 +210,11 @@ static inline CMSampleBufferRef copySampleBufferWithCurrentTimeStamp(CMSampleBuf
>         timeInfo[i].presentationTimeStamp = startTime;
>     }
>     
>-    CMSampleBufferRef newBuffer;
>-    CMSampleBufferCreateCopyWithNewTiming(kCFAllocatorDefault, originalBuffer, count, timeInfo.data(), &newBuffer);
>-    return newBuffer;
>+    CMSampleBufferRef newBuffer = nullptr;
>+    OSStatus error = CMSampleBufferCreateCopyWithNewTiming(kCFAllocatorDefault, originalBuffer, count, timeInfo.data(), &newBuffer);
>+    if (error)
>+        return nullptr;
>+    return adoptCF(newBuffer);
> }
> 
> void MediaRecorderPrivateWriter::appendVideoSampleBuffer(CMSampleBufferRef sampleBuffer)
>@@ -249,9 +251,32 @@ void MediaRecorderPrivateWriter::appendVideoSampleBuffer(CMSampleBufferRef sampl
>         }];
>         return;
>     }
>-    CMSampleBufferRef bufferWithCurrentTime = copySampleBufferWithCurrentTimeStamp(sampleBuffer);
>+    auto bufferWithCurrentTime = copySampleBufferWithCurrentTimeStamp(sampleBuffer);
>+    if (!bufferWithCurrentTime)
>+        return;
>+
>     auto locker = holdLock(m_videoLock);
>-    m_videoBufferPool.append(retainPtr(bufferWithCurrentTime));
>+    m_videoBufferPool.append(bufferWithCurrentTime);
>+}
>+
>+static inline RetainPtr<CMFormatDescriptionRef> createAudioFormatDescription(const AudioStreamDescription& description)
>+{
>+    auto& basicDescription = *WTF::get<const AudioStreamBasicDescription*>(description.platformDescription().description);
>+    CMFormatDescriptionRef format = nullptr;
>+    OSStatus error = CMAudioFormatDescriptionCreate(kCFAllocatorDefault, &basicDescription, 0, NULL, 0, NULL, NULL, &format);
>+    if (error)
>+        return nullptr;
>+    return adoptCF(format);
>+}
>+
>+static inline RetainPtr<CMSampleBufferRef> createAudioSampleBufferWithPacketDescriptions(CMFormatDescriptionRef format, size_t sampleCount)
>+{
>+    CMTime startTime = CMClockGetTime(CMClockGetHostTimeClock());
>+    CMSampleBufferRef sampleBuffer = nullptr;
>+    OSStatus error = CMAudioSampleBufferCreateWithPacketDescriptions(kCFAllocatorDefault, NULL, false, NULL, NULL, format, sampleCount, startTime, NULL, &sampleBuffer);
>+    if (error)
>+        return nullptr;
>+    return adoptCF(sampleBuffer);
> }
> 
> void MediaRecorderPrivateWriter::appendAudioSampleBuffer(const PlatformAudioData& data, const AudioStreamDescription& description, const WTF::MediaTime&, size_t sampleCount)
>@@ -259,11 +284,9 @@ void MediaRecorderPrivateWriter::appendAudioSampleBuffer(const PlatformAudioData
>     ASSERT(m_audioInput);
>     if ((!m_hasStartedWriting && m_videoInput) || m_isStopped)
>         return;
>-    CMSampleBufferRef sampleBuffer;
>-    CMFormatDescriptionRef format;
>-    OSStatus error;
>-    auto& basicDescription = *WTF::get<const AudioStreamBasicDescription*>(description.platformDescription().description);
>-    error = CMAudioFormatDescriptionCreate(kCFAllocatorDefault, &basicDescription, 0, NULL, 0, NULL, NULL, &format);
>+    auto format = createAudioFormatDescription(description);
>+    if (!format)
>+        return;
>     if (m_isFirstAudioSample) {
>         if (!m_videoInput) {
>             // audio-only recording.
>@@ -293,17 +316,16 @@ void MediaRecorderPrivateWriter::appendAudioSampleBuffer(const PlatformAudioData
>             }
>         }];
>     }
>-    CMTime startTime = CMClockGetTime(CMClockGetHostTimeClock());
> 
>-    error = CMAudioSampleBufferCreateWithPacketDescriptions(kCFAllocatorDefault, NULL, false, NULL, NULL, format, sampleCount, startTime, NULL, &sampleBuffer);
>-    if (error)
>+    auto sampleBuffer = createAudioSampleBufferWithPacketDescriptions(format.get(), sampleCount);
>+    if (!sampleBuffer)
>         return;
>-    error = CMSampleBufferSetDataBufferFromAudioBufferList(sampleBuffer, kCFAllocatorDefault, kCFAllocatorDefault, 0, downcast<WebAudioBufferList>(data).list());
>+    OSStatus error = CMSampleBufferSetDataBufferFromAudioBufferList(sampleBuffer.get(), kCFAllocatorDefault, kCFAllocatorDefault, 0, downcast<WebAudioBufferList>(data).list());
>     if (error)
>         return;
> 
>     auto locker = holdLock(m_audioLock);
>-    m_audioBufferPool.append(retainPtr(sampleBuffer));
>+    m_audioBufferPool.append(sampleBuffer);
> }
> 
> void MediaRecorderPrivateWriter::stopRecording()

Flags:

simon.fraser: review+
simon.fraser: commit-queue-

Actions: View | Formatted Diff | Diff

Attachments on bug 193016: 358031 | 358032 | 358146