Issue 1413713003: Adding the ability to create an RtpSender without a track.

Issue 1413713003: Adding the ability to create an RtpSender without a track. (Closed)

Created:
5 years, 2 months ago by Taylor Brandstetter

Modified:
5 years, 1 month ago

Reviewers:
pthatcher2, pthatcher1

CC:
webrtc-reviews_webrtc.org, interface-changes_webrtc.org, rwolff_gocast.it, yujie_mao (webrtc), Andrew MacDonald, tterriberry_mozilla.com, qiang.lu, niklas.enbom, peah-webrtc

Base URL:
https://chromium.googlesource.com/external/webrtc.git@master

Target Ref:
refs/pending/heads/master

Project:
webrtc

Visibility:
Public.

More Reviews

Description

Adding the ability to create an RtpSender without a track. This CL also changes AddStream to immediately create a sender, rather than waiting until the track is seen in SDP. And the PeerConnection now builds the list of "send streams" from the list of senders, rather than the collection of local media streams. Committed: https://crrev.com/ac9d92ccbe2b29590c53f702e11dc625820480d5 Cr-Commit-Position: refs/heads/master@{#10414}

Patch Set 1 #

Patch Set 2 : Fixing UUID generation. #

Patch Set 3 : Adding some unit tests for new methods on the sender. #

Total comments: 60

Patch Set 4 : Style improvements, support for SetTrack(nullptr), and unit tests. #

Patch Set 5 : Using constant instead of "audio"/"video", and fixing a bug with audio track stats. #

Patch Set 6 : Fixing build errors. #

Total comments: 4

Patch Set 7 : Some renaming, and getting rid of DCHECK. #

Patch Set 8 : Fixing patch conflicts. #

Created: 5 years, 2 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+700 lines, -388 lines)			Patch
M	talk/app/webrtc/audiotrack.cc	View	1 2 3 4	1 chunk	+1 line, -1 line	0 comments	Download
M	talk/app/webrtc/mediastreaminterface.h	View	1 2 3 4	1 chunk	+3 lines, -0 lines	0 comments	Download
M	talk/app/webrtc/mediastreamprovider.h	View		2 chunks	+4 lines, -5 lines	0 comments	Download
M	talk/app/webrtc/peerconnection.h	View	1 2 3 4 5 6 7	3 chunks	+5 lines, -6 lines	0 comments	Download
M	talk/app/webrtc/peerconnection.cc	View	1 2 3 4 5 6 7	12 chunks	+111 lines, -131 lines	0 comments	Download
M	talk/app/webrtc/peerconnection_unittest.cc	View	1 2 3 4 5	6 chunks	+59 lines, -30 lines	0 comments	Download
M	talk/app/webrtc/peerconnectioninterface.h	View	1 2 3 4 5 6 7	1 chunk	+6 lines, -0 lines	0 comments	Download
M	talk/app/webrtc/peerconnectioninterface_unittest.cc	View	1 2 3 4 5 6 7	1 chunk	+2 lines, -0 lines	0 comments	Download
M	talk/app/webrtc/peerconnectionproxy.h	View		1 chunk	+3 lines, -0 lines	0 comments	Download
M	talk/app/webrtc/rtpsender.h	View	1 2 3 4 5 6	5 chunks	+58 lines, -11 lines	0 comments	Download
M	talk/app/webrtc/rtpsender.cc	View	1 2 3 4 5 6	5 chunks	+159 lines, -47 lines	0 comments	Download
M	talk/app/webrtc/rtpsenderinterface.h	View	1 2 3 4 5 6	3 chunks	+20 lines, -0 lines	0 comments	Download
M	talk/app/webrtc/rtpsenderreceiver_unittest.cc	View	1 2 3	4 chunks	+210 lines, -3 lines	0 comments	Download
D	talk/app/webrtc/test/fakemediastreamsignaling.h	View		1 chunk	+0 lines, -140 lines	0 comments	Download
M	talk/app/webrtc/videotrack.cc	View	1 2 3 4	1 chunk	+1 line, -1 line	0 comments	Download
M	talk/app/webrtc/webrtcsession.cc	View	1 2 3 4 5	3 chunks	+0 lines, -3 lines	0 comments	Download
M	talk/libjingle_tests.gyp	View	1 2 3 4 5	1 chunk	+0 lines, -1 line	0 comments	Download
M	webrtc/base/helpers.h	View		1 chunk	+3 lines, -0 lines	0 comments	Download
M	webrtc/base/helpers.cc	View	1 2 3	3 chunks	+48 lines, -9 lines	0 comments	Download
M	webrtc/base/helpers_unittest.cc	View	1	1 chunk	+7 lines, -0 lines	0 comments	Download

Messages

Total messages: 20 (6 generated)

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages

Taylor Brandstetter

https://codereview.webrtc.org/1413713003/diff/40001/talk/app/webrtc/test/fakemediastreamsignaling.h File talk/app/webrtc/test/fakemediastreamsignaling.h (left): https://codereview.webrtc.org/1413713003/diff/40001/talk/app/webrtc/test/fakemediastreamsignaling.h#oldcode1 talk/app/webrtc/test/fakemediastreamsignaling.h:1: /* I could have deleted this in the MediaStreamSignaling ...

5 years, 2 months ago (2015-10-19 23:51:34 UTC) #2

pthatcher1

https://codereview.webrtc.org/1413713003/diff/40001/talk/app/webrtc/peerconnection.cc File talk/app/webrtc/peerconnection.cc (right): https://codereview.webrtc.org/1413713003/diff/40001/talk/app/webrtc/peerconnection.cc#newcode378 talk/app/webrtc/peerconnection.cc:378: if (sender->kind() == "audio") { We should use a ...

5 years, 2 months ago (2015-10-20 17:42:50 UTC) #3

Taylor Brandstetter

5 years, 2 months ago (2015-10-21 00:22:09 UTC) #4

https://codereview.webrtc.org/1413713003/diff/40001/talk/app/webrtc/peerconne...
File talk/app/webrtc/peerconnection.cc (right):

https://codereview.webrtc.org/1413713003/diff/40001/talk/app/webrtc/peerconne...
talk/app/webrtc/peerconnection.cc:378: if (sender->kind() == "audio") {
On 2015/10/20 17:42:49, pthatcher1 wrote:
> We should use a named constant for "audio" and "video".

Done.

https://codereview.webrtc.org/1413713003/diff/40001/talk/app/webrtc/peerconne...
talk/app/webrtc/peerconnection.cc:701: // AddStream is called again with the
same stream.
On 2015/10/20 17:42:49, pthatcher1 wrote:
> I'm a little lost as to how this comment matches the code.  The code seems to
> say "If there's already a sender for this track, call set_stream_id; 
otherwise,
> create one and call SetSsrc".  But the comment says "If there's already a
sender
> for this track, call SetSsrc".  
> 
> This comment almost seems to match code in OnLocalTrackSeen.

"Already configured in SDP" isn't the same as "already have a sender". This
comment is referring to the "FindTrackInfo"/"if (track_info)" part of the code.
I guess I'll move the comment down there so this is more obvious.

https://codereview.webrtc.org/1413713003/diff/40001/talk/app/webrtc/peerconne...
talk/app/webrtc/peerconnection.cc:717:
(*sender)->set_stream_id(local_stream->label());
On 2015/10/20 17:42:49, pthatcher1 wrote:
> Should this method be set_stream_label?

Since in the W3C spec it's called an "id", I figured it's best to use that
terminology as often as possible.

https://codereview.webrtc.org/1413713003/diff/40001/talk/app/webrtc/peerconne...
talk/app/webrtc/peerconnection.cc:799: if (kind == "audio") {
On 2015/10/20 17:42:49, pthatcher1 wrote:
> Same here with named constants.

Done.

https://codereview.webrtc.org/1413713003/diff/40001/talk/app/webrtc/peerconne...
talk/app/webrtc/peerconnection.cc:1637: cricket::MediaType media_type) {
On 2015/10/20 17:42:49, pthatcher1 wrote:
> Should we call this OnSenderAddedToSessionDescription?

Well, then it would be inconsistent with OnRemoteTrackSeen... And it IS called
when a new track (TrackInfo structure) is created. I plan to clean the TrackInfo
stuff up in a future CL though; we really have no reason to be caching something
that's already readily available in the local description.

https://codereview.webrtc.org/1413713003/diff/40001/talk/app/webrtc/peerconne...
talk/app/webrtc/peerconnection.cc:1638: RtpSenderInterface* sender =
FindSenderById(track_id);
On 2015/10/20 17:42:49, pthatcher1 wrote:
> Should we match the media_type/kind when finding senders as well as ID?

Then we wouldn't be able to tell "unknown sender" apart from "invalid media
type" at this level. Plus, with the change to using an enum for "kind", it
wouldn't save many lines of code anyway.

https://codereview.webrtc.org/1413713003/diff/40001/talk/app/webrtc/peerconne...
talk/app/webrtc/peerconnection.cc:1651: RTC_DCHECK(false && "Invalid media
type.");
On 2015/10/20 17:42:49, pthatcher1 wrote:
> This is an error from the application or the javascript (mismatch of SDP with
> track), possibly from SDP munging.  It seems bad to cause a DCHECK in that
case.
>  I think an LS_WARNING is sufficient.

DCHECKS only do anything in a debug builds (hence the 'D').

https://codereview.webrtc.org/1413713003/diff/40001/talk/app/webrtc/peerconne...
talk/app/webrtc/peerconnection.cc:1674: sender->kind() == "audio") ||
On 2015/10/20 17:42:49, pthatcher1 wrote:
> Can we change the sender->kind() to be a cricket::MediaType?  It would be
easier
> to compare them.

Done.

https://codereview.webrtc.org/1413713003/diff/40001/talk/app/webrtc/peerconne...
File talk/app/webrtc/peerconnectioninterface.h (right):

https://codereview.webrtc.org/1413713003/diff/40001/talk/app/webrtc/peerconne...
talk/app/webrtc/peerconnectioninterface.h:340: // |kind| must be "audio" or
"video".
On 2015/10/20 17:42:49, pthatcher1 wrote:
> I think we could use cricket::MediaType here and say it must be
MEDIA_TYPE_AUDIO
> or MEDIA_TYPE_VIDEO.

Done.

https://codereview.webrtc.org/1413713003/diff/40001/talk/app/webrtc/peerconne...
File talk/app/webrtc/peerconnectioninterface_unittest.cc (right):

https://codereview.webrtc.org/1413713003/diff/40001/talk/app/webrtc/peerconne...
talk/app/webrtc/peerconnectioninterface_unittest.cc:1954:
pc_->AddStream(reference_collection_->at(0));
On 2015/10/20 17:42:49, pthatcher1 wrote:
> Was this test wrong before?

No, there's an intentional change in behavior. Previously a sender only existed
if AddStream AND SetLocalDescription were called. But now, createSender must
return a sender even before SetLocalDescription is called. So the bounds of a
sender's lifetime had to change a bit.

https://codereview.webrtc.org/1413713003/diff/40001/talk/app/webrtc/rtpsender.cc
File talk/app/webrtc/rtpsender.cc (right):

https://codereview.webrtc.org/1413713003/diff/40001/talk/app/webrtc/rtpsender...
talk/app/webrtc/rtpsender.cc:102: LOG(LS_ERROR) << "SetTrack can't be called on
stopped RtpSender.";
On 2015/10/20 17:42:49, pthatcher1 wrote:
> on stopped => on a stopped

Done.

https://codereview.webrtc.org/1413713003/diff/40001/talk/app/webrtc/rtpsender...
talk/app/webrtc/rtpsender.cc:107: return false;
On 2015/10/20 17:42:49, pthatcher1 wrote:
> Can we not support this?  Why not just have it cause sending to stop?

I thought we weren't supposed to support this, but I guess it could end up being
useful internally.

https://codereview.webrtc.org/1413713003/diff/40001/talk/app/webrtc/rtpsender...
talk/app/webrtc/rtpsender.cc:142: void AudioRtpSender::SetSsrc(uint32_t ssrc) {
On 2015/10/20 17:42:49, pthatcher1 wrote:
> Do we support changing SSRCs for a given sender?  Would it be easier to just
> make the rule that you can only set it once?
> 
> And can we unset it?  That seems like something that complicates the logic a
bit
> here.

We talked about this a bit; basically, it was being supported BEFORE the
MediaStreamSignaling refactoring, and I wanted to be wary of making incompatible
changes in these CLs.

https://codereview.webrtc.org/1413713003/diff/40001/talk/app/webrtc/rtpsender...
talk/app/webrtc/rtpsender.cc:159: }
On 2015/10/20 17:42:49, pthatcher1 wrote:
> Do we support stats_ to be null?  Would it be easier to just say it must be
> non-null?

Yes, it's for unit tests, mainly.

https://codereview.webrtc.org/1413713003/diff/40001/talk/app/webrtc/rtpsender...
talk/app/webrtc/rtpsender.cc:183: RTC_DCHECK(!stopped_ && track_ && ssrc_);
On 2015/10/20 17:42:49, pthatcher1 wrote:
> Would it be easier to just do the check for stopped_, track_ and ssrc_ here
> rather than at all the call sites?

Maybe, but it would only save a few lines of code, and it would make SetTrack
more complicated. Would SetTrack get all the way to SetAudioSend, then fail?
What about the cases where SetAudioSend isn't called? I think this way is
cleaner.

https://codereview.webrtc.org/1413713003/diff/40001/talk/app/webrtc/rtpsender...
talk/app/webrtc/rtpsender.cc:238: }
On 2015/10/20 17:42:49, pthatcher1 wrote:
> Similarly as with audio: can we support this?

Done.

https://codereview.webrtc.org/1413713003/diff/40001/talk/app/webrtc/rtpsender...
talk/app/webrtc/rtpsender.cc:299: }
On 2015/10/20 17:42:49, pthatcher1 wrote:
> What if we have an ssrc_ but no track_?  Would we still want to call these
> provider_ methods?

This is inside "if (track_)" so it's safe. I rearranged the code a bit to make
this clearer though (adding a "ready_to_send()" method that returns "track_ &&
ssrc_).

https://codereview.webrtc.org/1413713003/diff/40001/talk/app/webrtc/rtpsender.h
File talk/app/webrtc/rtpsender.h (right):

https://codereview.webrtc.org/1413713003/diff/40001/talk/app/webrtc/rtpsender...
talk/app/webrtc/rtpsender.h:120: uint32_t ssrc_ = 0;
On 2015/10/20 17:42:49, pthatcher1 wrote:
> Can you put a comment about how 0 is "unset" and what the Sender does when
it's
> in that state?

It's in rtpsenderinterface.h.

https://codereview.webrtc.org/1413713003/diff/40001/talk/app/webrtc/rtpsender...
File talk/app/webrtc/rtpsenderinterface.h (right):

https://codereview.webrtc.org/1413713003/diff/40001/talk/app/webrtc/rtpsender...
talk/app/webrtc/rtpsenderinterface.h:53: // description).
On 2015/10/20 17:42:49, pthatcher1 wrote:
> Instead of setting SSRC to 0 here, could the caller instead call Stop() when
> it's missing from the local description?

Stop is permanent, SetSsrc(0) is not (as we discussed).

https://codereview.webrtc.org/1413713003/diff/40001/talk/app/webrtc/rtpsender...
talk/app/webrtc/rtpsenderinterface.h:55: // but what is there to guarantee the
peer does?
On 2015/10/20 17:42:49, pthatcher1 wrote:
> Do you mean the remote peer?  It doesn't get to pick our send SSRCs at all. 
It
> can only pick its own send SSRCs, which or our receive SSRCs.

I think you already answered this question, I just forgot to remove the comment.
Glad you caught it..

https://codereview.webrtc.org/1413713003/diff/40001/talk/app/webrtc/rtpsender...
talk/app/webrtc/rtpsenderinterface.h:56: virtual void SetSsrc(uint32_t ssrc) =
0;
On 2015/10/20 17:42:49, pthatcher1 wrote:
> I think we should call this set_ssrc.

I thought a method should only be lowercase_underscore if its action is trivial
(like just setting or returning a variable). SetSsrc does more than that; it's
on the same level as SetTrack, almost.

https://codereview.webrtc.org/1413713003/diff/40001/talk/app/webrtc/rtpsender...
talk/app/webrtc/rtpsenderinterface.h:60: virtual std::string kind() const = 0;
On 2015/10/20 17:42:49, pthatcher1 wrote:
> I think this should be a cricket::MediaType.

Done.

https://codereview.webrtc.org/1413713003/diff/40001/talk/app/webrtc/rtpsender...
File talk/app/webrtc/rtpsenderreceiver_unittest.cc (right):

https://codereview.webrtc.org/1413713003/diff/40001/talk/app/webrtc/rtpsender...
talk/app/webrtc/rtpsenderreceiver_unittest.cc:312: EXPECT_CALL(audio_provider_,
SetAudioSend(kAudioSsrc, false, _, _)).Times(1);
On 2015/10/20 17:42:49, pthatcher1 wrote:
> Should we test that this gets called even if SetTrack is never called?

It shouldn't get called if SetTrack is never called. In fact, the
"AudioSenderWithoutTrack" test above tests just that.

https://codereview.webrtc.org/1413713003/diff/40001/talk/app/webrtc/rtpsender...
talk/app/webrtc/rtpsenderreceiver_unittest.cc:362: }
On 2015/10/20 17:42:49, pthatcher1 wrote:
> It seems you don't have any tests for setting the SSRCs more than once.  Is
that
> because we don't want to support it?  I think that's a good idea.  But then we
> should probably have it return false when you try to, and we should test that
it
> returns false when you do so.

No, I just wasn't thorough enough. I added some more tests in a new patch.

https://codereview.webrtc.org/1413713003/diff/40001/webrtc/base/helpers.cc
File webrtc/base/helpers.cc (right):

https://codereview.webrtc.org/1413713003/diff/40001/webrtc/base/helpers.cc#ne...
webrtc/base/helpers.cc:270: str.push_back(kHex[((bytes[15] & ~0x4) | 0x8) %
16]);
On 2015/10/20 17:42:50, pthatcher1 wrote:
> Could you make a table with 8, 9, a, and b and then use kFoo[bytes[i] & 4]? 
> That would be a bit more readable.

Done.

https://codereview.webrtc.org/1413713003/diff/40001/webrtc/base/helpers.h
File webrtc/base/helpers.h (right):

https://codereview.webrtc.org/1413713003/diff/40001/webrtc/base/helpers.h#new...
webrtc/base/helpers.h:43: std::string CreateRandomUuid();
On 2015/10/20 17:42:50, pthatcher1 wrote:
> Why did you choose to use a UUID?  Is that part of the standard that I'm not
> aware of?

It's "recommended", and it's what Chrome currently does, so I figured it was the
safest bet.

pthatcher1

Looks much better. Just one naming nit, and I'm still a little concerned about DCHECK ...

5 years, 2 months ago (2015-10-22 07:34:14 UTC) #5

Looks much better.  Just one naming nit, and I'm still a little concerned about
DCHECK that can be caused by bad JS.

https://codereview.webrtc.org/1413713003/diff/40001/talk/app/webrtc/peerconne...
File talk/app/webrtc/peerconnection.cc (right):

https://codereview.webrtc.org/1413713003/diff/40001/talk/app/webrtc/peerconne...
talk/app/webrtc/peerconnection.cc:1651: RTC_DCHECK(false && "Invalid media
type.");
On 2015/10/21 00:22:08, Taylor Brandstetter wrote:
> On 2015/10/20 17:42:49, pthatcher1 wrote:
> > This is an error from the application or the javascript (mismatch of SDP
with
> > track), possibly from SDP munging.  It seems bad to cause a DCHECK in that
> case.
> >  I think an LS_WARNING is sufficient.
> 
> DCHECKS only do anything in a debug builds (hence the 'D').

But do we still want to allow bad JS to crash the tab, even in debug build?

https://codereview.webrtc.org/1413713003/diff/40001/talk/app/webrtc/rtpsender.cc
File talk/app/webrtc/rtpsender.cc (right):

https://codereview.webrtc.org/1413713003/diff/40001/talk/app/webrtc/rtpsender...
talk/app/webrtc/rtpsender.cc:183: RTC_DCHECK(!stopped_ && track_ && ssrc_);
On 2015/10/21 00:22:08, Taylor Brandstetter wrote:
> On 2015/10/20 17:42:49, pthatcher1 wrote:
> > Would it be easier to just do the check for stopped_, track_ and ssrc_ here
> > rather than at all the call sites?
> 
> Maybe, but it would only save a few lines of code, and it would make SetTrack
> more complicated. Would SetTrack get all the way to SetAudioSend, then fail?
> What about the cases where SetAudioSend isn't called? I think this way is
> cleaner.

This does look good.

https://codereview.webrtc.org/1413713003/diff/40001/talk/app/webrtc/rtpsender...
talk/app/webrtc/rtpsender.cc:299: }
On 2015/10/21 00:22:08, Taylor Brandstetter wrote:
> On 2015/10/20 17:42:49, pthatcher1 wrote:
> > What if we have an ssrc_ but no track_?  Would we still want to call these
> > provider_ methods?
> 
> This is inside "if (track_)" so it's safe. I rearranged the code a bit to make
> this clearer though (adding a "ready_to_send()" method that returns "track_ &&
> ssrc_).

Looks much better.

https://codereview.webrtc.org/1413713003/diff/40001/talk/app/webrtc/rtpsender...
File talk/app/webrtc/rtpsenderinterface.h (right):

https://codereview.webrtc.org/1413713003/diff/40001/talk/app/webrtc/rtpsender...
talk/app/webrtc/rtpsenderinterface.h:56: virtual void SetSsrc(uint32_t ssrc) =
0;
On 2015/10/21 00:22:09, Taylor Brandstetter wrote:
> On 2015/10/20 17:42:49, pthatcher1 wrote:
> > I think we should call this set_ssrc.
> 
> I thought a method should only be lowercase_underscore if its action is
trivial
> (like just setting or returning a variable). SetSsrc does more than that; it's
> on the same level as SetTrack, almost.

Yes, you're right.

https://codereview.webrtc.org/1413713003/diff/100001/talk/app/webrtc/rtpsender.h
File talk/app/webrtc/rtpsender.h (right):

https://codereview.webrtc.org/1413713003/diff/100001/talk/app/webrtc/rtpsende...
talk/app/webrtc/rtpsender.h:99: cricket::MediaType type() const override {
return cricket::MEDIA_TYPE_AUDIO; }
Should this be media_type()?

https://codereview.webrtc.org/1413713003/diff/100001/talk/app/webrtc/rtpsende...
talk/app/webrtc/rtpsender.h:111: bool ready_to_send() const { return track_ &&
ssrc_; }
Since the transport code already has a ready_to_send(), this might get a little
confusing since it's the same name with a different meaning.

Would "sending" make sense?  Obviously, it will only send if the track produces
frames, but I think that's OK.

Taylor Brandstetter

https://codereview.webrtc.org/1413713003/diff/40001/talk/app/webrtc/peerconnection.cc File talk/app/webrtc/peerconnection.cc (right): https://codereview.webrtc.org/1413713003/diff/40001/talk/app/webrtc/peerconnection.cc#newcode1651 talk/app/webrtc/peerconnection.cc:1651: RTC_DCHECK(false && "Invalid media type."); On 2015/10/22 07:34:13, pthatcher1 ...

5 years, 2 months ago (2015-10-22 19:26:42 UTC) #6

https://codereview.webrtc.org/1413713003/diff/40001/talk/app/webrtc/peerconne...
File talk/app/webrtc/peerconnection.cc (right):

https://codereview.webrtc.org/1413713003/diff/40001/talk/app/webrtc/peerconne...
talk/app/webrtc/peerconnection.cc:1651: RTC_DCHECK(false && "Invalid media
type.");
On 2015/10/22 07:34:13, pthatcher1 wrote:
> On 2015/10/21 00:22:08, Taylor Brandstetter wrote:
> > On 2015/10/20 17:42:49, pthatcher1 wrote:
> > > This is an error from the application or the javascript (mismatch of SDP
> with
> > > track), possibly from SDP munging.  It seems bad to cause a DCHECK in that
> > case.
> > >  I think an LS_WARNING is sufficient.
> > 
> > DCHECKS only do anything in a debug builds (hence the 'D').
> 
> But do we still want to allow bad JS to crash the tab, even in debug build?   

Oh, that's a good point. Ok, it's removed.

https://codereview.webrtc.org/1413713003/diff/100001/talk/app/webrtc/rtpsender.h
File talk/app/webrtc/rtpsender.h (right):

https://codereview.webrtc.org/1413713003/diff/100001/talk/app/webrtc/rtpsende...
talk/app/webrtc/rtpsender.h:99: cricket::MediaType type() const override {
return cricket::MEDIA_TYPE_AUDIO; }
On 2015/10/22 07:34:14, pthatcher1 wrote:
> Should this be media_type()?

Done.

https://codereview.webrtc.org/1413713003/diff/100001/talk/app/webrtc/rtpsende...
talk/app/webrtc/rtpsender.h:111: bool ready_to_send() const { return track_ &&
ssrc_; }
On 2015/10/22 07:34:14, pthatcher1 wrote:
> Since the transport code already has a ready_to_send(), this might get a
little
> confusing since it's the same name with a different meaning.
> 
> Would "sending" make sense?  Obviously, it will only send if the track
produces
> frames, but I think that's OK.

"sending" isn't really accurate since this method will return true when we
aren't currently sending. How about "can_send_track"; it means just about the
same thing as "ready_to_send" but the name won't be confused.

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1413713003/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1413713003/140001

5 years, 1 month ago (2015-10-26 17:43:52 UTC) #12

commit-bot: I haz the power

Try jobs failed on following builders: presubmit on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/1416)

5 years, 1 month ago (2015-10-26 17:46:09 UTC) #14

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1413713003/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1413713003/140001

5 years, 1 month ago (2015-10-26 18:47:07 UTC) #17

commit-bot: I haz the power

Patchset 8 (id:??) landed as https://crrev.com/ac9d92ccbe2b29590c53f702e11dc625820480d5 Cr-Commit-Position: refs/heads/master@{#10414}

5 years, 1 month ago (2015-10-26 18:48:36 UTC) #19

Taylor Brandstetter

5 years, 1 month ago (2015-10-26 21:10:55 UTC) #20

Message was sent while issue was closed.

A revert of this CL (patchset #8 id:140001) has been created in
https://codereview.webrtc.org/1426443007/ by deadbeef@webrtc.org.

The reason for reverting is: Causing a compiler warning, and causing
WebRtcBrowserTest.CallAndModifyStream to fail..

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages