Bug 225601 – Introduce an internal unit to render audio MediaStreamTrack(s)

• Status: RESOLVED
• Resolution: FIXED
• Component: WebRTC

[youenn fablet]

Introduce an internal unit to render audio MediaStreamTrack(s)

[youenn fablet]

Created attachment 428187 [details]
Patch

[youenn fablet]

Created attachment 428191 [details]
Patch

[Eric Carlson]

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/

[youenn fablet]

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/

[youenn fablet]

Created attachment 428361 [details]
Patch for landing

[EWS]

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].

[Radar WebKit Bug Importer]

<rdar://problem/77893147>