Delete unused members from VideoOptions

Delete unused members from VideoOptions

including options related to experimental constraints which are
recognized but never applied.

BUG=webrtc:5426
Committed: https://crrev.com/b163c3f1ba6d1934897ba077b1c2e3ae356c3dca
Cr-Commit-Position: refs/heads/master@{#11424}

[nisse-webrtc, 2016-01-27 08:28:12]

nisse@webrtc.org changed reviewers:
+ pbos@webrtc.org, perkj@webrtc.org

[nisse-webrtc, 2016-01-27 08:28:13]

This is an initial cleanup of unused VideoOptions. 

Intention is to keep only options which apply to an individual video stream, and
move construction time WebRtcVideoChannel2 configuration elsewhere,
e.g.,cpu_overuse_detection and disable_prerenderer_smoothing.  

The initial patch set keeps unsignalled_recv_stream_limit. It looks unused,
except that there are some test cases that set the option or check for its
presence. I think it should be deleted too, do you agree?

[nisse-webrtc, 2016-01-27 08:28:26]

The CQ bit was checked by nisse@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2016-01-27 08:28:29]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1642513002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1642513002/1

[commit-bot: I haz the power, 2016-01-27 09:45:35]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-01-27 09:45:35]

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...)

[hta-webrtc, 2016-01-27 10:21:40]

Description was changed from

==========
Delete unused members from VideoOptions

including options related to experimental constraints which are
recognized but never applied.

BUG=webrtc:5426
==========

to

==========
Delete unused members from VideoOptions

including options related to experimental constraints which are
recognized but never applied.

BUG=webrtc:5426
==========

[pbos-webrtc, 2016-01-27 11:41:31]

lgtm

https://codereview.webrtc.org/1642513002/diff/40001/talk/media/base/mediachan...
File talk/media/base/mediachannel.h (right):

https://codereview.webrtc.org/1642513002/diff/40001/talk/media/base/mediachan...
talk/media/base/mediachannel.h:304: // TODO(nisse): Document unit, there's an
unexplained "* 1000" where
Rename _kbps instead.

[pbos-webrtc, 2016-01-27 11:42:26]

https://codereview.webrtc.org/1642513002/diff/40001/talk/media/base/videoengi...
File talk/media/base/videoengine_unittest.h (left):

https://codereview.webrtc.org/1642513002/diff/40001/talk/media/base/videoengi...
talk/media/base/videoengine_unittest.h:1317: // Tests that we can send and
receive frames with early receive.
Actually, is this test invalid? Just removing the option it should still work
right?

[nisse-webrtc, 2016-01-27 12:59:05]

https://codereview.webrtc.org/1642513002/diff/40001/talk/media/base/mediachan...
File talk/media/base/mediachannel.h (right):

https://codereview.webrtc.org/1642513002/diff/40001/talk/media/base/mediachan...
talk/media/base/mediachannel.h:304: // TODO(nisse): Document unit, there's an
unexplained "* 1000" where
On 2016/01/27 11:41:30, pbos-webrtc wrote:
> Rename _kbps instead.

Done.

https://codereview.webrtc.org/1642513002/diff/40001/talk/media/base/videoengi...
File talk/media/base/videoengine_unittest.h (left):

https://codereview.webrtc.org/1642513002/diff/40001/talk/media/base/videoengi...
talk/media/base/videoengine_unittest.h:1317: // Tests that we can send and
receive frames with early receive.
On 2016/01/27 11:42:26, pbos-webrtc wrote:
> Actually, is this test invalid? Just removing the option it should still work
> right?

I have no idea if the test works or not, I don't think it's ever run.

Looking at how the other tests are hooked in, there should be a 

  TEST_F(...,  TwoStreamsSendAndUnsignalledRecv)

in webrtcvideoengine2_unittest.cc to make the test actually run, but there isn't
any such declaration nor any other reference to this method.

[pbos-webrtc, 2016-01-27 13:40:41]

On 2016/01/27 12:59:05, nisse-webrtc wrote:
>
https://codereview.webrtc.org/1642513002/diff/40001/talk/media/base/mediachan...
> File talk/media/base/mediachannel.h (right):
> 
>
https://codereview.webrtc.org/1642513002/diff/40001/talk/media/base/mediachan...
> talk/media/base/mediachannel.h:304: // TODO(nisse): Document unit, there's an
> unexplained "* 1000" where
> On 2016/01/27 11:41:30, pbos-webrtc wrote:
> > Rename _kbps instead.
> 
> Done.
> 
>
https://codereview.webrtc.org/1642513002/diff/40001/talk/media/base/videoengi...
> File talk/media/base/videoengine_unittest.h (left):
> 
>
https://codereview.webrtc.org/1642513002/diff/40001/talk/media/base/videoengi...
> talk/media/base/videoengine_unittest.h:1317: // Tests that we can send and
> receive frames with early receive.
> On 2016/01/27 11:42:26, pbos-webrtc wrote:
> > Actually, is this test invalid? Just removing the option it should still
work
> > right?
> 
> I have no idea if the test works or not, I don't think it's ever run.
> 
> Looking at how the other tests are hooked in, there should be a 
> 
>   TEST_F(...,  TwoStreamsSendAndUnsignalledRecv)
> 
> in webrtcvideoengine2_unittest.cc to make the test actually run, but there
isn't
> any such declaration nor any other reference to this method.

Looks like we had some other coverage for unsignalled, if that's the case then
feel free to drop this, and lgtm

[perkj_webrtc, 2016-01-28 10:25:20]

lgtm - but I think you should pthatchers approval. I don't know the history of
these unused options.

[nisse-webrtc, 2016-01-28 10:32:29]

nisse@webrtc.org changed reviewers:
+ pthatcher@webrtc.org

[nisse-webrtc, 2016-01-28 10:32:29]

@pthatcher, do you know the history of the constraints which are deleted here
(list in mediaconstraintsinterface.cc)?

They're all have a "goog"-prefix, so I guess they are/were experimental, but
they don't have any effect if set.

Regards,
/Niels

[pthatcher1, 2016-01-28 18:27:09]

They all look like things that were used once, but are no longer used.  Thanks
for cleaning them up.

My only suggestions for improvement are to make the comments on the remaining
ones more accurate and useful.

https://codereview.webrtc.org/1642513002/diff/60001/talk/media/base/mediachan...
File talk/media/base/mediachannel.h (right):

https://codereview.webrtc.org/1642513002/diff/60001/talk/media/base/mediachan...
talk/media/base/mediachannel.h:279: // codec options.
I'd phrase this as "comes from a getUserMedia constraint called
'googNoiseReduction', and WebRtcVideoEngine2 passes..."

"... by default, noise reduction is disabled ..."

https://codereview.webrtc.org/1642513002/diff/60001/talk/media/base/mediachan...
talk/media/base/mediachannel.h:284: // constraints. Checked in
WebRtcVideoChannel2::OnLoadUpdate, where
I'd phrase this as "comes from a PeerConnection constraint called
'googCpuOveruseDetection' and is checked in ...."

https://codereview.webrtc.org/1642513002/diff/60001/talk/media/base/mediachan...
talk/media/base/mediachannel.h:289: // from the MediaContentDescription. Used
only by conference mode
I'd phrase this as "comes from the remote description's SDP line
'a=x-google-flag:conference', and copied over by
VideoChannel::SetRemoteContent_w, and ultimately used by ..."

"... by default, the special screencasting behavior is not enabled ..."

https://codereview.webrtc.org/1642513002/diff/60001/talk/media/base/mediachan...
talk/media/base/mediachannel.h:294: // sets this from the constraints.
WebRtcVideoChannel2::SetOptions
I'd phrase this as "comes from PeerConnection constraint 'googDscp' and ..."

"... by default, rtc::DSCP_DEFAULT == DSCP_CS0 == 0 is used;  with dscp set,
DSCP_AF41 = 34 is used"

https://codereview.webrtc.org/1642513002/diff/60001/talk/media/base/mediachan...
talk/media/base/mediachannel.h:299: // WebRtcSession sets this from the
constraints. WebRtcVideoChannel2
I'd phrase this as "comes from PeerConnection constraint
'googSuspendBelowMinBitrate' and ..."

https://codereview.webrtc.org/1642513002/diff/60001/talk/media/base/mediachan...
talk/media/base/mediachannel.h:303: // this from the constraints. Copied to the
encoder config by
I'd phrase this as "... comes from PeerConnection constraint
'googScreencastMinBitrate' ..."

https://codereview.webrtc.org/1642513002/diff/60001/talk/media/base/mediachan...
talk/media/base/mediachannel.h:313: // WebRtcSession sets this from the
RTCConfiguration.
I'd phrase this as "comes from PeerConnection's RtcConfiguration, but is
currently only set by the command line flag 'disable-rtc-smoothness-algorithm'
..."

[pthatcher1, 2016-01-28 18:38:40]

On 2016/01/28 18:27:09, pthatcher1 wrote:
> They all look like things that were used once, but are no longer used.  Thanks
> for cleaning them up.
> 
> My only suggestions for improvement are to make the comments on the remaining
> ones more accurate and useful.
> 
>
https://codereview.webrtc.org/1642513002/diff/60001/talk/media/base/mediachan...
> File talk/media/base/mediachannel.h (right):
> 
>
https://codereview.webrtc.org/1642513002/diff/60001/talk/media/base/mediachan...
> talk/media/base/mediachannel.h:279: // codec options.
> I'd phrase this as "comes from a getUserMedia constraint called
> 'googNoiseReduction', and WebRtcVideoEngine2 passes..."
> 
> "... by default, noise reduction is disabled ..."
> 
>
https://codereview.webrtc.org/1642513002/diff/60001/talk/media/base/mediachan...
> talk/media/base/mediachannel.h:284: // constraints. Checked in
> WebRtcVideoChannel2::OnLoadUpdate, where
> I'd phrase this as "comes from a PeerConnection constraint called
> 'googCpuOveruseDetection' and is checked in ...."
> 
>
https://codereview.webrtc.org/1642513002/diff/60001/talk/media/base/mediachan...
> talk/media/base/mediachannel.h:289: // from the MediaContentDescription. Used
> only by conference mode
> I'd phrase this as "comes from the remote description's SDP line
> 'a=x-google-flag:conference', and copied over by
> VideoChannel::SetRemoteContent_w, and ultimately used by ..."
> 
> "... by default, the special screencasting behavior is not enabled ..."
> 
>
https://codereview.webrtc.org/1642513002/diff/60001/talk/media/base/mediachan...
> talk/media/base/mediachannel.h:294: // sets this from the constraints.
> WebRtcVideoChannel2::SetOptions
> I'd phrase this as "comes from PeerConnection constraint 'googDscp' and ..."
> 
> "... by default, rtc::DSCP_DEFAULT == DSCP_CS0 == 0 is used;  with dscp set,
> DSCP_AF41 = 34 is used"
> 
>
https://codereview.webrtc.org/1642513002/diff/60001/talk/media/base/mediachan...
> talk/media/base/mediachannel.h:299: // WebRtcSession sets this from the
> constraints. WebRtcVideoChannel2
> I'd phrase this as "comes from PeerConnection constraint
> 'googSuspendBelowMinBitrate' and ..."
> 
>
https://codereview.webrtc.org/1642513002/diff/60001/talk/media/base/mediachan...
> talk/media/base/mediachannel.h:303: // this from the constraints. Copied to
the
> encoder config by
> I'd phrase this as "... comes from PeerConnection constraint
> 'googScreencastMinBitrate' ..."
> 
>
https://codereview.webrtc.org/1642513002/diff/60001/talk/media/base/mediachan...
> talk/media/base/mediachannel.h:313: // WebRtcSession sets this from the
> RTCConfiguration.
> I'd phrase this as "comes from PeerConnection's RtcConfiguration, but is
> currently only set by the command line flag 'disable-rtc-smoothness-algorithm'
> ..."

lgtm, because timezones

[nisse-webrtc, 2016-01-29 08:14:01]

The CQ bit was checked by nisse@webrtc.org

[nisse-webrtc, 2016-01-29 08:14:02]

The patchset sent to the CQ was uploaded after l-g-t-m from pbos@webrtc.org,
perkj@webrtc.org, pthatcher@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1642513002/#ps80001 (title:
"Suggested comment edits.")

[commit-bot: I haz the power, 2016-01-29 08:14:05]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1642513002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1642513002/80001

[commit-bot: I haz the power, 2016-01-29 09:14:43]

Description was changed from

==========
Delete unused members from VideoOptions

including options related to experimental constraints which are
recognized but never applied.

BUG=webrtc:5426
==========

to

==========
Delete unused members from VideoOptions

including options related to experimental constraints which are
recognized but never applied.

BUG=webrtc:5426
==========

[commit-bot: I haz the power, 2016-01-29 09:14:43]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-01-29 09:14:52]

Description was changed from

==========
Delete unused members from VideoOptions

including options related to experimental constraints which are
recognized but never applied.

BUG=webrtc:5426
==========

to

==========
Delete unused members from VideoOptions

including options related to experimental constraints which are
recognized but never applied.

BUG=webrtc:5426

Committed: https://crrev.com/b163c3f1ba6d1934897ba077b1c2e3ae356c3dca
Cr-Commit-Position: refs/heads/master@{#11424}
==========

[commit-bot: I haz the power, 2016-01-29 09:14:53]

Patchset 5 (id:??) landed as
https://crrev.com/b163c3f1ba6d1934897ba077b1c2e3ae356c3dca
Cr-Commit-Position: refs/heads/master@{#11424}