Closed Bug 1089798 Opened 10 years ago Closed 10 years ago

DOMMediaStream needs an id field

Categories

(Core :: Audio/Video, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla38
Tracking Status
firefox38 --- fixed

People

(Reporter: bwc, Assigned: drno)

References

Details

(Keywords: dev-doc-complete)

Attachments

(3 files, 8 obsolete files)

3.17 KB, patch
smaug
: review+
bwc
: review+
Details | Diff | Splinter Review
31.07 KB, patch
drno
: review+
Details | Diff | Splinter Review
21.76 KB, patch
drno
: review+
Details | Diff | Splinter Review
See webidl in http://www.w3.org/TR/mediacapture-streams/ Needed for webrtc work on multistream support.
Assignee: nobody → drno
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)
Depends on: 1017888
Attachment #8559953 - Attachment is obsolete: true
Attachment #8560198 - Attachment is obsolete: true
Attachment #8560797 - Flags: review?(docfaraday)
Attachment #8559951 - Flags: review?(docfaraday)
Attachment #8559951 - Flags: review?(bugs)
Attachment #8559951 - Flags: review?(bugs) → review+
Comment on attachment 8560797 [details] [diff] [review] Part 2: Set IDs for incoming and outgoing streams Review of attachment 8560797 [details] [diff] [review]: ----------------------------------------------------------------- lgtm
Attachment #8560797 - Flags: review?(docfaraday) → review+
Comment on attachment 8559951 [details] [diff] [review] Part 1: WebIDL changes Review of attachment 8559951 [details] [diff] [review]: ----------------------------------------------------------------- lgtm
Attachment #8559951 - Flags: review?(docfaraday) → review+
Attachment #8560798 - Attachment is obsolete: true
Attachment #8562339 - Flags: review?(docfaraday)
Updated to match latest changes in 1017888.
Attachment #8562339 - Attachment is obsolete: true
Attachment #8562339 - Flags: review?(docfaraday)
Attachment #8562388 - Flags: review?(docfaraday)
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.
Attachment #8562388 - Flags: review?(docfaraday) → review+
Addressed bwc's comments. Carrying forward r+=bwc
Attachment #8562388 - Attachment is obsolete: true
Attachment #8563802 - Flags: review+
Did we want to get another try push here, since some stuff has changed?
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.
Un-bit rot. Carrying forward r+=bwc
Attachment #8560797 - Attachment is obsolete: true
Attachment #8565848 - Flags: review+
Un-bit rot. Carrying forward r+=bwc
Attachment #8563802 - Attachment is obsolete: true
Attachment #8565849 - Flags: review+
I don't see any new test failures.
Keywords: checkin-needed
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
Flags: needinfo?(drno)
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?
Flags: needinfo?(drno)
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
Attachment #8565848 - Attachment is obsolete: true
Attachment #8566731 - Flags: review+
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...
Keywords: checkin-needed
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: