Bug 171710 – [Mac] Audio capture fails when shouldCaptureAudioInUIProcess is set.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Jer Noble]

[Mac] Audio capture fails when shouldCaptureAudioInUIProcess is set.

[Jer Noble]

rdar://problem/31848689

[Jer Noble]

Created attachment 309137 [details]
Patch

[Jer Noble]

Created attachment 309141 [details]
Patch

[Jer Noble]

Created attachment 309145 [details]
Patch

[youenn fablet]

Comment on attachment 309145 [details]
Patch

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

> Source/WebCore/platform/mediastream/mac/RealtimeMediaSourceCenterMac.cpp:115
> +    if (!audioDeviceID.isEmpty() && audioFactory()) {

Can we have cases where we have some device id but no audio factory?
Shouldn't we assert that audioFactory is not null if device is not empty?

> Source/WebCore/platform/mediastream/mac/RealtimeMediaSourceCenterMac.cpp:117
>          if (audioSource)

If audioSource is null, shouldn't we reject the getUserMedia promise?
Or do we want to be able to resolve with only the video track if we are able to get it?

[Eric Carlson]

Comment on attachment 309145 [details]
Patch

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

>> Source/WebCore/platform/mediastream/mac/RealtimeMediaSourceCenterMac.cpp:117
>>          if (audioSource)
> 
> If audioSource is null, shouldn't we reject the getUserMedia promise?
> Or do we want to be able to resolve with only the video track if we are able to get it?

If audioSource is NULL when audioDeviceID is not empty is an internal failure because we shouldn't end up in createMediaStream if validateRequestConstraints failed, so an ASSERT is appropriate.

[youenn fablet]

(In reply to Eric Carlson from comment #6)
> Comment on attachment 309145 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=309145&action=review
> 
> >> Source/WebCore/platform/mediastream/mac/RealtimeMediaSourceCenterMac.cpp:117
> >>          if (audioSource)
> > 
> > If audioSource is null, shouldn't we reject the getUserMedia promise?
> > Or do we want to be able to resolve with only the video track if we are able to get it?
> 
> If audioSource is NULL when audioDeviceID is not empty is an internal
> failure because we shouldn't end up in createMediaStream if
> validateRequestConstraints failed, so an ASSERT is appropriate.

OK, so we should add an ASSERT for audio, an ASSERT for video.
Do we also want to return early if audio is failing so that we reject the promise.
Currently if audio fails and video succeeds, we are resolving the promise.

[Jer Noble]

(In reply to youenn fablet from comment #5)
> Comment on attachment 309145 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=309145&action=review
> 
> > Source/WebCore/platform/mediastream/mac/RealtimeMediaSourceCenterMac.cpp:115
> > +    if (!audioDeviceID.isEmpty() && audioFactory()) {
> 
> Can we have cases where we have some device id but no audio factory?
> Shouldn't we assert that audioFactory is not null if device is not empty?

Honestly, if we do anything it should be to require subclasses of RealtimeMediaSourceCenter to return a non-NULL value from defaultAudioFactory() and defaultVideoFactory() and then change the return value of audioFactory() and videoFactory() to a reference instead of a pointer. Then an ASSERT won't be necessary.

> > Source/WebCore/platform/mediastream/mac/RealtimeMediaSourceCenterMac.cpp:117
> >          if (audioSource)
> 
> If audioSource is null, shouldn't we reject the getUserMedia promise?
> Or do we want to be able to resolve with only the video track if we are able
> to get it?

We're not changing this behavior in this patch. Please file a bug to do this in a follow up.

[youenn fablet]

Let's land it, I'll fix the clean-up things in a follow-up.

[WebKit Commit Bot]

Comment on attachment 309145 [details]
Patch

Clearing flags on attachment: 309145

Committed r216446: <http://trac.webkit.org/changeset/216446>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.