Bug 167961 – [WebRTC] Implement Incoming libwebrtc audio source support.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[youenn fablet]

[WebRTC] Implement Incoming libwebrtc audio source support.

[youenn fablet]

Created attachment 300855 [details]
Patch

[youenn fablet]

Patch is not working in my branch.
startProducingData is not called.
The timer in an audio element consuming this track gets updated to "ty aN:aN", so something is probably not correct in the set-up of the audio source.

If somebody has an idea, please let me know.
Otherwise, I'll debug this tomorrow.

[youenn fablet]

Created attachment 301116 [details]
Patch

[Eric Carlson]

Comment on attachment 301116 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=301116&action=review

> Source/WebCore/platform/audio/mac/AudioSampleDataSource.cpp:299
> +    if (!m_ringBuffer || m_muted)

Nit: won't get this far if there isn't a ring buffer.

> Source/WebCore/platform/mediastream/mac/RealtimeIncomingAudioSource.cpp:116
> +    size_t dataSize = numberOfFrames * numberOfChannels * sizeof(uint16_t);
> +    CMBlockBufferRef dataBuffer;
> +    OSStatus error = CMBlockBufferCreateWithMemoryBlock(nullptr, nullptr, dataSize, nullptr, nullptr, 0, dataSize, 0, &dataBuffer);
> +    ASSERT_UNUSED(error, !error);
> +
> +    error = CMBlockBufferReplaceDataBytes(data, dataBuffer, 0 , dataSize);
> +    ASSERT(!error);
> +
> +    CMSampleBufferRef sampleBuffer;
> +    OSStatus result = CMAudioSampleBufferCreateWithPacketDescriptions(nullptr, nullptr, true, nullptr, nullptr, m_formatDescription.get(), numberOfFrames, startTime, nullptr, &sampleBuffer);
> +    ASSERT(sampleBuffer);
> +    ASSERT_UNUSED(result, !result);
> +    if (!sampleBuffer)
> +        return;
> +
> +    result = CMSampleBufferSetDataReady(sampleBuffer);
> +    ASSERT(!result);
> +

Nit: none of this is necessary if m_audioSourceObservers is empty. We should change the web audio audio observer callback to take a buffer list instead of a sample buffer.

[youenn fablet]

Created attachment 302205 [details]
Patch

[youenn fablet]

Created attachment 302287 [details]
Copying data

[Eric Carlson]

Comment on attachment 302287 [details]
Copying data

View in context: https://bugs.webkit.org/attachment.cgi?id=302287&action=review

> Source/WebCore/ChangeLog:8
> +        Hooking libwebrtc incoming audio source into WebCore audio rendering path.

Nit: "Hooking" -> "Hook"

> Source/WebCore/platform/mediastream/mac/RealtimeIncomingAudioSource.cpp:98
> +    WebAudioBufferList audioBufferList { CAAudioStreamDescription(m_streamFormat), WTF::safeCast<uint32_t>(numberOfFrames) };
> +    audioBufferList.buffer(0)->mDataByteSize = numberOfChannels * numberOfFrames * bitsPerSample / 8;
> +    audioBufferList.buffer(0)->mNumberChannels = numberOfChannels;
> +    memcpy(audioBufferList.buffer(0)->mData, audioData, audioBufferList.buffer(0)->mDataByteSize);
> +
> +    audioSamplesAvailable(mediaTime, audioBufferList, CAAudioStreamDescription(m_streamFormat), numberOfFrames);

Nit: please add a FIXME here, we should not have to copy this data because it will be copied into the data source on the other end of this call.

[youenn fablet]

Created attachment 302308 [details]
Patch for landing

[youenn fablet]

Thanks for the review.

(In reply to comment #7)
> Comment on attachment 302287 [details]
> Copying data
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=302287&action=review
> 
> > Source/WebCore/ChangeLog:8
> > +        Hooking libwebrtc incoming audio source into WebCore audio rendering path.
> 
> Nit: "Hooking" -> "Hook"

Done

> > Source/WebCore/platform/mediastream/mac/RealtimeIncomingAudioSource.cpp:98
> > +    WebAudioBufferList audioBufferList { CAAudioStreamDescription(m_streamFormat), WTF::safeCast<uint32_t>(numberOfFrames) };
> > +    audioBufferList.buffer(0)->mDataByteSize = numberOfChannels * numberOfFrames * bitsPerSample / 8;
> > +    audioBufferList.buffer(0)->mNumberChannels = numberOfChannels;
> > +    memcpy(audioBufferList.buffer(0)->mData, audioData, audioBufferList.buffer(0)->mDataByteSize);
> > +
> > +    audioSamplesAvailable(mediaTime, audioBufferList, CAAudioStreamDescription(m_streamFormat), numberOfFrames);
> 
> Nit: please add a FIXME here, we should not have to copy this data because
> it will be copied into the data source on the other end of this call.

Done

[WebKit Commit Bot]

Comment on attachment 302308 [details]
Patch for landing

Clearing flags on attachment: 302308

Committed r212769: <http://trac.webkit.org/changeset/212769>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.