Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1561073006:
Consolidate SetSendParameters into one setter. (Closed)
|
Created:
4 years, 11 months ago by pbos-webrtc Modified:
4 years, 11 months ago CC:
webrtc-reviews_webrtc.org Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionConsolidate SetSendParameters into one setter.
Removes unnecessary creation/removal of intermediate VideoSendStreams
due to only being partially configured before creation.
BUG=webrtc:5296, webrtc:5410
R=deadbeef@webrtc.org, pthatcher@webrtc.org
Committed: https://chromium.googlesource.com/external/webrtc/+/3afc8c40be2f8cb51b7308e42dbb2d7bbae7bca3
Patch Set 1 #
Total comments: 16
Patch Set 2 : pthatcher@ feedback #Patch Set 3 : Remove duplicate SetOptions code #
Total comments: 20
Patch Set 4 : rebase #Patch Set 5 : deadbeef@ feedback #
Total comments: 6
Patch Set 6 : optional rtcp_mode #Patch Set 7 : rebase #
Messages
Total messages: 25 (6 generated)
Description was changed from ========== Consolidate SetSendParameters into one setter. Removes unnecessary creation/removal of intermediate VideoSendStreams due to only being partially configured before creation. BUG=webrtc:5296 R=pthatcher@webrtc.org ========== to ========== Consolidate SetSendParameters into one setter. Removes unnecessary creation/removal of intermediate VideoSendStreams due to only being partially configured before creation. BUG=webrtc:5296, webrtc:5410 R=pthatcher@webrtc.org ==========
PTAL
The CQ bit was checked by pbos@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1561073006/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1561073006/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.webrtc.org/1561073006/diff/1/talk/media/webrtc/webrtcvideo... File talk/media/webrtc/webrtcvideoengine2.cc (right): https://codereview.webrtc.org/1561073006/diff/1/talk/media/webrtc/webrtcvideo... talk/media/webrtc/webrtcvideoengine2.cc:708: bool WebRtcVideoChannel2::FilterSendParameters( I think a better name here would be GetChangedSendParameters. https://codereview.webrtc.org/1561073006/diff/1/talk/media/webrtc/webrtcvideo... talk/media/webrtc/webrtcvideoengine2.cc:710: FilteredSendParameters* out_params) const { And this ChangedSendParameters* changed_params Filtering is just a side effect. The primary effect is finding what changed and what didn't. https://codereview.webrtc.org/1561073006/diff/1/talk/media/webrtc/webrtcvideo... talk/media/webrtc/webrtcvideoengine2.cc:752: if (!(new_options == old_options)) { Can you put a TODO to see if we can just do: if (params.options != options_) { out_params->options = rtc::Optional<VideoOptions>(params.options); } ? In other words, require that VideoSendParameters always have a full set of options not a partial set. https://codereview.webrtc.org/1561073006/diff/1/talk/media/webrtc/webrtcvideo... talk/media/webrtc/webrtcvideoengine2.cc:994: SetOptions(*options); If you pulled out most of the logic in SetSendParameters(VideoSendParameters) into a separate ChangeSendParameters(ChangedVideoSendParameters), then you could do this: if (*options != options_) { ChangedSendParameters changed_params; VideoOptions new_options = options_; options_.SetAll(options); changed_params.options = rtc::Optional<VideoOptions>(new_options); ChangeSendParameters(changed_params); } Or if you store the last set of VideoSendParameters, you could do this: VideoSendParameters new_params = send_params_; new_params.options.SetAll(options); SetSendParameters(new_params); I think I like the second better. https://codereview.webrtc.org/1561073006/diff/1/talk/media/webrtc/webrtcvideo... talk/media/webrtc/webrtcvideoengine2.cc:998: } These both return true, so you could probably just have it be: if (enable && options) { ... } return true; https://codereview.webrtc.org/1561073006/diff/1/talk/media/webrtc/webrtcvideo... talk/media/webrtc/webrtcvideoengine2.cc:1931: bool force_reconfigure = false; needs_restart means "RecreateWebRtcStream();" force_reconfigure means "last_dimensions_.width = 0;" That's confusing. It might be less confusing as: bool recreate_stream = false; bool reconfigure_stream = false; With a comment describing the difference. https://codereview.webrtc.org/1561073006/diff/1/talk/media/webrtc/webrtcvideo... talk/media/webrtc/webrtcvideoengine2.cc:1958: } If you're going to recreate the stream later, do you still need to do this? If so, do you need to do this before calling SetCodecAndOptions? If so, could you please carefully leave a comment explaining that?
https://codereview.webrtc.org/1561073006/diff/1/talk/media/webrtc/webrtcvideo... File talk/media/webrtc/webrtcvideoengine2.cc (right): https://codereview.webrtc.org/1561073006/diff/1/talk/media/webrtc/webrtcvideo... talk/media/webrtc/webrtcvideoengine2.cc:1806: void WebRtcVideoChannel2::WebRtcVideoSendStream::SetOptions( By the way, as far as I can tell, this is only used by VideoRtpSender::SetVideoSend, which gets its options from VideoSource::Initialize, which is only used to turn on video_noise_reduction, which shoudn't be a track constraint at all but is currently used by AppRTC with the constraint "googNoiseReduction". If we can come up with a better way to turn it on (or just always turn it on), We could remove this method (I think).
pthatcher@ feedback
PTAL https://codereview.webrtc.org/1561073006/diff/1/talk/media/webrtc/webrtcvideo... File talk/media/webrtc/webrtcvideoengine2.cc (right): https://codereview.webrtc.org/1561073006/diff/1/talk/media/webrtc/webrtcvideo... talk/media/webrtc/webrtcvideoengine2.cc:708: bool WebRtcVideoChannel2::FilterSendParameters( On 2016/01/07 21:03:43, pthatcher1 wrote: > I think a better name here would be GetChangedSendParameters. Done. https://codereview.webrtc.org/1561073006/diff/1/talk/media/webrtc/webrtcvideo... talk/media/webrtc/webrtcvideoengine2.cc:710: FilteredSendParameters* out_params) const { On 2016/01/07 21:03:43, pthatcher1 wrote: > And this ChangedSendParameters* changed_params > > Filtering is just a side effect. The primary effect is finding what changed and > what didn't. Done. https://codereview.webrtc.org/1561073006/diff/1/talk/media/webrtc/webrtcvideo... talk/media/webrtc/webrtcvideoengine2.cc:752: if (!(new_options == old_options)) { On 2016/01/07 21:03:43, pthatcher1 wrote: > Can you put a TODO to see if we can just do: > > if (params.options != options_) { > out_params->options = rtc::Optional<VideoOptions>(params.options); > } > > ? > > In other words, require that VideoSendParameters always have a full set of > options not a partial set. Done. https://codereview.webrtc.org/1561073006/diff/1/talk/media/webrtc/webrtcvideo... talk/media/webrtc/webrtcvideoengine2.cc:994: SetOptions(*options); On 2016/01/07 21:03:43, pthatcher1 wrote: > If you pulled out most of the logic in SetSendParameters(VideoSendParameters) > into a separate ChangeSendParameters(ChangedVideoSendParameters), then you could > do this: > > if (*options != options_) { > ChangedSendParameters changed_params; > VideoOptions new_options = options_; > options_.SetAll(options); > changed_params.options = rtc::Optional<VideoOptions>(new_options); > ChangeSendParameters(changed_params); > } > > Or if you store the last set of VideoSendParameters, you could do this: > > VideoSendParameters new_params = send_params_; > new_params.options.SetAll(options); > SetSendParameters(new_params); > > > I think I like the second better. Done. https://codereview.webrtc.org/1561073006/diff/1/talk/media/webrtc/webrtcvideo... talk/media/webrtc/webrtcvideoengine2.cc:998: } On 2016/01/07 21:03:43, pthatcher1 wrote: > These both return true, so you could probably just have it be: > > if (enable && options) { > ... > } > return true; Yeah that looks dumb. Assuming it has evolved over time. Done. https://codereview.webrtc.org/1561073006/diff/1/talk/media/webrtc/webrtcvideo... talk/media/webrtc/webrtcvideoengine2.cc:1806: void WebRtcVideoChannel2::WebRtcVideoSendStream::SetOptions( On 2016/01/08 00:34:57, pthatcher1 wrote: > By the way, as far as I can tell, this is only used by > VideoRtpSender::SetVideoSend, which gets its options from > VideoSource::Initialize, which is only used to turn on video_noise_reduction, > which shoudn't be a track constraint at all but is currently used by AppRTC with > the constraint "googNoiseReduction". If we can come up with a better way to > turn it on (or just always turn it on), We could remove this method (I think). Acknowledged. https://codereview.webrtc.org/1561073006/diff/1/talk/media/webrtc/webrtcvideo... talk/media/webrtc/webrtcvideoengine2.cc:1931: bool force_reconfigure = false; On 2016/01/07 21:03:43, pthatcher1 wrote: > needs_restart means "RecreateWebRtcStream();" > force_reconfigure means "last_dimensions_.width = 0;" > > That's confusing. It might be less confusing as: > > bool recreate_stream = false; > bool reconfigure_stream = false; > > With a comment describing the difference. Done. https://codereview.webrtc.org/1561073006/diff/1/talk/media/webrtc/webrtcvideo... talk/media/webrtc/webrtcvideoengine2.cc:1958: } On 2016/01/07 21:03:43, pthatcher1 wrote: > If you're going to recreate the stream later, do you still need to do this? > > If so, do you need to do this before calling SetCodecAndOptions? If so, could > you please carefully leave a comment explaining that? Nope, I don't. Made this lazy and reinitialize on next frame instead (if it hasn't been recreated already).
Remove duplicate SetOptions code
deadbeef@webrtc.org changed reviewers: + deadbeef@webrtc.org
I found some very minor things that may improve code readability, but functionally this looks great. https://codereview.webrtc.org/1561073006/diff/40001/talk/media/webrtc/webrtcv... File talk/media/webrtc/webrtcvideoengine2.cc (right): https://codereview.webrtc.org/1561073006/diff/40001/talk/media/webrtc/webrtcv... talk/media/webrtc/webrtcvideoengine2.cc:750: VideoOptions old_options = options_; I don't think you need "old_options", we can just compare "new_options" to "options_". https://codereview.webrtc.org/1561073006/diff/40001/talk/media/webrtc/webrtcv... talk/media/webrtc/webrtcvideoengine2.cc:784: int bitrate_kbps; Since this method is already pretty huge, I would define a "GetBitrateConfigForCodec" helper function and call it here. https://codereview.webrtc.org/1561073006/diff/40001/talk/media/webrtc/webrtcv... talk/media/webrtc/webrtcvideoengine2.cc:815: // reconfigure nit: move "// all senders." up a line. https://codereview.webrtc.org/1561073006/diff/40001/talk/media/webrtc/webrtcv... talk/media/webrtc/webrtcvideoengine2.cc:835: if (options_.cpu_overuse_detection) nit: add {} https://codereview.webrtc.org/1561073006/diff/40001/talk/media/webrtc/webrtcv... talk/media/webrtc/webrtcvideoengine2.cc:848: if (codec_set) { Instead of "codec_set", can you just use "if (changed_params.codec)"? https://codereview.webrtc.org/1561073006/diff/40001/talk/media/webrtc/webrtcv... talk/media/webrtc/webrtcvideoengine2.cc:941: void WebRtcVideoChannel2::SetSendCodec( What's the purpose of this method? https://codereview.webrtc.org/1561073006/diff/40001/talk/media/webrtc/webrtcv... talk/media/webrtc/webrtcvideoengine2.cc:1601: reconfigure_encoder_(false), nit: Can initialize this to false in the header file. https://codereview.webrtc.org/1561073006/diff/40001/talk/media/webrtc/webrtcv... talk/media/webrtc/webrtcvideoengine2.cc:1931: reconfigure_encoder_ = true; Maybe rename this to something like "pending_reconfigure_encoder_", or "reconfigure_encoder_on_next_frame_". Or just describe it in the header file. It was unclear to me at first why this was a member variable, until I realized that it didn't take effect until the next InputFrame. https://codereview.webrtc.org/1561073006/diff/40001/talk/media/webrtc/webrtcv... talk/media/webrtc/webrtcvideoengine2.cc:1937: recreate_stream = false; Instead of resetting recreate_stream to false, you could just return early here. That may be more clear to someone reading the code. https://codereview.webrtc.org/1561073006/diff/40001/talk/media/webrtc/webrtcv... talk/media/webrtc/webrtcvideoengine2.cc:1942: recreate_stream = false; Same here.
rebase
deadbeef@ feedback
PTAL, thanks! https://codereview.webrtc.org/1561073006/diff/40001/talk/media/webrtc/webrtcv... File talk/media/webrtc/webrtcvideoengine2.cc (right): https://codereview.webrtc.org/1561073006/diff/40001/talk/media/webrtc/webrtcv... talk/media/webrtc/webrtcvideoengine2.cc:750: VideoOptions old_options = options_; On 2016/01/21 23:47:16, Taylor Brandstetter wrote: > I don't think you need "old_options", we can just compare "new_options" to > "options_". Done. https://codereview.webrtc.org/1561073006/diff/40001/talk/media/webrtc/webrtcv... talk/media/webrtc/webrtcvideoengine2.cc:784: int bitrate_kbps; On 2016/01/21 23:47:16, Taylor Brandstetter wrote: > Since this method is already pretty huge, I would define a > "GetBitrateConfigForCodec" helper function and call it here. Done. https://codereview.webrtc.org/1561073006/diff/40001/talk/media/webrtc/webrtcv... talk/media/webrtc/webrtcvideoengine2.cc:815: // reconfigure On 2016/01/21 23:47:16, Taylor Brandstetter wrote: > nit: move "// all senders." up a line. Done. https://codereview.webrtc.org/1561073006/diff/40001/talk/media/webrtc/webrtcv... talk/media/webrtc/webrtcvideoengine2.cc:835: if (options_.cpu_overuse_detection) On 2016/01/21 23:47:16, Taylor Brandstetter wrote: > nit: add {} Done. https://codereview.webrtc.org/1561073006/diff/40001/talk/media/webrtc/webrtcv... talk/media/webrtc/webrtcvideoengine2.cc:848: if (codec_set) { On 2016/01/21 23:47:16, Taylor Brandstetter wrote: > Instead of "codec_set", can you just use "if (changed_params.codec)"? Done. https://codereview.webrtc.org/1561073006/diff/40001/talk/media/webrtc/webrtcv... talk/media/webrtc/webrtcvideoengine2.cc:941: void WebRtcVideoChannel2::SetSendCodec( On 2016/01/21 23:47:16, Taylor Brandstetter wrote: > What's the purpose of this method? Moved over in this patchset, removed. Thanks. https://codereview.webrtc.org/1561073006/diff/40001/talk/media/webrtc/webrtcv... talk/media/webrtc/webrtcvideoengine2.cc:1601: reconfigure_encoder_(false), On 2016/01/21 23:47:16, Taylor Brandstetter wrote: > nit: Can initialize this to false in the header file. If so I'd like to do that for all members, maybe in a separate CL. https://codereview.webrtc.org/1561073006/diff/40001/talk/media/webrtc/webrtcv... talk/media/webrtc/webrtcvideoengine2.cc:1931: reconfigure_encoder_ = true; On 2016/01/21 23:47:16, Taylor Brandstetter wrote: > Maybe rename this to something like "pending_reconfigure_encoder_", or > "reconfigure_encoder_on_next_frame_". Or just describe it in the header file. It > was unclear to me at first why this was a member variable, until I realized that > it didn't take effect until the next InputFrame. pending_encoder_reconfiguration_ done. https://codereview.webrtc.org/1561073006/diff/40001/talk/media/webrtc/webrtcv... talk/media/webrtc/webrtcvideoengine2.cc:1937: recreate_stream = false; On 2016/01/21 23:47:16, Taylor Brandstetter wrote: > Instead of resetting recreate_stream to false, you could just return early here. > That may be more clear to someone reading the code. Done. https://codereview.webrtc.org/1561073006/diff/40001/talk/media/webrtc/webrtcv... talk/media/webrtc/webrtcvideoengine2.cc:1942: recreate_stream = false; On 2016/01/21 23:47:16, Taylor Brandstetter wrote: > Same here. Done.
lgtm!
lgtm The rest of the comments are small things that I'll let you decide on. https://codereview.webrtc.org/1561073006/diff/80001/talk/media/webrtc/webrtcv... File talk/media/webrtc/webrtcvideoengine2.cc (right): https://codereview.webrtc.org/1561073006/diff/80001/talk/media/webrtc/webrtcv... talk/media/webrtc/webrtcvideoengine2.cc:784: : webrtc::RtcpMode::kCompound; Would it make sense to check send_params_.rtcp.reduced_size to see if it changed or not and have changed_params->rtcp_mode be Optional? Or is that just more trouble than it's worth? If so, could you leave a comment saying it's more trouble than it's worth? https://codereview.webrtc.org/1561073006/diff/80001/talk/media/webrtc/webrtcv... talk/media/webrtc/webrtcvideoengine2.cc:825: } Would it make sense to move this logic into GetChangedSendParameters? Than you wouldn't need the bitrate_config_changed value in here, and you could just call call_->SetBitrateConfig immediately. https://codereview.webrtc.org/1561073006/diff/80001/talk/media/webrtc/webrtcv... talk/media/webrtc/webrtcvideoengine2.cc:1719: // TODO(pbos): Apply this on the VideoAdapter instead! Actually, nothing calls VideoChannel::ApplyViewRequest any more, so you can just remove this.
optional rtcp_mode
rebase
https://codereview.webrtc.org/1561073006/diff/80001/talk/media/webrtc/webrtcv... File talk/media/webrtc/webrtcvideoengine2.cc (right): https://codereview.webrtc.org/1561073006/diff/80001/talk/media/webrtc/webrtcv... talk/media/webrtc/webrtcvideoengine2.cc:784: : webrtc::RtcpMode::kCompound; On 2016/01/26 23:00:41, pthatcher1 wrote: > Would it make sense to check send_params_.rtcp.reduced_size to see if it changed > or not and have changed_params->rtcp_mode be Optional? Or is that just more > trouble than it's worth? If so, could you leave a comment saying it's more > trouble than it's worth? Done for consistency. https://codereview.webrtc.org/1561073006/diff/80001/talk/media/webrtc/webrtcv... talk/media/webrtc/webrtcvideoengine2.cc:825: } On 2016/01/26 23:00:42, pthatcher1 wrote: > Would it make sense to move this logic into GetChangedSendParameters? Than you > wouldn't need the bitrate_config_changed value in here, and you could just call > call_->SetBitrateConfig immediately. I think this unfortunately makes it more complex, I was hoping to remove max_bandwidth_bps because of it and only provide a bitrate_config, but max_bandwidth_bps changing is used inside SetSendParameters. It's possibly possible, but I'd like that to be left for a follow-up to avoid breaking. https://codereview.webrtc.org/1561073006/diff/80001/talk/media/webrtc/webrtcv... talk/media/webrtc/webrtcvideoengine2.cc:1719: // TODO(pbos): Apply this on the VideoAdapter instead! On 2016/01/26 23:00:42, pthatcher1 wrote: > Actually, nothing calls VideoChannel::ApplyViewRequest any more, so you can > just remove this. Will do here: https://codereview.webrtc.org/1613433002/
Description was changed from ========== Consolidate SetSendParameters into one setter. Removes unnecessary creation/removal of intermediate VideoSendStreams due to only being partially configured before creation. BUG=webrtc:5296, webrtc:5410 R=pthatcher@webrtc.org ========== to ========== Consolidate SetSendParameters into one setter. Removes unnecessary creation/removal of intermediate VideoSendStreams due to only being partially configured before creation. BUG=webrtc:5296, webrtc:5410 R=deadbeef@webrtc.org, pthatcher@webrtc.org Committed: https://chromium.googlesource.com/external/webrtc/+/3afc8c40be2f8cb51b7308e42... ==========
Message was sent while issue was closed.
Description was changed from ========== Consolidate SetSendParameters into one setter. Removes unnecessary creation/removal of intermediate VideoSendStreams due to only being partially configured before creation. BUG=webrtc:5296, webrtc:5410 R=deadbeef@webrtc.org, pthatcher@webrtc.org Committed: https://chromium.googlesource.com/external/webrtc/+/3afc8c40be2f8cb51b7308e42... ========== to ========== Consolidate SetSendParameters into one setter. Removes unnecessary creation/removal of intermediate VideoSendStreams due to only being partially configured before creation. BUG=webrtc:5296, webrtc:5410 R=deadbeef@webrtc.org, pthatcher@webrtc.org Committed: https://crrev.com/3afc8c40be2f8cb51b7308e42dbb2d7bbae7bca3 Cr-Commit-Position: refs/heads/master@{#11399} ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) manually as 3afc8c40be2f8cb51b7308e42dbb2d7bbae7bca3 (presubmit successful).
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/3afc8c40be2f8cb51b7308e42dbb2d7bbae7bca3 Cr-Commit-Position: refs/heads/master@{#11399} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
