| Summary: | [MediaStream Mac] implement WebAudioSourceProvider | ||||||
|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Eric Carlson <eric.carlson> | ||||
| Component: | Media | Assignee: | Eric Carlson <eric.carlson> | ||||
| Status: | RESOLVED FIXED | ||||||
| Severity: | Normal | CC: | jer.noble, webkit-bug-importer | ||||
| Priority: | P2 | Keywords: | InRadar | ||||
| Version: | WebKit Nightly Build | ||||||
| Hardware: | Unspecified | ||||||
| OS: | Unspecified | ||||||
| Attachments: |
|
||||||
|
Description
Eric Carlson
2015-09-21 15:11:05 PDT
Created attachment 261692 [details]
Proposed patch.
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? (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! Committed r190115: https://trac.webkit.org/r190115 Plus https://trac.webkit.org/r190117 to fix the Yosemite 32-bit build. |