Bug 171711 – RELEASE_ASSERT at WebAudioSourceProviderAVFObjC::provideInput()

• Component: New Bugs
• Status: RESOLVED FIXED

[Jer Noble]

RELEASE_ASSERT at WebAudioSourceProviderAVFObjC::provideInput()

[Jer Noble]

rdar://problem/31995807

[Jer Noble]

Created attachment 309140 [details]
Patch

[youenn fablet]

Comment on attachment 309140 [details]
Patch

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

> Source/WebCore/ChangeLog:10
> +        format of WebAudioSourceProviderAVFObjC, notify the MediaStreamAudioSourceNode that the number of

The last sentence is mssing a verb.

> Source/WebCore/platform/mediastream/mac/WebAudioSourceProviderAVFObjC.mm:83
> +    unsigned channelCount = std::min(list.bufferCount(), bus->numberOfChannels());

I do not clearly understand  why the outputDescription is different from the input in terms of number of channels.
Is it the case that they are different sometimes or could it stay like this for multiple calls?
If it is something that can stay stable, could we enforce them to be equal in some ways?
Maybe worth beefing up the change log if I am not missing something obvious.

> Source/WebCore/platform/mediastream/mac/WebAudioSourceProviderAVFObjC.mm:142
> +        m_client->setFormat(numberOfChannels, sampleRate);

Why do we always need to call setFormat both synchronously and asynchronously?

> Source/WebCore/platform/mediastream/mac/WebAudioSourceProviderAVFObjC.mm:145
>      callOnMainThread([protectedThis = WTFMove(protectedThis), numberOfChannels, sampleRate] {

If we are to touch this code, we could modernize it by using makeRef and removing protectedThis definition.

[Jer Noble]

Comment on attachment 309140 [details]
Patch

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

>> Source/WebCore/ChangeLog:10
>> +        format of WebAudioSourceProviderAVFObjC, notify the MediaStreamAudioSourceNode that the number of
> 
> The last sentence is mssing a verb.

Changed.

>> Source/WebCore/platform/mediastream/mac/WebAudioSourceProviderAVFObjC.mm:83
>> +    unsigned channelCount = std::min(list.bufferCount(), bus->numberOfChannels());
> 
> I do not clearly understand  why the outputDescription is different from the input in terms of number of channels.
> Is it the case that they are different sometimes or could it stay like this for multiple calls?
> If it is something that can stay stable, could we enforce them to be equal in some ways?
> Maybe worth beefing up the change log if I am not missing something obvious.

In practice, it may never be. But since we are writing data to a variable length structure, we should guarantee that we never iterate over the end of it, and this is a cheap way of doing so.

>> Source/WebCore/platform/mediastream/mac/WebAudioSourceProviderAVFObjC.mm:142
>> +        m_client->setFormat(numberOfChannels, sampleRate);
> 
> Why do we always need to call setFormat both synchronously and asynchronously?

Calling synchronously means we're holding our own lock, so we don't need to worry about making changes out from underneath provideInput() in another thread. MediaElementAudioSourceNode has its own lock it uses to synchronize against the main thread, so calling setFormat() on a background thread should be safe here. I'll remove the async call below.

>> Source/WebCore/platform/mediastream/mac/WebAudioSourceProviderAVFObjC.mm:145
>>      callOnMainThread([protectedThis = WTFMove(protectedThis), numberOfChannels, sampleRate] {
> 
> If we are to touch this code, we could modernize it by using makeRef and removing protectedThis definition.

Ok.

[Jer Noble]

Created attachment 309630 [details]
Patch

[youenn fablet]

> >> Source/WebCore/platform/mediastream/mac/WebAudioSourceProviderAVFObjC.mm:83
> >> +    unsigned channelCount = std::min(list.bufferCount(), bus->numberOfChannels());
> > 
> > I do not clearly understand  why the outputDescription is different from the input in terms of number of channels.
> > Is it the case that they are different sometimes or could it stay like this for multiple calls?
> > If it is something that can stay stable, could we enforce them to be equal in some ways?
> > Maybe worth beefing up the change log if I am not missing something obvious.
> 
> In practice, it may never be. But since we are writing data to a variable
> length structure, we should guarantee that we never iterate over the end of
> it, and this is a cheap way of doing so.

OK, maybe add a debug assert then?

[Jer Noble]

Created attachment 309634 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 309634 [details]
Patch for landing

Clearing flags on attachment: 309634

Committed r216630: <http://trac.webkit.org/changeset/216630>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Ryan Haddad]

(In reply to WebKit Commit Bot from comment #8)
> Comment on attachment 309634 [details]
> Patch for landing
> 
> Clearing flags on attachment: 309634
> 
> Committed r216630: <http://trac.webkit.org/changeset/216630>

This caused many webrtc LayoutTest assertion failures:

https://build.webkit.org/results/Apple%20El%20Capitan%20Debug%20WK2%20(Tests)/r216631%20(1036)/results.html

https://build.webkit.org/results/Apple%20iOS%2010%20Simulator%20Debug%20WK2%20(Tests)/r216632%20(1248)/results.html

[Ryan Haddad]

Reverted r216630 for reason:

This change caused assertion failures with webrtc LayoutTests.

Committed r216642: <http://trac.webkit.org/changeset/216642>

[Jer Noble]

(In reply to Ryan Haddad from comment #11)
> Reverted r216630 for reason:
> 
> This change caused assertion failures with webrtc LayoutTests.
> 
> Committed r216642: <http://trac.webkit.org/changeset/216642>

Looks like two ASSERTs are happening:

1) AudioContext doesn't like to be reconfigured off the main thread; their lock is explicitly main thread only.

2) The RealtimeAudioInputSource isn't calling prepare() on its audio provider, which leads to a mismatch in the channel count between the provider and the audio node.

I'll post a new patch.

[Jer Noble]

(In reply to Jer Noble from comment #12)
> (In reply to Ryan Haddad from comment #11)
> > Reverted r216630 for reason:
> > 
> > This change caused assertion failures with webrtc LayoutTests.
> > 
> > Committed r216642: <http://trac.webkit.org/changeset/216642>
> 
> Looks like two ASSERTs are happening:
> 
> 1) AudioContext doesn't like to be reconfigured off the main thread; their
> lock is explicitly main thread only.
> 
> 2) The RealtimeAudioInputSource isn't calling prepare() on its audio
> provider, which leads to a mismatch in the channel count between the
> provider and the audio node.

Correction, after the change to fix 1), 2) is not called before we start getting asked for samples.

> I'll post a new patch.

[Jer Noble]

Created attachment 309668 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 309668 [details]
Patch for landing

Clearing flags on attachment: 309668

Committed r216645: <http://trac.webkit.org/changeset/216645>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.