Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2710493008:
Recreate WebrtcVideoSendStream if screen content setting is changed. (Closed)
|
Created:
3 years, 10 months ago by sprang_webrtc Modified:
3 years, 9 months ago CC:
webrtc-reviews_webrtc.org, video-team_agora.io, zhengzhonghou_agora.io, stefan-webrtc, tterriberry_mozilla.com, the sun, mflodman Target Ref:
refs/heads/master Project:
webrtc Visibility:
Public. |
DescriptionRecreate WebrtcVideoSendStream if screen content setting is changed.
This avoids the situation where an encoder, not supporting certain
screen content settings, is created for a config where screencast is
off, and later ReconfigureEncoder() is called updating the configuration
but not the encoder instance, causing an inconsistency in the encoder's
InitEncode() call.
TBR=pthatcher@webrtc.org
BUG=webrtc:4172
Review-Url: https://codereview.webrtc.org/2710493008
Cr-Commit-Position: refs/heads/master@{#16921}
Committed: https://chromium.googlesource.com/external/webrtc/+/f24a06406a5e0863fee82d6470bb655d3b4ac2af
Patch Set 1 #Patch Set 2 : Removed diff not intended to be part of this cl #
Total comments: 4
Patch Set 3 : updated tests, SetCodec() instead of Recreate... #Patch Set 4 : Fix uses of incorrect stream #
Total comments: 28
Patch Set 5 : Updated tests #
Total comments: 4
Patch Set 6 : Comments, add forced codec allocation param #Patch Set 7 : Fixed race in test, formatting #
Total comments: 6
Patch Set 8 : readability #
Messages
Total messages: 57 (33 generated)
The CQ bit was checked by sprang@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/...
sprang@webrtc.org changed reviewers: + pthatcher@webrtc.org
pthatcher@webrtc.org changed reviewers: + deadbeef@webrtc.org
Unit test? Also, I think Taylor should review it. https://codereview.webrtc.org/2710493008/diff/20001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvideoengine2.cc (right): https://codereview.webrtc.org/2710493008/diff/20001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvideoengine2.cc:1618: // stream so that the correct encoder is created. This could use a little more detail about why it's necessary. https://codereview.webrtc.org/2710493008/diff/20001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvideoengine2.cc:1620: } else { Shouldn't this be "else if (parameters_.options != old_options)" ? Otherwise, we'd reconfigure even if the options didn't change.
I agree that a test would be good, assuming the current fake classes support such a test. https://codereview.webrtc.org/2710493008/diff/20001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvideoengine2.cc (right): https://codereview.webrtc.org/2710493008/diff/20001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvideoengine2.cc:1620: } else { On 2017/02/21 23:00:34, pthatcher1 wrote: > Shouldn't this be "else if (parameters_.options != old_options)" ? Otherwise, > we'd reconfigure even if the options didn't change. +1
The CQ bit was checked by sprang@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: Try jobs failed on following builders: linux_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_rel/builds/23427) linux_ubsan_vptr on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_ubsan_vptr/builds...) mac_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_rel/builds/23099)
Turns out this worked by accident for me. I actually needed to do SetCodec() rather than just Recreate... Added and updates unit tests. The previous behavior of not recreating streams when changing content type was actually incorrect, so I'll revert to the behavior prior to that. There's at least a bunch of stats which is sliced based on which content type the VideoSendStream is created with. I'm thinking we current suffer from cross-pollination between those data sets... ptal https://codereview.webrtc.org/2710493008/diff/20001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvideoengine2.cc (right): https://codereview.webrtc.org/2710493008/diff/20001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvideoengine2.cc:1620: } else { On 2017/02/21 23:00:34, pthatcher1 wrote: > Shouldn't this be "else if (parameters_.options != old_options)" ? Otherwise, > we'd reconfigure even if the options didn't change. Yes, I screwed up copy/paste from a chromium checkout where I was iterating and testing...
Turns out this worked by accident for me. I actually needed to do SetCodec() rather than just Recreate... Added and updates unit tests. The previous behavior of not recreating streams when changing content type was actually incorrect, so I'll revert to the behavior prior to that. There's at least a bunch of stats which is sliced based on which content type the VideoSendStream is created with. I'm thinking we current suffer from cross-pollination between those data sets... ptal https://codereview.webrtc.org/2710493008/diff/20001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvideoengine2.cc (right): https://codereview.webrtc.org/2710493008/diff/20001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvideoengine2.cc:1620: } else { On 2017/02/21 23:00:34, pthatcher1 wrote: > Shouldn't this be "else if (parameters_.options != old_options)" ? Otherwise, > we'd reconfigure even if the options didn't change. Yes, I screwed up copy/paste from a chromium checkout where I was iterating and testing...
The CQ bit was checked by sprang@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: Try jobs failed on following builders: android_arm64_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_arm64_rel/build...)
https://codereview.webrtc.org/2710493008/diff/60001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvideoengine2.cc (right): https://codereview.webrtc.org/2710493008/diff/60001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvideoengine2.cc:1617: parameters_.codec_settings) { This needs to test that all the other parameters_.options haven't changed, doesn't it? https://codereview.webrtc.org/2710493008/diff/60001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvideoengine2.cc:1621: allocated_encoder_.codec.params["recreate"] = "true"; What is this codec parameter? Is it already defined lower in the stack? It looks really hacky. https://codereview.webrtc.org/2710493008/diff/60001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvideoengine2_unittest.cc (right): https://codereview.webrtc.org/2710493008/diff/60001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvideoengine2_unittest.cc:909: TEST_F(WebRtcVideoEngine2Test, RecreatesEncoderOnContentTypeChange) { I think you need a test for when both the content type changes and something else from VideoOptions changes at the same time.
I'm actually not sure I fully understand the bug that's being fixed here. RequiresEncoderReset returns "true" if "mode" (screencast or realtime) is changing, so why is that not sufficient? https://cs.chromium.org/chromium/src/third_party/webrtc/modules/video_coding/... https://codereview.webrtc.org/2710493008/diff/60001/webrtc/media/engine/fakew... File webrtc/media/engine/fakewebrtcvideoengine.h (right): https://codereview.webrtc.org/2710493008/diff/60001/webrtc/media/engine/fakew... webrtc/media/engine/fakewebrtcvideoengine.h:205: } while (wait_time > 0 && created_video_encoder_event_.Wait(wait_time)); Why are start_offset_ms/wait_time needed? https://codereview.webrtc.org/2710493008/diff/60001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvideoengine2.cc (right): https://codereview.webrtc.org/2710493008/diff/60001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvideoengine2.cc:1617: parameters_.codec_settings) { On 2017/02/22 22:24:14, pthatcher1 wrote: > This needs to test that all the other parameters_.options haven't changed, > doesn't it? Why? A whole new webrtc::VideoEncoder will be created. https://codereview.webrtc.org/2710493008/diff/60001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvideoengine2.cc:1913: stream_->ReconfigureVideoEncoder(encoder_config.Copy()); If ReconfigureVideoEncoder doesn't support changing the screencast flag, should it DCHECK that the flag is unchanged? https://codereview.webrtc.org/2710493008/diff/60001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvideoengine2_unittest.cc (right): https://codereview.webrtc.org/2710493008/diff/60001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvideoengine2_unittest.cc:932: ASSERT_TRUE(encoder_factory.WaitForCreatedVideoEncoders(1)); Should this be checking that the created encoder and/or send stream has the right content type applied? https://codereview.webrtc.org/2710493008/diff/60001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvideoengine2_unittest.cc:1743: TEST_F(WebRtcVideoChannel2Test, RecreateStreamForScreencast) { It seems odd to make this test the opposite of what it originally was intended to test. Is there any value to having it now? Also, from the comments on the original CL: pbos: I still think you want to use [ReconfigureVideoEncoder] for screenshare being turned on or not. SetCodecAndApplyOptions is significantly more expensive (causes a recreate of the stream). https://codereview.webrtc.org/2710493008/diff/60001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvideoengine2_unittest.cc:2238: EXPECT_EQ(1, fake_call_->GetNumCreatedSendStreams()); This expectation doesn't seem necessary for this test https://codereview.webrtc.org/2710493008/diff/60001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvideoengine2_unittest.cc:2262: EXPECT_EQ(2, fake_call_->GetNumCreatedSendStreams()); Same here https://codereview.webrtc.org/2710493008/diff/60001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvideoengine2_unittest.cc:4254: // Fetch the new stream created for the screenshare content. |screenshare| can be false, so this comment may be misleading.
The CQ bit was checked by sprang@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/...
> I'm actually not sure I fully understand the bug that's being fixed here. > RequiresEncoderReset returns "true" if "mode" (screencast or realtime) is > changing, so why is that not sufficient? With an external encoder, that will just call Release() and then InitEncode() on that instance. What I want is to force a new instance to be created by factory. I don't think it actually recreates the VideoSendStream either, so stats would still be an issue. My primary use case is for simulcast screenshare, where we need to switch from using a plain vp8_impl to using a simulcast encoding adapter. https://codereview.webrtc.org/2710493008/diff/60001/webrtc/media/engine/fakew... File webrtc/media/engine/fakewebrtcvideoengine.h (right): https://codereview.webrtc.org/2710493008/diff/60001/webrtc/media/engine/fakew... webrtc/media/engine/fakewebrtcvideoengine.h:205: } while (wait_time > 0 && created_video_encoder_event_.Wait(wait_time)); On 2017/02/23 00:46:49, Taylor Brandstetter wrote: > Why are start_offset_ms/wait_time needed? It was racy, so if |num_encoders| reached X before a call to WaitForCreatedVideoEncoders(X), it would block for kEventTimeoutMs and return false. This update is also a little stricter and wait for a total of kEventTimeoutMs, instead of resetting the timer for each update of the count until it reaches the desired value. Turns out this was actually needed for my test though, so if you feel it's unnecessary I can remove it. https://codereview.webrtc.org/2710493008/diff/60001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvideoengine2.cc (right): https://codereview.webrtc.org/2710493008/diff/60001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvideoengine2.cc:1621: allocated_encoder_.codec.params["recreate"] = "true"; On 2017/02/22 22:24:14, pthatcher1 wrote: > What is this codec parameter? Is it already defined lower in the stack? It > looks really hacky. It is kinda hacky, and is workaround just to avoid reuse in WebRtcVideoSendStream::CreateVideoEncoder(). I guess the alternative is to add a screenshare flag to the AllocatedEncoder. Or I can perhaps do allocated_encoder_.codec.params["is_screenshare"] = parameters_.options.is_screencast.value_or(false) ? "true" : "false"; https://codereview.webrtc.org/2710493008/diff/60001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvideoengine2.cc:1913: stream_->ReconfigureVideoEncoder(encoder_config.Copy()); On 2017/02/23 00:46:49, Taylor Brandstetter wrote: > If ReconfigureVideoEncoder doesn't support changing the screencast flag, should > it DCHECK that the flag is unchanged? Good idea. I'll add it to webrtc::VideoSendStream itself, since it's the one counting on it. https://codereview.webrtc.org/2710493008/diff/60001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvideoengine2_unittest.cc (right): https://codereview.webrtc.org/2710493008/diff/60001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvideoengine2_unittest.cc:909: TEST_F(WebRtcVideoEngine2Test, RecreatesEncoderOnContentTypeChange) { On 2017/02/22 22:24:14, pthatcher1 wrote: > I think you need a test for when both the content type changes and something > else from VideoOptions changes at the same time. I added a change to another option just to verify i still recreates the stream. However, I spent several hours today trying make a test where we can distinguish between recreating the stream and actually changing the codec. This seems impossible as we can't directly detect creation of the SimulcastEncoderAdapter everything gets wrapped in. The FakeEncoder instances (which we can observe) are called from InitEncode() in the adapter, and that is actually called on recreating the stream... https://codereview.webrtc.org/2710493008/diff/60001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvideoengine2_unittest.cc:932: ASSERT_TRUE(encoder_factory.WaitForCreatedVideoEncoders(1)); On 2017/02/23 00:46:49, Taylor Brandstetter wrote: > Should this be checking that the created encoder and/or send stream has the > right content type applied? Done. https://codereview.webrtc.org/2710493008/diff/60001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvideoengine2_unittest.cc:1743: TEST_F(WebRtcVideoChannel2Test, RecreateStreamForScreencast) { On 2017/02/23 00:46:49, Taylor Brandstetter wrote: > It seems odd to make this test the opposite of what it originally was intended > to test. Is there any value to having it now? Maybe not, it's covered by RecreatesEncoderOnContentTypeChange() now. > Also, from the comments on the original CL: > > pbos: I still think you want to use [ReconfigureVideoEncoder] for screenshare > being turned on or not. SetCodecAndApplyOptions is significantly more expensive > (causes a recreate of the stream). Yes, that is a bit unfortunate, but something that will be needed. We need to recreate the stream both for the stats thing and also to make sure the correct encoder is created. For screenshare simulcast we currently need a simulcast encoder adapter, or we'll get crashes. https://codereview.webrtc.org/2710493008/diff/60001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvideoengine2_unittest.cc:2238: EXPECT_EQ(1, fake_call_->GetNumCreatedSendStreams()); On 2017/02/23 00:46:49, Taylor Brandstetter wrote: > This expectation doesn't seem necessary for this test This one, no. It's already assert just above. https://codereview.webrtc.org/2710493008/diff/60001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvideoengine2_unittest.cc:2262: EXPECT_EQ(2, fake_call_->GetNumCreatedSendStreams()); On 2017/02/23 00:46:49, Taylor Brandstetter wrote: > Same here I actually changed this to an assert, since we will need to do send_stream = fake_call_->GetVideoSendStreams().front(); to make sure |send_stream| points the last stream create, probably the only instance that is actually valid. https://codereview.webrtc.org/2710493008/diff/60001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvideoengine2_unittest.cc:4254: // Fetch the new stream created for the screenshare content. On 2017/02/23 00:46:49, Taylor Brandstetter wrote: > |screenshare| can be false, so this comment may be misleading. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_ubsan on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_ubsan/builds/9955) linux_ubsan_vptr on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_ubsan_vptr/builds...)
https://codereview.webrtc.org/2710493008/diff/60001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvideoengine2.cc (right): https://codereview.webrtc.org/2710493008/diff/60001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvideoengine2.cc:1617: parameters_.codec_settings) { On 2017/02/23 00:46:49, Taylor Brandstetter wrote: > On 2017/02/22 22:24:14, pthatcher1 wrote: > > This needs to test that all the other parameters_.options haven't changed, > > doesn't it? > > Why? A whole new webrtc::VideoEncoder will be created. First, it's not clear from the code that this case is a superset of the second case. Second, because if someone adds something else to the second case (say... do something funny when max bitrate changes), then it will surprisingly not happen when both content type and max bitrate change. In other words, there's an implicit superset behavior that isn't clear that could easily be broken by adding to the second case and making the first case no longer a superset of behavior. https://codereview.webrtc.org/2710493008/diff/60001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvideoengine2.cc:1621: allocated_encoder_.codec.params["recreate"] = "true"; On 2017/02/23 15:10:14, språng wrote: > On 2017/02/22 22:24:14, pthatcher1 wrote: > > What is this codec parameter? Is it already defined lower in the stack? It > > looks really hacky. > > It is kinda hacky, and is workaround just to avoid reuse in > WebRtcVideoSendStream::CreateVideoEncoder(). > > I guess the alternative is to add a screenshare flag to the AllocatedEncoder. > Or I can perhaps do > allocated_encoder_.codec.params["is_screenshare"] = > parameters_.options.is_screencast.value_or(false) ? "true" : "false"; It seems like we should just have a small refactoring to avoid needing this hack; bool screencast_changed = ...; if (screencast_changed && parameters_.codec_settings) { CreateAndSetEncoder(*parameters_.codec_settings); } else { ReconfigureEncoder(); } void CreateAndSetEncoderIfCodecChanged(const VideoCodecSettings& codec_settings) { // Moved from old CreateVideoEncoder if (codec_settings.codec == allocated_encoder_.codec && allocated_encoder_.encoder != nullptr) { } CreateAndSetEncoderIfCodecChanged(const VideoCodecSettings& codec_settings); } void CreateAndSetEncoder(const VideoCodecSettings& codec_settings) { // Same as old SetCodec, except with new CreateVideoEncoder which always creates. } AllocatedEncoder CreateVideoEncoder(const VideoCodec& codec) { // Same as old CreateVideoEncoder, except always creates (doesn't do the check for allocated_encoder_). }
Looks good. Would just be nice if the `params["recreate"]` thing could be avoided. https://codereview.webrtc.org/2710493008/diff/60001/webrtc/media/engine/fakew... File webrtc/media/engine/fakewebrtcvideoengine.h (right): https://codereview.webrtc.org/2710493008/diff/60001/webrtc/media/engine/fakew... webrtc/media/engine/fakewebrtcvideoengine.h:205: } while (wait_time > 0 && created_video_encoder_event_.Wait(wait_time)); On 2017/02/23 15:10:14, språng wrote: > On 2017/02/23 00:46:49, Taylor Brandstetter wrote: > > Why are start_offset_ms/wait_time needed? > > It was racy, so if |num_encoders| reached X before a call to > WaitForCreatedVideoEncoders(X), it would block for kEventTimeoutMs and return > false. Ah, I see. I think just changing it to a "do while" is sufficient though; the stricter waiting seems unnecessary. I don't feel strongly about it though. https://codereview.webrtc.org/2710493008/diff/60001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvideoengine2.cc (right): https://codereview.webrtc.org/2710493008/diff/60001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvideoengine2.cc:1621: allocated_encoder_.codec.params["recreate"] = "true"; On 2017/02/23 21:05:33, pthatcher1 wrote: > On 2017/02/23 15:10:14, språng wrote: > > On 2017/02/22 22:24:14, pthatcher1 wrote: > > > What is this codec parameter? Is it already defined lower in the stack? It > > > looks really hacky. > > > > It is kinda hacky, and is workaround just to avoid reuse in > > WebRtcVideoSendStream::CreateVideoEncoder(). > > > > I guess the alternative is to add a screenshare flag to the AllocatedEncoder. > > Or I can perhaps do > > allocated_encoder_.codec.params["is_screenshare"] = > > parameters_.options.is_screencast.value_or(false) ? "true" : "false"; > > It seems like we should just have a small refactoring to avoid needing this > hack; > > bool screencast_changed = ...; > if (screencast_changed && parameters_.codec_settings) { > CreateAndSetEncoder(*parameters_.codec_settings); > } else { > ReconfigureEncoder(); > } > > void CreateAndSetEncoderIfCodecChanged(const VideoCodecSettings& codec_settings) > { > // Moved from old CreateVideoEncoder > if (codec_settings.codec == allocated_encoder_.codec && > allocated_encoder_.encoder != nullptr) { > } > CreateAndSetEncoderIfCodecChanged(const VideoCodecSettings& codec_settings); > } > > void CreateAndSetEncoder(const VideoCodecSettings& codec_settings) { > // Same as old SetCodec, except with new CreateVideoEncoder which always > creates. > } > > AllocatedEncoder CreateVideoEncoder(const VideoCodec& codec) { > // Same as old CreateVideoEncoder, except always creates (doesn't do the check > for allocated_encoder_). > } > > > Or, just make CreateVideoEncoder a bit smarter. The issue is that it returns early if the codec isn't changing, even though later it uses things other than the codec: https://cs.chromium.org/chromium/src/third_party/webrtc/media/engine/webrtcvi... https://codereview.webrtc.org/2710493008/diff/60001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvideoengine2_unittest.cc (right): https://codereview.webrtc.org/2710493008/diff/60001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvideoengine2_unittest.cc:1743: TEST_F(WebRtcVideoChannel2Test, RecreateStreamForScreencast) { On 2017/02/23 15:10:14, språng wrote: > On 2017/02/23 00:46:49, Taylor Brandstetter wrote: > > It seems odd to make this test the opposite of what it originally was intended > > to test. Is there any value to having it now? > > Maybe not, it's covered by RecreatesEncoderOnContentTypeChange() now. > > > Also, from the comments on the original CL: > > > > pbos: I still think you want to use [ReconfigureVideoEncoder] for screenshare > > being turned on or not. SetCodecAndApplyOptions is significantly more > expensive > > (causes a recreate of the stream). > > Yes, that is a bit unfortunate, but something that will be needed. We need to > recreate the stream both for the stats thing and also to make sure the correct > encoder is created. For screenshare simulcast we currently need a simulcast > encoder adapter, or we'll get crashes. Acknowledged. https://codereview.webrtc.org/2710493008/diff/60001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvideoengine2_unittest.cc:2262: EXPECT_EQ(2, fake_call_->GetNumCreatedSendStreams()); On 2017/02/23 15:10:14, språng wrote: > On 2017/02/23 00:46:49, Taylor Brandstetter wrote: > > Same here > > I actually changed this to an assert, since we will need to do > send_stream = fake_call_->GetVideoSendStreams().front(); > to make sure |send_stream| points the last stream create, probably the only > instance that is actually valid. Acknowledged. https://codereview.webrtc.org/2710493008/diff/80001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvideoengine2_unittest.cc (right): https://codereview.webrtc.org/2710493008/diff/80001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvideoengine2_unittest.cc:18: //#include "webrtc/base/gunit.h" Left over from debugging? https://codereview.webrtc.org/2710493008/diff/80001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvideoengine2_unittest.cc:961: // a non is_screencast option just verify it doesn not affect recreation. nit: "doesn not" typo
The CQ bit was checked by sprang@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/2710493008/diff/60001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvideoengine2.cc (right): https://codereview.webrtc.org/2710493008/diff/60001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvideoengine2.cc:1617: parameters_.codec_settings) { On 2017/02/23 21:05:33, pthatcher1 wrote: > On 2017/02/23 00:46:49, Taylor Brandstetter wrote: > > On 2017/02/22 22:24:14, pthatcher1 wrote: > > > This needs to test that all the other parameters_.options haven't changed, > > > doesn't it? > > > > Why? A whole new webrtc::VideoEncoder will be created. > > First, it's not clear from the code that this case is a superset of the second > case. > > Second, because if someone adds something else to the second case (say... do > something funny when max bitrate changes), then it will surprisingly not happen > when both content type and max bitrate change. > > In other words, there's an implicit superset behavior that isn't clear that > could easily be broken by adding to the second case and making the first case no > longer a superset of behavior. Agree that it's not entirely obvious that ReconfigureEncoder() is a subset of SetCodec(). At least partially. SetCodec() will recreate the streams (this seems clear and has test coverage), and that definitely is a subset of reconfiguring the encoder. That the way the configuration is updated in ReconfigureEncoder() is a subset of SetCodec() is however not obvious. It seems ReconfigureEncoder() (currently) comprises calls to * CreateVideoEncoderConfig(), which is called at the beginning of SetCodec() * ConfigureVideoEncoderSettings() which is called from RecreateWebRtcStream() which is called at the end of SetCodec(). Writing a test to verify that no other parameters are missed will be very difficult however. I propose simply calling ReconfigureEncoder() again if other parameters have changed. I guess this may incur a slight performance hit when switching content type and other options and the same time, but I think it's not critical. The encoder will do most of the initialization on InitEncode() which won't happen until the next frame anyway, so it may be that there won't even be an extra reconfig. https://codereview.webrtc.org/2710493008/diff/60001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvideoengine2.cc:1621: allocated_encoder_.codec.params["recreate"] = "true"; On 2017/02/24 00:13:55, Taylor Brandstetter wrote: > On 2017/02/23 21:05:33, pthatcher1 wrote: > > On 2017/02/23 15:10:14, språng wrote: > > > On 2017/02/22 22:24:14, pthatcher1 wrote: > > > > What is this codec parameter? Is it already defined lower in the stack? > It > > > > looks really hacky. > > > > > > It is kinda hacky, and is workaround just to avoid reuse in > > > WebRtcVideoSendStream::CreateVideoEncoder(). > > > > > > I guess the alternative is to add a screenshare flag to the > AllocatedEncoder. > > > Or I can perhaps do > > > allocated_encoder_.codec.params["is_screenshare"] = > > > parameters_.options.is_screencast.value_or(false) ? "true" : "false"; > > > > It seems like we should just have a small refactoring to avoid needing this > > hack; > > > > bool screencast_changed = ...; > > if (screencast_changed && parameters_.codec_settings) { > > CreateAndSetEncoder(*parameters_.codec_settings); > > } else { > > ReconfigureEncoder(); > > } > > > > void CreateAndSetEncoderIfCodecChanged(const VideoCodecSettings& > codec_settings) > > { > > // Moved from old CreateVideoEncoder > > if (codec_settings.codec == allocated_encoder_.codec && > > allocated_encoder_.encoder != nullptr) { > > } > > CreateAndSetEncoderIfCodecChanged(const VideoCodecSettings& codec_settings); > > } > > > > void CreateAndSetEncoder(const VideoCodecSettings& codec_settings) { > > // Same as old SetCodec, except with new CreateVideoEncoder which always > > creates. > > } > > > > AllocatedEncoder CreateVideoEncoder(const VideoCodec& codec) { > > // Same as old CreateVideoEncoder, except always creates (doesn't do the > check > > for allocated_encoder_). > > } > > > > > > > > Or, just make CreateVideoEncoder a bit smarter. The issue is that it returns > early if the codec isn't changing, even though later it uses things other than > the codec: > https://cs.chromium.org/chromium/src/third_party/webrtc/media/engine/webrtcvi... CreateVideoEncoder doesn't know about the change in parameters though, so in order to detect a change there, we need to add a is_screenshare member to AllocatedEncoder, which I guess is doable if you prefer that. Instead of adding the suggest new versions of SetCodec(), is just added a parameter to force an encoder allocation. wdyt? https://codereview.webrtc.org/2710493008/diff/80001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvideoengine2_unittest.cc (right): https://codereview.webrtc.org/2710493008/diff/80001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvideoengine2_unittest.cc:18: //#include "webrtc/base/gunit.h" On 2017/02/24 00:13:55, Taylor Brandstetter wrote: > Left over from debugging? Oops, yes. For some reason eclipse can't properly process the TEST_F macros if included from base/gunit.h so I temporarily changed it for easier development. Why do we have both of these anyway? https://codereview.webrtc.org/2710493008/diff/80001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvideoengine2_unittest.cc:961: // a non is_screencast option just verify it doesn not affect recreation. On 2017/02/24 00:13:55, Taylor Brandstetter wrote: > nit: "doesn not" typo Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_tsan2 on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_tsan2/builds/20251)
The CQ bit was checked by sprang@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.
Just needs little readability improvement. After that, I'll let Taylor be the final word. https://codereview.webrtc.org/2710493008/diff/120001/webrtc/media/engine/webr... File webrtc/media/engine/webrtcvideoengine2.cc (right): https://codereview.webrtc.org/2710493008/diff/120001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvideoengine2.cc:1591: SetCodec(*codec_settings, false); This would be much more clear as: bool force_encoder_allocation = false; SetCodec(*codec_settings, force_encoder_allocation); https://codereview.webrtc.org/2710493008/diff/120001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvideoengine2.cc:1620: SetCodec(*parameters_.codec_settings, true); And here: bool force_encoder_allocation = true; SetCodec(*codec_settings, force_encoder_allocation); https://codereview.webrtc.org/2710493008/diff/120001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvideoengine2.cc:1786: SetCodec(*parameters_.codec_settings, false); And these two: bool force_encoder_allocation = false; SetCodec(..., force_encoder_allocation);
https://codereview.webrtc.org/2710493008/diff/120001/webrtc/media/engine/webr... File webrtc/media/engine/webrtcvideoengine2.cc (right): https://codereview.webrtc.org/2710493008/diff/120001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvideoengine2.cc:1591: SetCodec(*codec_settings, false); On 2017/02/27 18:30:38, pthatcher1 wrote: > This would be much more clear as: > > bool force_encoder_allocation = false; > SetCodec(*codec_settings, force_encoder_allocation); Done. https://codereview.webrtc.org/2710493008/diff/120001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvideoengine2.cc:1620: SetCodec(*parameters_.codec_settings, true); On 2017/02/27 18:30:38, pthatcher1 wrote: > And here: > > bool force_encoder_allocation = true; > SetCodec(*codec_settings, force_encoder_allocation); Done. https://codereview.webrtc.org/2710493008/diff/120001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvideoengine2.cc:1786: SetCodec(*parameters_.codec_settings, false); On 2017/02/27 18:30:38, pthatcher1 wrote: > And these two: > > bool force_encoder_allocation = false; > SetCodec(..., force_encoder_allocation); Done.
lgtm
The CQ bit was checked by sprang@webrtc.org
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
Try jobs failed on following builders: presubmit on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/14213)
The CQ bit was checked by sprang@webrtc.org
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
Try jobs failed on following builders: presubmit on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/14220)
sprang@webrtc.org changed reviewers: + magjed@webrtc.org
Sounded like a had pthatcher@'s approval, but no official stamp yet. +magjed@, can you stamp so we get this landed quickly? Maybe take a pass over the webrtc/video stuff too in case that was missed.
ping pthatcher, any further comment?
Description was changed from ========== Recreate WebrtcVideoSendStream if screen content setting is changed. This avoids the situation where an encoder, not supporting certain screen content settings, is created for a config where screencast is off, and later ReconfigureEncoder() is called updating the configuration but not the encoder instance, causing an inconsistency in the encoder's InitEncode() call. BUG=webrtc:4172 ========== to ========== Recreate WebrtcVideoSendStream if screen content setting is changed. This avoids the situation where an encoder, not supporting certain screen content settings, is created for a config where screencast is off, and later ReconfigureEncoder() is called updating the configuration but not the encoder instance, causing an inconsistency in the encoder's InitEncode() call. TBR=pthatcher@webrtc.org BUG=webrtc:4172 ==========
Setting pthatcher@ as tbr, as per https://codereview.webrtc.org/2710493008/#msg36, so we can land this.
The CQ bit was checked by sprang@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": 140001, "attempt_start_ts": 1488315831107090,
"parent_rev": "dd36501341d89ecf05431263ac107178cd85b80a", "commit_rev":
"f24a06406a5e0863fee82d6470bb655d3b4ac2af"}
Message was sent while issue was closed.
Description was changed from ========== Recreate WebrtcVideoSendStream if screen content setting is changed. This avoids the situation where an encoder, not supporting certain screen content settings, is created for a config where screencast is off, and later ReconfigureEncoder() is called updating the configuration but not the encoder instance, causing an inconsistency in the encoder's InitEncode() call. TBR=pthatcher@webrtc.org BUG=webrtc:4172 ========== to ========== Recreate WebrtcVideoSendStream if screen content setting is changed. This avoids the situation where an encoder, not supporting certain screen content settings, is created for a config where screencast is off, and later ReconfigureEncoder() is called updating the configuration but not the encoder instance, causing an inconsistency in the encoder's InitEncode() call. TBR=pthatcher@webrtc.org BUG=webrtc:4172 Review-Url: https://codereview.webrtc.org/2710493008 Cr-Commit-Position: refs/heads/master@{#16921} Committed: https://chromium.googlesource.com/external/webrtc/+/f24a06406a5e0863fee82d647... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://chromium.googlesource.com/external/webrtc/+/f24a06406a5e0863fee82d647...
Message was sent while issue was closed.
On 2017/02/28 21:23:32, commit-bot: I haz the power wrote: > Committed patchset #8 (id:140001) as > https://chromium.googlesource.com/external/webrtc/+/f24a06406a5e0863fee82d647... I'm seeing errors on one of the trybots now. Could there be a race? https://build.chromium.org/p/tryserver.webrtc/builders/linux_msan/builds/1832... [ RUN ] WebRtcVideoEngine2Test.RecreatesEncoderOnContentTypeChange ... ../../webrtc/media/engine/webrtcvideoengine2_unittest.cc:949: Failure Value of: encoder_factory.encoders().back()->GetCodecSettings().mode Actual: 0 Expected: webrtc::kScreensharing Which is: 1 ... [ FAILED ] WebRtcVideoEngine2Test.RecreatesEncoderOnContentTypeChange (74 ms) |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
