Bug 172391 – [MediaStream] Allow transition from autoplay to play when a capture stream begins.

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[Eric Carlson]

Allow audio/video elements to transition from autoplay to play when a capture stream begin.

[Radar WebKit Bug Importer]

<rdar://problem/32304934>

[Eric Carlson]

Created attachment 310714 [details]
WIP patch

No test yet.

[Build Bot]

Attachment 310714 [details] did not pass style-queue:


ERROR: Source/WebCore/ChangeLog:9:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[youenn fablet]

Comment on attachment 310714 [details]
WIP patch

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

> Source/WebCore/dom/Document.cpp:3497
> +        mediaStreamStateChanged();

Change the name to captureStateChanged() or mediaStreamCaptureStateChanged() maybe?

> Source/WebCore/dom/Document.h:1771
> +        HashSet<HTMLMediaElement*> m_mediaStreamStateChangeElements;

Might be able to use HashSet<std::reference_wrapper<HTMLMediaElement>>
Then in the mediaStreamStateChanged, you would have:
for (HTMLMediaElement& mediaElement : m_mediaStreamStateChangeElements)
        mediaElement.mediaStreamStateChanged();

> Source/WebCore/html/HTMLMediaElement.cpp:7500
> +        play();

There is resumeAutoplaying() which does almost the same but is also setting m_autoplaying = true as well.
Can we reuse it instead?

[youenn fablet]

Looks good, will have a go at it whenever I am finishing my build

[Eric Carlson]

Created attachment 310914 [details]
Proposed patch.

[Eric Carlson]

Created attachment 310915 [details]
Proposed patch.

[youenn fablet]

Comment on attachment 310915 [details]
Proposed patch.

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

> Source/WebCore/dom/Document.h:1772
> +        String m_idHashSalt;

nit: unneeded whitespaces

> LayoutTests/webrtc/autoplay_remote_stream.html:40
> +                        assert_true(Object.isFrozen(trackEvent.streams), "Object.isFrozen() should return true");

nit: Probably do not need all of these asserts since they are tested in other tests already.

> LayoutTests/webrtc/autoplay_remote_stream.html:49
> +                remote_video.srcObject = stream;

Shouldn't we check that remote_video is not autoplaying after setting srcObject?
IIRC, one issue was that the remote_video element was not visible or in the DOM at the time srcObject is set.
Maybe we should test these two cases?

> LayoutTests/webrtc/autoplay_remote_stream.html:65
> +                remote_video.play().then(() => resolve() );

then(resolve) maybe

[youenn fablet]

Created attachment 311072 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 311072 [details]
Patch

Clearing flags on attachment: 311072

Committed r217311: <http://trac.webkit.org/changeset/217311>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.