[webkit-reviews] review granted: [Bug 215591] AudioContext.getOutputTimestamp() is missing : [Attachment 406753] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Chris Dumez <cdumez at apple.com>'s
request for review:
Bug 215591: AudioContext.getOutputTimestamp() is missing
https://bugs.webkit.org/show_bug.cgi?id=215591

Attachment 406753: Patch

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




--- Comment #3 from Darin Adler <darin at apple.com> ---
Comment on attachment 406753
  --> https://bugs.webkit.org/attachment.cgi?id=406753
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.