Bug 171710 - [Mac] Audio capture fails when shouldCaptureAudioInUIProcess is set.
Summary: [Mac] Audio capture fails when shouldCaptureAudioInUIProcess is set.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jer Noble
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-05-04 20:40 PDT by Jer Noble
Modified: 2017-05-08 13:03 PDT (History)
5 users (show)

See Also:


Attachments
Patch (8.21 KB, patch)
2017-05-04 20:48 PDT, Jer Noble
no flags Details | Formatted Diff | Diff
Patch (8.23 KB, patch)
2017-05-04 21:09 PDT, Jer Noble
no flags Details | Formatted Diff | Diff
Patch (17.14 KB, patch)
2017-05-04 22:35 PDT, Jer Noble
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jer Noble 2017-05-04 20:40:59 PDT
[Mac] Audio capture fails when shouldCaptureAudioInUIProcess is set.
Comment 1 Jer Noble 2017-05-04 20:48:18 PDT
rdar://problem/31848689
Comment 2 Jer Noble 2017-05-04 20:48:45 PDT
Created attachment 309137 [details]
Patch
Comment 3 Jer Noble 2017-05-04 21:09:49 PDT
Created attachment 309141 [details]
Patch
Comment 4 Jer Noble 2017-05-04 22:35:42 PDT
Created attachment 309145 [details]
Patch
Comment 5 youenn fablet 2017-05-05 08:01:54 PDT
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?
Comment 6 Eric Carlson 2017-05-05 08:11:17 PDT
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.
Comment 7 youenn fablet 2017-05-05 08:20:47 PDT
(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.
Comment 8 Jer Noble 2017-05-05 08:56:11 PDT
(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.
Comment 9 youenn fablet 2017-05-08 12:34:13 PDT
Let's land it, I'll fix the clean-up things in a follow-up.
Comment 10 WebKit Commit Bot 2017-05-08 13:03:49 PDT
Comment on attachment 309145 [details]
Patch

Clearing flags on attachment: 309145

Committed r216446: <http://trac.webkit.org/changeset/216446>
Comment 11 WebKit Commit Bot 2017-05-08 13:03:51 PDT
All reviewed patches have been landed.  Closing bug.