Bug 174375 – [MediaStream] a capture source failure should end the MediaStreamTrack

• Status: ASSIGNED
• Component: Media

[Eric Carlson]

A MediaStreamTrack should go to ended if its realtime source fails.

[Eric Carlson]

Created attachment 315135 [details]
Proposed patch.

[Eric Carlson]

<rdar://problem/32626802>

[Eric Carlson]

Created attachment 315165 [details]
Proposed patch.

[youenn fablet]

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?

[Build Bot]

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

[Build Bot]

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

[Eric Carlson]

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.

[Eric Carlson]

Created attachment 315259 [details]
Updated patch.

[youenn fablet]

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.

[Eric Carlson]

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.

[Eric Carlson]

Created attachment 315270 [details]
Patch for landing.

[WebKit Commit Bot]

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

[Eric Carlson]

Created attachment 315273 [details]
Patch for landing.

[WebKit Commit Bot]

Comment on attachment 315273 [details]
Patch for landing.

Clearing flags on attachment: 315273

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