New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

make gum demos use adapter.js and attachMediaStream #517

Merged
merged 12 commits into from Jun 22, 2015

Conversation

Projects
None yet
6 participants
@fippo
Contributor

fippo commented Jun 5, 2015

this makes http://webrtc.github.io/samples/src/content/getusermedia/gum/ http://webrtc.github.io/samples/src/content/getusermedia/audio/ use adapter.js and attachMediaStream instead of reinventing that functionality.

@googlebot googlebot added the cla: yes label Jun 5, 2015

fippo added some commits Jun 5, 2015

@fippo

This comment has been minimized.

Show comment
Hide comment
@fippo

fippo Jun 7, 2015

Contributor

@jan-ivar: I didn't update the video.src = stream in source/ and resolution/ to use attachMediaStream in order to avoid a merge conflict. Can you update those please?

Contributor

fippo commented Jun 7, 2015

@jan-ivar: I didn't update the video.src = stream in source/ and resolution/ to use attachMediaStream in order to avoid a merge conflict. Can you update those please?

@jan-ivar

This comment has been minimized.

Show comment
Hide comment
@jan-ivar

jan-ivar Jun 7, 2015

Collaborator

Sure thing. Done in #514 and #515

Collaborator

jan-ivar commented Jun 7, 2015

Sure thing. Done in #514 and #515

@KaptenJansson

This comment has been minimized.

Show comment
Hide comment
@KaptenJansson

KaptenJansson Jun 8, 2015

Member

fippo@ I've reviewed and merged #514 and #515

Member

KaptenJansson commented Jun 8, 2015

fippo@ I've reviewed and merged #514 and #515

@KaptenJansson

This comment has been minimized.

Show comment
Hide comment
@KaptenJansson

KaptenJansson Jun 11, 2015

Member

@fippo Anything else needed here before merge?

Member

KaptenJansson commented Jun 11, 2015

@fippo Anything else needed here before merge?

@fippo

This comment has been minimized.

Show comment
Hide comment
@fippo

fippo Jun 11, 2015

Contributor

no, go ahead :-)

Contributor

fippo commented Jun 11, 2015

no, go ahead :-)

@fippo

This comment has been minimized.

Show comment
Hide comment
@fippo

fippo Jun 12, 2015

Contributor

going through all those demos with Edge... couple of fixes incoming, will do them here.

Contributor

fippo commented Jun 12, 2015

going through all those demos with Edge... couple of fixes incoming, will do them here.

fippo added some commits Jun 12, 2015

@fippo

This comment has been minimized.

Show comment
Hide comment
@fippo

fippo Jun 12, 2015

Contributor

since I heard "screenshots or it did not happen":
edge-gum

Contributor

fippo commented Jun 12, 2015

since I heard "screenshots or it did not happen":
edge-gum

@juberti

This comment has been minimized.

Show comment
Hide comment
@juberti

juberti Jun 12, 2015

Contributor

@samdutton FWIW the previous non-use of adapter was intentional, but I think it's probably time to change that.

Contributor

juberti commented Jun 12, 2015

@samdutton FWIW the previous non-use of adapter was intentional, but I think it's probably time to change that.

@@ -103,7 +103,8 @@ video.onplay = function() {
function getMedia(constraints) {
if (stream) {
video.src = null;
stream.stop();
stream.getAudioTracks().forEach(function(track) { track.stop(); });

This comment has been minimized.

@juberti

juberti Jun 12, 2015

Contributor

possible helper function for adapter? seems like this will be used in multiple places

@juberti

juberti Jun 12, 2015

Contributor

possible helper function for adapter? seems like this will be used in multiple places

This comment has been minimized.

@fippo

fippo Jun 13, 2015

Contributor

cc-ing @alvestrand
one of the other things I noticed (here and in MicrosoftEdge/Demos#68) is that we might also need a way to shim to deattach a stream from an element. Possibly attachMediaStream with a null stream.

@fippo

fippo Jun 13, 2015

Contributor

cc-ing @alvestrand
one of the other things I noticed (here and in MicrosoftEdge/Demos#68) is that we might also need a way to shim to deattach a stream from an element. Possibly attachMediaStream with a null stream.

This comment has been minimized.

@KaptenJansson

KaptenJansson Jun 22, 2015

Member

@fippo, maybe create an issue in the adapter repo for that? Then bump adapter.js in a separate PR.

@KaptenJansson

KaptenJansson Jun 22, 2015

Member

@fippo, maybe create an issue in the adapter repo for that? Then bump adapter.js in a separate PR.

This comment has been minimized.

@KaptenJansson

KaptenJansson Jun 22, 2015

Member

@juberti, filed https://github.com/webrtc/adapter/issues/58 for adding a helper function for stopping all tracks, all video and all audio tracks.

@KaptenJansson

KaptenJansson Jun 22, 2015

Member

@juberti, filed https://github.com/webrtc/adapter/issues/58 for adding a helper function for stopping all tracks, all video and all audio tracks.

This comment has been minimized.

@fippo

fippo Jun 22, 2015

Contributor

done (for both issues)

@fippo

fippo Jun 22, 2015

Contributor

done (for both issues)

@samdutton

This comment has been minimized.

Show comment
Hide comment
@samdutton

samdutton Jun 12, 2015

Contributor

Excellent!
On 12 Jun 2015 6:45 pm, "Philipp Hancke" notifications@github.com wrote:

going through all those demos with Edge... couple of fixes incoming, will
do them here.


Reply to this email directly or view it on GitHub
#517 (comment).

Contributor

samdutton commented Jun 12, 2015

Excellent!
On 12 Jun 2015 6:45 pm, "Philipp Hancke" notifications@github.com wrote:

going through all those demos with Edge... couple of fixes incoming, will
do them here.


Reply to this email directly or view it on GitHub
#517 (comment).

fippo added some commits Jun 13, 2015

@KaptenJansson

This comment has been minimized.

Show comment
Hide comment
@KaptenJansson

KaptenJansson Jun 13, 2015

Member

@alvestrand @juberti @samdutton please take another look at the latest commits.

Member

KaptenJansson commented Jun 13, 2015

@alvestrand @juberti @samdutton please take another look at the latest commits.

@samdutton

This comment has been minimized.

Show comment
Hide comment
@samdutton

samdutton Jun 19, 2015

Contributor

LGTM – though see notes about querySelector().

FWIW the previous non-use of adapter was intentional, but I think it's probably time to change that.

Agree @juberti .

Contributor

samdutton commented Jun 19, 2015

LGTM – though see notes about querySelector().

FWIW the previous non-use of adapter was intentional, but I think it's probably time to change that.

Agree @juberti .

fippo added some commits Jun 19, 2015

Merge remote-tracking branch 'upstream/master' into harmonize-demos
Conflicts:
	src/content/getusermedia/canvas/js/main.js
@fippo

This comment has been minimized.

Show comment
Hide comment
@fippo

fippo Jun 19, 2015

Contributor

done. Shall I update index.html and adapter.js here as well or in a separate PR?

Contributor

fippo commented Jun 19, 2015

done. Shall I update index.html and adapter.js here as well or in a separate PR?

@samdutton

This comment has been minimized.

Show comment
Hide comment
@samdutton

samdutton Jun 19, 2015

Contributor

update index.html and adapter.js here

SGTM.

Contributor

samdutton commented Jun 19, 2015

update index.html and adapter.js here

SGTM.

@fippo

This comment has been minimized.

Show comment
Hide comment
@fippo

fippo Jun 19, 2015

Contributor

done. Now we just need to figure out who will end up writing a blog post... but that can be done after merging.

Contributor

fippo commented Jun 19, 2015

done. Now we just need to figure out who will end up writing a blog post... but that can be done after merging.

@KaptenJansson

This comment has been minimized.

Show comment
Hide comment
@KaptenJansson

KaptenJansson Jun 22, 2015

Member

This LGTM. Merging. I will not update gh-pages just yet, please let me know if you want that to coincide with a blog post or announcement.

Member

KaptenJansson commented Jun 22, 2015

This LGTM. Merging. I will not update gh-pages just yet, please let me know if you want that to coincide with a blog post or announcement.

KaptenJansson added a commit that referenced this pull request Jun 22, 2015

Merge pull request #517 from fippo/harmonize-demos
make gum demos use adapter.js and attachMediaStream

@KaptenJansson KaptenJansson merged commit 5024289 into webrtc:master Jun 22, 2015

2 checks passed

cla/google All necessary CLAs are signed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@fippo

This comment has been minimized.

Show comment
Hide comment
@fippo

fippo Jun 24, 2015

Contributor

@KaptenJansson can you merge gh-pages please? Blog post is ready :-)

Contributor

fippo commented Jun 24, 2015

@KaptenJansson can you merge gh-pages please? Blog post is ready :-)

@KaptenJansson

This comment has been minimized.

Show comment
Hide comment
@KaptenJansson
Member

KaptenJansson commented Jun 25, 2015

Done

@fippo fippo deleted the fippo:harmonize-demos branch Jul 22, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment