Summary: | Introduce an internal unit to render audio MediaStreamTrack(s) | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | youenn fablet <youennf> | ||||||||
Component: | WebRTC | Assignee: | youenn fablet <youennf> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | eric.carlson, ews-watchlist, glenn, hta, jer.noble, philipj, sergio, tommyw, webkit-bug-importer, youennf | ||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||
Version: | WebKit Nightly Build | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
See Also: | https://bugs.webkit.org/show_bug.cgi?id=225603 | ||||||||||
Attachments: |
|
Description
youenn fablet
2021-05-10 11:28:44 PDT
Created attachment 428187 [details]
Patch
Created attachment 428191 [details]
Patch
Comment on attachment 428191 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=428191&action=review > Source/WebCore/platform/mediastream/cocoa/AudioMediaStreamTrackRendererCocoa.cpp:2 > + * Copyright (C) 2017-2020 Apple Inc. All rights reserved. s/2017-2020/2017-2021/ > Source/WebCore/platform/mediastream/cocoa/AudioMediaStreamTrackRendererCocoa.cpp:125 > + m_dataSource->pushSamples(sampleTime, audioData, sampleCount); Is it OK to push samples into a newly created source, before the main thread setup block runs? > Source/WebCore/platform/mediastream/cocoa/AudioMediaStreamTrackRendererCocoa.h:2 > + * Copyright (C) 2017 Apple Inc. All rights reserved. s/2017/2017-2021/ > Source/WebCore/platform/mediastream/cocoa/AudioMediaStreamTrackRendererInternalUnit.cpp:2 > + * Copyright (C) 2020 Apple Inc. All rights reserved. s/2020/2021/ > Source/WebCore/platform/mediastream/cocoa/AudioMediaStreamTrackRendererInternalUnit.cpp:100 > + auto audioUnitDeviceID = device ? device->deviceID() : 0; `device` can't be NULL because of the early return above. > Source/WebCore/platform/mediastream/cocoa/AudioMediaStreamTrackRendererInternalUnit.cpp:130 > + if (auto error = AudioOutputUnitStart(m_remoteIOUnit)) { > + RELEASE_LOG_ERROR(WebRTC, "AudioMediaStreamTrackRendererInternalUnit::start AudioOutputUnitStart failed, error = %d", error); > + AudioComponentInstanceDispose(m_remoteIOUnit); > + m_remoteIOUnit = nullptr; > + } Shouldn't this return so we don't set m_isStarted to true? > Source/WebCore/platform/mediastream/cocoa/AudioMediaStreamTrackRendererInternalUnit.h:2 > + * Copyright (C) 2020 Apple Inc. All rights reserved. s/2020/2021/ > Source/WebCore/platform/mediastream/cocoa/AudioMediaStreamTrackRendererUnit.cpp:2 > + * Copyright (C) 2020 Apple Inc. All rights reserved. Ditto > Source/WebCore/platform/mediastream/cocoa/AudioMediaStreamTrackRendererUnit.cpp:84 > +void AudioMediaStreamTrackRendererUnit::addSource(Ref<AudioSampleDataSource>&& source) > +{ > + RELEASE_LOG(WebRTC, "AudioMediaStreamTrackRendererUnit::addSource"); > + This is currently always called on the main thread. If that will always be true, it is probably worth ASSERTing that here. > Source/WebCore/platform/mediastream/cocoa/AudioMediaStreamTrackRendererUnit.cpp:98 > +void AudioMediaStreamTrackRendererUnit::removeSource(AudioSampleDataSource& source) > +{ > + RELEASE_LOG(WebRTC, "AudioMediaStreamTrackRendererUnit::removeSource"); > + Ditto > Source/WebCore/platform/mediastream/cocoa/AudioMediaStreamTrackRendererUnit.cpp:154 > + return 0; All exit points return 0. Will they always? > Source/WebCore/platform/mediastream/cocoa/AudioMediaStreamTrackRendererUnit.h:2 > + * Copyright (C) 2020 Apple Inc. All rights reserved. s/2020/2021/ Thanks for the review, I updated accordingly. (In reply to Eric Carlson from comment #3) > Comment on attachment 428191 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=428191&action=review > > > Source/WebCore/platform/mediastream/cocoa/AudioMediaStreamTrackRendererCocoa.cpp:2 > > + * Copyright (C) 2017-2020 Apple Inc. All rights reserved. > > s/2017-2020/2017-2021/ > > > Source/WebCore/platform/mediastream/cocoa/AudioMediaStreamTrackRendererCocoa.cpp:125 > > + m_dataSource->pushSamples(sampleTime, audioData, sampleCount); > > Is it OK to push samples into a newly created source, before the main thread > setup block runs? Yes, the main thread block will trigger rendering of audio but we can still store audio in the ring buffer before that. > > Source/WebCore/platform/mediastream/cocoa/AudioMediaStreamTrackRendererCocoa.h:2 > > + * Copyright (C) 2017 Apple Inc. All rights reserved. > > s/2017/2017-2021/ > > > Source/WebCore/platform/mediastream/cocoa/AudioMediaStreamTrackRendererInternalUnit.cpp:2 > > + * Copyright (C) 2020 Apple Inc. All rights reserved. > > s/2020/2021/ > > > Source/WebCore/platform/mediastream/cocoa/AudioMediaStreamTrackRendererInternalUnit.cpp:100 > > + auto audioUnitDeviceID = device ? device->deviceID() : 0; > > `device` can't be NULL because of the early return above. Not really, in case we are given an empty string, we want to reset deviceID to 0. > > Source/WebCore/platform/mediastream/cocoa/AudioMediaStreamTrackRendererInternalUnit.cpp:130 > > + if (auto error = AudioOutputUnitStart(m_remoteIOUnit)) { > > + RELEASE_LOG_ERROR(WebRTC, "AudioMediaStreamTrackRendererInternalUnit::start AudioOutputUnitStart failed, error = %d", error); > > + AudioComponentInstanceDispose(m_remoteIOUnit); > > + m_remoteIOUnit = nullptr; > > + } > > Shouldn't this return so we don't set m_isStarted to true? Seems better indeed! > > Source/WebCore/platform/mediastream/cocoa/AudioMediaStreamTrackRendererInternalUnit.h:2 > > + * Copyright (C) 2020 Apple Inc. All rights reserved. > > s/2020/2021/ > > > Source/WebCore/platform/mediastream/cocoa/AudioMediaStreamTrackRendererUnit.cpp:2 > > + * Copyright (C) 2020 Apple Inc. All rights reserved. > > Ditto > > > Source/WebCore/platform/mediastream/cocoa/AudioMediaStreamTrackRendererUnit.cpp:84 > > +void AudioMediaStreamTrackRendererUnit::addSource(Ref<AudioSampleDataSource>&& source) > > +{ > > + RELEASE_LOG(WebRTC, "AudioMediaStreamTrackRendererUnit::addSource"); > > + > > This is currently always called on the main thread. If that will always be > true, it is probably worth ASSERTing that here. Sounds good but we cannot do it right now since this is currently used in GPUProcess from a work queue. Once we switch to an internal source in bug 225603, we will be able to add it. > > Source/WebCore/platform/mediastream/cocoa/AudioMediaStreamTrackRendererUnit.cpp:98 > > +void AudioMediaStreamTrackRendererUnit::removeSource(AudioSampleDataSource& source) > > +{ > > + RELEASE_LOG(WebRTC, "AudioMediaStreamTrackRendererUnit::removeSource"); > > + > > Ditto > > > Source/WebCore/platform/mediastream/cocoa/AudioMediaStreamTrackRendererUnit.cpp:154 > > + return 0; > > All exit points return 0. Will they always? > > > Source/WebCore/platform/mediastream/cocoa/AudioMediaStreamTrackRendererUnit.h:2 > > + * Copyright (C) 2020 Apple Inc. All rights reserved. > > s/2020/2021/ Created attachment 428361 [details]
Patch for landing
Committed r277364 (237622@main): <https://commits.webkit.org/237622@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 428361 [details]. |