[Cocoa] Use AVAssetWriterDelegate to implement MediaRecorder
Created attachment 388423 [details] WIP
Created attachment 388428 [details] WIP
Comment on attachment 388423 [details] WIP View in context: https://bugs.webkit.org/attachment.cgi?id=388423&action=review At some point it would be nice to have VideoSampleBufferCompressor and AudioSampleBufferCompressor as C++ classes so they can be reused more easily. > Source/WebCore/platform/mediarecorder/cocoa/AudioSampleBufferCompressor.mm:90 > + m_serialDispatchQueue = dispatch_queue_create([@"com.apple.WebAudioSampleBufferCompressor" UTF8String], DISPATCH_QUEUE_SERIAL); Nit: no need to round trip through an NSString, a char* should work. > Source/WebCore/platform/mediarecorder/cocoa/AudioSampleBufferCompressor.mm:112 > + auto error = CMBufferQueueMarkEndOfData(m_outputBufferQueue.get()); This should be added to CoreMediaSoftLink > Source/WebCore/platform/mediarecorder/cocoa/AudioSampleBufferCompressor.mm:127 > + const auto *audioFormatListItem = CMAudioFormatDescriptionGetRichestDecodableFormat(formatDescription); Ditto > Source/WebCore/platform/mediarecorder/cocoa/AudioSampleBufferCompressor.mm:134 > + if (auto error = AudioFormatGetProperty(kAudioFormatProperty_FormatInfo, 0, NULL, &size, &m_destinationFormat)) { This should be soft linked, maybe create a new AudioToolboxSoftLink file? > Source/WebCore/platform/mediarecorder/cocoa/AudioSampleBufferCompressor.mm:140 > + if (auto error = AudioConverterNew(&m_sourceFormat, &m_destinationFormat, &converter)) { Ditto > Source/WebCore/platform/mediarecorder/cocoa/AudioSampleBufferCompressor.mm:147 > + const void *cookie = CMAudioFormatDescriptionGetMagicCookie(formatDescription, &cookieSize); This should be added to CoreMediaSoftLink > Source/WebCore/platform/mediarecorder/cocoa/AudioSampleBufferCompressor.mm:149 > + if (auto error = AudioConverterSetProperty(m_converter.get(), kAudioConverterDecompressionMagicCookie, (UInt32)cookieSize, cookie)) { AudioToolboxSoftLink > Source/WebCore/platform/mediarecorder/cocoa/AudioSampleBufferCompressor.mm:156 > + if (auto error = AudioConverterGetProperty(m_converter.get(), kAudioConverterCurrentInputStreamDescription, &size, &m_sourceFormat)) { Ditto. > Source/WebCore/platform/mediarecorder/cocoa/AudioSampleBufferCompressor.mm:177 > + UInt32 outputBitRate = 64000; > + if (m_destinationFormat.mSampleRate >= 44100) > + outputBitRate = 192000; > + else if (m_destinationFormat.mSampleRate < 22000) > + outputBitRate = 32000; Named constants might help readers understand the values. > Source/WebCore/platform/mediarecorder/cocoa/AudioSampleBufferCompressor.mm:180 > + if (auto error = AudioConverterSetProperty(m_converter.get(), kAudioConverterEncodeBitRate, size, &outputBitRate)) { AudioToolboxSoftLink > Source/WebCore/platform/mediarecorder/cocoa/AudioSampleBufferCompressor.mm:197 > + m_sourceBuffer.reserveCapacity([self computeBufferSizeForAudioFormat:m_sourceFormat maxOutputPacketSize:0 duration:0.1]); > + m_destinationBuffer.reserveCapacity([self computeBufferSizeForAudioFormat:m_destinationFormat maxOutputPacketSize:m_maxOutputPacketSize duration:0.1]); Named constants would be good here too (is this LOW_WATER_TIME_IN_SECOND?) > Source/WebCore/platform/mediarecorder/cocoa/AudioSampleBufferCompressor.mm:386 > + while (0 < numBytesToCopy) { This would probably be better as "numBytesToCopy > 0" > Source/WebCore/platform/mediarecorder/cocoa/AudioSampleBufferCompressor.mm:459 > + // We assume that the source format is PCM, which is not VBR, but CBR. > + // So, there is not packet descriptions describing the layout of each packet. Should we ASSERT this assumption? > Source/WebCore/platform/mediarecorder/cocoa/AudioSampleBufferCompressor.mm:482 > +- (CMSampleBufferRef)pickOutputSampleBuffer "getOutputSampleBuffer" would be a better match for the typical WebKit naming style. > Source/WebCore/platform/mediarecorder/cocoa/VideoSampleBufferCompressor.mm:55 > + auto error = CMBufferQueueCreate(kCFAllocatorDefault, 0, CMBufferQueueGetCallbacksForUnsortedSampleBuffers(), &m_outputBufferQueue); CoreMediaSoftLink > Source/WebCore/platform/mediarecorder/cocoa/VideoSampleBufferCompressor.mm:84 > + auto error = VTCompressionSessionCompleteFrames(m_vtSession.get(), kCMTimeInvalid); This should be soft linked, maybe add a VTCompressionSessionSoftLink? > Source/WebCore/platform/mediarecorder/cocoa/VideoSampleBufferCompressor.mm:87 > + error = CMBufferQueueMarkEndOfData(m_outputBufferQueue); CoreMediaSoftLink > Source/WebCore/platform/mediarecorder/cocoa/VideoSampleBufferCompressor.mm:112 > + auto error = VTCompressionSessionCreate(kCFAllocatorDefault, dimensions.width, dimensions.height, m_outputCodecType, (__bridge CFDictionaryRef)encoderSpecifications, NULL, NULL, videoCompressionCallback, (__bridge void *)self, &vtSession); VTCompressionSessionSoftLink > Source/WebCore/platform/mediarecorder/cocoa/VideoSampleBufferCompressor.mm:126 > + error = VTSessionSetProperty(m_vtSession.get(), kVTCompressionPropertyKey_RealTime, kCFBooleanTrue); > + RELEASE_LOG_ERROR_IF(error, MediaStream, "WebVideoSampleBufferCompressor VTSessionSetProperty kVTCompressionPropertyKey_RealTime failed with %d", error); > + error = VTSessionSetProperty(m_vtSession.get(), kVTCompressionPropertyKey_MaxKeyFrameIntervalDuration, (__bridge CFTypeRef)@(m_maxKeyFrameIntervalDuration)); > + RELEASE_LOG_ERROR_IF(error, MediaStream, "WebVideoSampleBufferCompressor VTSessionSetProperty kVTCompressionPropertyKey_MaxKeyFrameIntervalDuration failed with %d", error); > + error = VTSessionSetProperty(m_vtSession.get(), kVTCompressionPropertyKey_ExpectedFrameRate, (__bridge CFTypeRef)@(m_expectedFrameRate)); > + RELEASE_LOG_ERROR_IF(error, MediaStream, "WebVideoSampleBufferCompressor VTSessionSetProperty kVTCompressionPropertyKey_ExpectedFrameRate failed with %d", error); > + > + error = VTCompressionSessionPrepareToEncodeFrames(m_vtSession.get()); Ditto > Source/WebCore/platform/mediarecorder/cocoa/VideoSampleBufferCompressor.mm:142 > + auto error = VTCompressionSessionEncodeFrame(m_vtSession.get(), imageBuffer, presentationTimeStamp, duration, NULL, (__bridge void *)self, NULL); Ditto.
Created attachment 388434 [details] Rebasing
Created attachment 388452 [details] Patch
Created attachment 388661 [details] Patch
Comment on attachment 388661 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=388661&action=review > Source/WebCore/platform/mediarecorder/cocoa/AudioSampleBufferCompressor.mm:124 > + const void *cookie = CMAudioFormatDescriptionGetMagicCookie(formatDescription, &cookieSize); This should be added to CoreMediaSoftLink. > Source/WebCore/platform/mediarecorder/cocoa/AudioSampleBufferCompressor.mm:273 > + if (auto error = CMBlockBufferReplaceDataBytes(data, buffer.get(), 0, dataSize)) { Ditto > Source/WebCore/platform/mediarecorder/cocoa/MediaRecorderPrivateWriterCocoa.mm:281 > + if (![m_videoAssetWriterInput isReadyForMoreMediaData]) > + [m_writer flush]; Will the AssetWriter always be ready for more media data after flushing the writer?
Created attachment 388847 [details] Patch for landing
(In reply to Eric Carlson from comment #7) > Comment on attachment 388661 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=388661&action=review > > > Source/WebCore/platform/mediarecorder/cocoa/AudioSampleBufferCompressor.mm:124 > > + const void *cookie = CMAudioFormatDescriptionGetMagicCookie(formatDescription, &cookieSize); > > This should be added to CoreMediaSoftLink. Done > > Source/WebCore/platform/mediarecorder/cocoa/AudioSampleBufferCompressor.mm:273 > > + if (auto error = CMBlockBufferReplaceDataBytes(data, buffer.get(), 0, dataSize)) { > > Ditto Done > > Source/WebCore/platform/mediarecorder/cocoa/MediaRecorderPrivateWriterCocoa.mm:281 > > + if (![m_videoAssetWriterInput isReadyForMoreMediaData]) > > + [m_writer flush]; > > Will the AssetWriter always be ready for more media data after flushing the > writer? This works as per my testing but I do not see any guarantee for that. I guess the other approach is to use requestMediaDataWhenReadyOnQueue.
> > Will the AssetWriter always be ready for more media data after flushing the > > writer? > > This works as per my testing but I do not see any guarantee for that. > I guess the other approach is to use requestMediaDataWhenReadyOnQueue. Actually, no this does not work, let's just stop pushing samples when not being ready for now.
Created attachment 388867 [details] Patch
Created attachment 388875 [details] Patch for landing
Created attachment 388994 [details] Patch for landing
Comment on attachment 388994 [details] Patch for landing Clearing flags on attachment: 388994 Committed r255345: <https://trac.webkit.org/changeset/255345>
All reviewed patches have been landed. Closing bug.
<rdar://problem/58985368>
Any particular reason to land commented out code?
(In reply to zalan from comment #17) > Any particular reason to land commented out code? Which code are you referring to?
(In reply to youenn fablet from comment #18) > (In reply to zalan from comment #17) > > Any particular reason to land commented out code? > > Which code are you referring to? /* auto block = makeBlockPtr([this, weakThis = makeWeakPtr(this), completionHandler = WTFMove(completionHandler)]() mutable { if (weakThis) { appendEndsPreviousSampleDurationMarker(m_videoAssetWriterInput.get(), m_lastVideoPresentationTime, m_lastVideoDecodingTime); [m_videoAssetWriterInput markAsFinished]; } completionHandler(); }); [m_videoAssetWriterInput requestMediaDataWhenReadyOnQueue:dispatch_get_main_queue() usingBlock:block.get()]; */
Oh right, should fix that.
Re-opened since this is blocked by bug 206933
Created attachment 389240 [details] Fix include path and commented out code
The commit-queue encountered the following flaky tests while processing attachment 389240 [details]: editing/spelling/spellcheck-attribute.html bug 206178 (authors: g.czajkowski@samsung.com, mark.lam@apple.com, and rniwa@webkit.org) The commit-queue is continuing to process your patch.
The commit-queue encountered the following flaky tests while processing attachment 389240 [details]: editing/spelling/spellcheck-async-remove-frame.html bug 158401 (authors: morrita@google.com, rniwa@webkit.org, and tony@chromium.org) imported/w3c/web-platform-tests/IndexedDB/fire-success-event-exception.html bug 206554 (authors: shvaikalesh@gmail.com and youennf@gmail.com) The commit-queue is continuing to process your patch.
Comment on attachment 389240 [details] Fix include path and commented out code Clearing flags on attachment: 389240 Committed r255424: <https://trac.webkit.org/changeset/255424>
Reverted r255424 for reason: Breaks internal builds. Committed r255439: <https://trac.webkit.org/changeset/255439>
Created attachment 389501 [details] Patch
Created attachment 389528 [details] Patch
Created attachment 389637 [details] Patch
Created attachment 389642 [details] Patch
Created attachment 389652 [details] Patch
Comment on attachment 389652 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=389652&action=review > Source/WebCore/ChangeLog:20 > + Note that, whenever we request data, we flush the writer and insert an end of video sample to amke sure video data gets flushed. s/amke/make/ > Source/WebCore/ChangeLog:21 > + Therefore data should not be requested to fast to get adequate video compression. s/to fast/too fast/ > Source/WebCore/platform/mediarecorder/cocoa/AudioSampleBufferCompressor.mm:38 > +#define LOW_WATER_TIME_IN_SECOND 0.1 s/LOW_WATER_TIME_IN_SECOND/LOW_WATER_TIME_IN_SECONDS/ > Source/WebCore/platform/mediarecorder/cocoa/AudioSampleBufferCompressor.mm:68 > + if (auto error = CMBufferQueueCreate(kCFAllocatorDefault, 0, CMBufferQueueGetCallbacksForUnsortedSampleBuffers(), &inputBufferQueue)) { CMBufferQueueGetCallbacksForUnsortedSampleBuffers should be soft linked. > Source/WebCore/platform/mediarecorder/cocoa/AudioSampleBufferCompressor.mm:75 > + if (auto error = CMBufferQueueCreate(kCFAllocatorDefault, 0, CMBufferQueueGetCallbacksForUnsortedSampleBuffers(), &outputBufferQueue)) { Ditto > Source/WebCore/platform/mediarecorder/cocoa/AudioSampleBufferCompressor.mm:204 > + CMSetAttachment(buffer, kCMSampleBufferAttachmentKey_TrimDurationAtStart, trimAtStartDict, kCMAttachmentMode_ShouldPropagate); This constant should be soft linked > Source/WebCore/platform/mediarecorder/cocoa/AudioSampleBufferCompressor.mm:287 > + CMSetAttachment(sampleBuffer, kCMSampleBufferAttachmentKey_GradualDecoderRefresh, (__bridge CFTypeRef)m_gdrCountNum.get(), kCMAttachmentMode_ShouldPropagate); Ditto. > Source/WebCore/platform/mediarecorder/cocoa/MediaRecorderPrivateWriterCocoa.mm:300 > + CMSetAttachment(sampleBuffer.get(), kCMSampleBufferAttachmentKey_EndsPreviousSampleDuration, kCFBooleanTrue, kCMAttachmentMode_ShouldPropagate); Soft link > Source/WebCore/platform/mediarecorder/cocoa/VideoSampleBufferCompressor.mm:65 > + if (auto error = CMBufferQueueCreate(kCFAllocatorDefault, 0, CMBufferQueueGetCallbacksForUnsortedSampleBuffers(), &outputBufferQueue)) { Soft link > Source/WebCore/platform/mediarecorder/cocoa/VideoSampleBufferCompressor.mm:107 > + NSDictionary *encoderSpecifications = @{(__bridge NSString *)kVTVideoEncoderSpecification_EnableHardwareAcceleratedVideoEncoder:@YES}; Ditto. > Source/WebCore/platform/mediarecorder/cocoa/VideoSampleBufferCompressor.mm:118 > + error = VTSessionSetProperty(m_vtSession.get(), kVTCompressionPropertyKey_RealTime, kCFBooleanTrue); Ditto. > Source/WebCore/platform/mediarecorder/cocoa/VideoSampleBufferCompressor.mm:120 > + error = VTSessionSetProperty(m_vtSession.get(), kVTCompressionPropertyKey_MaxKeyFrameIntervalDuration, (__bridge CFTypeRef)@(m_maxKeyFrameIntervalDuration)); Ditto. > Source/WebCore/platform/mediarecorder/cocoa/VideoSampleBufferCompressor.mm:122 > + error = VTSessionSetProperty(m_vtSession.get(), kVTCompressionPropertyKey_ExpectedFrameRate, (__bridge CFTypeRef)@(m_expectedFrameRate)); Ditto.
Created attachment 389803 [details] Patch
Comment on attachment 389803 [details] Patch Clearing flags on attachment: 389803 Committed r255818: <https://trac.webkit.org/changeset/255818>
Re-opened since this is blocked by bug 207270
Created attachment 389938 [details] Patch
Comment on attachment 389938 [details] Patch Clearing flags on attachment: 389938 Committed r255910: <https://trac.webkit.org/changeset/255910>
Attempt to fix internal builds: https://trac.webkit.org/changeset/255970/webkit
Re-opened since this is blocked by bug 207345
Created attachment 390057 [details] Patch
The commit-queue encountered the following flaky tests while processing attachment 390057 [details]: editing/spelling/spellcheck-attribute.html bug 206178 (authors: g.czajkowski@samsung.com, mark.lam@apple.com, and rniwa@webkit.org) imported/w3c/web-platform-tests/web-animations/timing-model/timelines/update-and-send-events-replacement.html bug 207335 (author: graouts@apple.com) The commit-queue is continuing to process your patch.
Comment on attachment 390057 [details] Patch Clearing flags on attachment: 390057 Committed r256010: <https://trac.webkit.org/changeset/256010>
Rolled out in https://trac.webkit.org/changeset/256493/webkit due to crashes. Details are in radar.
Crashes were probably due to use of reserveCapacity since it is calling asanSetInitialBufferSizeTo. Instead we could use resize.
Created attachment 401142 [details] Patch
Committed r262619: <https://trac.webkit.org/changeset/262619> All reviewed patches have been landed. Closing bug and clearing flags on attachment 401142 [details].
Reopening to attach new patch.
Created attachment 401159 [details] Build fix
Committed r262625: <https://trac.webkit.org/changeset/262625> All reviewed patches have been landed. Closing bug and clearing flags on attachment 401159 [details].
Created attachment 401178 [details] A follow-up patch to fix a build failure
Tools/Scripts/svn-apply failed to apply attachment 401178 [details] to trunk. Please resolve the conflicts and upload a new patch.
It was fixed in r262641. <https://trac.webkit.org/changeset/262641>
Reverted r262619, r262625, and r262641 for reason: Caused mediarecorder layout test crashes. Committed r262663: <https://trac.webkit.org/changeset/262663>
Created attachment 401312 [details] One more reserveCapacity
Created attachment 401321 [details] Rebasing
Committed r262708: <https://trac.webkit.org/changeset/262708> All reviewed patches have been landed. Closing bug and clearing flags on attachment 401321 [details].