Reland of Always call RemoteBitrateEstimator::IncomingPacket from Call.

Reland of Always call RemoteBitrateEstimator::IncomingPacket from Call. (patchset #1 id:1 of https://codereview.webrtc.org/2668973003/ )

Reason for revert:
Intending to fix issues and reland.

Original issue's description:
> Revert of Always call RemoteBitrateEstimator::IncomingPacket from Call. (patchset #9 id:160001 of https://codereview.webrtc.org/2659563002/ )
> 
> Reason for revert:
> This change causes excessive logging when running tests, and possibly also broke perf tests, see https://build.chromium.org/p/client.webrtc.perf/builders/Linux%20Trusty/builds/1040/steps/webrtc_perf_tests/logs/stdio
> 
> 
> Original issue's description:
> > Always call RemoteBitrateEstimator::IncomingPacket from Call.
> > 
> > Delete the calls from RtpStreamReceiver (for video) and
> > AudioReceiveStream.
> > 
> > BUG=webrtc:6847
> > 
> > Review-Url: https://codereview.webrtc.org/2659563002
> > Cr-Commit-Position: refs/heads/master@{#16393}
> > Committed: https://chromium.googlesource.com/external/webrtc/+/6d4dd593a824cfc9c67f6e9fd3a08014d08350a0
> 
> TBR=stefan@webrtc.org,brandtr@webrtc.org
> # Skipping CQ checks because original CL landed less than 1 days ago.
> NOPRESUBMIT=true
> NOTREECHECKS=true
> NOTRY=true
> BUG=webrtc:6847
> 
> Review-Url: https://codereview.webrtc.org/2668973003
> Cr-Commit-Position: refs/heads/master@{#16400}
> Committed: https://chromium.googlesource.com/external/webrtc/+/14245cc9397284cabab5057c0c661953e2d0cdec

TBR=stefan@webrtc.org,brandtr@webrtc.org
BUG=webrtc:6847
Review-Url: https://codereview.webrtc.org/2673523003
Cr-Commit-Position: refs/heads/master@{#16440}
Committed: https://chromium.googlesource.com/external/webrtc/+/d44ce0563f39b5357e050a57cbdf91e34d45bdaa

[nisse-webrtc, 2017-02-02 08:19:55]

Created Reland of Always call RemoteBitrateEstimator::IncomingPacket from Call.

[nisse-webrtc, 2017-02-02 09:53:39]

There are some threading issues I'd need a little help to sort out.

https://codereview.webrtc.org/2673523003/diff/1/webrtc/call/call.cc
File webrtc/call/call.cc (right):

https://codereview.webrtc.org/2673523003/diff/1/webrtc/call/call.cc#newcode1249
webrtc/call/call.cc:1249:
congestion_controller_->OnReceivedPacket(packet.arrival_time_ms(),
webrtc_perf_tests crashes in the first test
(CallPerfTest.PlaysOutAudioAndVideoInSyncWithVideoNtpDrift), because this call
is directed to RemoteBitrateEstimatorAbsSendTime::IncomingPacket, where the
DCHECK

  RTC_DCHECK(network_thread_.CalledOnValidThread());

fails.

[brandtr, 2017-02-02 10:14:17]

On 2017/02/02 09:53:39, nisse-webrtc wrote:
> There are some threading issues I'd need a little help to sort out.
> 
> https://codereview.webrtc.org/2673523003/diff/1/webrtc/call/call.cc
> File webrtc/call/call.cc (right):
> 
>
https://codereview.webrtc.org/2673523003/diff/1/webrtc/call/call.cc#newcode1249
> webrtc/call/call.cc:1249:
> congestion_controller_->OnReceivedPacket(packet.arrival_time_ms(),
> webrtc_perf_tests crashes in the first test
> (CallPerfTest.PlaysOutAudioAndVideoInSyncWithVideoNtpDrift), because this call
> is directed to RemoteBitrateEstimatorAbsSendTime::IncomingPacket, where the
> DCHECK
> 
>   RTC_DCHECK(network_thread_.CalledOnValidThread());
> 
> fails.

Do you know what thread it fails on? It seems to me that |network_thread_| is
bound to the libjingle worker thread in production code (*), and the test
transport thread in test code.

(*) It is my naive understanding that all objects at the Call level are created
by the worker thread, and that packets also are delivered on the worker thread.
I base this on the fact that all the call hierarchies that I have seen originate
in libjingle member functions ending in "_w".

[nisse-webrtc, 2017-02-02 10:19:34]

On 2017/02/02 10:14:17, brandtr wrote:
> On 2017/02/02 09:53:39, nisse-webrtc wrote:
> > There are some threading issues I'd need a little help to sort out.
> > 
> > https://codereview.webrtc.org/2673523003/diff/1/webrtc/call/call.cc
> > File webrtc/call/call.cc (right):
> > 
> >
>
https://codereview.webrtc.org/2673523003/diff/1/webrtc/call/call.cc#newcode1249
> > webrtc/call/call.cc:1249:
> > congestion_controller_->OnReceivedPacket(packet.arrival_time_ms(),
> > webrtc_perf_tests crashes in the first test
> > (CallPerfTest.PlaysOutAudioAndVideoInSyncWithVideoNtpDrift), because this
call
> > is directed to RemoteBitrateEstimatorAbsSendTime::IncomingPacket, where the
> > DCHECK
> > 
> >   RTC_DCHECK(network_thread_.CalledOnValidThread());
> > 
> > fails.
> 
> Do you know what thread it fails on? It seems to me that |network_thread_| is
> bound to the libjingle worker thread in production code (*), and the test
> transport thread in test code.
> 
> (*) It is my naive understanding that all objects at the Call level are
created
> by the worker thread, and that packets also are delivered on the worker
thread.
> I base this on the fact that all the call hierarchies that I have seen
originate
> in libjingle member functions ending in "_w".

For some reason, I can't access thread names in gdb (it doesn't recognize
rtc::Thread at all).
I'll try some printf debugging to find out.

I wonder if the method wasn't called at all by the test before this change, or
if it was called,
where the thread jump occurred.

[nisse-webrtc, 2017-02-02 11:48:55]

On 2017/02/02 10:19:34, nisse-webrtc wrote:
> On 2017/02/02 10:14:17, brandtr wrote:
> > On 2017/02/02 09:53:39, nisse-webrtc wrote:
> > > There are some threading issues I'd need a little help to sort out.
> > > 
> > > https://codereview.webrtc.org/2673523003/diff/1/webrtc/call/call.cc
> > > File webrtc/call/call.cc (right):
> > > 
> > >
> >
>
https://codereview.webrtc.org/2673523003/diff/1/webrtc/call/call.cc#newcode1249
> > > webrtc/call/call.cc:1249:
> > > congestion_controller_->OnReceivedPacket(packet.arrival_time_ms(),
> > > webrtc_perf_tests crashes in the first test
> > > (CallPerfTest.PlaysOutAudioAndVideoInSyncWithVideoNtpDrift), because this
> call
> > > is directed to RemoteBitrateEstimatorAbsSendTime::IncomingPacket, where
the
> > > DCHECK
> > > 
> > >   RTC_DCHECK(network_thread_.CalledOnValidThread());
> > > 
> > > fails.
> > 
> > Do you know what thread it fails on? It seems to me that |network_thread_|
is
> > bound to the libjingle worker thread in production code (*), and the test
> > transport thread in test code.
> > 
> > (*) It is my naive understanding that all objects at the Call level are
> created
> > by the worker thread, and that packets also are delivered on the worker
> thread.
> > I base this on the fact that all the call hierarchies that I have seen
> originate
> > in libjingle member functions ending in "_w".
> 
> For some reason, I can't access thread names in gdb (it doesn't recognize
> rtc::Thread at all).
> I'll try some printf debugging to find out.
> 
> I wonder if the method wasn't called at all by the test before this change, or
> if it was called,
> where the thread jump occurred.

Digging a bit more, it seems that CallPerfTest generates salls to
Call::DeliverRtp from separate threads for audio and video. Threads are
PlatformThread. For one of the threads (I think that's the one for video), we
rtc::Thread::Current()->name() "Thread 0x0x7fffe00012", while, for some unknown
reason, rtc::Thread::Current() returns NULL for the other thread. I don't know
if ThreadChecker is expected to be compatible with PlatformThread?

Two separate threads obviously breaks the threading assumptions of
RemoteBitrateEstimatorAbsSendTime. I guess the reason it works prior to the cl
is that the RemoteBitrateEstimatorAbsSend was never called for audio; the audio
code only did send side bandwidth estimation, using the RemoteEstimatorProxy.

Not sure what to do about this, try to exclude audio from the receive side bwe,
or add a thread jump somewhere to get all calls on the same thread?

[nisse-webrtc, 2017-02-03 09:05:22]

The CQ bit was checked by nisse@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-03 09:05:33]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-02-03 09:05:35]

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

[commit-bot: I haz the power, 2017-02-03 09:05:36]

Dry run: This issue passed the CQ dry run.

[nisse-webrtc, 2017-02-03 09:35:23]

Description was changed from

==========
Reland of Always call RemoteBitrateEstimator::IncomingPacket from Call.
(patchset #1 id:1 of https://codereview.webrtc.org/2668973003/ )

Reason for revert:
Intending to fix issues and reland.

Original issue's description:
> Revert of Always call RemoteBitrateEstimator::IncomingPacket from Call.
(patchset #9 id:160001 of https://codereview.webrtc.org/2659563002/ )
> 
> Reason for revert:
> This change causes excessive logging when running tests, and possibly also
broke perf tests, see
https://build.chromium.org/p/client.webrtc.perf/builders/Linux%20Trusty/build...
> 
> 
> Original issue's description:
> > Always call RemoteBitrateEstimator::IncomingPacket from Call.
> > 
> > Delete the calls from RtpStreamReceiver (for video) and
> > AudioReceiveStream.
> > 
> > BUG=webrtc:6847
> > 
> > Review-Url: https://codereview.webrtc.org/2659563002
> > Cr-Commit-Position: refs/heads/master@{#16393}
> > Committed:
https://chromium.googlesource.com/external/webrtc/+/6d4dd593a824cfc9c67f6e9fd...
> 
> TBR=stefan@webrtc.org,brandtr@webrtc.org
> # Skipping CQ checks because original CL landed less than 1 days ago.
> NOPRESUBMIT=true
> NOTREECHECKS=true
> NOTRY=true
> BUG=webrtc:6847
> 
> Review-Url: https://codereview.webrtc.org/2668973003
> Cr-Commit-Position: refs/heads/master@{#16400}
> Committed:
https://chromium.googlesource.com/external/webrtc/+/14245cc9397284cabab5057c0...

TBR=stefan@webrtc.org,brandtr@webrtc.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=webrtc:6847
==========

to

==========
Reland of Always call RemoteBitrateEstimator::IncomingPacket from Call.
(patchset #1 id:1 of https://codereview.webrtc.org/2668973003/ )

Reason for revert:
Intending to fix issues and reland.

Original issue's description:
> Revert of Always call RemoteBitrateEstimator::IncomingPacket from Call.
(patchset #9 id:160001 of https://codereview.webrtc.org/2659563002/ )
> 
> Reason for revert:
> This change causes excessive logging when running tests, and possibly also
broke perf tests, see
https://build.chromium.org/p/client.webrtc.perf/builders/Linux%20Trusty/build...
> 
> 
> Original issue's description:
> > Always call RemoteBitrateEstimator::IncomingPacket from Call.
> > 
> > Delete the calls from RtpStreamReceiver (for video) and
> > AudioReceiveStream.
> > 
> > BUG=webrtc:6847
> > 
> > Review-Url: https://codereview.webrtc.org/2659563002
> > Cr-Commit-Position: refs/heads/master@{#16393}
> > Committed:
https://chromium.googlesource.com/external/webrtc/+/6d4dd593a824cfc9c67f6e9fd...
> 
> TBR=stefan@webrtc.org,brandtr@webrtc.org
> # Skipping CQ checks because original CL landed less than 1 days ago.
> NOPRESUBMIT=true
> NOTREECHECKS=true
> NOTRY=true
> BUG=webrtc:6847
> 
> Review-Url: https://codereview.webrtc.org/2668973003
> Cr-Commit-Position: refs/heads/master@{#16400}
> Committed:
https://chromium.googlesource.com/external/webrtc/+/14245cc9397284cabab5057c0...

TBR=stefan@webrtc.org,brandtr@webrtc.org
BUG=webrtc:6847
==========

[nisse-webrtc, 2017-02-03 09:35:50]

The CQ bit was checked by nisse@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-03 09:35:57]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[nisse-webrtc, 2017-02-03 09:39:13]

I hope this is now ready for reland. I'll also give webrtc_perf_tests a run
locally.

[brandtr, 2017-02-03 09:50:30]

lgtm with {}-nits.

https://codereview.webrtc.org/2673523003/diff/180001/webrtc/call/call.cc
File webrtc/call/call.cc (right):

https://codereview.webrtc.org/2673523003/diff/180001/webrtc/call/call.cc#newc...
webrtc/call/call.cc:1243: if (!transport_cc &&
header.extension.hasTransportSequenceNumber)
Add {}.

https://codereview.webrtc.org/2673523003/diff/180001/webrtc/call/call.cc#newc...
webrtc/call/call.cc:1258: (transport_cc &&
header.extension.hasTransportSequenceNumber))
{}

[nisse-webrtc, 2017-02-03 10:19:47]

Following perf tests failed:

FAILED TESTS (5/98):
 ./out/Debug/webrtc_perf_tests:
FullStackTest.ForemanCif30kbpsWithoutPacketlossH264
 ./out/Debug/webrtc_perf_tests: FullStackTest.ForemanCifWithoutPacketlossH264
 ./out/Debug/webrtc_perf_tests: FullStackTest.ForemanCifPlr5H264
 ./out/Debug/webrtc_perf_tests: FullStackTest.ForemanCifPlr5H264Flexfec
 ./out/Debug/webrtc_perf_tests: RampUpTest.UpDownUpAbsSendTimeSimulcastRedRtx

Björn: Se also followup clhttps://codereview.webrtc.org/2669463006/

https://codereview.webrtc.org/2673523003/diff/180001/webrtc/call/call.cc
File webrtc/call/call.cc (right):

https://codereview.webrtc.org/2673523003/diff/180001/webrtc/call/call.cc#newc...
webrtc/call/call.cc:1243: if (!transport_cc &&
header.extension.hasTransportSequenceNumber)
On 2017/02/03 09:50:29, brandtr wrote:
> Add {}.

Done.

https://codereview.webrtc.org/2673523003/diff/180001/webrtc/call/call.cc#newc...
webrtc/call/call.cc:1258: (transport_cc &&
header.extension.hasTransportSequenceNumber))
On 2017/02/03 09:50:29, brandtr wrote:
> {}

Done.

[nisse-webrtc, 2017-02-06 08:16:59]

On 2017/02/03 10:19:47, nisse-webrtc wrote:
> Following perf tests failed:
> 
> FAILED TESTS (5/98):
>  ./out/Debug/webrtc_perf_tests:
> FullStackTest.ForemanCif30kbpsWithoutPacketlossH264
>  ./out/Debug/webrtc_perf_tests: FullStackTest.ForemanCifWithoutPacketlossH264
>  ./out/Debug/webrtc_perf_tests: FullStackTest.ForemanCifPlr5H264
>  ./out/Debug/webrtc_perf_tests: FullStackTest.ForemanCifPlr5H264Flexfec
>  ./out/Debug/webrtc_perf_tests: RampUpTest.UpDownUpAbsSendTimeSimulcastRedRtx

Rampup test fixed with patchset #7. The other tests fail on master too, when I
run them locally, so unlikely to be caused by this cl.

[stefan-webrtc, 2017-02-06 08:39:45]

lgtm

[nisse-webrtc, 2017-02-06 09:27:35]

The CQ bit was checked by nisse@webrtc.org

[nisse-webrtc, 2017-02-06 09:27:35]

The patchset sent to the CQ was uploaded after l-g-t-m from brandtr@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2673523003/#ps220001 (title:
"Include padding size in bwe input.")

[commit-bot: I haz the power, 2017-02-06 09:27:39]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-02-06 10:23:01]

CQ is committing da patch.

Bot data: {"patchset_id": 220001, "attempt_start_ts": 1486373255418370,
"parent_rev": "656610fbe7a1339f5576cafb8564b5d7609c6be9", "commit_rev":
"d44ce0563f39b5357e050a57cbdf91e34d45bdaa"}

[commit-bot: I haz the power, 2017-02-06 10:23:04]

Description was changed from

==========
Reland of Always call RemoteBitrateEstimator::IncomingPacket from Call.
(patchset #1 id:1 of https://codereview.webrtc.org/2668973003/ )

Reason for revert:
Intending to fix issues and reland.

Original issue's description:
> Revert of Always call RemoteBitrateEstimator::IncomingPacket from Call.
(patchset #9 id:160001 of https://codereview.webrtc.org/2659563002/ )
> 
> Reason for revert:
> This change causes excessive logging when running tests, and possibly also
broke perf tests, see
https://build.chromium.org/p/client.webrtc.perf/builders/Linux%20Trusty/build...
> 
> 
> Original issue's description:
> > Always call RemoteBitrateEstimator::IncomingPacket from Call.
> > 
> > Delete the calls from RtpStreamReceiver (for video) and
> > AudioReceiveStream.
> > 
> > BUG=webrtc:6847
> > 
> > Review-Url: https://codereview.webrtc.org/2659563002
> > Cr-Commit-Position: refs/heads/master@{#16393}
> > Committed:
https://chromium.googlesource.com/external/webrtc/+/6d4dd593a824cfc9c67f6e9fd...
> 
> TBR=stefan@webrtc.org,brandtr@webrtc.org
> # Skipping CQ checks because original CL landed less than 1 days ago.
> NOPRESUBMIT=true
> NOTREECHECKS=true
> NOTRY=true
> BUG=webrtc:6847
> 
> Review-Url: https://codereview.webrtc.org/2668973003
> Cr-Commit-Position: refs/heads/master@{#16400}
> Committed:
https://chromium.googlesource.com/external/webrtc/+/14245cc9397284cabab5057c0...

TBR=stefan@webrtc.org,brandtr@webrtc.org
BUG=webrtc:6847
==========

to

==========
Reland of Always call RemoteBitrateEstimator::IncomingPacket from Call.
(patchset #1 id:1 of https://codereview.webrtc.org/2668973003/ )

Reason for revert:
Intending to fix issues and reland.

Original issue's description:
> Revert of Always call RemoteBitrateEstimator::IncomingPacket from Call.
(patchset #9 id:160001 of https://codereview.webrtc.org/2659563002/ )
> 
> Reason for revert:
> This change causes excessive logging when running tests, and possibly also
broke perf tests, see
https://build.chromium.org/p/client.webrtc.perf/builders/Linux%20Trusty/build...
> 
> 
> Original issue's description:
> > Always call RemoteBitrateEstimator::IncomingPacket from Call.
> > 
> > Delete the calls from RtpStreamReceiver (for video) and
> > AudioReceiveStream.
> > 
> > BUG=webrtc:6847
> > 
> > Review-Url: https://codereview.webrtc.org/2659563002
> > Cr-Commit-Position: refs/heads/master@{#16393}
> > Committed:
https://chromium.googlesource.com/external/webrtc/+/6d4dd593a824cfc9c67f6e9fd...
> 
> TBR=stefan@webrtc.org,brandtr@webrtc.org
> # Skipping CQ checks because original CL landed less than 1 days ago.
> NOPRESUBMIT=true
> NOTREECHECKS=true
> NOTRY=true
> BUG=webrtc:6847
> 
> Review-Url: https://codereview.webrtc.org/2668973003
> Cr-Commit-Position: refs/heads/master@{#16400}
> Committed:
https://chromium.googlesource.com/external/webrtc/+/14245cc9397284cabab5057c0...

TBR=stefan@webrtc.org,brandtr@webrtc.org
BUG=webrtc:6847

Review-Url: https://codereview.webrtc.org/2673523003
Cr-Commit-Position: refs/heads/master@{#16440}
Committed:
https://chromium.googlesource.com/external/webrtc/+/d44ce0563f39b5357e050a57c...
==========

[commit-bot: I haz the power, 2017-02-06 10:23:05]

Committed patchset #7 (id:220001) as
https://chromium.googlesource.com/external/webrtc/+/d44ce0563f39b5357e050a57c...