Share code between AudioDestinationIOS and AudioDestinationMac

[youenn fablet]

Share code between AudioDestinationIOS and AudioDestinationMac

[youenn fablet]

Created attachment 381092 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/56340866>

[youenn fablet]

Created attachment 381094 [details]
Patch

[Eric Carlson]

Comment on attachment 381094 [details]
Patch

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

This looks fine to me modulo my one question. Jer should take a look since he wrote the iOS implementation originally.

> Source/WebCore/platform/audio/cocoa/AudioDestinationCocoa.cpp:142
> +

The Mac no longer clamps values at 0db. Should it?

[youenn fablet]

Another question: do we need AudioDestinationIOS::frameSizeChangedProc?

[youenn fablet]

I also plan to write some tests with Internals that can change the AudioDestination to an AudioDestinationMock.
That should give us some ability to tests things such as a variable number of frames to process for instance.

[Eric Carlson]

Comment on attachment 381094 [details]
Patch

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

> Source/WebCore/platform/audio/ios/AudioDestinationIOS.cpp:170
>  void AudioDestinationIOS::frameSizeChangedProc(void *inRefCon, AudioUnit, AudioUnitPropertyID, AudioUnitScope, AudioUnitElement)
>  {
>      AudioDestinationIOS* audioOutput = static_cast<AudioDestinationIOS*>(inRefCon);
>      UInt32 bufferSize = 0;
>      UInt32 dataSize = sizeof(bufferSize);
> -    AudioUnitGetProperty(audioOutput->m_outputUnit, kAudioUnitProperty_MaximumFramesPerSlice, kAudioUnitScope_Global, 0, (void*)&bufferSize, &dataSize);
> +    AudioUnitGetProperty(audioOutput->outputUnit(), kAudioUnitProperty_MaximumFramesPerSlice, kAudioUnitScope_Global, 0, (void*)&bufferSize, &dataSize);
>      fprintf(stderr, ">>>> frameSizeChanged = %lu\n", static_cast<unsigned long>(bufferSize));
>  }

We definitely don't need the fprintf!

I suspect this is old debugging code that should never have shipped and can be removed.

[Jer Noble]

(In reply to Eric Carlson from comment #7)
> Comment on attachment 381094 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=381094&action=review
> 
> We definitely don't need the fprintf!
> 
> I suspect this is old debugging code that should never have shipped and can
> be removed.

Yep.

[Jer Noble]

This LGTM; I like how the iOS and Mac versions have been hollowed out to just ::configure() calls. I also like the idea of a AudioDestinationMock().

[youenn fablet]

Created attachment 381181 [details]
Patch

[youenn fablet]

Created attachment 381183 [details]
Patch

[Eric Carlson]

Comment on attachment 381183 [details]
Patch

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

> Source/WebCore/platform/audio/cocoa/AudioDestinationCocoa.cpp:44
> +#include <wtf/SoftLinking.h>
> +SOFT_LINK_FRAMEWORK(AudioToolbox)
> +SOFT_LINK(AudioToolbox, AudioComponentInstanceDispose, OSStatus, (AudioComponentInstance inInstance), (inInstance))
> +SOFT_LINK(AudioToolbox, AudioOutputUnitStart, OSStatus, (AudioUnit ci), (ci))
> +SOFT_LINK(AudioToolbox, AudioOutputUnitStop, OSStatus, (AudioUnit ci), (ci))

Shouldn't we soft link on macOS as well?

> Source/WebCore/platform/audio/cocoa/AudioDestinationCocoa.cpp:59
> +        LOG(Media, "AudioDestination::create(%u, %u, %f) - unhandled input channels", numberOfInputChannels, numberOfOutputChannels, sampleRate);

It might be worth changing this to WTFLogAlways.

> Source/WebCore/platform/audio/cocoa/AudioDestinationCocoa.cpp:63
> +        LOG(Media, "AudioDestination::create(%u, %u, %f) - unhandled output channels", numberOfInputChannels, numberOfOutputChannels, sampleRate);

Ditto.

> Source/WebCore/platform/audio/cocoa/AudioDestinationCocoa.cpp:150
> +    for (UInt32 i = 0; i < numberOfBuffers; ++i) {
> +        UInt32 bytesPerFrame = buffers[i].mDataByteSize / numberOfFrames;
> +        UInt32 byteOffset = frameOffset * bytesPerFrame;
> +        auto* memory = reinterpret_cast<float*>(reinterpret_cast<char*>(buffers[i].mData) + byteOffset);
> +        bus.setChannelMemory(i, memory, framesThisTime);
> +    }

macOS currently clamps values at 0db. Should we continue to do that?

[youenn fablet]

Created attachment 381290 [details]
Patch

[youenn fablet]

Created attachment 381292 [details]
Patch

[youenn fablet]

Thanks for the comments, I took them all.

> > Source/WebCore/platform/audio/cocoa/AudioDestinationCocoa.cpp:150
> > +    for (UInt32 i = 0; i < numberOfBuffers; ++i) {
> > +        UInt32 bytesPerFrame = buffers[i].mDataByteSize / numberOfFrames;
> > +        UInt32 byteOffset = frameOffset * bytesPerFrame;
> > +        auto* memory = reinterpret_cast<float*>(reinterpret_cast<char*>(buffers[i].mData) + byteOffset);
> > +        bus.setChannelMemory(i, memory, framesThisTime);
> > +    }
> 
> macOS currently clamps values at 0db. Should we continue to do that?

I added the clamping back for extra safety.
I tried a simple example with and without clamping and found no difference on my iMac.

[WebKit Commit Bot]

Comment on attachment 381292 [details]
Patch

Clearing flags on attachment: 381292

Committed r251367: <https://trac.webkit.org/changeset/251367>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.