Bug 215591

Created attachment 406750 [details]
Patch

Created attachment 406753 [details]
Patch

Comment on attachment 406753 [details]
Patch

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

> Source/WebCore/Modules/webaudio/AudioContext.cpp:105
> +        return AudioTimestamp { 0, 0 };

Should not need to write the class name here.

> Source/WebCore/Modules/webaudio/AudioContext.cpp:114
> +    if (position.position.seconds() > currentTime())
> +        position.position = Seconds { currentTime() };

We often write this kind of clamping using std::max instead of an if statement.

> Source/WebCore/Modules/webaudio/AudioContext.cpp:118
> +    if (performanceTime < 0.0)
> +        performanceTime = 0.0;

We often write this kind of clamping using std::max instead of an if statement.

> Source/WebCore/Modules/webaudio/AudioContext.cpp:120
> +    return AudioTimestamp { position.position.seconds(), performanceTime };

Should not need to write the class name here.

> Source/WebCore/Modules/webaudio/AudioContext.h:33
> +struct AudioTimestamp;

Typically the struct paragraph is separate and below the class paragraph.

> Source/WebCore/Modules/webaudio/BaseAudioContext.cpp:771
> +    AutoLocker locker(*this);
> +    return m_outputPosition;

The locking here doesn’t prevent races, but I suppose it prevents undefined behavior or getting mismatched structure values.

> Source/WebCore/platform/audio/AudioIOCallback.h:30
>  #ifndef AudioIOCallback_h
>  #define AudioIOCallback_h

#pragma once

> Source/WebCore/platform/audio/AudioIOCallback.h:53
> +    static WARN_UNUSED_RETURN bool decode(Decoder& decoder, AudioIOPosition& result)

I think we want the version that returns Optional rather than the version that returns bool.

> Source/WebCore/platform/audio/cocoa/AudioDestinationCocoa.cpp:161
> +static MonotonicTime machAbsoluteTimeToMonotonicTime(uint64_t machAbsoluteTime)
> +{
> +    // Based on listing #2 from Apple QA 1398, but modified to be thread-safe.
> +    static mach_timebase_info_data_t timebaseInfo;
> +    static std::once_flag initializeTimerOnceFlag;
> +    std::call_once(initializeTimerOnceFlag, [] {
> +        kern_return_t kr = mach_timebase_info(&timebaseInfo);
> +        ASSERT_UNUSED(kr, kr == KERN_SUCCESS);
> +        ASSERT(timebaseInfo.denom);
> +    });
> +
> +    return MonotonicTime::fromRawSeconds((machAbsoluteTime * timebaseInfo.numer) / (1.0e9 * timebaseInfo.denom));
> +}

This seems like a peculiar place for this function, which doesn’t seem to be specific to audio, just to Cocoa. Should we put this alongside the MonotonicTime class instead?

> Source/WebCore/platform/audio/cocoa/AudioDestinationCocoa.h:70
> +    OSStatus render(const AudioTimeStamp* timestamp, UInt32 numberOfFrames, AudioBufferList* ioData);

No need for the argument name "timestamp" here.

Created attachment 406758 [details]
Patch

Created attachment 406760 [details]
Patch

Created attachment 406761 [details]
Patch

Committed r265797: <https://trac.webkit.org/changeset/265797>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 406761 [details].

<rdar://problem/67288515>

Follow-up landed in <https://trac.webkit.org/changeset/265819> to address fast/mediastream/getUserMedia-webaudio.html test failure.