Byron Campen [:bwc] Reporter
	Description • 10 years ago

See webidl in http://www.w3.org/TR/mediacapture-streams/

Needed for webrtc work on multistream support.

	Nils Ohlmeier [:drno] Assignee
	Comment 4 • 9 years ago

Remaining work:
- unit tests
- mochitests
- and rebase this on top of bug 1017888 (as it probably makes more sense to land this right after it)

	Byron Campen [:bwc] Reporter
	Comment 7 • 9 years ago

Comment on attachment 8560797 [details] [diff] [review]
Part 2: Set IDs for incoming and outgoing streams

Review of attachment 8560797 [details] [diff] [review]:
-----------------------------------------------------------------

lgtm

	Byron Campen [:bwc] Reporter
	Comment 8 • 9 years ago

Comment on attachment 8559951 [details] [diff] [review]
Part 1: WebIDL changes

Review of attachment 8559951 [details] [diff] [review]:
-----------------------------------------------------------------

lgtm

	Nils Ohlmeier [:drno] Assignee
	Comment 10 • 9 years ago

Updated to match latest changes in 1017888.

	Nils Ohlmeier [:drno] Assignee
	Comment 11 • 9 years ago

Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5d6b3ecc814a

	Byron Campen [:bwc] Reporter
	Comment 12 • 9 years ago

Comment on attachment 8562388 [details] [diff] [review]
Part 3: mochitest and unitest changes for MediaStream IDs

Review of attachment 8562388 [details] [diff] [review]:
-----------------------------------------------------------------

Some nits, some things slightly larger than nits. r+ with fixes for them, or justifications.

::: media/webrtc/signaling/test/jsep_session_unittest.cpp
@@ +190,5 @@
> +  }
> +
> +  void
> +  AddTracksToStream(JsepSessionImpl* side,
> +                    const std::string stream_id,

Couldn't hurt to make this a reference.

@@ +198,5 @@
> +  }
> +
> +  void
> +  AddTracksToStream(JsepSessionImpl* side,
> +                    const std::string stream_id,

Same here.

@@ +213,5 @@
>        side->AddTrack(mst);
>      }
>    }
>  
> +  const unsigned long HasMediaStream(std::vector<RefPtr<JsepTrack>> tracks) {

bool HasMediaStream(const std::vector<RefPtr<JsepTrack>>&) const?

@@ +222,5 @@
> +    }
> +    return 0;
> +  }
> +
> +  const std::string GetFirstLocalStreamId(JsepSessionImpl* side) {

Probably should make this function const, as well as the 4 following functions. Also, I think we can pass a const JsepSessionImpl& to all of these.

@@ +244,5 @@
> +  std::vector<std::string> GetLocalUniqueStreamIds(JsepSessionImpl* side) {
> +    auto ids = GetLocalStreamIds(side);
> +    std::sort(ids.begin(), ids.end());
> +    auto it = std::unique(ids.begin(), ids.end());
> +    ids.resize( std::distance(ids.begin(), it));

Maybe have a helper function that does this to a std::vector<std::string> and reuse?

@@ +260,5 @@
> +      ids.push_back((*i)->GetStreamId());
> +    }
> +    std::cout << "remote ids: ";
> +    for (auto i = ids.begin(); i != ids.end(); i++)
> +      std::cout << *i << " ";

Use braces here.

@@ +261,5 @@
> +    }
> +    std::cout << "remote ids: ";
> +    for (auto i = ids.begin(); i != ids.end(); i++)
> +      std::cout << *i << " ";
> +    std::cout << "\n";

std::endl

@@ +272,5 @@
> +    auto it = std::unique(ids.begin(), ids.end());
> +    ids.resize( std::distance(ids.begin(), it));
> +    std::cout << "unique remote ids: ";
> +    for (auto i = ids.begin(); i != ids.end(); i++)
> +      std::cout << *i << " ";

Use braces here.

@@ +273,5 @@
> +    ids.resize( std::distance(ids.begin(), it));
> +    std::cout << "unique remote ids: ";
> +    for (auto i = ids.begin(); i != ids.end(); i++)
> +      std::cout << *i << " ";
> +    std::cout << "\n";

std::endl

@@ +1124,5 @@
>    }
>  }
>  
> +TEST_P(JsepSessionTest, RenegotiationBothAddTracksToExistingStream)
> +{

Let's bail out here if this is a datachannel-only test (ie; GetParam() == "datachannel"). We end up using the fake datachannel id in that case, which is probably not what we want.

@@ +1132,5 @@
> +  OfferAnswer();
> +
> +  auto oHasStream = HasMediaStream(mSessionOff.GetLocalTracks());
> +  auto aHasStream = HasMediaStream(mSessionAns.GetLocalTracks());
> +  ASSERT_EQ(oHasStream, GetLocalUniqueStreamIds(&mSessionOff).size());

Don't both of these come from mSessionOff.GetLocalTracks()?

@@ +1133,5 @@
> +
> +  auto oHasStream = HasMediaStream(mSessionOff.GetLocalTracks());
> +  auto aHasStream = HasMediaStream(mSessionAns.GetLocalTracks());
> +  ASSERT_EQ(oHasStream, GetLocalUniqueStreamIds(&mSessionOff).size());
> +  ASSERT_EQ(aHasStream, GetLocalUniqueStreamIds(&mSessionAns).size());

Don't both of these come from mSessionAns.GetLocalTracks()?

@@ +1147,5 @@
> +  std::vector<SdpMediaSection::MediaType> extraTypes;
> +  extraTypes.push_back(SdpMediaSection::kAudio);
> +  extraTypes.push_back(SdpMediaSection::kVideo);
> +  AddTracksToStream(&mSessionOff, GetFirstLocalStreamId(&mSessionOff), extraTypes);
> +  AddTracksToStream(&mSessionAns, GetFirstLocalStreamId(&mSessionAns), extraTypes);

Use firstOffId and firstAnsId on these two.

	Nils Ohlmeier [:drno] Assignee
	Comment 13 • 9 years ago

Addressed bwc's comments.

Carrying forward r+=bwc

	Byron Campen [:bwc] Reporter
	Comment 14 • 9 years ago

Did we want to get another try push here, since some stuff has changed?

	Nils Ohlmeier [:drno] Assignee
	Comment 15 • 9 years ago

Yes I might to rebase this anyhow (as it depends on the 1017888, which might have changed in relevant areas) and a re-submit a try later today.

	Nils Ohlmeier [:drno] Assignee
	Comment 16 • 9 years ago

Un-bit rot.

Carrying forward r+=bwc

	Nils Ohlmeier [:drno] Assignee
	Comment 17 • 9 years ago

Un-bit rot.

Carrying forward r+=bwc

	Nils Ohlmeier [:drno] Assignee
	Comment 18 • 9 years ago

Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=271e29772fb1

	Nils Ohlmeier [:drno] Assignee
	Comment 19 • 9 years ago

I don't see any new test failures.

	Ryan VanderMeulen [:RyanVM]
	Comment 20 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/4b194434cfee
https://hg.mozilla.org/integration/mozilla-inbound/rev/abf7a473323c
https://hg.mozilla.org/integration/mozilla-inbound/rev/a6ccfddbdac0

	Carsten Book [:Tomcat]
	Comment 21 • 9 years ago

sorry had to back this out, seems this caused a bustage in nexus builds - https://treeherder.mozilla.org/logviewer.html#?job_id=6762695&repo=mozilla-inbound

	Nils Ohlmeier [:drno] Assignee
	Comment 22 • 9 years ago

Thanks for catching it. Went unnoticed because "B2G Device Image opt" was not in my try build. How can I include that in a try run?

	Nils Ohlmeier [:drno] Assignee
	Comment 23 • 9 years ago

Fix for build bustage on Nexus 5-L (I hope).

Carrying forward r+=bwc

This fix should not bust anything, but the be safe: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4b410db1e877

	Nils Ohlmeier [:drno] Assignee
	Comment 24 • 9 years ago

No new build issues in the try run, lets land this and hope I included the right header to make Nexus 5-L build happy...

	Ryan VanderMeulen [:RyanVM]
	Comment 25 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/f97ab75f35af
https://hg.mozilla.org/integration/mozilla-inbound/rev/d7755b48d817
https://hg.mozilla.org/integration/mozilla-inbound/rev/d3febf8ec84d

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 26 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/f97ab75f35af
https://hg.mozilla.org/mozilla-central/rev/d7755b48d817
https://hg.mozilla.org/mozilla-central/rev/d3febf8ec84d

	Jean-Yves Perrier [:teoli]
	Comment 27 • 9 years ago

Doc updated:
https://developer.mozilla.org/en-US/docs/Web/API/MediaStream/id
https://developer.mozilla.org/en-US/docs/Web/API/MediaStream
and
https://developer.mozilla.org/en-US/Firefox/Releases/41#InterfacesAPIsDOM