Closed
Bug 1089798
Opened 10 years ago
Closed 10 years ago
DOMMediaStream needs an id field
Categories
(Core :: Audio/Video, defect)
Core
Audio/Video
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 | ||
Updated•10 years ago
|
Assignee: nobody → drno
Assignee | ||
Comment 1•10 years ago
|
||
Assignee | ||
Comment 2•10 years ago
|
||
Assignee | ||
Comment 3•10 years ago
|
||
Assignee | ||
Comment 4•10 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)
Assignee | ||
Comment 5•10 years ago
|
||
Attachment #8559953 -
Attachment is obsolete: true
Attachment #8560198 -
Attachment is obsolete: true
Attachment #8560797 -
Flags: review?(docfaraday)
Assignee | ||
Comment 6•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Attachment #8559951 -
Flags: review?(docfaraday)
Attachment #8559951 -
Flags: review?(bugs)
Updated•10 years ago
|
Attachment #8559951 -
Flags: review?(bugs) → review+
Reporter | ||
Comment 7•10 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
Attachment #8560797 -
Flags: review?(docfaraday) → review+
Reporter | ||
Comment 8•10 years ago
|
||
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+
Assignee | ||
Comment 9•10 years ago
|
||
Attachment #8560798 -
Attachment is obsolete: true
Attachment #8562339 -
Flags: review?(docfaraday)
Assignee | ||
Comment 10•10 years ago
|
||
Updated to match latest changes in 1017888.
Attachment #8562339 -
Attachment is obsolete: true
Attachment #8562339 -
Flags: review?(docfaraday)
Attachment #8562388 -
Flags: review?(docfaraday)
Assignee | ||
Comment 11•10 years ago
|
||
Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5d6b3ecc814a
Reporter | ||
Comment 12•10 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.
Attachment #8562388 -
Flags: review?(docfaraday) → review+
Assignee | ||
Comment 13•10 years ago
|
||
Addressed bwc's comments. Carrying forward r+=bwc
Attachment #8562388 -
Attachment is obsolete: true
Attachment #8563802 -
Flags: review+
Reporter | ||
Comment 14•10 years ago
|
||
Did we want to get another try push here, since some stuff has changed?
Assignee | ||
Comment 15•10 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.
Assignee | ||
Comment 16•10 years ago
|
||
Un-bit rot. Carrying forward r+=bwc
Attachment #8560797 -
Attachment is obsolete: true
Attachment #8565848 -
Flags: review+
Assignee | ||
Comment 17•10 years ago
|
||
Un-bit rot. Carrying forward r+=bwc
Attachment #8563802 -
Attachment is obsolete: true
Attachment #8565849 -
Flags: review+
Assignee | ||
Comment 18•10 years ago
|
||
Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=271e29772fb1
Comment 20•10 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
Flags: in-testsuite+
Keywords: checkin-needed
Comment 21•10 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
Flags: needinfo?(drno)
Assignee | ||
Comment 22•10 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?
Flags: needinfo?(drno)
Assignee | ||
Comment 23•10 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
Attachment #8565848 -
Attachment is obsolete: true
Attachment #8566731 -
Flags: review+
Assignee | ||
Comment 24•10 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...
Keywords: checkin-needed
Comment 25•10 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
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/f97ab75f35af https://hg.mozilla.org/mozilla-central/rev/d7755b48d817 https://hg.mozilla.org/mozilla-central/rev/d3febf8ec84d
Status: NEW → RESOLVED
Closed: 10 years ago
status-firefox38:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
Updated•9 years ago
|
Keywords: dev-doc-needed
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
Keywords: dev-doc-needed → dev-doc-complete
You need to log in
before you can comment on or make changes to this bug.
Description
•