Bug 206582 – [Cocoa] Use AVAssetWriterDelegate to implement MediaRecorder

• Status: RESOLVED
• Resolution: FIXED
• Component: WebRTC

[youenn fablet]

[Cocoa] Use AVAssetWriterDelegate to implement MediaRecorder

[youenn fablet]

Created attachment 388423 [details]
WIP

[youenn fablet]

Created attachment 388428 [details]
WIP

[Eric Carlson]

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.

[youenn fablet]

Created attachment 388434 [details]
Rebasing

[youenn fablet]

Created attachment 388452 [details]
Patch

[youenn fablet]

Created attachment 388661 [details]
Patch

[Eric Carlson]

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?

[youenn fablet]

Created attachment 388847 [details]
Patch for landing

[youenn fablet]

(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.

[youenn fablet]

> > 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.

[youenn fablet]

Created attachment 388867 [details]
Patch

[youenn fablet]

Created attachment 388875 [details]
Patch for landing

[youenn fablet]

Created attachment 388994 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 388994 [details]
Patch for landing

Clearing flags on attachment: 388994

Committed r255345: <https://trac.webkit.org/changeset/255345>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/58985368>

[zalan]

Any particular reason to land commented out code?

[youenn fablet]

(In reply to zalan from comment #17)
> Any particular reason to land commented out code?

Which code are you referring to?

[zalan]

(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()];
*/

[youenn fablet]

Oh right, should fix that.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 206933

[youenn fablet]

Created attachment 389240 [details]
Fix include path and commented out code

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 389240 [details]:

editing/spelling/spellcheck-attribute.html bug 206178 (authors: [e-mail], [e-mail], and [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 389240 [details]:

editing/spelling/spellcheck-async-remove-frame.html bug 158401 (authors: [e-mail], [e-mail], and [e-mail])
imported/w3c/web-platform-tests/IndexedDB/fire-success-event-exception.html bug 206554 (authors: [e-mail] and [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 389240 [details]:

editing/spelling/spellcheck-attribute.html bug 206178 (authors: [e-mail], [e-mail], and [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

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>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Ryan Haddad]

Reverted r255424 for reason:

Breaks internal builds.

Committed r255439: <https://trac.webkit.org/changeset/255439>

[youenn fablet]

Created attachment 389501 [details]
Patch

[youenn fablet]

Created attachment 389528 [details]
Patch

[youenn fablet]

Created attachment 389637 [details]
Patch

[youenn fablet]

Created attachment 389642 [details]
Patch

[youenn fablet]

Created attachment 389652 [details]
Patch

[Eric Carlson]

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.

[youenn fablet]

Created attachment 389803 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 389803 [details]
Patch

Clearing flags on attachment: 389803

Committed r255818: <https://trac.webkit.org/changeset/255818>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 207270

[youenn fablet]

Created attachment 389938 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 389938 [details]
Patch

Clearing flags on attachment: 389938

Committed r255910: <https://trac.webkit.org/changeset/255910>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Ryan Haddad]

Attempt to fix internal builds: https://trac.webkit.org/changeset/255970/webkit

[WebKit Commit Bot]

Re-opened since this is blocked by bug 207345

[youenn fablet]

Created attachment 390057 [details]
Patch

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 390057 [details]:

editing/spelling/spellcheck-attribute.html bug 206178 (authors: [e-mail], [e-mail], and [e-mail])
imported/w3c/web-platform-tests/web-animations/timing-model/timelines/update-and-send-events-replacement.html bug 207335 (author: [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 390057 [details]
Patch

Clearing flags on attachment: 390057

Committed r256010: <https://trac.webkit.org/changeset/256010>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Ryan Haddad]

Rolled out in https://trac.webkit.org/changeset/256493/webkit due to crashes. Details are in radar.

[youenn fablet]

Crashes were probably due to use of reserveCapacity since it is calling asanSetInitialBufferSizeTo. Instead we could use resize.

[youenn fablet]

Created attachment 401142 [details]
Patch

[EWS]

Committed r262619: <https://trac.webkit.org/changeset/262619>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 401142 [details].

[youenn fablet]

Reopening to attach new patch.

[youenn fablet]

Created attachment 401159 [details]
Build fix

[EWS]

Committed r262625: <https://trac.webkit.org/changeset/262625>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 401159 [details].

[Peng Liu]

Reopening to attach new patch.

[Peng Liu]

Created attachment 401178 [details]
A follow-up patch to fix a build failure

[EWS]

Tools/Scripts/svn-apply failed to apply attachment 401178 [details] to trunk.
Please resolve the conflicts and upload a new patch.

[Peng Liu]

It was fixed in r262641. <https://trac.webkit.org/changeset/262641>

[Ryan Haddad]

Reverted r262619, r262625, and r262641 for reason:

Caused mediarecorder layout test crashes.

Committed r262663: <https://trac.webkit.org/changeset/262663>

[Ryan Haddad]

Reverted r262619, r262625, and r262641 for reason:

Caused mediarecorder layout test crashes.

Committed r262663: <https://trac.webkit.org/changeset/262663>

[youenn fablet]

Created attachment 401312 [details]
One more reserveCapacity

[youenn fablet]

Created attachment 401321 [details]
Rebasing

[EWS]

Committed r262708: <https://trac.webkit.org/changeset/262708>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 401321 [details].