Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1711763003:
New flag is_screencast in VideoOptions. (Closed)
|
Created:
4 years, 10 months ago by nisse-webrtc Modified:
4 years, 9 months ago CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionNew flag is_screencast in VideoOptions.
This cl copies the value of cricket::VideoCapturer::IsScreencast into
a flag in VideoOptions. It is passed on via the chain
VideortpSender::SetVideoSend
WebRtcVideoChannel2::SetVideoSend
WebRtcVideoChannel2::SetOptions
WebRtcVideoChannel2::WebRtcVideoSendStream::SetOptions
Where it's used, in
WebRtcVideoChannel2::WebRtcVideoSendStream::OnFrame, we can look it up
in parameters_, instead of calling capturer_->IsScreencast().
Doesn't touch screencast logic related to cpu adaptation, since that
code is in flux in a different cl.
Also drop the is_screencast flag from the Dimensions struct, and drop separate options argument from ConfigureVideoEncoderSettings and SetCodecAndOptions, instead always using the options recorded in VideoSendStreamParameters::options.
In the tests, changed FakeVideoCapturer::is_screencast to be a construction time flag. Generally, unittests of screencast have to both use a capturer configured for screencast, and set the screencast flag using SetSendParameters. Since the automatic connection via VideoSource and VideoRtpSender isn't involved in the unit tests.
Note that using SetSendParameters to set the screencast flag doesn't make sense, since it's not per-stream. SetVideoSend would be more appropriate. That should be fixed if/when we drop VideoOptions from SetSendParameters.
BUG=webrtc:5426
R=pbos@webrtc.org, perkj@webrtc.org, pthatcher@webrtc.org
Committed: https://crrev.com/60653ba3cc58aecdff594a1f72cfaefbad9c407b
Cr-Commit-Position: refs/heads/master@{#11837}
Patch Set 1 #Patch Set 2 : Drop is_screencast from Dimensions struct and ConfigureVideoEncoderSettings arg list. #Patch Set 3 : Fix tests. Drop separate option argument from ConfigureVideoEncoderSettings and SetCodecAndOptions. #Patch Set 4 : Fix use-after-free of FakeVideoSendStream. #
Total comments: 11
Patch Set 5 : In WebRtcVideoSendStream::SetOptions, modify only options specified by the caller. #
Total comments: 2
Patch Set 6 : Rename testcode capture init methods. #
Total comments: 6
Patch Set 7 : Address pbos' comments.' #
Total comments: 4
Patch Set 8 : Don't recreate send stream on SetOptions. #
Total comments: 2
Patch Set 9 : Rebase. Delete capturer_is_screencast_ flag.' #Patch Set 10 : Rebase, on top of suspend_below_min_bitrate change. #
Total comments: 6
Patch Set 11 : Fix OnLoadUpdate is_screencast check. Don't set FakeVideoCapturer into screencast mode in the video… #
Messages
Total messages: 56 (19 generated)
nisse@webrtc.org changed reviewers: + pbos@webrtc.org, perkj@webrtc.org, pthatcher@webrtc.org
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/patch-status/1711763003/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1711763003/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_ubsan_vptr on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_ubsan_vptr/builds...) win_x64_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_rel/builds/12587)
Updated version. I notice that some bots fail, not obvious why on first quick look. One consequence of this change is that it's no longer has any effect on the encoder to change the capturer's idea if IsScreencast in the middle of a stream. It's only the value at the time of VideoSource::Initialize which gets copied to VideoOptions, and then only propagated on VideoRtpSender::SetVideoSend. I hope that's no problem?
Description was changed from ========== New flag is_screencast in VideoOptions. This cl copies the value of cricket::VideoCapturer::IsScreencast into a flag in VideoOptions. It is passed on via the chain VideortpSender::SetVideoSend WebRtcVideoChannel2::SetVideoSend WebRtcVideoChannel2::SetOptions WebRtcVideoChannel2::WebRtcVideoSendStream::SetOptions Where it's used, in WebRtcVideoChannel2::WebRtcVideoSendStream::OnFrame, we can look it up in parameters_, instead of calling capturer_->IsScreencast(). Doesn't touch screencast logic related to cpu adaptation, since that code is in flux in a different cl. This is the first version, I think it can be improved by also dropping the is_screencast flag from the Dimensions struct. BUG=webrtc:5426 ========== to ========== New flag is_screencast in VideoOptions. This cl copies the value of cricket::VideoCapturer::IsScreencast into a flag in VideoOptions. It is passed on via the chain VideortpSender::SetVideoSend WebRtcVideoChannel2::SetVideoSend WebRtcVideoChannel2::SetOptions WebRtcVideoChannel2::WebRtcVideoSendStream::SetOptions Where it's used, in WebRtcVideoChannel2::WebRtcVideoSendStream::OnFrame, we can look it up in parameters_, instead of calling capturer_->IsScreencast(). Doesn't touch screencast logic related to cpu adaptation, since that code is in flux in a different cl. Also drop the is_screencast flag from the Dimensions struct, and drop separate options argument from ConfigureVideoEncoderSettings and SetCodecAndOptions, instead always using the options recorded in VideoSendStreamParameters::options. In the tests, changed FakeVideoCapturer::is_screencast to be a construction time flag. Generally, unittests of screencast have to both use a capturer configured for screencast, and set the screencast flag using SetSendParameters. Since the automatic connection via VideoSource and VideoRtpSender isn't involved in the unit tests. Note that using SetSendParameters to set the screencast flag doesn't make sense, since it's not per-stream. SetVideoSend would be more appropriate. That should be fixed if/when we drop VideoOptions from SetSendParameters. BUG=webrtc:5426 ==========
Updated version passes local tests.
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/patch-status/1711763003/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1711763003/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_asan on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_asan/builds/12566)
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/patch-status/1711763003/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1711763003/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.webrtc.org/1711763003/diff/60001/webrtc/api/videosource_un... File webrtc/api/videosource_unittest.cc (right): https://codereview.webrtc.org/1711763003/diff/60001/webrtc/api/videosource_un... webrtc/api/videosource_unittest.cc:119: void InitScreencast(bool is_screencast) { Why isn't this just called Init? https://codereview.webrtc.org/1711763003/diff/60001/webrtc/media/base/videoca... File webrtc/media/base/videocapturer_unittest.cc (right): https://codereview.webrtc.org/1711763003/diff/60001/webrtc/media/base/videoca... webrtc/media/base/videocapturer_unittest.cc:43: void InitScreencast(bool is_screencast) { Same here. https://codereview.webrtc.org/1711763003/diff/60001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvideoengine2.cc (right): https://codereview.webrtc.org/1711763003/diff/60001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvideoengine2.cc:1670: parameters_.options = options; If you don't, then the options that come from SetVideoSend will blow away the ones that come from SetSendParameters. In fact, I think that's what's happening right now, which is bad. I think that means suspend_below_min_bitrate and screencast_min_bitrate_kbps are being blown away every time the track is disable and then re-enable or the track is changed via RtpSender.SetTrack. Since VideoOptions is now just 2 things from PeerConnection constraints and 2 things from the VideoSource, is it time to break it up into something more simple?
https://codereview.webrtc.org/1711763003/diff/60001/webrtc/api/videosource_un... File webrtc/api/videosource_unittest.cc (right): https://codereview.webrtc.org/1711763003/diff/60001/webrtc/api/videosource_un... webrtc/api/videosource_unittest.cc:119: void InitScreencast(bool is_screencast) { On 2016/02/20 08:32:59, pthatcher1 wrote: > Why isn't this just called Init? or CreateVideoCapturer https://codereview.webrtc.org/1711763003/diff/60001/webrtc/media/base/mediach... File webrtc/media/base/mediachannel.h (right): https://codereview.webrtc.org/1711763003/diff/60001/webrtc/media/base/mediach... webrtc/media/base/mediachannel.h:255: SetFrom(&suspend_below_min_bitrate, change.suspend_below_min_bitrate); not this cl but is suspend_below_min_bitrate and screencast_min_bitrate_kbps contraints from getusermedia? They should really but constraints or some kind of settings on either rtpvideosender or peerconnection... https://codereview.webrtc.org/1711763003/diff/60001/webrtc/media/base/videoen... File webrtc/media/base/videoengine_unittest.h (right): https://codereview.webrtc.org/1711763003/diff/60001/webrtc/media/base/videoen... webrtc/media/base/videoengine_unittest.h:131: virtual cricket::FakeVideoCapturer* CreateFakeVideoCapturer( Suggest CreateFakeScreenCast and keeep CreateFakeVideoCapturer or remove CreateFakeVideoCapturer()
https://codereview.webrtc.org/1711763003/diff/60001/webrtc/api/videosource_un... File webrtc/api/videosource_unittest.cc (right): https://codereview.webrtc.org/1711763003/diff/60001/webrtc/api/videosource_un... webrtc/api/videosource_unittest.cc:119: void InitScreencast(bool is_screencast) { On 2016/02/20 08:32:59, pthatcher1 wrote: > Why isn't this just called Init? Only because when calling it, InitScreencast(true), the longer name gives a hint about the meaning of the argument. https://codereview.webrtc.org/1711763003/diff/60001/webrtc/media/base/mediach... File webrtc/media/base/mediachannel.h (right): https://codereview.webrtc.org/1711763003/diff/60001/webrtc/media/base/mediach... webrtc/media/base/mediachannel.h:255: SetFrom(&suspend_below_min_bitrate, change.suspend_below_min_bitrate); On 2016/02/21 18:09:06, perkj_webrtc wrote: > not this cl but is suspend_below_min_bitrate and screencast_min_bitrate_kbps > contraints from getusermedia? They should really but constraints or some kind of > settings on either rtpvideosender or peerconnection... By the comment I added below, they're PeerConnection constraints. Not entirely clear if they are per stream or not. So we'll need to sort that out later. https://codereview.webrtc.org/1711763003/diff/60001/webrtc/media/base/videoen... File webrtc/media/base/videoengine_unittest.h (right): https://codereview.webrtc.org/1711763003/diff/60001/webrtc/media/base/videoen... webrtc/media/base/videoengine_unittest.h:131: virtual cricket::FakeVideoCapturer* CreateFakeVideoCapturer( On 2016/02/21 18:09:06, perkj_webrtc wrote: > Suggest CreateFakeScreenCast and keeep CreateFakeVideoCapturer or remove > CreateFakeVideoCapturer() Bot options are fine to me. I kept the no-args method for now, only to reduce the amount of changes. https://codereview.webrtc.org/1711763003/diff/60001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvideoengine2.cc (right): https://codereview.webrtc.org/1711763003/diff/60001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvideoengine2.cc:1670: parameters_.options = options; On 2016/02/20 08:33:00, pthatcher1 wrote: > If you don't, then the options that come from SetVideoSend will blow away the > ones that come from SetSendParameters. In fact, I think that's what's happening > right now, which is bad. I think that means suspend_below_min_bitrate and > screencast_min_bitrate_kbps are being blown away every time the track is disable > and then re-enable or the track is changed via RtpSender.SetTrack. > > Since VideoOptions is now just 2 things from PeerConnection constraints and 2 > things from the VideoSource, is it time to break it up into something more > simple? I'll change to SetAll. I think splitting VideoOptions is best left for a different cl, ok?
https://codereview.webrtc.org/1711763003/diff/60001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvideoengine2.cc (right): https://codereview.webrtc.org/1711763003/diff/60001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvideoengine2.cc:1670: parameters_.options = options; On 2016/02/22 07:52:34, nisse-webrtc wrote: > On 2016/02/20 08:33:00, pthatcher1 wrote: > > If you don't, then the options that come from SetVideoSend will blow away the > > ones that come from SetSendParameters. In fact, I think that's what's > happening > > right now, which is bad. I think that means suspend_below_min_bitrate and > > screencast_min_bitrate_kbps are being blown away every time the track is > disable > > and then re-enable or the track is changed via RtpSender.SetTrack. > > > > Since VideoOptions is now just 2 things from PeerConnection constraints and 2 > > things from the VideoSource, is it time to break it up into something more > > simple? > > I'll change to SetAll. I think splitting VideoOptions is best left for a > different cl, ok? Done.
lgtm But I'd like to see a different name there :). https://codereview.webrtc.org/1711763003/diff/80001/webrtc/api/videosource_un... File webrtc/api/videosource_unittest.cc (right): https://codereview.webrtc.org/1711763003/diff/80001/webrtc/api/videosource_un... webrtc/api/videosource_unittest.cc:119: void InitScreencast(bool is_screencast) { I still don't like the name. How about two methods: Init() // Like current InitScreencast(false); InitScreencast() // Like current InitScreencast(true); ? Make a third that those two call if you think it's worth it. I'm more concerned with the call sites than the implementations.
https://codereview.webrtc.org/1711763003/diff/80001/webrtc/api/videosource_un... File webrtc/api/videosource_unittest.cc (right): https://codereview.webrtc.org/1711763003/diff/80001/webrtc/api/videosource_un... webrtc/api/videosource_unittest.cc:119: void InitScreencast(bool is_screencast) { On 2016/02/25 06:57:06, pthatcher1 wrote: > I still don't like the name. > > How about two methods: > > Init() // Like current InitScreencast(false); > InitScreencast() // Like current InitScreencast(true); > > ? > > Make a third that those two call if you think it's worth it. I'm more concerned > with the call sites than the implementations. I've renamed the shared method to InitCapturer(bool is_screencast), and changed InitScreencast as you sugggested.
lgtm
lgtm https://codereview.webrtc.org/1711763003/diff/100001/webrtc/media/engine/webr... File webrtc/media/engine/webrtcvideoengine2.cc (right): https://codereview.webrtc.org/1711763003/diff/100001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvideoengine2.cc:1731: void WebRtcVideoChannel2::WebRtcVideoSendStream::SetCodecAndOptions( Should this be SetCodecAndApplyOptions or something? https://codereview.webrtc.org/1711763003/diff/100001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvideoengine2.cc:1809: parameters_.options = *params.options; Should this be a SetAll? https://codereview.webrtc.org/1711763003/diff/100001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvideoengine2.cc:1899: LOG(LS_INFO) << "SetDimensions: " << width << "x" << height; Should we log is_screencast somewhere else?
The CQ bit was checked by nisse@webrtc.org
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/1711763003/#ps100001 (title: "Rename testcode capture init methods.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1711763003/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1711763003/100001
The CQ bit was unchecked by nisse@webrtc.org
https://codereview.webrtc.org/1711763003/diff/100001/webrtc/media/engine/webr... File webrtc/media/engine/webrtcvideoengine2.cc (right): https://codereview.webrtc.org/1711763003/diff/100001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvideoengine2.cc:1731: void WebRtcVideoChannel2::WebRtcVideoSendStream::SetCodecAndOptions( On 2016/02/25 14:23:28, pbos-webrtc wrote: > Should this be SetCodecAndApplyOptions or something? Done. https://codereview.webrtc.org/1711763003/diff/100001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvideoengine2.cc:1809: parameters_.options = *params.options; On 2016/02/25 14:23:28, pbos-webrtc wrote: > Should this be a SetAll? This is a bit confusing. There is a SetAll call in GetChangedSendParameters. So the params.options we have here is the shared options from WebRtcVideoChannel2::send_params_.options, with the caller's options applied on top. I think the problem to solve is to not override the "source" options passed in via SetVideoSend and SetOptions. I'll change it to .SetAll, because I think that gets us closer to right. I think it will be alright as long as options belonging to the source are never passed to SetSendParameters. But perhaps it would be even better to use SetAll here but not with the combined options (the result of .SetAll in GetChangedSendParameters), but the unmodified options passed to SetSendParameters? https://codereview.webrtc.org/1711763003/diff/100001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvideoengine2.cc:1899: LOG(LS_INFO) << "SetDimensions: " << width << "x" << height; On 2016/02/25 14:23:28, pbos-webrtc wrote: > Should we log is_screencast somewhere else? Unless you have a concrete suggestion on where to log it, I think we can omit logging until need arises.
https://codereview.webrtc.org/1711763003/diff/120001/webrtc/media/engine/webr... File webrtc/media/engine/webrtcvideoengine2.cc (right): https://codereview.webrtc.org/1711763003/diff/120001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvideoengine2.cc:1770: << "RecreateWebRtcStream (send) because of SetCodecAndApplyOptions; options=" git cl format https://codereview.webrtc.org/1711763003/diff/120001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvideoengine2.cc:1893: int height) { I still think you want to use this function for screenshare being turned on or not. SetCodecAndApplyOptions is significantly more expensive (causes a recreate of the stream). Can you then add back the logging and previous dimensions?
https://codereview.webrtc.org/1711763003/diff/120001/webrtc/media/engine/webr... File webrtc/media/engine/webrtcvideoengine2.cc (right): https://codereview.webrtc.org/1711763003/diff/120001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvideoengine2.cc:1893: int height) { On 2016/02/25 15:26:28, pbos-webrtc wrote: > I still think you want to use this function for screenshare being turned on or > not. SetCodecAndApplyOptions is significantly more expensive (causes a recreate > of the stream). > > Can you then add back the logging and previous dimensions? To expand on this, the point is that if only is_screencast is changed, we should reconfigure the encoder similarly to SetDimensions, and not recreate the stream. Questions: Is there anything that it is reasonable to pass to SetOptions/SetVideoSend, which requires a restart of the stream? Besides is_screencast, it's only options set by ExtractVideoOptions in videosource.cc.
New version. Please have a look at the new testcase, WebRtcVideoChannel2Test.NoRecreateStreamForScreencast, I'm not sure I'm using the API correctly. About SetCodecAndApplyOptions, there's actually only one flag in VideoOptions it uses: suspend_below_min_bitrate. This is copied into the webrtc::VideoSendStream::Config passed to CreateVideoSendStream. So if this flag can be changed via SetVideoSend ---> SetOptions, we'd have to check for that case and recreate the stream. https://codereview.webrtc.org/1711763003/diff/120001/webrtc/media/engine/webr... File webrtc/media/engine/webrtcvideoengine2.cc (right): https://codereview.webrtc.org/1711763003/diff/120001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvideoengine2.cc:1893: int height) { On 2016/02/25 16:07:38, nisse-webrtc wrote: > On 2016/02/25 15:26:28, pbos-webrtc wrote: > > I still think you want to use this function for screenshare being turned on or > > not. SetCodecAndApplyOptions is significantly more expensive (causes a > recreate > > of the stream). > > > > Can you then add back the logging and previous dimensions? > > To expand on this, the point is that if only is_screencast is changed, we should > reconfigure the encoder similarly to SetDimensions, and not recreate the stream. > > Questions: Is there anything that it is reasonable to pass to > SetOptions/SetVideoSend, which requires a restart of the stream? Besides > is_screencast, it's only options set by ExtractVideoOptions in videosource.cc. My current understanding is that SetOptions should never recreate the stream. So I did the simple change of letting SetOptions set the pending_encoder_reconfiguration_ flag (examined when the next frame arrives), instead of calling SetCodecAndApplyOptions.
https://codereview.webrtc.org/1711763003/diff/140001/webrtc/media/engine/webr... File webrtc/media/engine/webrtcvideoengine2.cc (right): https://codereview.webrtc.org/1711763003/diff/140001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvideoengine2.cc:1763: parameters_.options.suspend_below_min_bitrate.value_or(false); This one is now ignored through SetOptions currently, right?
https://codereview.webrtc.org/1711763003/diff/140001/webrtc/media/engine/webr... File webrtc/media/engine/webrtcvideoengine2.cc (right): https://codereview.webrtc.org/1711763003/diff/140001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvideoengine2.cc:1763: parameters_.options.suspend_below_min_bitrate.value_or(false); On 2016/02/26 13:06:57, pbos-webrtc wrote: > This one is now ignored through SetOptions currently, right? If passed through SetOptions, parameters_.options.suspend_below_min_bitrate is updated. But SetCodecAndApplyOptions isn't called from there, so the new value won't have any effect until SetCodecAndApplyOptions is called for some other reason.
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/patch-status/1711763003/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1711763003/160001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Please have another look. It's now rebased on top of https://codereview.webrtc.org/1745003002/, which was landed earlier today. Which should fix issues with suspend_below_min_bitrate not handled consistently. https://codereview.webrtc.org/1711763003/diff/180001/webrtc/media/engine/webr... File webrtc/media/engine/webrtcvideoengine2_unittest.cc (right): https://codereview.webrtc.org/1711763003/diff/180001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvideoengine2_unittest.cc:1528: EXPECT_TRUE(channel_->SetSendParameters(send_parameters_)); Maybe this test is somewhat redundant, behavior is also tested by the UsesCorrectSettingsForScreencast test. https://codereview.webrtc.org/1711763003/diff/180001/webrtc/video/video_send_... File webrtc/video/video_send_stream.cc (right): https://codereview.webrtc.org/1711763003/diff/180001/webrtc/video/video_send_... webrtc/video/video_send_stream.cc:479: LOG(LS_WARNING) << "(Re)configureVideoEncoder: SetSendCodec failed " I moved failure logging down here, at pbos' suggestion.
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/patch-status/1711763003/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1711763003/180001
https://codereview.webrtc.org/1711763003/diff/180001/webrtc/media/engine/webr... File webrtc/media/engine/webrtcvideoengine2.cc (right): https://codereview.webrtc.org/1711763003/diff/180001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvideoengine2.cc:1925: if (parameters_.options.is_screencast) This only checks if the optional is set, not what its value is? If I'm correct, update the tests to catch this bug before fixing it. https://codereview.webrtc.org/1711763003/diff/180001/webrtc/media/engine/webr... File webrtc/media/engine/webrtcvideoengine2_unittest.cc (right): https://codereview.webrtc.org/1711763003/diff/180001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvideoengine2_unittest.cc:1564: } Should we check switching from screencast? I think that might be a bug in the current implementation.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_rel on tryserver.webrtc (JOB_TIMED_OUT, no build URL) win_baremetal on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
https://codereview.webrtc.org/1711763003/diff/180001/webrtc/media/engine/webr... File webrtc/media/engine/webrtcvideoengine2.cc (right): https://codereview.webrtc.org/1711763003/diff/180001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvideoengine2.cc:1925: if (parameters_.options.is_screencast) On 2016/03/01 16:11:36, pbos-webrtc wrote: > This only checks if the optional is set, not what its value is? If I'm correct, > update the tests to catch this bug before fixing it. You're right, there's an ".value_or(false)" missing there. I'll fix before I retry committing. https://codereview.webrtc.org/1711763003/diff/180001/webrtc/media/engine/webr... File webrtc/media/engine/webrtcvideoengine2_unittest.cc (right): https://codereview.webrtc.org/1711763003/diff/180001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvideoengine2_unittest.cc:1528: EXPECT_TRUE(channel_->SetSendParameters(send_parameters_)); On 2016/03/01 13:24:40, nisse-webrtc wrote: > Maybe this test is somewhat redundant, behavior is also tested by the > UsesCorrectSettingsForScreencast test. Something is broken, and the tests too... First, VideoSourceInitialize is intended to make sure that the is_screencast option always is present, with value taken from capturer->IsScreencast(). But I added logging to OnLoadUpdate, and at that point, is_screencast is always unset when running the rtc_media_unittests. (The tests are not using VideoSource, I guess, so we have a somewhat poor test coverage of the propagatino logic. So testcases must be updated to explicitly set the flag using SetSendParameters or SetVideoSend or so). Are there any higher level "end2end" tests which test cpu adaptation? I tried peerconnection_unittests and rtc_pc_unitttests, but neither hit the OnLoadUpdate method. But despite these two bugs in OnLoadUpdate and in the testcode, the WebRtcVideoChannel2Test.DoesNotAdaptOnOveruseWhenScreensharing test succeeds. It's logs an unwanted resolution change, [000:008] (webrtcvideoengine2.cc:1917): OnLoadUpdate 0 [000:008] (webrtcvideoengine2.cc:1925): OnLoadUpdate: unset [000:008] (videoadapter.cc:559): VAdapt CPU Request: down Steps: 1 Changed: true To: 960x540 but the checks EXPECT_EQ(codec.width, send_stream->GetLastWidth()); EXPECT_EQ(codec.height, send_stream->GetLastHeight()); are still true, GetLastWidth and GetLastHeight return 1280x720. So it seems the new resolution isn't applied. Some more debugging needed.
On 2016/03/02 08:09:51, nisse-webrtc wrote: > https://codereview.webrtc.org/1711763003/diff/180001/webrtc/media/engine/webr... > File webrtc/media/engine/webrtcvideoengine2.cc (right): > > https://codereview.webrtc.org/1711763003/diff/180001/webrtc/media/engine/webr... > webrtc/media/engine/webrtcvideoengine2.cc:1925: if > (parameters_.options.is_screencast) > On 2016/03/01 16:11:36, pbos-webrtc wrote: > > This only checks if the optional is set, not what its value is? If I'm > correct, > > update the tests to catch this bug before fixing it. > > You're right, there's an ".value_or(false)" missing there. I'll fix before I > retry committing. > > https://codereview.webrtc.org/1711763003/diff/180001/webrtc/media/engine/webr... > File webrtc/media/engine/webrtcvideoengine2_unittest.cc (right): > > https://codereview.webrtc.org/1711763003/diff/180001/webrtc/media/engine/webr... > webrtc/media/engine/webrtcvideoengine2_unittest.cc:1528: > EXPECT_TRUE(channel_->SetSendParameters(send_parameters_)); > On 2016/03/01 13:24:40, nisse-webrtc wrote: > > Maybe this test is somewhat redundant, behavior is also tested by the > > UsesCorrectSettingsForScreencast test. > > Something is broken, and the tests too... > > First, VideoSourceInitialize is intended to make sure that the is_screencast > option always is present, with value taken from capturer->IsScreencast(). > > But I added logging to OnLoadUpdate, and at that point, is_screencast is always > unset when running the rtc_media_unittests. (The tests are not using > VideoSource, I guess, so we have a somewhat poor test coverage of the > propagatino logic. So testcases must be updated to explicitly set the flag using > SetSendParameters or SetVideoSend or so). Are there any higher level "end2end" > tests which test cpu adaptation? I tried peerconnection_unittests and > rtc_pc_unitttests, but neither hit the OnLoadUpdate method. > > But despite these two bugs in OnLoadUpdate and in the testcode, the > WebRtcVideoChannel2Test.DoesNotAdaptOnOveruseWhenScreensharing test succeeds. > It's logs an unwanted resolution change, > > [000:008] (webrtcvideoengine2.cc:1917): OnLoadUpdate 0 > [000:008] (webrtcvideoengine2.cc:1925): OnLoadUpdate: unset > [000:008] (videoadapter.cc:559): VAdapt CPU Request: down Steps: 1 Changed: true > To: 960x540 > > but the checks > > EXPECT_EQ(codec.width, send_stream->GetLastWidth()); > EXPECT_EQ(codec.height, send_stream->GetLastHeight()); > > are still true, GetLastWidth and GetLastHeight return 1280x720. So it seems the > new resolution isn't applied. Some more debugging needed.
I've fixed the OnLoadUpdate bug spotted by pbos. In the tests, videoengine-related tests no longer set the FakeVideoCapturer in screencast mode. All its screencast logic should be based on the VideoOptions. The VideoCapturer has it's own screencast logic, disabling videoadapter use in screencast, and we don't want that because it masks bugs in OnLoadUpdate, making DoesNotAdaptOnOveruseWhenScreensharing always succeed. In videoengine_unittests.h, there were several unexplained calls to SetScreencast, all apparently added in 28e20752 henrike@webrtc.org 2013-07-10 imported from "revision 359 from libjingles". I simply deleted most of them, but in the AddRemoveCapturer test I had to replace it by a call to SetVideoSend to set the VideoOptions::is_screencast flag. I have no idea what's going on, and why any of these tests might be related to screencasting.
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/patch-status/1711763003/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1711763003/200001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_libfuzzer_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_libfuzzer_rel/bui...)
Description was changed from ========== New flag is_screencast in VideoOptions. This cl copies the value of cricket::VideoCapturer::IsScreencast into a flag in VideoOptions. It is passed on via the chain VideortpSender::SetVideoSend WebRtcVideoChannel2::SetVideoSend WebRtcVideoChannel2::SetOptions WebRtcVideoChannel2::WebRtcVideoSendStream::SetOptions Where it's used, in WebRtcVideoChannel2::WebRtcVideoSendStream::OnFrame, we can look it up in parameters_, instead of calling capturer_->IsScreencast(). Doesn't touch screencast logic related to cpu adaptation, since that code is in flux in a different cl. Also drop the is_screencast flag from the Dimensions struct, and drop separate options argument from ConfigureVideoEncoderSettings and SetCodecAndOptions, instead always using the options recorded in VideoSendStreamParameters::options. In the tests, changed FakeVideoCapturer::is_screencast to be a construction time flag. Generally, unittests of screencast have to both use a capturer configured for screencast, and set the screencast flag using SetSendParameters. Since the automatic connection via VideoSource and VideoRtpSender isn't involved in the unit tests. Note that using SetSendParameters to set the screencast flag doesn't make sense, since it's not per-stream. SetVideoSend would be more appropriate. That should be fixed if/when we drop VideoOptions from SetSendParameters. BUG=webrtc:5426 ========== to ========== New flag is_screencast in VideoOptions. This cl copies the value of cricket::VideoCapturer::IsScreencast into a flag in VideoOptions. It is passed on via the chain VideortpSender::SetVideoSend WebRtcVideoChannel2::SetVideoSend WebRtcVideoChannel2::SetOptions WebRtcVideoChannel2::WebRtcVideoSendStream::SetOptions Where it's used, in WebRtcVideoChannel2::WebRtcVideoSendStream::OnFrame, we can look it up in parameters_, instead of calling capturer_->IsScreencast(). Doesn't touch screencast logic related to cpu adaptation, since that code is in flux in a different cl. Also drop the is_screencast flag from the Dimensions struct, and drop separate options argument from ConfigureVideoEncoderSettings and SetCodecAndOptions, instead always using the options recorded in VideoSendStreamParameters::options. In the tests, changed FakeVideoCapturer::is_screencast to be a construction time flag. Generally, unittests of screencast have to both use a capturer configured for screencast, and set the screencast flag using SetSendParameters. Since the automatic connection via VideoSource and VideoRtpSender isn't involved in the unit tests. Note that using SetSendParameters to set the screencast flag doesn't make sense, since it's not per-stream. SetVideoSend would be more appropriate. That should be fixed if/when we drop VideoOptions from SetSendParameters. BUG=webrtc:5426 R=pbos@webrtc.org, perkj@webrtc.org, pthatcher@webrtc.org Committed: https://chromium.googlesource.com/external/webrtc/+/60653ba3cc58aecdff594a1f7... ==========
Message was sent while issue was closed.
Committed patchset #11 (id:200001) manually as 60653ba3cc58aecdff594a1f72cfaefbad9c407b (presubmit successful).
Message was sent while issue was closed.
Description was changed from ========== New flag is_screencast in VideoOptions. This cl copies the value of cricket::VideoCapturer::IsScreencast into a flag in VideoOptions. It is passed on via the chain VideortpSender::SetVideoSend WebRtcVideoChannel2::SetVideoSend WebRtcVideoChannel2::SetOptions WebRtcVideoChannel2::WebRtcVideoSendStream::SetOptions Where it's used, in WebRtcVideoChannel2::WebRtcVideoSendStream::OnFrame, we can look it up in parameters_, instead of calling capturer_->IsScreencast(). Doesn't touch screencast logic related to cpu adaptation, since that code is in flux in a different cl. Also drop the is_screencast flag from the Dimensions struct, and drop separate options argument from ConfigureVideoEncoderSettings and SetCodecAndOptions, instead always using the options recorded in VideoSendStreamParameters::options. In the tests, changed FakeVideoCapturer::is_screencast to be a construction time flag. Generally, unittests of screencast have to both use a capturer configured for screencast, and set the screencast flag using SetSendParameters. Since the automatic connection via VideoSource and VideoRtpSender isn't involved in the unit tests. Note that using SetSendParameters to set the screencast flag doesn't make sense, since it's not per-stream. SetVideoSend would be more appropriate. That should be fixed if/when we drop VideoOptions from SetSendParameters. BUG=webrtc:5426 R=pbos@webrtc.org, perkj@webrtc.org, pthatcher@webrtc.org Committed: https://chromium.googlesource.com/external/webrtc/+/60653ba3cc58aecdff594a1f7... ========== to ========== New flag is_screencast in VideoOptions. This cl copies the value of cricket::VideoCapturer::IsScreencast into a flag in VideoOptions. It is passed on via the chain VideortpSender::SetVideoSend WebRtcVideoChannel2::SetVideoSend WebRtcVideoChannel2::SetOptions WebRtcVideoChannel2::WebRtcVideoSendStream::SetOptions Where it's used, in WebRtcVideoChannel2::WebRtcVideoSendStream::OnFrame, we can look it up in parameters_, instead of calling capturer_->IsScreencast(). Doesn't touch screencast logic related to cpu adaptation, since that code is in flux in a different cl. Also drop the is_screencast flag from the Dimensions struct, and drop separate options argument from ConfigureVideoEncoderSettings and SetCodecAndOptions, instead always using the options recorded in VideoSendStreamParameters::options. In the tests, changed FakeVideoCapturer::is_screencast to be a construction time flag. Generally, unittests of screencast have to both use a capturer configured for screencast, and set the screencast flag using SetSendParameters. Since the automatic connection via VideoSource and VideoRtpSender isn't involved in the unit tests. Note that using SetSendParameters to set the screencast flag doesn't make sense, since it's not per-stream. SetVideoSend would be more appropriate. That should be fixed if/when we drop VideoOptions from SetSendParameters. BUG=webrtc:5426 R=pbos@webrtc.org, perkj@webrtc.org, pthatcher@webrtc.org Committed: https://crrev.com/60653ba3cc58aecdff594a1f72cfaefbad9c407b Cr-Commit-Position: refs/heads/master@{#11837} ==========
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/60653ba3cc58aecdff594a1f72cfaefbad9c407b Cr-Commit-Position: refs/heads/master@{#11837} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
