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.