Bug 149419 – [MediaStream Mac] implement WebAudioSourceProvider

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[Eric Carlson]

Make live audio available to WebAudio.

[Radar WebKit Bug Importer]

<rdar://problem/22789888>

[Eric Carlson]

Created attachment 261692 [details]
Proposed patch.

[Darin Adler]

Comment on attachment 261692 [details]
Proposed patch.

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

> Source/WebCore/Modules/webaudio/AudioContext.cpp:466
> +    MediaStreamTrackVector audioTracks = mediaStream->getAudioTracks();

I think I like auto better here than a type name.

> Source/WebCore/Modules/webaudio/AudioContext.cpp:487
> +    RefPtr<MediaStreamAudioSourceNode> node = MediaStreamAudioSourceNode::create(*this, *mediaStream, *providerTrack);

Consider using auto here instead of RefPtr. Also, type should probably be Ref, not RefPtr.

> Source/WebCore/Modules/webaudio/MediaStreamAudioDestinationNode.cpp:54
> +    audioSources.reserveCapacity(1);
>      audioSources.append(m_source);

Should be reserveInitialCapacity(1) and then uncheckedAppend(m_source) or we could just construct the vector with a single element instead of appending (not sure what the syntax is for that, but I am pretty sure we support it).

> Source/WebCore/Modules/webaudio/MediaStreamAudioDestinationNode.cpp:55
>      m_stream = MediaStream::create(*context->scriptExecutionContext(), MediaStreamPrivate::create(audioSources, Vector<RefPtr<RealtimeMediaSource>>()));

Seems like this should move the audio sources Vector in. I’d expect to see a WTF::move here.

> Source/WebCore/Modules/webaudio/MediaStreamAudioSourceNode.cpp:65
> +    AudioSourceProvider* audioSourceProvider = m_audioTrack->audioSourceProvider();
> +    if (audioSourceProvider)
> +        audioSourceProvider->setClient(nullptr);

I don’t understand why this is OK. If audioSourceProvider is null, then we have left a dangling client pointer pointing to this node and it seems we will inevitably crash.

> Source/WebCore/Modules/webaudio/MediaStreamAudioSourceNode.cpp:90
> +        if (sourceSampleRate != sampleRate()) {
> +            double scaleFactor = sourceSampleRate / sampleRate();
> +            m_multiChannelResampler = std::make_unique<MultiChannelResampler>(scaleFactor, numberOfChannels);
> +        } else
> +            m_multiChannelResampler = nullptr;

Is it OK for efficiency purposes to call sampleRate() twice? I would use == instead of != and do the shorter nullptr case first.

> Source/WebCore/Modules/webaudio/MediaStreamAudioSourceNode.h:53
> +    void reset() override { };

No semicolon needed here.

> Source/WebCore/Modules/webaudio/MediaStreamAudioSourceNode.h:68
>      RefPtr<MediaStream> m_mediaStream;
>      RefPtr<MediaStreamTrack> m_audioTrack;

These should be Ref instead of RefPtr.

> Source/WebCore/platform/mediastream/mac/WebAudioSourceProviderAVFObjC.h:67
> +    // AudioSourceProvider
> +    virtual void provideInput(AudioBus*, size_t) override;
> +    virtual void setClient(AudioSourceProviderClient*) override;
> +
> +    // AVAudioCaptureSource::Observer
> +    void prepare(const AudioStreamBasicDescription *) override;
> +    void unprepare() override;
> +    void process(CMFormatDescriptionRef, CMSampleBufferRef) override;

Why use the virtual keyword on one set of overrides, but not on the other?

> Source/WebCore/platform/mediastream/mac/WebAudioSourceProviderAVFObjC.mm:76
> +    setClient(nullptr);

I am not sure that sharing code with setClient is that valuable. I think that this might be better:

    if (m_connected)
        m_captureSource->removeObserver(this);

> Source/WebCore/platform/mediastream/mac/WebAudioSourceProviderAVFObjC.mm:105
> +    size_t framesAvailable = static_cast<size_t>(endFrame - (m_readCount + m_writeAheadCount));

Why the use of size_t here? If the underlying type is uint64_t, then size_t seems really risky since on 32-bit platforms it will truncate the value. Why not uint64_t? Not sure why size_t gets involved at all, nor why everything is 64-bit.

> Source/WebCore/platform/mediastream/mac/WebAudioSourceProviderAVFObjC.mm:114
> +        AudioChannel* channel = bus->channel(i);

Should use a reference here instead of a pointer.

> Source/WebCore/platform/mediastream/mac/WebAudioSourceProviderAVFObjC.mm:117
> +        m_list->mBuffers[i].mNumberChannels = 1;
> +        m_list->mBuffers[i].mData = channel->mutableData();
> +        m_list->mBuffers[i].mDataByteSize = channel->length() * sizeof(float);

Maybe put a reference to m_list->mBuffers[i] into a local variable?

> Source/WebCore/platform/mediastream/mac/WebAudioSourceProviderAVFObjC.mm:161
> +void WebAudioSourceProviderAVFObjC::prepare(const AudioStreamBasicDescription *format)

Misplaced * here.

> Source/WebCore/platform/mediastream/mac/WebAudioSourceProviderAVFObjC.mm:165
> +    size_t bytesPerFrame = format->mBytesPerFrame;

Strange use of size_t.

> Source/WebCore/platform/mediastream/mac/WebAudioSourceProviderAVFObjC.mm:190
> +    m_ringBuffer->allocate(numberOfChannels, bytesPerFrame, static_cast<size_t>(kRingBufferDuration * sampleRate));

I think we need to check the range of sampleRate. What prevents overflow here?

> Source/WebCore/platform/mediastream/mac/WebAudioSourceProviderAVFObjC.mm:194
> +    m_listBufferSize = offsetof(AudioBufferList, mBuffers) + (sizeof(AudioBuffer) * std::max(1, numberOfChannels));

I think we need to check the range of numberOfChannels. What prevents overflow here?

> Source/WebCore/platform/mediastream/mac/WebAudioSourceProviderAVFObjC.mm:195
> +    m_list = std::unique_ptr<AudioBufferList>((AudioBufferList*) ::operator new (m_listBufferSize));

The use of a C style cast here is strange. There must be a better way to write this.

> Source/WebCore/platform/mediastream/mac/WebAudioSourceProviderAVFObjC.mm:201
> +        strongThis->m_client->setFormat(numberOfChannels, sampleRate);

What guarantees m_client won’t be null when this is called on the main thread?

[Eric Carlson]

(In reply to comment #3)
> Comment on attachment 261692 [details]
> Proposed patch.
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=261692&action=review
> 
> > Source/WebCore/Modules/webaudio/AudioContext.cpp:466
> > +    MediaStreamTrackVector audioTracks = mediaStream->getAudioTracks();
> 
> I think I like auto better here than a type name.
> 
Fixed.

> > Source/WebCore/Modules/webaudio/AudioContext.cpp:487
> > +    RefPtr<MediaStreamAudioSourceNode> node = MediaStreamAudioSourceNode::create(*this, *mediaStream, *providerTrack);
> 
> Consider using auto here instead of RefPtr. Also, type should probably be
> Ref, not RefPtr.
> 
Fixed.

> > Source/WebCore/Modules/webaudio/MediaStreamAudioDestinationNode.cpp:54
> > +    audioSources.reserveCapacity(1);
> >      audioSources.append(m_source);
> 
> Should be reserveInitialCapacity(1) and then uncheckedAppend(m_source) or we
> could just construct the vector with a single element instead of appending
> (not sure what the syntax is for that, but I am pretty sure we support it).
> 
> > Source/WebCore/Modules/webaudio/MediaStreamAudioDestinationNode.cpp:55
> >      m_stream = MediaStream::create(*context->scriptExecutionContext(), MediaStreamPrivate::create(audioSources, Vector<RefPtr<RealtimeMediaSource>>()));
> 
> Seems like this should move the audio sources Vector in. I’d expect to see a
> WTF::move here.
> 
Fixed.

> > Source/WebCore/Modules/webaudio/MediaStreamAudioSourceNode.cpp:65
> > +    AudioSourceProvider* audioSourceProvider = m_audioTrack->audioSourceProvider();
> > +    if (audioSourceProvider)
> > +        audioSourceProvider->setClient(nullptr);
> 
> I don’t understand why this is OK. If audioSourceProvider is null, then we
> have left a dangling client pointer pointing to this node and it seems we
> will inevitably crash.
> 
Fixed.

> > Source/WebCore/Modules/webaudio/MediaStreamAudioSourceNode.cpp:90
> > +        if (sourceSampleRate != sampleRate()) {
> > +            double scaleFactor = sourceSampleRate / sampleRate();
> > +            m_multiChannelResampler = std::make_unique<MultiChannelResampler>(scaleFactor, numberOfChannels);
> > +        } else
> > +            m_multiChannelResampler = nullptr;
> 
> Is it OK for efficiency purposes to call sampleRate() twice? I would use ==
> instead of != and do the shorter nullptr case first.
> 
Fixed.

> > Source/WebCore/Modules/webaudio/MediaStreamAudioSourceNode.h:53
> > +    void reset() override { };
> 
> No semicolon needed here.
> 
Fixed.

> > Source/WebCore/Modules/webaudio/MediaStreamAudioSourceNode.h:68
> >      RefPtr<MediaStream> m_mediaStream;
> >      RefPtr<MediaStreamTrack> m_audioTrack;
> 
> These should be Ref instead of RefPtr.
> 
Fixed.

> > Source/WebCore/platform/mediastream/mac/WebAudioSourceProviderAVFObjC.h:67
> > +    // AudioSourceProvider
> > +    virtual void provideInput(AudioBus*, size_t) override;
> > +    virtual void setClient(AudioSourceProviderClient*) override;
> > +
> > +    // AVAudioCaptureSource::Observer
> > +    void prepare(const AudioStreamBasicDescription *) override;
> > +    void unprepare() override;
> > +    void process(CMFormatDescriptionRef, CMSampleBufferRef) override;
> 
> Why use the virtual keyword on one set of overrides, but not on the other?
> 
Fixed.

> > Source/WebCore/platform/mediastream/mac/WebAudioSourceProviderAVFObjC.mm:76
> > +    setClient(nullptr);
> 
> I am not sure that sharing code with setClient is that valuable. I think
> that this might be better:
> 
>     if (m_connected)
>         m_captureSource->removeObserver(this);
> 
Fixed.

> > Source/WebCore/platform/mediastream/mac/WebAudioSourceProviderAVFObjC.mm:105
> > +    size_t framesAvailable = static_cast<size_t>(endFrame - (m_readCount + m_writeAheadCount));
> 
> Why the use of size_t here? If the underlying type is uint64_t, then size_t
> seems really risky since on 32-bit platforms it will truncate the value. Why
> not uint64_t? Not sure why size_t gets involved at all, nor why everything
> is 64-bit.
> 
Fixed.

> > Source/WebCore/platform/mediastream/mac/WebAudioSourceProviderAVFObjC.mm:114
> > +        AudioChannel* channel = bus->channel(i);
> 
> Should use a reference here instead of a pointer.
> 
Fixed.

> > Source/WebCore/platform/mediastream/mac/WebAudioSourceProviderAVFObjC.mm:117
> > +        m_list->mBuffers[i].mNumberChannels = 1;
> > +        m_list->mBuffers[i].mData = channel->mutableData();
> > +        m_list->mBuffers[i].mDataByteSize = channel->length() * sizeof(float);
> 
> Maybe put a reference to m_list->mBuffers[i] into a local variable?
> 
Fixed.

> > Source/WebCore/platform/mediastream/mac/WebAudioSourceProviderAVFObjC.mm:161
> > +void WebAudioSourceProviderAVFObjC::prepare(const AudioStreamBasicDescription *format)
> 
> Misplaced * here.
> 
Fixed.

> > Source/WebCore/platform/mediastream/mac/WebAudioSourceProviderAVFObjC.mm:165
> > +    size_t bytesPerFrame = format->mBytesPerFrame;
> 
> Strange use of size_t.
> 
Indeed, fixed.

> > Source/WebCore/platform/mediastream/mac/WebAudioSourceProviderAVFObjC.mm:190
> > +    m_ringBuffer->allocate(numberOfChannels, bytesPerFrame, static_cast<size_t>(kRingBufferDuration * sampleRate));
> 
> I think we need to check the range of sampleRate. What prevents overflow
> here?
> 
Added an ASSERT.

> > Source/WebCore/platform/mediastream/mac/WebAudioSourceProviderAVFObjC.mm:194
> > +    m_listBufferSize = offsetof(AudioBufferList, mBuffers) + (sizeof(AudioBuffer) * std::max(1, numberOfChannels));
> 
> I think we need to check the range of numberOfChannels. What prevents
> overflow here?
> 
Ditto.

> > Source/WebCore/platform/mediastream/mac/WebAudioSourceProviderAVFObjC.mm:195
> > +    m_list = std::unique_ptr<AudioBufferList>((AudioBufferList*) ::operator new (m_listBufferSize));
> 
> The use of a C style cast here is strange. There must be a better way to
> write this.
> 
Fixed.

> > Source/WebCore/platform/mediastream/mac/WebAudioSourceProviderAVFObjC.mm:201
> > +        strongThis->m_client->setFormat(numberOfChannels, sampleRate);
> 
> What guarantees m_client won’t be null when this is called on the main
> thread?
>
Nothing, fixed.

Thanks for the thorough review!

[Eric Carlson]

Committed r190115: https://trac.webkit.org/r190115

[Eric Carlson]

Plus https://trac.webkit.org/r190117 to fix the Yosemite 32-bit build.