Adding "first packet received" notification to PeerConnectionObserver.

Adding "first packet received" notification to PeerConnectionObserver.

R=glaznev@webrtc.org, pthatcher@webrtc.org, tkchin@webrtc.org

Committed: https://crrev.com/42265a8cc3b3f3db4aa2c29005aed2fb4393adef
Cr-Commit-Position: refs/heads/master@{#11401}

Committed: https://crrev.com/08a6eab75e13613183509d91d3892c1db57f6fc5
Cr-Commit-Position: refs/heads/master@{#11404}

[Taylor Brandstetter, 2016-01-14 23:31:06]

deadbeef@webrtc.org changed reviewers:
+ pthatcher@webrtc.org

[pthatcher1, 2016-01-14 23:50:09]

https://codereview.chromium.org/1581693006/diff/1/talk/app/webrtc/java/src/or...
File talk/app/webrtc/java/src/org/webrtc/PeerConnection.java (right):

https://codereview.chromium.org/1581693006/diff/1/talk/app/webrtc/java/src/or...
talk/app/webrtc/java/src/org/webrtc/PeerConnection.java:91: public void
onFirstPacketReceived();
I'm worried about the "SCTP packet" here.  SCTP may send internal packets before
you even choose to send a message over it, and the BaseChannel doesn't
differentiate.  So this may fire before you're expecting it.

I think we need to update BaseChannel to check for RTP and not just !rtcp here:

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libjin...

That would remove the chance of being tricked by internal SCTP packets.

Further, I don't think there is any value in firing up when we get the first
"real" SCTP packet, since a small SCTP message will get process as quickly as
one packet anyway, and an application that wants to do signalling over SCTP
would find it impossible to differentiate between the first media and the first
signalling.

https://codereview.chromium.org/1581693006/diff/1/talk/app/webrtc/webrtcsessi...
File talk/app/webrtc/webrtcsession.cc (right):

https://codereview.chromium.org/1581693006/diff/1/talk/app/webrtc/webrtcsessi...
talk/app/webrtc/webrtcsession.cc:1906: if (has_received_packet_) {
Should that be "!has_recieved_packet_"?

[Taylor Brandstetter, 2016-01-15 17:06:58]

https://codereview.chromium.org/1581693006/diff/1/talk/app/webrtc/java/src/or...
File talk/app/webrtc/java/src/org/webrtc/PeerConnection.java (right):

https://codereview.chromium.org/1581693006/diff/1/talk/app/webrtc/java/src/or...
talk/app/webrtc/java/src/org/webrtc/PeerConnection.java:91: public void
onFirstPacketReceived();
On 2016/01/14 23:50:09, pthatcher1 wrote:
> I'm worried about the "SCTP packet" here.  SCTP may send internal packets
before
> you even choose to send a message over it, and the BaseChannel doesn't
> differentiate.  So this may fire before you're expecting it.
> 
> I think we need to update BaseChannel to check for RTP and not just !rtcp
here:
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libjin...
> 
> That would remove the chance of being tricked by internal SCTP packets.
> 
> Further, I don't think there is any value in firing up when we get the first
> "real" SCTP packet, since a small SCTP message will get process as quickly as
> one packet anyway, and an application that wants to do signalling over SCTP
> would find it impossible to differentiate between the first media and the
first
> signalling.

I'll just change it to "first media packet received", since like you said,
knowing when the first data message is received is already possible.

https://codereview.chromium.org/1581693006/diff/1/talk/app/webrtc/webrtcsessi...
File talk/app/webrtc/webrtcsession.cc (right):

https://codereview.chromium.org/1581693006/diff/1/talk/app/webrtc/webrtcsessi...
talk/app/webrtc/webrtcsession.cc:1906: if (has_received_packet_) {
On 2016/01/14 23:50:09, pthatcher1 wrote:
> Should that be "!has_recieved_packet_"?

Yep; I would have noticed this pretty quickly if I remembered to run the unit
tests...

[pthatcher1, 2016-01-15 19:36:49]

lgtm

[Taylor Brandstetter, 2016-01-21 21:19:54]

The CQ bit was checked by deadbeef@webrtc.org

[commit-bot: I haz the power, 2016-01-21 21:20:04]

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

[commit-bot: I haz the power, 2016-01-21 21:27:06]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-01-21 21:27:07]

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

[Taylor Brandstetter, 2016-01-21 21:34:13]

deadbeef@webrtc.org changed reviewers:
+ glaznev@webrtc.org, tkchin@webrtc.org

[Taylor Brandstetter, 2016-01-21 21:34:13]

glaznev@: Please review Java changes.
tkchin@: Please review Objective-C changes.

[tkchin_webrtc, 2016-01-21 23:17:51]

On 2016/01/21 21:34:13, Taylor Brandstetter wrote:
> glaznev@: Please review Java changes.
> tkchin@: Please review Objective-C changes.

lgtm

What are we expecting to use this new event for?

[Taylor Brandstetter, 2016-01-21 23:50:26]

On 2016/01/21 23:17:51, tkchin_webrtc wrote:
> On 2016/01/21 21:34:13, Taylor Brandstetter wrote:
> > glaznev@: Please review Java changes.
> > tkchin@: Please review Objective-C changes.
> 
> lgtm
> 
> What are we expecting to use this new event for?

The idea was that an application could interpret receiving a media packet as
consent from the peer to receive media packets itself. So this event could be
used to speed up the "early ICE warmup" scenarios, assuming the media path is
faster than the signalling path.

[AlexG, 2016-01-22 02:40:54]

lgtm

https://codereview.webrtc.org/1581693006/diff/60001/talk/app/webrtc/java/jni/...
File talk/app/webrtc/java/jni/peerconnection_jni.cc (right):

https://codereview.webrtc.org/1581693006/diff/60001/talk/app/webrtc/java/jni/...
talk/app/webrtc/java/jni/peerconnection_jni.cc:379: jmethodID m =
GetMethodID(jni(), *j_observer_class_,
nit: alignment

https://codereview.webrtc.org/1581693006/diff/60001/talk/app/webrtc/java/test...
File talk/app/webrtc/java/testcommon/src/org/webrtc/PeerConnectionTest.java
(right):

https://codereview.webrtc.org/1581693006/diff/60001/talk/app/webrtc/java/test...
talk/app/webrtc/java/testcommon/src/org/webrtc/PeerConnectionTest.java:293:
public synchronized void onFirstMediaPacketReceived() {}
nit: no need in synchronized for empty function

[Taylor Brandstetter, 2016-01-27 17:35:17]

The CQ bit was checked by deadbeef@webrtc.org

[commit-bot: I haz the power, 2016-01-27 17:35:25]

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

[commit-bot: I haz the power, 2016-01-27 18:03:40]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-01-27 18:03:41]

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

[Taylor Brandstetter, 2016-01-27 20:10:48]

Description was changed from

==========
Adding "first packet received" notification to PeerConnectionObserver.
==========

to

==========
Adding "first packet received" notification to PeerConnectionObserver.

R=glaznev@webrtc.org, pthatcher@webrtc.org, tkchin@webrtc.org

Committed:
https://chromium.googlesource.com/external/webrtc/+/42265a8cc3b3f3db4aa2c2900...
==========

[Taylor Brandstetter, 2016-01-27 20:10:49]

Committed patchset #4 (id:60001) manually as
42265a8cc3b3f3db4aa2c29005aed2fb4393adef (presubmit successful).

[commit-bot: I haz the power, 2016-01-27 20:10:53]

Description was changed from

==========
Adding "first packet received" notification to PeerConnectionObserver.

R=glaznev@webrtc.org, pthatcher@webrtc.org, tkchin@webrtc.org

Committed:
https://chromium.googlesource.com/external/webrtc/+/42265a8cc3b3f3db4aa2c2900...
==========

to

==========
Adding "first packet received" notification to PeerConnectionObserver.

R=glaznev@webrtc.org, pthatcher@webrtc.org, tkchin@webrtc.org

Committed: https://crrev.com/42265a8cc3b3f3db4aa2c29005aed2fb4393adef
Cr-Commit-Position: refs/heads/master@{#11401}
==========

[commit-bot: I haz the power, 2016-01-27 20:10:54]

Patchset 4 (id:??) landed as
https://crrev.com/42265a8cc3b3f3db4aa2c29005aed2fb4393adef
Cr-Commit-Position: refs/heads/master@{#11401}

[Taylor Brandstetter, 2016-01-27 21:03:23]

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

The reason for reverting is: Seems that the end-to-end unit tests are now flaky:
https://build.chromium.org/p/client.webrtc/builders/Win64%20Debug/builds/6283

Will reland after fixing the test flakiness..

[Taylor Brandstetter, 2016-01-27 21:23:49]

Description was changed from

==========
Adding "first packet received" notification to PeerConnectionObserver.

R=glaznev@webrtc.org, pthatcher@webrtc.org, tkchin@webrtc.org

Committed: https://crrev.com/42265a8cc3b3f3db4aa2c29005aed2fb4393adef
Cr-Commit-Position: refs/heads/master@{#11401}
==========

to

==========
Adding "first packet received" notification to PeerConnectionObserver.

R=glaznev@webrtc.org, pthatcher@webrtc.org, tkchin@webrtc.org

Committed: https://crrev.com/42265a8cc3b3f3db4aa2c29005aed2fb4393adef
Cr-Commit-Position: refs/heads/master@{#11401}
==========

[Taylor Brandstetter, 2016-01-27 21:27:01]

Just had to change "EXPECT_TRUE" to "EXPECT_TRUE_WAIT" in LocalP2PTest. This is
because if somehow 3 frames are sent/received on the worker thread without
hopping to the signaling thread, the "packet received" message will still be
stuck in the signaling thread queue.

[Taylor Brandstetter, 2016-01-27 21:39:00]

Description was changed from

==========
Adding "first packet received" notification to PeerConnectionObserver.

R=glaznev@webrtc.org, pthatcher@webrtc.org, tkchin@webrtc.org

Committed: https://crrev.com/42265a8cc3b3f3db4aa2c29005aed2fb4393adef
Cr-Commit-Position: refs/heads/master@{#11401}
==========

to

==========
Adding "first packet received" notification to PeerConnectionObserver.

R=glaznev@webrtc.org, pthatcher@webrtc.org, tkchin@webrtc.org

Committed: https://crrev.com/42265a8cc3b3f3db4aa2c29005aed2fb4393adef
Cr-Commit-Position: refs/heads/master@{#11401}

Committed:
https://chromium.googlesource.com/external/webrtc/+/08a6eab75e13613183509d91d...
==========

[Taylor Brandstetter, 2016-01-27 21:39:01]

Committed patchset #5 (id:80001) manually as
08a6eab75e13613183509d91d3892c1db57f6fc5 (presubmit successful).

[commit-bot: I haz the power, 2016-01-27 21:39:11]

Description was changed from

==========
Adding "first packet received" notification to PeerConnectionObserver.

R=glaznev@webrtc.org, pthatcher@webrtc.org, tkchin@webrtc.org

Committed: https://crrev.com/42265a8cc3b3f3db4aa2c29005aed2fb4393adef
Cr-Commit-Position: refs/heads/master@{#11401}

Committed:
https://chromium.googlesource.com/external/webrtc/+/08a6eab75e13613183509d91d...
==========

to

==========
Adding "first packet received" notification to PeerConnectionObserver.

R=glaznev@webrtc.org, pthatcher@webrtc.org, tkchin@webrtc.org

Committed: https://crrev.com/42265a8cc3b3f3db4aa2c29005aed2fb4393adef
Cr-Commit-Position: refs/heads/master@{#11401}

Committed: https://crrev.com/08a6eab75e13613183509d91d3892c1db57f6fc5
Cr-Commit-Position: refs/heads/master@{#11404}
==========

[commit-bot: I haz the power, 2016-01-27 21:39:12]

Patchset 5 (id:??) landed as
https://crrev.com/08a6eab75e13613183509d91d3892c1db57f6fc5
Cr-Commit-Position: refs/heads/master@{#11404}

[terelius, 2016-01-28 13:05:55]

A revert of this CL (patchset #5 id:80001) has been created in
https://codereview.webrtc.org/1647483004/ by terelius@webrtc.org.

The reason for reverting is: onFirstMediaPacketReceived() breaks bot..