Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2659563002:
Always call RemoteBitrateEstimator::IncomingPacket from Call. (Closed)
|
Created:
3 years, 11 months ago by nisse-webrtc Modified:
3 years, 10 months ago CC:
webrtc-reviews_webrtc.org, video-team_agora.io, yujie_mao (webrtc), zhengzhonghou_agora.io, stefan-webrtc, tlegrand-webrtc, tterriberry_mozilla.com, the sun, mflodman, philipel Target Ref:
refs/heads/master Project:
webrtc Visibility:
Public. |
DescriptionAlways 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
Patch Set 1 #Patch Set 2 : Update received_rtp_header_extensions_ also for audio and video receive streams. #Patch Set 3 : Delete unused code, and update AudioReceiveStream test. #Patch Set 4 : Take transport_cc into account. #
Total comments: 1
Patch Set 5 : Rebase. #Patch Set 6 : Updated comment and log message for half-configured send side BWE. #
Total comments: 13
Patch Set 7 : Smallish comment improvements. #Patch Set 8 : Added a comment for the rtx stream config. #Patch Set 9 : Rebased. #
Messages
Total messages: 28 (10 generated)
nisse@webrtc.org changed reviewers: + stefan@webrtc.org
Do you think this works? I suspect the RemoteBitrateEstimator callback is invoked with RTP header extensions unparsed. Looking at the code, I find one implementation which wants the abs send time extension. Otherwise, we'd need deeper parsing up-front in Call. If we can pull this off, we only need to move the RemoveStream calls to Call too, and then the receive streams no longer need to know anything about the RemoteBitrateEstimator or the CongestionController.
Seems to almost works now. But fails some EndToEnd tests related to the new jitter buffer. It's not obvious to me what those tests are doing and how they interact with my changes...
https://codereview.webrtc.org/2659563002/diff/60001/webrtc/call/call.cc File webrtc/call/call.cc (right): https://codereview.webrtc.org/2659563002/diff/60001/webrtc/call/call.cc#newco... webrtc/call/call.cc:1242: if (transport_cc != header.extension.hasTransportSequenceNumber) { I intended to use the check if (transport_cc && !header.extension.hasTransportSequenceNumber) but then tests fail. Additional logging shows that we get transport_cc = 0, hasTransportSequenceNumber = 1 for the TestWithNewVideoJitterBuffer/EndToEndTest.*TransportFeedbackNotConfigured/* tests. Can that reasonably happen? I though that transport_cc being false meant the RTP extension wasn't negotiated, hence shouldn't be included in RtpHeaderExtensionMap, and then the RTP parser should never set hasTransportSequenceNumber true, no matter what bits arrive on the wire. Am I missing something, or is there a bug somewhere?
nisse@webrtc.org changed reviewers: + brandtr@webrtc.org
Rasmus, can you have a look? I'm a bit puzzled by the transport_cc == false, hasTransportSequenceNumber == true case.
On 2017/01/30 08:41:31, nisse-webrtc wrote: > Rasmus, can you have a look? > > I'm a bit puzzled by the transport_cc == false, hasTransportSequenceNumber == > true case. That is a weird, but valid, scenario: transport_cc is set if the RTCP feedback message has been negotiated, whereas hasTransportSequenceNumber shows whether the parsed header has the RTP header extension. These are two different things, both of which must be negotiated for send-side BWE to function properly. AFAIK, the RemoteEstimatorProxy doesn't check that transport_cc is set, before sending the RTCP feedback message. So this should be checked in the CongestionController, as per our offline discussion last week. transport_cc is set here: https://cs.chromium.org/chromium/src/third_party/webrtc/media/engine/webrtcvi... extension is set here: https://cs.chromium.org/chromium/src/third_party/webrtc/media/engine/webrtcvi... To get a feeling for how these things are set, and how they differ in the SDP, have a look in webrtcvideoengine2.cc!
On 2017/01/30 08:53:27, brandtr_google wrote: > That is a weird, but valid, scenario: transport_cc is set if the RTCP feedback > message has been negotiated, whereas hasTransportSequenceNumber shows whether > the parsed header has the RTP header extension. These are two different things, > both of which must be negotiated for send-side BWE to function properly. I see. I've updated the comment and log message for this case. Do you think we need any more sophisticated fallback logic for this odd case, than simply not calling back into the CongestionController, and hence not doing any bandwidth estimation at all? > AFAIK, > the RemoteEstimatorProxy doesn't check that transport_cc is set, before sending > the RTCP feedback message. So this should be checked in the > CongestionController, as per our offline discussion last week. I'm leaving this check in Call instead. I think it's better if the CongstionController is blissfully ignorant of this complication...
On 2017/01/30 09:32:54, nisse-webrtc wrote: > On 2017/01/30 08:53:27, brandtr_google wrote: > > That is a weird, but valid, scenario: transport_cc is set if the RTCP feedback > > message has been negotiated, whereas hasTransportSequenceNumber shows whether > > the parsed header has the RTP header extension. These are two different > things, > > both of which must be negotiated for send-side BWE to function properly. > > I see. I've updated the comment and log message for this case. Do you think we > need any more sophisticated fallback logic for this odd case, than simply not > calling back into the CongestionController, and hence not doing any bandwidth > estimation at all? Don't think so, because my understanding is that we should either 1) use send-side BWE, or 2) use abs-send-time and REMB. So weird combinations of these are are not interesting. I think Stefan has more informed opinions about this, however. > > AFAIK, > > the RemoteEstimatorProxy doesn't check that transport_cc is set, before > sending > > the RTCP feedback message. So this should be checked in the > > CongestionController, as per our offline discussion last week. > > I'm leaving this check in Call instead. I think it's better if the > CongstionController is blissfully ignorant of this complication... Sounds good.
Looks good. Left some comments. https://codereview.webrtc.org/2659563002/diff/100001/webrtc/audio/audio_recei... File webrtc/audio/audio_receive_stream.cc (right): https://codereview.webrtc.org/2659563002/diff/100001/webrtc/audio/audio_recei... webrtc/audio/audio_receive_stream.cc:73: : remote_bitrate_estimator_(remote_bitrate_estimator), Can |remote_bitrate_estimator_| be completely removed from this class? https://codereview.webrtc.org/2659563002/diff/100001/webrtc/call/call.cc File webrtc/call/call.cc (right): https://codereview.webrtc.org/2659563002/diff/100001/webrtc/call/call.cc#newc... webrtc/call/call.cc:212: : extensions(extensions), transport_cc(transport_cc) {} Should we store a bool for REMB here too? The mechanism for the feedback is different, but |transport_cc| and |remb| in VideoReceiveStream::Config::Rtp are conceptually similar. https://codereview.webrtc.org/2659563002/diff/100001/webrtc/call/call.cc#newc... webrtc/call/call.cc:219: // True if send side bandwith estimation was negotiated. Mention that |transport_cc| only has to do with the RTCP feedback message. I.e., only one part of negotiating send-side BWE. https://codereview.webrtc.org/2659563002/diff/100001/webrtc/call/call.cc#newc... webrtc/call/call.cc:538: // TODO(nisse): Used only when UseSendSideBwe(config) is true. Could this TODO be removed now? https://codereview.webrtc.org/2659563002/diff/100001/webrtc/call/call.cc#newc... webrtc/call/call.cc:685: receive_rtp_config_[config.rtp.rtx_ssrc] = receive_config; config.rtp.transport_cc describes if the RTCP feedback message has been negotiated for the video payload type. When storing the same ReceiveRtpConfig for the RTX payload type here, that information may therefore be incorrect. Not sure that it matters in practice, but could warrant a comment here.
https://codereview.webrtc.org/2659563002/diff/100001/webrtc/audio/audio_recei... File webrtc/audio/audio_receive_stream.cc (right): https://codereview.webrtc.org/2659563002/diff/100001/webrtc/audio/audio_recei... webrtc/audio/audio_receive_stream.cc:73: : remote_bitrate_estimator_(remote_bitrate_estimator), On 2017/01/30 12:03:53, brandtr wrote: > Can |remote_bitrate_estimator_| be completely removed from this class? I plan that for the next cl. There's a call to RemoveStream which also needs to be moved over to Call. https://codereview.webrtc.org/2659563002/diff/100001/webrtc/call/call.cc File webrtc/call/call.cc (right): https://codereview.webrtc.org/2659563002/diff/100001/webrtc/call/call.cc#newc... webrtc/call/call.cc:212: : extensions(extensions), transport_cc(transport_cc) {} On 2017/01/30 12:03:53, brandtr wrote: > Should we store a bool for REMB here too? The mechanism for the feedback is > different, but |transport_cc| and |remb| in VideoReceiveStream::Config::Rtp are > conceptually similar. Maybe. Are you suggesting I do something about it in this cl? Then I'd need a little more guidance. The intention is that this struct and the below mapping should hold all information needed for the media-type-independent processing of received packets. https://codereview.webrtc.org/2659563002/diff/100001/webrtc/call/call.cc#newc... webrtc/call/call.cc:219: // True if send side bandwith estimation was negotiated. On 2017/01/30 12:03:53, brandtr wrote: > Mention that |transport_cc| only has to do with the RTCP feedback message. I.e., > only one part of negotiating send-side BWE. Done. https://codereview.webrtc.org/2659563002/diff/100001/webrtc/call/call.cc#newc... webrtc/call/call.cc:538: // TODO(nisse): Used only when UseSendSideBwe(config) is true. On 2017/01/30 12:03:53, brandtr wrote: > Could this TODO be removed now? Done. https://codereview.webrtc.org/2659563002/diff/100001/webrtc/call/call.cc#newc... webrtc/call/call.cc:685: receive_rtp_config_[config.rtp.rtx_ssrc] = receive_config; On 2017/01/30 12:03:53, brandtr wrote: > config.rtp.transport_cc describes if the RTCP feedback message has been > negotiated for the video payload type. When storing the same ReceiveRtpConfig > for the RTX payload type here, that information may therefore be incorrect. > > Not sure that it matters in practice, but could warrant a comment here. If it's more correct to set transport_cc always false in the entry for the rtx ssrc, that should be a fairly easy change.
ping?
Nice! lgtm
The CQ bit was checked by nisse@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
https://codereview.webrtc.org/2659563002/diff/100001/webrtc/call/call.cc File webrtc/call/call.cc (right): https://codereview.webrtc.org/2659563002/diff/100001/webrtc/call/call.cc#newc... webrtc/call/call.cc:685: receive_rtp_config_[config.rtp.rtx_ssrc] = receive_config; On 2017/01/30 12:03:53, brandtr wrote: > config.rtp.transport_cc describes if the RTCP feedback message has been > negotiated for the video payload type. When storing the same ReceiveRtpConfig > for the RTX payload type here, that information may therefore be incorrect. > > Not sure that it matters in practice, but could warrant a comment here. Added a comment now.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios64_sim_ios10_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_ios10_dbg/bui...) ios64_sim_ios9_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_ios9_dbg/buil...) ios_arm64_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_dbg/builds/17000) ios_arm64_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_rel/builds/16833) ios_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_dbg/builds/22263)
lgtm :) https://codereview.webrtc.org/2659563002/diff/100001/webrtc/call/call.cc File webrtc/call/call.cc (right): https://codereview.webrtc.org/2659563002/diff/100001/webrtc/call/call.cc#newc... webrtc/call/call.cc:212: : extensions(extensions), transport_cc(transport_cc) {} On 2017/01/30 14:56:57, nisse-webrtc wrote: > On 2017/01/30 12:03:53, brandtr wrote: > > Should we store a bool for REMB here too? The mechanism for the feedback is > > different, but |transport_cc| and |remb| in VideoReceiveStream::Config::Rtp > are > > conceptually similar. > > Maybe. Are you suggesting I do something about it in this cl? Then I'd need a > little more guidance. > > The intention is that this struct and the below mapping should hold all > information needed for the media-type-independent processing of received > packets. Actually, there is probably no direct value in keeping the REMB status here. Currently, it is set on the RTP module here: https://cs.chromium.org/chromium/src/third_party/webrtc/video/rtp_stream_rece..., and that won't change with this CL. https://codereview.webrtc.org/2659563002/diff/100001/webrtc/call/call.cc#newc... webrtc/call/call.cc:685: receive_rtp_config_[config.rtp.rtx_ssrc] = receive_config; On 2017/01/30 14:56:57, nisse-webrtc wrote: > On 2017/01/30 12:03:53, brandtr wrote: > > config.rtp.transport_cc describes if the RTCP feedback message has been > > negotiated for the video payload type. When storing the same ReceiveRtpConfig > > for the RTX payload type here, that information may therefore be incorrect. > > > > Not sure that it matters in practice, but could warrant a comment here. > > If it's more correct to set transport_cc always false in the entry for the rtx > ssrc, that should be a fairly easy change. I think neither is more correct, so it's probably better to keep the current behaviour: that transport_cc for the RTX payloads are set based on transport_cc for the video payload. Then received RTX packets will not be thrown away, from a BWE perspective.
The CQ bit was checked by nisse@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from stefan@webrtc.org, brandtr@webrtc.org Link to the patchset: https://codereview.webrtc.org/2659563002/#ps160001 (title: "Rebased.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
CQ is committing da patch.
Bot data: {"patchset_id": 160001, "attempt_start_ts": 1485943938577800,
"parent_rev": "803dc29bb6773b60dadf5b630ef6a00b8e72f77f", "commit_rev":
"6d4dd593a824cfc9c67f6e9fd3a08014d08350a0"}
Message was sent while issue was closed.
Description was changed from ========== Always call RemoteBitrateEstimator::IncomingPacket from Call. Delete the calls from RtpStreamReceiver (for video) and AudioReceiveStream. BUG=webrtc:6847 ========== to ========== 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... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001) as https://chromium.googlesource.com/external/webrtc/+/6d4dd593a824cfc9c67f6e9fd...
Message was sent while issue was closed.
A revert of this CL (patchset #9 id:160001) has been created in https://codereview.webrtc.org/2668973003/ by nisse@webrtc.org. The reason for reverting is: 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... .
Message was sent while issue was closed.
On 2017/02/01 16:10:22, nisse-webrtc wrote: > A revert of this CL (patchset #9 id:160001) has been created in > https://codereview.webrtc.org/2668973003/ by mailto:nisse@webrtc.org. > > The reason for reverting is: 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... > . I chatted with sheriff about this, and have the following theory: The excessive DCHECK failures in video_quality_test.cc could be due to this (reverted) CL: https://codereview.webrtc.org/2630333002, because they seem to have stopped happening after the revert. After there revert, there were RampUpTest failures however: https://uberchromegw.corp.google.com/i/client.webrtc.perf/builders/Android32%..., which could conceivably be caused by some change in how absSendTime headers are treated, after this CL.
Message was sent while issue was closed.
A revert of this CL (patchset #9 id:160001) has been created in https://codereview.webrtc.org/2667163002/ by guidou@webrtc.org. The reason for reverting is: Suspect of breaking FYI bots. See https://build.chromium.org/p/chromium.webrtc.fyi/builders/Win7%20Tester/build... https://build.chromium.org/p/chromium.webrtc.fyi/builders/Linux%20Tester/buil... https://build.chromium.org/p/chromium.webrtc.fyi/builders/Win10%20Tester. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
