Bug 187627 – Support connecting a MediaStreamAudioDestinationNode to RTCPeerConnection

• Component: WebRTC
• Status: RESOLVED FIXED

[youenn fablet]

We basically need to implement MediaStreamAudioSource

[youenn fablet]

Created attachment 344901 [details]
Patch

[youenn fablet]

<rdar://problem/35334400>

[youenn fablet]

Created attachment 344905 [details]
Patch

[youenn fablet]

Created attachment 344906 [details]
Patch

[Eric Carlson]

Comment on attachment 344906 [details]
Patch

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

> Source/WebCore/Modules/webaudio/MediaStreamAudioSourceCocoa.cpp:58
> +    if (bus.numberOfChannels() != 1 && bus.numberOfChannels() != 2)
> +        return;

It is probably worth adding a release error log here.

> Source/WebCore/Modules/webaudio/MediaStreamAudioSourceCocoa.cpp:93
> +        auto* buffer = audioBufferList.buffer(0);
> +        buffer->mDataByteSize = numberOfFrames * sizeof(float);
> +        buffer->mNumberChannels = 1;
> +        if (!muted())
> +            memcpy(buffer->mData, bus.channel(0)->data(), buffer->mDataByteSize);
> +        else
> +            memset(buffer->mData, 0, buffer->mDataByteSize);
> +    } else {
> +        auto* buffer = audioBufferList.buffer(0);
> +        buffer->mDataByteSize = numberOfFrames * sizeof(float);
> +        buffer->mNumberChannels = 1;
> +        if (!muted())
> +            memcpy(buffer->mData, bus.channel(0)->data(), buffer->mDataByteSize);
> +        else
> +            memset(buffer->mData, 0, buffer->mDataByteSize);
> +
> +        buffer = audioBufferList.buffer(1);
> +        buffer->mDataByteSize = numberOfFrames * sizeof(float);
> +        buffer->mNumberChannels = 1;
> +        if (!muted())
> +            memcpy(buffer->mData, bus.channel(1)->data(), buffer->mDataByteSize);
> +        else
> +            memset(buffer->mData, 0, buffer->mDataByteSize);
> +

A helper function would simplify this, something like:

static void setupBuffer(WebAudioBufferList audioBufferList, int channel, size_t numberOfFrames)
{
    auto* buffer = audioBufferList.buffer(channel);
    buffer->mDataByteSize = numberOfFrames * sizeof(float);
    buffer->mNumberChannels = 1;
    if (!muted())
        memcpy(buffer->mData, bus.channel(channel)->data(), buffer->mDataByteSize);
    else
        memset(buffer->mData, 0, buffer->mDataByteSize);
}

[youenn fablet]

Created attachment 344965 [details]
Patch

[Jer Noble]

Comment on attachment 344965 [details]
Patch

r=me, with nits.

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

> Source/WebCore/Modules/webaudio/MediaStreamAudioDestinationNode.cpp:48
> +static inline Ref<MediaStream> createMediaStream(ScriptExecutionContext& context, Ref<RealtimeMediaSource>&& source)
> +{
> +    auto mediaStreamPrivate = MediaStreamPrivate::create(Vector<Ref<RealtimeMediaSource>>::from(WTFMove(source)), { });
> +    return MediaStream::create(context, WTFMove(mediaStreamPrivate));
> +}
> +

This seems really awkward. Why isn't this a static create() method on MediaStream, taking a context and source?

> Source/WebCore/Modules/webaudio/MediaStreamAudioSource.h:-58
> -    void setAudioFormat(size_t numberOfChannels, float sampleRate);
> -    void consumeAudio(AudioBus*, size_t numberOfFrames);
> -
> -    void addAudioConsumer(AudioDestinationConsumer*);
> -    bool removeAudioConsumer(AudioDestinationConsumer*);
> -    const Vector<RefPtr<AudioDestinationConsumer>>& audioConsumers() const { return m_audioConsumers; }

It looks like this was the only use of AudioDestinationConsumer, so you might as well just remove AudioDestinationConsumer.h entirely.

[youenn fablet]

Created attachment 345000 [details]
Patch for landing

[youenn fablet]

Thanks for the review.


> A helper function would simplify this, something like:
> 
> static void setupBuffer(WebAudioBufferList audioBufferList, int channel,
> size_t numberOfFrames)

Added a helper routine.

> > +static inline Ref<MediaStream> createMediaStream(ScriptExecutionContext& context, Ref<RealtimeMediaSource>&& source)
> > +{
> > +    auto mediaStreamPrivate = MediaStreamPrivate::create(Vector<Ref<RealtimeMediaSource>>::from(WTFMove(source)), { });
> > +    return MediaStream::create(context, WTFMove(mediaStreamPrivate));
> > +}
> > +
> 
> This seems really awkward. Why isn't this a static create() method on
> MediaStream, taking a context and source?

OK, added a create to MediaStreamPrivate.

> > Source/WebCore/Modules/webaudio/MediaStreamAudioSource.h:-58
> > -    void setAudioFormat(size_t numberOfChannels, float sampleRate);
> > -    void consumeAudio(AudioBus*, size_t numberOfFrames);
> > -
> > -    void addAudioConsumer(AudioDestinationConsumer*);
> > -    bool removeAudioConsumer(AudioDestinationConsumer*);
> > -    const Vector<RefPtr<AudioDestinationConsumer>>& audioConsumers() const { return m_audioConsumers; }
> 
> It looks like this was the only use of AudioDestinationConsumer, so you
> might as well just remove AudioDestinationConsumer.h entirely.

OK

[WebKit Commit Bot]

Comment on attachment 345000 [details]
Patch for landing

Clearing flags on attachment: 345000

Committed r233829: <https://trac.webkit.org/changeset/233829>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.