[webkit-reviews] review granted: [Bug 225601] Introduce an internal unit to render audio MediaStreamTrack(s) : [Attachment 428191] Patch

[bugzilla-daemon at webkit.org]

Eric Carlson <eric.carlson at apple.com> has granted youenn fablet
<youennf at gmail.com>'s request for review:
Bug 225601: Introduce an internal unit to render audio MediaStreamTrack(s)
https://bugs.webkit.org/show_bug.cgi?id=225601

Attachment 428191: Patch

https://bugs.webkit.org/attachment.cgi?id=428191&action=review




--- Comment #3 from Eric Carlson <eric.carlson at apple.com> ---
Comment on attachment 428191
  --> https://bugs.webkit.org/attachment.cgi?id=428191
Patch

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

>
Source/WebCore/platform/mediastream/cocoa/AudioMediaStreamTrackRendererCocoa.cp
p:2
> + * Copyright (C) 2017-2020 Apple Inc. All rights reserved.

s/2017-2020/2017-2021/

>
Source/WebCore/platform/mediastream/cocoa/AudioMediaStreamTrackRendererCocoa.cp
p: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/AudioMediaStreamTrackRendererInternal
Unit.cpp:2
> + * Copyright (C) 2020 Apple Inc. All rights reserved.

s/2020/2021/

>
Source/WebCore/platform/mediastream/cocoa/AudioMediaStreamTrackRendererInternal
Unit.cpp:100
> +    auto audioUnitDeviceID = device ? device->deviceID() : 0;

`device` can't be NULL because of the early return above.

>
Source/WebCore/platform/mediastream/cocoa/AudioMediaStreamTrackRendererInternal
Unit.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/AudioMediaStreamTrackRendererInternal
Unit.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/