1089798 - DOMMediaStream needs an id field

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Byron Campen [:bwc]]

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

Needed for webrtc work on multistream support.

Comment 1

[Nils Ohlmeier [:drno]]

Attachment: Part 1: WebIDL changes

Comment 2

[Nils Ohlmeier [:drno]]

Attachment: Part 2: Set IDs for incoming streams

Comment 3

[Nils Ohlmeier [:drno]]

Attachment: bug_1089798_set_ids_for_local_streams.patch

Comment 4

[Nils Ohlmeier [: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)

Comment 5

[Nils Ohlmeier [:drno]]

Attachment: Part 2: Set IDs for incoming and outgoing streams

Comment 6

[Nils Ohlmeier [:drno]]

Attachment: Part 3: mochitest tweaks for MediaStream.id

Comment 7

[Byron Campen [:bwc]]

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

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

lgtm

Comment 8

[Byron Campen [:bwc]]

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

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

lgtm

Comment 9

[Nils Ohlmeier [:drno]]

Attachment: Part 3: mochitest and unitest changes for MediaStream IDs

Comment 10

[Nils Ohlmeier [:drno]]

Attachment: Part 3: mochitest and unitest changes for MediaStream IDs

Updated to match latest changes in 1017888.

Comment 11

[Nils Ohlmeier [:drno]]

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

Comment 12

[Byron Campen [:bwc]]

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.

Comment 13

[Nils Ohlmeier [:drno]]

Attachment: Part 3: mochitest and unitest changes for MediaStream IDs

Addressed bwc's comments.

Carrying forward r+=bwc

Comment 14

[Byron Campen [:bwc]]

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

Comment 15

[Nils Ohlmeier [:drno]]

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.

Comment 16

[Nils Ohlmeier [:drno]]

Attachment: Part 2: Set IDs for incoming and outgoing streams

Un-bit rot.

Carrying forward r+=bwc

Comment 17

[Nils Ohlmeier [:drno]]

Attachment: Part 3: mochitest and unitest changes for MediaStream IDs

Un-bit rot.

Carrying forward r+=bwc

Comment 18

[Nils Ohlmeier [:drno]]

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

Comment 19

[Nils Ohlmeier [:drno]]

I don't see any new test failures.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

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

Comment 21

[Carsten Book [:Tomcat]]

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

Comment 22

[Nils Ohlmeier [: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?

Comment 23

[Nils Ohlmeier [:drno]]

Attachment: 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

Comment 24

[Nils Ohlmeier [:drno]]

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...

Comment 25

[Ryan VanderMeulen [:RyanVM]]

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

Comment 26

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if 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

Comment 27

[Jean-Yves Perrier [:teoli]]

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