DOMMediaStream needs an id field

RESOLVED FIXED in Firefox 38

Status

()

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: bwc, Assigned: drno)

Tracking

({dev-doc-complete})

Trunk
mozilla38
dev-doc-complete
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox38 fixed)

Details

Attachments

(3 attachments, 8 obsolete attachments)

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
(Reporter)

Description

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

Needed for webrtc work on multistream support.
(Assignee)

Updated

4 years ago
Assignee: nobody → drno
(Assignee)

Comment 1

4 years ago
Created attachment 8559951 [details] [diff] [review]
Part 1: WebIDL changes
(Assignee)

Comment 2

4 years ago
Created attachment 8559953 [details] [diff] [review]
Part 2: Set IDs for incoming streams
(Assignee)

Comment 3

4 years ago
Created attachment 8560198 [details] [diff] [review]
bug_1089798_set_ids_for_local_streams.patch
(Assignee)

Comment 4

4 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)

Updated

4 years ago
Depends on: 1017888
(Assignee)

Comment 5

4 years ago
Created attachment 8560797 [details] [diff] [review]
Part 2: Set IDs for incoming and outgoing streams
Attachment #8559953 - Attachment is obsolete: true
Attachment #8560198 - Attachment is obsolete: true
Attachment #8560797 - Flags: review?(docfaraday)
(Assignee)

Comment 6

4 years ago
Created attachment 8560798 [details] [diff] [review]
Part 3: mochitest tweaks for MediaStream.id
(Assignee)

Updated

4 years ago
Attachment #8559951 - Flags: review?(docfaraday)
Attachment #8559951 - Flags: review?(bugs)
Attachment #8559951 - Flags: review?(bugs) → review+
(Reporter)

Comment 7

4 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

4 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

4 years ago
Created attachment 8562339 [details] [diff] [review]
Part 3: mochitest and unitest changes for MediaStream IDs
Attachment #8560798 - Attachment is obsolete: true
Attachment #8562339 - Flags: review?(docfaraday)
(Assignee)

Comment 10

4 years ago
Created attachment 8562388 [details] [diff] [review]
Part 3: mochitest and unitest changes for MediaStream IDs

Updated to match latest changes in 1017888.
Attachment #8562339 - Attachment is obsolete: true
Attachment #8562339 - Flags: review?(docfaraday)
Attachment #8562388 - Flags: review?(docfaraday)
(Reporter)

Comment 12

4 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

4 years ago
Created attachment 8563802 [details] [diff] [review]
Part 3: mochitest and unitest changes for MediaStream IDs

Addressed bwc's comments.

Carrying forward r+=bwc
Attachment #8562388 - Attachment is obsolete: true
Attachment #8563802 - Flags: review+
(Reporter)

Comment 14

4 years ago
Did we want to get another try push here, since some stuff has changed?
(Assignee)

Comment 15

4 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

4 years ago
Created attachment 8565848 [details] [diff] [review]
Part 2: Set IDs for incoming and outgoing streams

Un-bit rot.

Carrying forward r+=bwc
Attachment #8560797 - Attachment is obsolete: true
Attachment #8565848 - Flags: review+
(Assignee)

Comment 17

4 years ago
Created attachment 8565849 [details] [diff] [review]
Part 3: mochitest and unitest changes for MediaStream IDs

Un-bit rot.

Carrying forward r+=bwc
Attachment #8563802 - Attachment is obsolete: true
Attachment #8565849 - Flags: review+
(Assignee)

Comment 19

4 years ago
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)
(Assignee)

Comment 22

4 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

4 years ago
Created attachment 8566731 [details] [diff] [review]
bug_1089798_media_stream_id_implementation.patch

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

4 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
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
Last Resolved: 4 years ago
status-firefox38: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
Keywords: dev-doc-needed
You need to log in before you can comment on or make changes to this bug.