Forward the SignalFirstPacketReceived to RtpReceiver.

Forward the SignalFirstPacketReceived to RtpReceiver.

The RtpReceiverObserverInterface is created.
The SignalFirstPacketReceived will be forwarded from BaseChannel to WebRtcSession.
WebRtcSession will forward SignalFirstAudioPacketReceived and SignalFirstVideoPacketReceived to the RtpReceiverInterface.
The application can listen to the Signal by implementing and registering a RtpReceiverObserver.

Committed: https://crrev.com/184a3fd6489b2602a21c415cad3c51a7b035040b
Cr-Commit-Position: refs/heads/master@{#13139}

[Zhi Huang, 2016-05-20 18:48:36]

zhihuang@webrtc.org changed reviewers:
+ deadbeef@google.com, pthatcher@webrtc.org

[Zhi Huang, 2016-05-20 18:48:36]

This CL works fine on the Callee side. If we want to make it work on the caller
side, we may need more changes. Because currently we do not have the interface
to create the receiver. It seems that we can only register the observer for
receiver after setting the remote description. It might be too late for the
caller. We may need an interface like CreateReceiver() and use the existing
receivers when setting the remote desc.

[pthatcher2, 2016-05-20 20:23:12]

pthatcher@chromium.org changed reviewers:
+ deadbeef@webrtc.org, pthatcher@chromium.org
- deadbeef@google.com

[pthatcher2, 2016-05-20 20:23:14]

https://codereview.webrtc.org/1999853002/diff/20001/webrtc/api/peerconnection...
File webrtc/api/peerconnection_unittest.cc (right):

https://codereview.webrtc.org/1999853002/diff/20001/webrtc/api/peerconnection...
webrtc/api/peerconnection_unittest.cc:156: void
onFirstPacketReceived(cricket::MediaType media_type) override {
OnFirstPacketReceived

https://codereview.webrtc.org/1999853002/diff/20001/webrtc/api/peerconnection...
webrtc/api/peerconnection_unittest.cc:812: RtpReceiverObservers() {
rtp_receiver_observers()

https://codereview.webrtc.org/1999853002/diff/20001/webrtc/api/peerconnection...
webrtc/api/peerconnection_unittest.cc:907: DoRegisterRtpReceiverObservers();
Why not just RegisterRtpReceiverObservers()?

https://codereview.webrtc.org/1999853002/diff/20001/webrtc/api/peerconnection...
webrtc/api/peerconnection_unittest.cc:1367: // the observer before sending the
data.
Here's a simple solution:  whenever an observer is added after the event has
fired, tell the observer that the evern has fired by calling
OnFirstPacketReceived.

https://codereview.webrtc.org/1999853002/diff/20001/webrtc/api/peerconnection...
webrtc/api/peerconnection_unittest.cc:1380: EXPECT_TRUE(false);
I think you can add a message to the EXPECT, like "Unknown RtpReceiver kind"

https://codereview.webrtc.org/1999853002/diff/20001/webrtc/api/peerconnection...
webrtc/api/peerconnection_unittest.cc:1405: CheckRtpReceiverObserverAreCalled();
If it's only called once, just put the code inline and don't bother with a
separate function.

https://codereview.webrtc.org/1999853002/diff/20001/webrtc/api/rtpreceiver.cc
File webrtc/api/rtpreceiver.cc (right):

https://codereview.webrtc.org/1999853002/diff/20001/webrtc/api/rtpreceiver.cc...
webrtc/api/rtpreceiver.cc:91: rtp_receiver_observer_ = observer;
We could add:

if (received_first_packet_) {
  observer::OnFirstPacketReceived();
}

https://codereview.webrtc.org/1999853002/diff/20001/webrtc/api/rtpreceiver.cc...
webrtc/api/rtpreceiver.cc:97: }
Right here we could store:

received_first_packet_ = true;

https://codereview.webrtc.org/1999853002/diff/20001/webrtc/api/rtpreceiver.cc...
webrtc/api/rtpreceiver.cc:161: }
Same here.

https://codereview.webrtc.org/1999853002/diff/20001/webrtc/api/rtpreceiver.h
File webrtc/api/rtpreceiver.h (right):

https://codereview.webrtc.org/1999853002/diff/20001/webrtc/api/rtpreceiver.h#...
webrtc/api/rtpreceiver.h:69: void onFirstAudioPacketReceived();
OnFirstAudioPacketReceived()

https://codereview.webrtc.org/1999853002/diff/20001/webrtc/api/rtpreceiver.h#...
webrtc/api/rtpreceiver.h:76: RtpReceiverObserverInterface*
rtp_receiver_observer_;
observer_ is probably long enough of  a name.

https://codereview.webrtc.org/1999853002/diff/20001/webrtc/api/rtpreceiver.h#...
webrtc/api/rtpreceiver.h:107: RtpReceiverObserverInterface* observer) override;
Since there is only one, we should call these "Set" instead of "Register".  And
We don't need to say 
"RtpReceiver" again.  We could just name it "SetObserver".

https://codereview.webrtc.org/1999853002/diff/20001/webrtc/api/rtpreceiver.h#...
webrtc/api/rtpreceiver.h:110: void onFirstVideoPacketReceived();
Same here

https://codereview.webrtc.org/1999853002/diff/20001/webrtc/api/rtpreceiver.h#...
webrtc/api/rtpreceiver.h:123: RtpReceiverObserverInterface*
rtp_receiver_observer_;
Same here

https://codereview.webrtc.org/1999853002/diff/20001/webrtc/api/rtpreceiverint...
File webrtc/api/rtpreceiverinterface.h (right):

https://codereview.webrtc.org/1999853002/diff/20001/webrtc/api/rtpreceiverint...
webrtc/api/rtpreceiverinterface.h:27: class RtpReceiverObserverInterface :
public rtc::RefCountInterface {
Why does it need to be refcounted?

https://codereview.webrtc.org/1999853002/diff/20001/webrtc/api/rtpreceiverint...
webrtc/api/rtpreceiverinterface.h:29: virtual void
onFirstPacketReceived(cricket::MediaType media_type) = 0;
OnFirstPacketReceived

https://codereview.webrtc.org/1999853002/diff/20001/webrtc/api/webrtcsession.cc
File webrtc/api/webrtcsession.cc (right):

https://codereview.webrtc.org/1999853002/diff/20001/webrtc/api/webrtcsession....
webrtc/api/webrtcsession.cc:1893: channel->content_name() == cricket::CN_AUDIO)
{
We should use the media type, not the content name.

[zhihuang1, 2016-05-24 01:01:43]

zhihuang@chromium.org changed reviewers:
+ zhihuang@chromium.org
- pthatcher@chromium.org

[zhihuang1, 2016-05-24 01:01:44]

https://codereview.webrtc.org/1999853002/diff/20001/webrtc/api/peerconnection...
File webrtc/api/peerconnection_unittest.cc (right):

https://codereview.webrtc.org/1999853002/diff/20001/webrtc/api/peerconnection...
webrtc/api/peerconnection_unittest.cc:812: RtpReceiverObservers() {
On 2016/05/20 20:23:13, pthatcher2 wrote:
> rtp_receiver_observers()

Done.

https://codereview.webrtc.org/1999853002/diff/20001/webrtc/api/peerconnection...
webrtc/api/peerconnection_unittest.cc:907: DoRegisterRtpReceiverObservers();
On 2016/05/20 20:23:13, pthatcher2 wrote:
> Why not just RegisterRtpReceiverObservers()?

Done.

https://codereview.webrtc.org/1999853002/diff/20001/webrtc/api/peerconnection...
webrtc/api/peerconnection_unittest.cc:1367: // the observer before sending the
data.
On 2016/05/20 20:23:13, pthatcher2 wrote:
> Here's a simple solution:  whenever an observer is added after the event has
> fired, tell the observer that the evern has fired by calling
> OnFirstPacketReceived.

This is a nice and easy solution.

https://codereview.webrtc.org/1999853002/diff/20001/webrtc/api/peerconnection...
webrtc/api/peerconnection_unittest.cc:1380: EXPECT_TRUE(false);
On 2016/05/20 20:23:13, pthatcher2 wrote:
> I think you can add a message to the EXPECT, like "Unknown RtpReceiver kind"

Done.

https://codereview.webrtc.org/1999853002/diff/20001/webrtc/api/peerconnection...
webrtc/api/peerconnection_unittest.cc:1405: CheckRtpReceiverObserverAreCalled();
On 2016/05/20 20:23:13, pthatcher2 wrote:
> If it's only called once, just put the code inline and don't bother with a
> separate function.

I will make it a helper function to check both the caller and callee side.

https://codereview.webrtc.org/1999853002/diff/20001/webrtc/api/rtpreceiver.h
File webrtc/api/rtpreceiver.h (right):

https://codereview.webrtc.org/1999853002/diff/20001/webrtc/api/rtpreceiver.h#...
webrtc/api/rtpreceiver.h:69: void onFirstAudioPacketReceived();
On 2016/05/20 20:23:13, pthatcher2 wrote:
> OnFirstAudioPacketReceived()

Done.

https://codereview.webrtc.org/1999853002/diff/20001/webrtc/api/rtpreceiver.h#...
webrtc/api/rtpreceiver.h:76: RtpReceiverObserverInterface*
rtp_receiver_observer_;
On 2016/05/20 20:23:14, pthatcher2 wrote:
> observer_ is probably long enough of  a name.

Done.

https://codereview.webrtc.org/1999853002/diff/20001/webrtc/api/rtpreceiver.h#...
webrtc/api/rtpreceiver.h:107: RtpReceiverObserverInterface* observer) override;
On 2016/05/20 20:23:14, pthatcher2 wrote:
> Since there is only one, we should call these "Set" instead of "Register". 
And
> We don't need to say 
> "RtpReceiver" again.  We could just name it "SetObserver".

Done.

https://codereview.webrtc.org/1999853002/diff/20001/webrtc/api/rtpreceiver.h#...
webrtc/api/rtpreceiver.h:110: void onFirstVideoPacketReceived();
On 2016/05/20 20:23:14, pthatcher2 wrote:
> Same here

Done.

https://codereview.webrtc.org/1999853002/diff/20001/webrtc/api/webrtcsession.cc
File webrtc/api/webrtcsession.cc (right):

https://codereview.webrtc.org/1999853002/diff/20001/webrtc/api/webrtcsession....
webrtc/api/webrtcsession.cc:1893: channel->content_name() == cricket::CN_AUDIO)
{
On 2016/05/20 20:23:14, pthatcher2 wrote:
> We should use the media type, not the content name.

Done.

[pthatcher1, 2016-06-08 17:35:56]

https://codereview.webrtc.org/1999853002/diff/40001/webrtc/api/peerconnection...
File webrtc/api/peerconnection_unittest.cc (right):

https://codereview.webrtc.org/1999853002/diff/40001/webrtc/api/peerconnection...
webrtc/api/peerconnection_unittest.cc:151: MockRtpReceiverObserver()
I'd suggest just passing the kind (or expected cricket::MediaType; see below)
into the constructor.

https://codereview.webrtc.org/1999853002/diff/40001/webrtc/api/peerconnection...
webrtc/api/peerconnection_unittest.cc:161: }
Given the unlikelyhood of getting the wrong media type, an
ASSERT_EQ(expected_media_type_, media_type) here might make more sense.  Then
you just have store "first_packet_received_ = true" here and the checking code
will be much more simple.

https://codereview.webrtc.org/1999853002/diff/40001/webrtc/api/peerconnection...
webrtc/api/peerconnection_unittest.cc:168: bool IsFirstAudioPacketReceived() {
return is_first_audio_packet_recevied_; }
Using lowercase first_x_packet_received() is probably a better fit.  The "Is"
prefix sounds awkward, and the simple reading of a member variable matches the
lowercase style more.

https://codereview.webrtc.org/1999853002/diff/40001/webrtc/api/peerconnection...
webrtc/api/peerconnection_unittest.cc:176: bool is_first_video_packet_recevied_;
Just "first_x_packet_received_" would be enough.  The "is_" sounds awkward.

https://codereview.webrtc.org/1999853002/diff/40001/webrtc/api/peerconnection...
webrtc/api/peerconnection_unittest.cc:176: bool is_first_video_packet_recevied_;
You can use "= false" here and avoid needing to set it in the constructor.

https://codereview.webrtc.org/1999853002/diff/40001/webrtc/api/peerconnection...
webrtc/api/peerconnection_unittest.cc:177: std::string kind_;
I think it might make more sense to store the expected_media_type_ here, going
along with the comment above about using an ASSERT_EQ.

https://codereview.webrtc.org/1999853002/diff/40001/webrtc/api/peerconnection...
webrtc/api/peerconnection_unittest.cc:1380: }
With my other suggestion about ASSERT_EQ(media_type, ....), this could just be:

EXPECT_TRUE_WAIT(observer->first_packet_received(), kMaxWaitForFrameMs);

https://codereview.webrtc.org/1999853002/diff/40001/webrtc/api/rtpreceiver.cc
File webrtc/api/rtpreceiver.cc (right):

https://codereview.webrtc.org/1999853002/diff/40001/webrtc/api/rtpreceiver.cc...
webrtc/api/rtpreceiver.cc:33: observer_(nullptr) {
You don't need this if you use "= nullptr" in the other spot.

https://codereview.webrtc.org/1999853002/diff/40001/webrtc/api/rtpreceiver.cc...
webrtc/api/rtpreceiver.cc:102: received_first_packet_ = true;
Do you have a unit test that covers setting the observer after the signal has
fired?  You could do so by simply setting a new observer after it's fired and
then seeing if the second observer fired.

https://codereview.webrtc.org/1999853002/diff/40001/webrtc/api/rtpreceiver.cc...
webrtc/api/rtpreceiver.cc:168:
observer_->OnFirstPacketReceived(cricket::MEDIA_TYPE_VIDEO);
Might as well add a media_type() getter to the RtpReceiver so that you can just
do media_type() here instead of duplicating MEDIA_TYPE_VIDEO.

Also, this might allow having shared code with a common RtpReceiverBase, which
Taylor is planning on adding.

https://codereview.webrtc.org/1999853002/diff/40001/webrtc/api/rtpreceiver.h
File webrtc/api/rtpreceiver.h (right):

https://codereview.webrtc.org/1999853002/diff/40001/webrtc/api/rtpreceiver.h#...
webrtc/api/rtpreceiver.h:75: RtpReceiverObserverInterface* observer_;
observer_ = nullptr;  

?

https://codereview.webrtc.org/1999853002/diff/40001/webrtc/api/rtpreceiver.h#...
webrtc/api/rtpreceiver.h:122: RtpReceiverObserverInterface* observer_;
Same here.

https://codereview.webrtc.org/1999853002/diff/40001/webrtc/api/rtpreceiverint...
File webrtc/api/rtpreceiverinterface.h (right):

https://codereview.webrtc.org/1999853002/diff/40001/webrtc/api/rtpreceiverint...
webrtc/api/rtpreceiverinterface.h:29: virtual void
OnFirstPacketReceived(cricket::MediaType media_type) = 0;
Should we just use the kind here instead of media_type?  That would be more
consistent for the API and make the tests more simple.

https://codereview.webrtc.org/1999853002/diff/40001/webrtc/api/webrtcsession.cc
File webrtc/api/webrtcsession.cc (right):

https://codereview.webrtc.org/1999853002/diff/40001/webrtc/api/webrtcsession....
webrtc/api/webrtcsession.cc:1894: has_received_audio_packet_ = true;
To be consistent with naming, might as well call it
received_first_audio_packet_.

https://codereview.webrtc.org/1999853002/diff/40001/webrtc/api/webrtcsession.h
File webrtc/api/webrtcsession.h (right):

https://codereview.webrtc.org/1999853002/diff/40001/webrtc/api/webrtcsession....
webrtc/api/webrtcsession.h:534: bool has_received_audio_packet_ = false;
You don't need the "has_" prefix.

https://codereview.webrtc.org/1999853002/diff/40001/webrtc/pc/channel.h
File webrtc/pc/channel.h (right):

https://codereview.webrtc.org/1999853002/diff/40001/webrtc/pc/channel.h#newco...
webrtc/pc/channel.h:596: cricket::MediaType MediaType() override { return
cricket::MEDIA_TYPE_DATA; }
media_type()

[Zhi Huang, 2016-06-09 00:37:35]

zhihuang@webrtc.org changed reviewers:
- zhihuang@chromium.org

[Zhi Huang, 2016-06-09 00:37:36]

https://codereview.webrtc.org/1999853002/diff/20001/webrtc/api/rtpreceiver.cc
File webrtc/api/rtpreceiver.cc (right):

https://codereview.webrtc.org/1999853002/diff/20001/webrtc/api/rtpreceiver.cc...
webrtc/api/rtpreceiver.cc:91: rtp_receiver_observer_ = observer;
On 2016/05/20 20:23:13, pthatcher2 wrote:
> We could add:
> 
> if (received_first_packet_) {
>   observer::OnFirstPacketReceived();
> }

This is a good point! In this way, we can fire the signal even the observer is
not set before the receiving the first packet.

https://codereview.webrtc.org/1999853002/diff/40001/webrtc/api/peerconnection...
File webrtc/api/peerconnection_unittest.cc (right):

https://codereview.webrtc.org/1999853002/diff/40001/webrtc/api/peerconnection...
webrtc/api/peerconnection_unittest.cc:1380: }
On 2016/06/08 17:35:55, pthatcher1 wrote:
> With my other suggestion about ASSERT_EQ(media_type, ....), this could just
be:
> 
> EXPECT_TRUE_WAIT(observer->first_packet_received(), kMaxWaitForFrameMs);

This make the code much cleaner.

https://codereview.webrtc.org/1999853002/diff/40001/webrtc/api/rtpreceiver.cc
File webrtc/api/rtpreceiver.cc (right):

https://codereview.webrtc.org/1999853002/diff/40001/webrtc/api/rtpreceiver.cc...
webrtc/api/rtpreceiver.cc:168:
observer_->OnFirstPacketReceived(cricket::MEDIA_TYPE_VIDEO);
On 2016/06/08 17:35:55, pthatcher1 wrote:
> Might as well add a media_type() getter to the RtpReceiver so that you can
just
> do media_type() here instead of duplicating MEDIA_TYPE_VIDEO.
> 
> Also, this might allow having shared code with a common RtpReceiverBase, which
> Taylor is planning on adding.

Done.

https://codereview.webrtc.org/1999853002/diff/40001/webrtc/api/rtpreceiverint...
File webrtc/api/rtpreceiverinterface.h (right):

https://codereview.webrtc.org/1999853002/diff/40001/webrtc/api/rtpreceiverint...
webrtc/api/rtpreceiverinterface.h:29: virtual void
OnFirstPacketReceived(cricket::MediaType media_type) = 0;
On 2016/06/08 17:35:55, pthatcher1 wrote:
> Should we just use the kind here instead of media_type?  That would be more
> consistent for the API and make the tests more simple.

The hangouts team want this interface. Since I have added media_type() to the
Audio/VideoRtpReceiver, I think this should be fine.

https://codereview.webrtc.org/1999853002/diff/40001/webrtc/api/webrtcsession.cc
File webrtc/api/webrtcsession.cc (right):

https://codereview.webrtc.org/1999853002/diff/40001/webrtc/api/webrtcsession....
webrtc/api/webrtcsession.cc:1894: has_received_audio_packet_ = true;
On 2016/06/08 17:35:55, pthatcher1 wrote:
> To be consistent with naming, might as well call it
> received_first_audio_packet_.

Done.

https://codereview.webrtc.org/1999853002/diff/40001/webrtc/pc/channel.h
File webrtc/pc/channel.h (right):

https://codereview.webrtc.org/1999853002/diff/40001/webrtc/pc/channel.h#newco...
webrtc/pc/channel.h:596: cricket::MediaType MediaType() override { return
cricket::MEDIA_TYPE_DATA; }
On 2016/06/08 17:35:56, pthatcher1 wrote:
> media_type()

Done.

https://codereview.webrtc.org/1999853002/diff/60001/webrtc/api/rtpreceiver.h
File webrtc/api/rtpreceiver.h (right):

https://codereview.webrtc.org/1999853002/diff/60001/webrtc/api/rtpreceiver.h#...
webrtc/api/rtpreceiver.h:84: RtpReceiverObserverInterface* observer_ = nullptr;
Should we use a unique_ptr here as SetObserver() could be called multiple times?

[pthatcher1, 2016-06-13 23:27:27]

lgtm

You can update the stylistic suggestion if you'd like, but it doesn't need
another round of review.

https://codereview.webrtc.org/1999853002/diff/60001/webrtc/api/peerconnection...
File webrtc/api/peerconnection_unittest.cc (right):

https://codereview.webrtc.org/1999853002/diff/60001/webrtc/api/peerconnection...
webrtc/api/peerconnection_unittest.cc:1357: void
CheckRtpReceiverObserverAreCalled(PeerConnectionTestClient* client) {
This should probably be:

EXPECT_TRUE_WAIT(std::all_of(client->rtp_receiver_observers(),
[](std::unique_ptr<MockRtpReceiverObserver> observer) { return
observer->first_packet_received(); }), kMaxWaitForFramesMs);

To avoid waiting many times.

(I'm not sure if that compiles, but you get the idea :).


But then, it might be cleaner to make the method thus:

bool AllObserversReceived(const
std::vector<std::unique_ptr<MockRtpReceiverObserver>>& observers) {
  return std::all_of(client->rtp_receiver_observers(),
[](std::unique_ptr<MockRtpReceiverObserver> observer) { return
observer->first_packet_received(); });
}

And then later:

EXPECT_TRUE_WAIT(AllObserversReceived(initializing_client()->rtp_receiver_observers()),
kMaxWaitForFramesMs);
EXPECT_TRUE_WAIT(AllObserversReceived(receiving_client()->rtp_receiver_observers()),
kMaxWaitForFramesMs);

https://codereview.webrtc.org/1999853002/diff/60001/webrtc/api/rtpreceiver.h
File webrtc/api/rtpreceiver.h (right):

https://codereview.webrtc.org/1999853002/diff/60001/webrtc/api/rtpreceiver.h#...
webrtc/api/rtpreceiver.h:84: RtpReceiverObserverInterface* observer_ = nullptr;
On 2016/06/09 00:37:36, Zhi Huang wrote:
> Should we use a unique_ptr here as SetObserver() could be called multiple
times?

Then the AudioRtpReceiver would take ownership of the observer, and SetObserver
should take a unique_ptr.  But does that make sense?  I can imagine observers
that one might want to pass in that aren't owned.

[Zhi Huang, 2016-06-14 17:49:00]

The CQ bit was checked by zhihuang@webrtc.org

[Zhi Huang, 2016-06-14 17:49:00]

The patchset sent to the CQ was uploaded after l-g-t-m from pthatcher@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1999853002/#ps80001 (title:
"Modified the unit test.")

[commit-bot: I haz the power, 2016-06-14 17:49:12]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1999853002/80001

[commit-bot: I haz the power, 2016-06-14 18:47:18]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-06-14 18:47:23]

Description was changed from

==========
Forward the SignalFirstPacketReceived to RtpReceiver.

The RtpReceiverObserverInterface is created.
The SignalFirstPacketReceived will be forwarded from BaseChannel to
WebRtcSession.
WebRtcSession will forward SignalFirstAudioPacketReceived and
SignalFirstVideoPacketReceived to the RtpReceiverInterface.
The application can listen to the Signal by implementing and registering a
RtpReceiverObserver.
==========

to

==========
Forward the SignalFirstPacketReceived to RtpReceiver.

The RtpReceiverObserverInterface is created.
The SignalFirstPacketReceived will be forwarded from BaseChannel to
WebRtcSession.
WebRtcSession will forward SignalFirstAudioPacketReceived and
SignalFirstVideoPacketReceived to the RtpReceiverInterface.
The application can listen to the Signal by implementing and registering a
RtpReceiverObserver.

Committed: https://crrev.com/184a3fd6489b2602a21c415cad3c51a7b035040b
Cr-Commit-Position: refs/heads/master@{#13139}
==========

[commit-bot: I haz the power, 2016-06-14 18:47:24]

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