Bug 225601 - Introduce an internal unit to render audio MediaStreamTrack(s)
Summary: Introduce an internal unit to render audio MediaStreamTrack(s)
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebRTC (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: youenn fablet
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-05-10 11:28 PDT by youenn fablet
Modified: 2021-05-12 01:58 PDT (History)
10 users (show)

See Also:


Attachments
Patch (69.78 KB, patch)
2021-05-10 11:34 PDT, youenn fablet
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (72.52 KB, patch)
2021-05-10 12:04 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch for landing (72.51 KB, patch)
2021-05-12 01:23 PDT, youenn fablet
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description youenn fablet 2021-05-10 11:28:44 PDT
Introduce an internal unit to render audio MediaStreamTrack(s)
Comment 1 youenn fablet 2021-05-10 11:34:40 PDT
Created attachment 428187 [details]
Patch
Comment 2 youenn fablet 2021-05-10 12:04:44 PDT
Created attachment 428191 [details]
Patch
Comment 3 Eric Carlson 2021-05-11 10:13:43 PDT
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/
Comment 4 youenn fablet 2021-05-12 01:22:37 PDT
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/
Comment 5 youenn fablet 2021-05-12 01:23:17 PDT
Created attachment 428361 [details]
Patch for landing
Comment 6 EWS 2021-05-12 01:57:44 PDT
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].
Comment 7 Radar WebKit Bug Importer 2021-05-12 01:58:16 PDT
<rdar://problem/77893147>