Bug 174375 - [MediaStream] a capture source failure should end the MediaStreamTrack
Summary: [MediaStream] a capture source failure should end the MediaStreamTrack
Status: ASSIGNED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: Other
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Eric Carlson
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-07-11 11:11 PDT by Eric Carlson
Modified: 2017-08-07 10:54 PDT (History)
4 users (show)

See Also:


Attachments
Proposed patch. (22.38 KB, patch)
2017-07-11 11:29 PDT, Eric Carlson
no flags Details | Formatted Diff | Diff
Proposed patch. (21.35 KB, patch)
2017-07-11 14:16 PDT, Eric Carlson
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews106 for mac-elcapitan-wk2 (1.06 MB, application/zip)
2017-07-11 18:34 PDT, Build Bot
no flags Details
Updated patch. (23.62 KB, patch)
2017-07-12 10:47 PDT, Eric Carlson
no flags Details | Formatted Diff | Diff
Patch for landing. (23.61 KB, patch)
2017-07-12 12:29 PDT, Eric Carlson
commit-queue: commit-queue-
Details | Formatted Diff | Diff
Patch for landing. (23.61 KB, patch)
2017-07-12 12:40 PDT, Eric Carlson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Carlson 2017-07-11 11:11:13 PDT
A MediaStreamTrack should go to ended if its realtime source fails.
Comment 1 Eric Carlson 2017-07-11 11:29:20 PDT
Created attachment 315135 [details]
Proposed patch.
Comment 2 Eric Carlson 2017-07-11 11:32:35 PDT
<rdar://problem/32626802>
Comment 3 Eric Carlson 2017-07-11 14:16:48 PDT
Created attachment 315165 [details]
Proposed patch.
Comment 4 youenn fablet 2017-07-11 16:43:11 PDT
Comment on attachment 315165 [details]
Proposed patch.

Looks mostly good to me.
My main issue is the case of captureFailed being called within startProducingData (see below).


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

> Source/WebCore/platform/mediastream/RealtimeMediaSource.cpp:186
> +void RealtimeMediaSource::captureFailed()

Can there be cases where captureFailed will be called as part of RealtimeMediaSource::startProducingData()?
If so, we would call sourceStopped in captureFailed() and then sourceStarted() in RealtimeMediaSource::start().

> Source/WebCore/platform/mediastream/mac/AVVideoCaptureSource.mm:-463
> -    m_lastImage = nullptr;

Shouldn't we remove m_lastImage from the class declaration as well?

> Source/WebKit2/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:391
> +    if (!oldState && newState)

If oldState is saying that we are capturing audio-only and newState that we are capturing video-only, we are both ending and starting a capture session.
Not sure this is an actual issue.

> Source/WebKit2/UIProcess/WebPageProxy.cpp:6466
> +    WebCore::MediaProducer::MediaStateFlags mediaCaptureMask = WebCore::MediaProducer::HasActiveAudioCaptureDevice | WebCore::MediaProducer::HasActiveVideoCaptureDevice | WebCore::MediaProducer::HasMutedAudioCaptureDevice | WebCore::MediaProducer::HasMutedVideoCaptureDevice | WebCore::MediaProducer::HasInterruptedAudioCaptureDevice | WebCore::MediaProducer::HasInterruptedVideoCaptureDevice;

Maybe worth adding this mask in MediaProducer.h?

> LayoutTests/fast/mediastream/media-stream-track-source-failure.html:17
> +        testRunner.setUserMediaPermission(true);

I think this is no longer needed as this is the default now.

> LayoutTests/fast/mediastream/media-stream-track-source-failure.html:67
> +                setTimeout(() => reject("Device state did not change in .5 second"), 500);

Maybe increase the timeout to 5 seconds for slow bots?
Comment 5 Build Bot 2017-07-11 18:34:36 PDT
Comment on attachment 315165 [details]
Proposed patch.

Attachment 315165 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/4104202

New failing tests:
fast/mediastream/media-stream-track-source-failure.html
Comment 6 Build Bot 2017-07-11 18:34:37 PDT
Created attachment 315199 [details]
Archive of layout-test-results from ews106 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6
Comment 7 Eric Carlson 2017-07-12 10:47:13 PDT
Comment on attachment 315165 [details]
Proposed patch.

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

>> Source/WebCore/platform/mediastream/RealtimeMediaSource.cpp:186
>> +void RealtimeMediaSource::captureFailed()
> 
> Can there be cases where captureFailed will be called as part of RealtimeMediaSource::startProducingData()?
> If so, we would call sourceStopped in captureFailed() and then sourceStarted() in RealtimeMediaSource::start().

True, fixed.

>> Source/WebCore/platform/mediastream/mac/AVVideoCaptureSource.mm:-463
>> -    m_lastImage = nullptr;
> 
> Shouldn't we remove m_lastImage from the class declaration as well?

Indeed, fixed.

>> Source/WebKit2/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:391
>> +    if (!oldState && newState)
> 
> If oldState is saying that we are capturing audio-only and newState that we are capturing video-only, we are both ending and starting a capture session.
> Not sure this is an actual issue.

It is a problem, fixed.

>> Source/WebKit2/UIProcess/WebPageProxy.cpp:6466
>> +    WebCore::MediaProducer::MediaStateFlags mediaCaptureMask = WebCore::MediaProducer::HasActiveAudioCaptureDevice | WebCore::MediaProducer::HasActiveVideoCaptureDevice | WebCore::MediaProducer::HasMutedAudioCaptureDevice | WebCore::MediaProducer::HasMutedVideoCaptureDevice | WebCore::MediaProducer::HasInterruptedAudioCaptureDevice | WebCore::MediaProducer::HasInterruptedVideoCaptureDevice;
> 
> Maybe worth adding this mask in MediaProducer.h?

Done.

>> LayoutTests/fast/mediastream/media-stream-track-source-failure.html:17
>> +        testRunner.setUserMediaPermission(true);
> 
> I think this is no longer needed as this is the default now.

Removed.

>> LayoutTests/fast/mediastream/media-stream-track-source-failure.html:67
>> +                setTimeout(() => reject("Device state did not change in .5 second"), 500);
> 
> Maybe increase the timeout to 5 seconds for slow bots?

Good idea, fixed.
Comment 8 Eric Carlson 2017-07-12 10:47:37 PDT
Created attachment 315259 [details]
Updated patch.
Comment 9 youenn fablet 2017-07-12 12:07:14 PDT
Comment on attachment 315259 [details]
Updated patch.

r=me

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

> Source/WebKit2/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:399
> +        UserMediaProcessManager::singleton().endedCaptureSession(*this);

The only case that might not be handled here is if we stop capturing audio but we start capturing video at the same time.
In that case, we are supposed to send the video extension but revoke the audio one.

As a side note, it is surprising to have startedCaptureSession that does nothing and endedCaptureSession that revokes extension.
Maybe we could do some future refactoring so that one grants extension and the other revokes it.
Comment 10 Eric Carlson 2017-07-12 12:25:23 PDT
Comment on attachment 315259 [details]
Updated patch.

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

>> Source/WebKit2/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:399
>> +        UserMediaProcessManager::singleton().endedCaptureSession(*this);
> 
> The only case that might not be handled here is if we stop capturing audio but we start capturing video at the same time.
> In that case, we are supposed to send the video extension but revoke the audio one.
> 
> As a side note, it is surprising to have startedCaptureSession that does nothing and endedCaptureSession that revokes extension.
> Maybe we could do some future refactoring so that one grants extension and the other revokes it.

Good point, I will get rid of the "else".

startedCaptureSession can't happen until *after* we extend the sandbox, which we do in willCreateMediaStream. We can remove startedCaptureSession and add it later if we need it.
Comment 11 Eric Carlson 2017-07-12 12:29:59 PDT
Created attachment 315270 [details]
Patch for landing.
Comment 12 WebKit Commit Bot 2017-07-12 12:32:16 PDT
Comment on attachment 315270 [details]
Patch for landing.

Rejecting attachment 315270 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-01', 'validate-changelog', '--check-oops', '--non-interactive', 315270, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

ChangeLog entry in LayoutTests/ChangeLog contains OOPS!.

Full output: http://webkit-queues.webkit.org/results/4108882
Comment 13 Eric Carlson 2017-07-12 12:40:45 PDT
Created attachment 315273 [details]
Patch for landing.
Comment 14 WebKit Commit Bot 2017-07-12 13:19:48 PDT
Comment on attachment 315273 [details]
Patch for landing.

Clearing flags on attachment: 315273

Committed r219419: <http://trac.webkit.org/changeset/219419>