Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2534173002:
Wire up x-google-{min,start,max}-bitrate to WebRtcVoiceMediaChannel. (Closed)
|
Created:
4 years ago by stefan-webrtc Modified:
3 years, 8 months ago CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionWire up x-google-{min,start,max}-bitrate to WebRtcVoiceMediaChannel.
BUG=webrtc:6793
Committed: https://crrev.com/13f1a0a9cab1aaa872b37d3a3aa3e894659c71c2
Cr-Commit-Position: refs/heads/master@{#15337}
Patch Set 1 #Patch Set 2 : . #Patch Set 3 : Clean up comment. #
Total comments: 12
Patch Set 4 : Most comments addressed. #Patch Set 5 : Comment addressed. #Patch Set 6 : . #
Total comments: 4
Messages
Total messages: 29 (9 generated)
Clean up comment.
stefan@webrtc.org changed reviewers: + minyue@webrtc.org, solenberg@webrtc.org
An attempt to wire this up the same way it is used in webrtcvideoengine2.cc. Please take a look.
please consider my comment inline. can discuss offline also https://codereview.webrtc.org/2534173002/diff/40001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/2534173002/diff/40001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine.cc:1647: // bitrate config for the codec. This allows FEC to be applied above the Not sure about the reasoning on FEC in the comment. https://codereview.webrtc.org/2534173002/diff/40001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine.cc:1972: if (send_codec_spec_set_ && send_codec_spec == send_codec_spec_) { I don't think this is the place we reset codec. please consider line 1255
https://codereview.webrtc.org/2534173002/diff/40001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/2534173002/diff/40001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine.cc:283: int bitrate_kbps; nit: always default init https://codereview.webrtc.org/2534173002/diff/40001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine.cc:295: config.start_bitrate_bps = -1; Change the optional fields in the config to use rtc::Optional https://codereview.webrtc.org/2534173002/diff/40001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine.cc:1645: if (params.max_bandwidth_bps >= 0) { Is this logic the same for video? Does it make sense to make a util? https://codereview.webrtc.org/2534173002/diff/40001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine.cc:1972: if (send_codec_spec_set_ && send_codec_spec == send_codec_spec_) { On 2016/11/29 22:10:55, minyue-webrtc wrote: > I don't think this is the place we reset codec. please consider line 1255 I don't get the logic involving send_codec_spec_set_. IIUC it is there so that we never reset bw when the same codec is set multiple times. Can it be simplified by always clearing bitrate_config_.start_bitrate_bps after pushing the config to call_ (as well as clearing it here - or preferably at line 2039-ish - when codec specs are identical)?
Comment addressed.
.
https://codereview.webrtc.org/2534173002/diff/40001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/2534173002/diff/40001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine.cc:283: int bitrate_kbps; On 2016/11/30 11:22:27, the sun wrote: > nit: always default init Done. https://codereview.webrtc.org/2534173002/diff/40001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine.cc:295: config.start_bitrate_bps = -1; On 2016/11/30 11:22:27, the sun wrote: > Change the optional fields in the config to use rtc::Optional I'd prefer making interface changes later. https://codereview.webrtc.org/2534173002/diff/40001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine.cc:1645: if (params.max_bandwidth_bps >= 0) { On 2016/11/30 11:22:27, the sun wrote: > Is this logic the same for video? Does it make sense to make a util? It is the same, not sure about making it a util though... There are many small things in these files that are similar for video and audio. I made the GetBitrateConfigForCodec() a util though, that seems more natural. https://codereview.webrtc.org/2534173002/diff/40001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine.cc:1647: // bitrate config for the codec. This allows FEC to be applied above the On 2016/11/29 22:10:55, minyue-webrtc wrote: > Not sure about the reasoning on FEC in the comment. I made it a bit more generic. https://codereview.webrtc.org/2534173002/diff/40001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine.cc:1972: if (send_codec_spec_set_ && send_codec_spec == send_codec_spec_) { On 2016/11/30 11:22:27, the sun wrote: > On 2016/11/29 22:10:55, minyue-webrtc wrote: > > I don't think this is the place we reset codec. please consider line 1255 > > I don't get the logic involving send_codec_spec_set_. IIUC it is there so that > we never reset bw when the same codec is set multiple times. Can it be > simplified by always clearing bitrate_config_.start_bitrate_bps after pushing > the config to call_ (as well as clearing it here - or preferably at line > 2039-ish - when codec specs are identical)? > I don't think the send_codec_spec_set_ check is needed, removed it. That I think makes it more like you suggested. Also moved it down. https://codereview.webrtc.org/2534173002/diff/40001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine.cc:1972: if (send_codec_spec_set_ && send_codec_spec == send_codec_spec_) { On 2016/11/29 22:10:55, minyue-webrtc wrote: > I don't think this is the place we reset codec. please consider line 1255 I tried to the changes into WebRtcAudioSendStream instead, but it prevents us from using cricket::AudioCodec to determine the BitrateConfig, which makes it difficult to reuse the same function for doing that for both audio and video. Since there's no clear benefit of having it in WebRtcAudioSendStream I would prefer this way.
The CQ bit was checked by stefan@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/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm, but consider my comment. https://codereview.webrtc.org/2534173002/diff/100001/webrtc/media/engine/webr... File webrtc/media/engine/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/2534173002/diff/100001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvoiceengine.cc:2009: } If you move call_->SetBitrateConfig() here instead, do you even need to store the config in WVoMC?
https://codereview.webrtc.org/2534173002/diff/100001/webrtc/media/engine/webr... File webrtc/media/engine/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/2534173002/diff/100001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvoiceengine.cc:2009: } On 2016/11/30 13:55:51, the sun wrote: > If you move call_->SetBitrateConfig() here instead, do you even need to store > the config in WVoMC? It isn't that simple, since params.max_bandwidth_bps should override this, see line 1622. Maybe if I have SetSendCodec() return a bitrate config that I can use there? WDYT?
On 2016/11/30 14:15:30, stefan-webrtc (holmer) wrote: > https://codereview.webrtc.org/2534173002/diff/100001/webrtc/media/engine/webr... > File webrtc/media/engine/webrtcvoiceengine.cc (right): > > https://codereview.webrtc.org/2534173002/diff/100001/webrtc/media/engine/webr... > webrtc/media/engine/webrtcvoiceengine.cc:2009: } > On 2016/11/30 13:55:51, the sun wrote: > > If you move call_->SetBitrateConfig() here instead, do you even need to store > > the config in WVoMC? > > It isn't that simple, since params.max_bandwidth_bps should override this, see > line 1622. Maybe if I have SetSendCodec() return a bitrate config that I can use > there? WDYT? Yes, if by "return" you mean to make it an output argument of SetSendCodec(), then I think that's better than keeping the state in the class.
lgtm % one suggestion for you to decide https://codereview.webrtc.org/2534173002/diff/100001/webrtc/media/engine/webr... File webrtc/media/engine/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/2534173002/diff/100001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvoiceengine.cc:1625: call_->SetBitrateConfig(bitrate_config_); can we determine bitrate_config in one util function, assuming SetCallBitrateConfig, we can repeat some lines like const AudioCodec* codec = WebRtcVoiceCodecs::GetPreferredCodec(...) but it may be easier to maintain.
On 2016/11/30 14:20:41, the sun wrote: > On 2016/11/30 14:15:30, stefan-webrtc (holmer) wrote: > > > https://codereview.webrtc.org/2534173002/diff/100001/webrtc/media/engine/webr... > > File webrtc/media/engine/webrtcvoiceengine.cc (right): > > > > > https://codereview.webrtc.org/2534173002/diff/100001/webrtc/media/engine/webr... > > webrtc/media/engine/webrtcvoiceengine.cc:2009: } > > On 2016/11/30 13:55:51, the sun wrote: > > > If you move call_->SetBitrateConfig() here instead, do you even need to > store > > > the config in WVoMC? > > > > It isn't that simple, since params.max_bandwidth_bps should override this, see > > line 1622. Maybe if I have SetSendCodec() return a bitrate config that I can > use > > there? WDYT? > > Yes, if by "return" you mean to make it an output argument of SetSendCodec(), > then I think that's better than keeping the state in the class. It turned out not too be that easy due to if params.max_bandwidth_bps changes but the codec doesn't, we want to use the old BitrateConfig. Either we store the BitrateConfig, or we store the previously used cricket::AudioCodec so we can get the BitrateConfig when we need it. I think it's probably better to store the BitrateConfig as is.
Decided to make no changes. Let me know if you disagree, or I'll land this. https://codereview.webrtc.org/2534173002/diff/100001/webrtc/media/engine/webr... File webrtc/media/engine/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/2534173002/diff/100001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvoiceengine.cc:1625: call_->SetBitrateConfig(bitrate_config_); On 2016/11/30 14:25:25, minyue-webrtc wrote: > can we determine bitrate_config in one util function, assuming > SetCallBitrateConfig, we can repeat some lines like > > const AudioCodec* codec = WebRtcVoiceCodecs::GetPreferredCodec(...) > > but it may be easier to maintain. I don't know. It's probably doable, but it doesn't seem unreasonable to me that we get the BitrateConfig inside SetSendCodec if a new codec is actually determined. Here we use the already stored BitrateConfig and update it if necessary, so it shouldn't be responsible for figuring out if the codec has changed.
On 2016/11/30 14:44:35, stefan-webrtc (holmer) wrote: > On 2016/11/30 14:20:41, the sun wrote: > > On 2016/11/30 14:15:30, stefan-webrtc (holmer) wrote: > > > > > > https://codereview.webrtc.org/2534173002/diff/100001/webrtc/media/engine/webr... > > > File webrtc/media/engine/webrtcvoiceengine.cc (right): > > > > > > > > > https://codereview.webrtc.org/2534173002/diff/100001/webrtc/media/engine/webr... > > > webrtc/media/engine/webrtcvoiceengine.cc:2009: } > > > On 2016/11/30 13:55:51, the sun wrote: > > > > If you move call_->SetBitrateConfig() here instead, do you even need to > > store > > > > the config in WVoMC? > > > > > > It isn't that simple, since params.max_bandwidth_bps should override this, > see > > > line 1622. Maybe if I have SetSendCodec() return a bitrate config that I can > > use > > > there? WDYT? > > > > Yes, if by "return" you mean to make it an output argument of SetSendCodec(), > > then I think that's better than keeping the state in the class. > > It turned out not too be that easy due to if params.max_bandwidth_bps changes > but the codec doesn't, we want to use the old BitrateConfig. Either we store the > BitrateConfig, or we store the previously used cricket::AudioCodec so we can get > the BitrateConfig when we need it. I think it's probably better to store the > BitrateConfig as is. Ok, thanks for trying. Fire at will!
The CQ bit was checked by stefan@webrtc.org
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": 100001, "attempt_start_ts": 1480519261259420,
"parent_rev": "22487f96f379829026cdc747c554d6ba5a0dcb4b", "commit_rev":
"76e8009a20d25e0adb221b432103758f41191dbb"}
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from
==========
Wire up x-google-{min,start,max}-bitrate to WebRtcVoiceMediaChannel.
BUG=webrtc:6793
==========
to
==========
Wire up x-google-{min,start,max}-bitrate to WebRtcVoiceMediaChannel.
BUG=webrtc:6793
Committed: https://crrev.com/13f1a0a9cab1aaa872b37d3a3aa3e894659c71c2
Cr-Commit-Position: refs/heads/master@{#15337}
==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/13f1a0a9cab1aaa872b37d3a3aa3e894659c71c2 Cr-Commit-Position: refs/heads/master@{#15337}
Message was sent while issue was closed.
deadbeef@webrtc.org changed reviewers: + deadbeef@webrtc.org
Message was sent while issue was closed.
Unless I'm mistaken, this CL will set the bitrate config for the whole call
based on the "x-google-{min,start,max}-bitrate" and "b=AS" for just the audio m=
section. This was already being done for the video "m=" section, but it's not
right, as mentioned in a TODO by pbos.
Message was sent while issue was closed.
On 2017/03/24 16:08:40, Taylor Brandstetter wrote:
> Unless I'm mistaken, this CL will set the bitrate config for the whole call
> based on the "x-google-{min,start,max}-bitrate" and "b=AS" for just the audio
m=
> section. This was already being done for the video "m=" section, but it's not
> right, as mentioned in a TODO by pbos.
See https://bugs.chromium.org/p/chromium/issues/detail?id=703903
Message was sent while issue was closed.
On 2017/03/24 16:08:40, Taylor Brandstetter wrote:
> Unless I'm mistaken, this CL will set the bitrate config for the whole call
> based on the "x-google-{min,start,max}-bitrate" and "b=AS" for just the audio
m=
> section. This was already being done for the video "m=" section, but it's not
> right, as mentioned in a TODO by pbos.
Yes, that is unfortunately the way it is implemented and used. I think what we
want to do is to get rid of x-google-{min,start,max}-bitrate and promote using
RtpSender.SetParameters and, perhaps, the future RtpTransportController API for
setting BWE parameters.
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
