A MediaStreamTrack should go to ended if its realtime source fails.
Created attachment 315135 [details] Proposed patch.
<rdar://problem/32626802>
Created attachment 315165 [details] Proposed patch.
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 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
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 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.
Created attachment 315259 [details] Updated patch.
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 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.
Created attachment 315270 [details] Patch for landing.
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
Created attachment 315273 [details] Patch for landing.
Comment on attachment 315273 [details] Patch for landing. Clearing flags on attachment: 315273 Committed r219419: <http://trac.webkit.org/changeset/219419>