Removing VCMCodecDataBase::Codec and VideoCodingModule::Codec.

Removing VCMCodecDataBase::Codec and VideoCodingModule::Codec.

This CL brings us one step closer to removing CodecDatabase and
GenericEncoder, by removing the static VCM::Codec(). Codec specific
methods are moved to video_encoder.cc (they already belonged to this
class) and getting default generic codec settings has been moved to a
test specific file.

This CL also makes video_encoder.h pass style guide and lint checks,
since these checks are triggered with the new video_encoder.cc file.

BUG=webrtc:8064
Review-Url: https://codereview.webrtc.org/2993923002
Cr-Commit-Position: refs/heads/master@{#19303}
Committed: https://chromium.googlesource.com/external/webrtc/+/351424e942323185c24e92516ecb2a4c454db9f0

[mflodman, 2017-08-08 15:13:54]

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

[commit-bot: I haz the power, 2017-08-08 15:13:59]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[mflodman, 2017-08-08 15:21:20]

mflodman@webrtc.org changed reviewers:
+ brandtr@webrtc.org

[mflodman, 2017-08-08 15:21:21]

Rasmus,
This is the first step in removing the codec database and generic encoder.

I moved the get default codec specific settings to video_encoder.cc, lets discus
if this is the right place for this. This forced other changes to remove lint
warnings, which I solved rather than suppressing lint checks in more files.

https://codereview.webrtc.org/2993923002/diff/1/webrtc/api/video_codecs/video...
File webrtc/api/video_codecs/video_encoder.cc (right):

https://codereview.webrtc.org/2993923002/diff/1/webrtc/api/video_codecs/video...
webrtc/api/video_codecs/video_encoder.cc:15: VideoCodecVP8
VideoEncoder::GetDefaultVp8Settings() {
Default settings are the same as previously in codec_database.cc.

https://codereview.webrtc.org/2993923002/diff/1/webrtc/api/video_codecs/video...
File webrtc/api/video_codecs/video_encoder.h (right):

https://codereview.webrtc.org/2993923002/diff/1/webrtc/api/video_codecs/video...
webrtc/api/video_codecs/video_encoder.h:44: explicit Result(Error error) :
error(error) {}
All changes in this header are to pass lint checks, which are now triggered with
the new cc-file.

https://codereview.webrtc.org/2993923002/diff/1/webrtc/common_video/include/v...
File webrtc/common_video/include/video_frame.h (right):

https://codereview.webrtc.org/2993923002/diff/1/webrtc/common_video/include/v...
webrtc/common_video/include/video_frame.h:33: EncodedImage();
And same here, video_encoder.cc triggered lint checks on this file and the ctor
definitions are moved to the cc-file.

https://codereview.webrtc.org/2993923002/diff/1/webrtc/modules/video_coding/g...
File webrtc/modules/video_coding/generic_encoder_unittest.cc (right):

https://codereview.webrtc.org/2993923002/diff/1/webrtc/modules/video_coding/g...
webrtc/modules/video_coding/generic_encoder_unittest.cc:35: return
Result(Result::OK);
Needed after making the 'Result' ctor explicit.

https://codereview.webrtc.org/2993923002/diff/1/webrtc/test/video_codec_setti...
File webrtc/test/video_codec_settings.h (right):

https://codereview.webrtc.org/2993923002/diff/1/webrtc/test/video_codec_setti...
webrtc/test/video_codec_settings.h:27: static void CodecSettings(VideoCodecType
codec_type, VideoCodec* settings) {
Renamed and moved from codec_database.cc, no functional changes.

[commit-bot: I haz the power, 2017-08-08 15:38:29]

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

[commit-bot: I haz the power, 2017-08-08 15:38:30]

Dry run: Try jobs failed on following builders:
  android_clang_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/android_clang_dbg/build...)
  android_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/android_dbg/builds/26321)

[mflodman, 2017-08-09 05:00:31]

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

[commit-bot: I haz the power, 2017-08-09 05:00:34]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-08-09 05:33:25]

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

[commit-bot: I haz the power, 2017-08-09 05:33:26]

Dry run: This issue passed the CQ dry run.

[mflodman, 2017-08-10 07:38:51]

Ping Rasmus

[brandtr, 2017-08-10 08:05:35]

lgtm, but please take a look at my comments :)

https://codereview.webrtc.org/2993923002/diff/1/webrtc/api/video_codecs/video...
File webrtc/api/video_codecs/video_encoder.cc (right):

https://codereview.webrtc.org/2993923002/diff/1/webrtc/api/video_codecs/video...
webrtc/api/video_codecs/video_encoder.cc:42: vp9_settings.flexibleMode = false;
Add newline, for consistency with GetDefaultVp8Settings.

https://codereview.webrtc.org/2993923002/diff/20001/webrtc/api/video_codecs/v...
File webrtc/api/video_codecs/video_encoder.cc (right):

https://codereview.webrtc.org/2993923002/diff/20001/webrtc/api/video_codecs/v...
webrtc/api/video_codecs/video_encoder.cc:15: VideoCodecVP8
VideoEncoder::GetDefaultVp8Settings() {
Re. you comment about the placement of these default codec settings getters: I
think it's fine to keep these functions here for now, but longer term it might
make sense to move them somewhere else. Maybe the encoder factory?

I guess it also depends on what our plan is for webrtc::VideoCodec. My feeling
is that pbos@ was working on replacing it with the EncoderSpecificSettings
class?

https://codereview.webrtc.org/2993923002/diff/20001/webrtc/api/video_codecs/v...
webrtc/api/video_codecs/video_encoder.cc:18: 
Should we explicitly set a default complexity as well? Here and in the VP9
version.

https://codereview.webrtc.org/2993923002/diff/20001/webrtc/api/video_codecs/v...
webrtc/api/video_codecs/video_encoder.cc:42: vp9_settings.flexibleMode = false;
nit: add newline to be symmetric with functions above and below.

https://codereview.webrtc.org/2993923002/diff/20001/webrtc/api/video_codecs/v...
File webrtc/api/video_codecs/video_encoder.h (right):

https://codereview.webrtc.org/2993923002/diff/20001/webrtc/api/video_codecs/v...
webrtc/api/video_codecs/video_encoder.h:159: virtual int32_t SetRates(uint32_t
bitrate, uint32_t framerate);
Does it make sense to make it RTC_DEPRECATED?

https://codereview.webrtc.org/2993923002/diff/20001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/codecs/test/videoprocessor_integrationtest.h
(right):

https://codereview.webrtc.org/2993923002/diff/20001/webrtc/modules/video_codi...
webrtc/modules/video_coding/codecs/test/videoprocessor_integrationtest.h:638:
webrtc::test::CodecSettings(codec_type, &config->codec_settings);
This class is already in the webrtc::test scope. Or did you add this to make it
clear that it's a helper function?

https://codereview.webrtc.org/2993923002/diff/20001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/codecs/test/videoprocessor_unittest.cc (right):

https://codereview.webrtc.org/2993923002/diff/20001/webrtc/modules/video_codi...
webrtc/modules/video_coding/codecs/test/videoprocessor_unittest.cc:49:
webrtc::test::CodecSettings(kVideoCodecVP8, &config_.codec_settings);
Here too.

https://codereview.webrtc.org/2993923002/diff/20001/webrtc/test/video_codec_s...
File webrtc/test/video_codec_settings.h (right):

https://codereview.webrtc.org/2993923002/diff/20001/webrtc/test/video_codec_s...
webrtc/test/video_codec_settings.h:78: settings->qpMax = 56;
This is unrelated to this CL, but does it really make sense to have qpMax = 56
here? I thought H.264 used a different QP scale than VP8/VP9.

[mflodman, 2017-08-10 08:16:29]

The CQ bit was checked by mflodman@webrtc.org

[mflodman, 2017-08-10 08:16:35]

The patchset sent to the CQ was uploaded after l-g-t-m from brandtr@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2993923002/#ps40001 (title:
"Review comments from brandtr.")

[commit-bot: I haz the power, 2017-08-10 08:16:42]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-08-10 08:19:09]

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

[commit-bot: I haz the power, 2017-08-10 08:19:10]

Try jobs failed on following builders:
  ios_arm64_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_dbg/builds/22615)

[mflodman, 2017-08-10 08:23:17]

The CQ bit was unchecked by mflodman@webrtc.org

[mflodman, 2017-08-10 08:23:56]

https://codereview.webrtc.org/2993923002/diff/20001/webrtc/api/video_codecs/v...
File webrtc/api/video_codecs/video_encoder.cc (right):

https://codereview.webrtc.org/2993923002/diff/20001/webrtc/api/video_codecs/v...
webrtc/api/video_codecs/video_encoder.cc:15: VideoCodecVP8
VideoEncoder::GetDefaultVp8Settings() {
On 2017/08/10 08:05:34, brandtr wrote:
> Re. you comment about the placement of these default codec settings getters: I
> think it's fine to keep these functions here for now, but longer term it might
> make sense to move them somewhere else. Maybe the encoder factory?
> 
> I guess it also depends on what our plan is for webrtc::VideoCodec. My feeling
> is that pbos@ was working on replacing it with the EncoderSpecificSettings
> class?

That's a good point and I agree the factory would be a good place in the future.
I'll move them here for now, but will look at this.

https://codereview.webrtc.org/2993923002/diff/20001/webrtc/api/video_codecs/v...
webrtc/api/video_codecs/video_encoder.cc:18: 
On 2017/08/10 08:05:35, brandtr wrote:
> Should we explicitly set a default complexity as well? Here and in the VP9
> version.

We probably should, but I don't really know what to put there. I'll go ahead and
submit this CL since it's "only" moving stuff and then follow up with you and
Åsa about this.

https://codereview.webrtc.org/2993923002/diff/20001/webrtc/api/video_codecs/v...
webrtc/api/video_codecs/video_encoder.cc:42: vp9_settings.flexibleMode = false;
On 2017/08/10 08:05:35, brandtr wrote:
> nit: add newline to be symmetric with functions above and below.

Done.

https://codereview.webrtc.org/2993923002/diff/20001/webrtc/api/video_codecs/v...
File webrtc/api/video_codecs/video_encoder.h (right):

https://codereview.webrtc.org/2993923002/diff/20001/webrtc/api/video_codecs/v...
webrtc/api/video_codecs/video_encoder.h:159: virtual int32_t SetRates(uint32_t
bitrate, uint32_t framerate);
On 2017/08/10 08:05:35, brandtr wrote:
> Does it make sense to make it RTC_DEPRECATED?

Yes, done.

https://codereview.webrtc.org/2993923002/diff/20001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/codecs/test/videoprocessor_integrationtest.h
(right):

https://codereview.webrtc.org/2993923002/diff/20001/webrtc/modules/video_codi...
webrtc/modules/video_coding/codecs/test/videoprocessor_integrationtest.h:638:
webrtc::test::CodecSettings(codec_type, &config->codec_settings);
On 2017/08/10 08:05:35, brandtr wrote:
> This class is already in the webrtc::test scope. Or did you add this to make
it
> clear that it's a helper function?

For clarity, but I can remove if you prefer that?

https://codereview.webrtc.org/2993923002/diff/20001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/codecs/test/videoprocessor_unittest.cc (right):

https://codereview.webrtc.org/2993923002/diff/20001/webrtc/modules/video_codi...
webrtc/modules/video_coding/codecs/test/videoprocessor_unittest.cc:49:
webrtc::test::CodecSettings(kVideoCodecVP8, &config_.codec_settings);
On 2017/08/10 08:05:35, brandtr wrote:
> Here too.

Acknowledged.

https://codereview.webrtc.org/2993923002/diff/20001/webrtc/test/video_codec_s...
File webrtc/test/video_codec_settings.h (right):

https://codereview.webrtc.org/2993923002/diff/20001/webrtc/test/video_codec_s...
webrtc/test/video_codec_settings.h:78: settings->qpMax = 56;
On 2017/08/10 08:05:35, brandtr wrote:
> This is unrelated to this CL, but does it really make sense to have qpMax = 56
> here? I thought H.264 used a different QP scale than VP8/VP9.

Good point, likely not.

[mflodman, 2017-08-10 08:24:05]

The CQ bit was checked by mflodman@webrtc.org

[mflodman, 2017-08-10 08:24:06]

The patchset sent to the CQ was uploaded after l-g-t-m from brandtr@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2993923002/#ps60001 (title:
"Rebase")

[commit-bot: I haz the power, 2017-08-10 08:24:10]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-08-10 08:27:00]

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

[commit-bot: I haz the power, 2017-08-10 08:27:02]

Try jobs failed on following builders:
  ios_arm64_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_dbg/builds/22616)

[mflodman, 2017-08-10 09:12:11]

The CQ bit was checked by mflodman@webrtc.org

[mflodman, 2017-08-10 09:12:12]

The patchset sent to the CQ was uploaded after l-g-t-m from brandtr@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2993923002/#ps80001 (title:
"Remove deprate - let sprang handle when method is removed")

[commit-bot: I haz the power, 2017-08-10 09:12:20]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-08-10 09:43:15]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1502356331663440,
"parent_rev": "71a62b9a19c8ac8446c96660f3096cfcadc6505b", "commit_rev":
"351424e942323185c24e92516ecb2a4c454db9f0"}

[commit-bot: I haz the power, 2017-08-10 09:43:22]

Description was changed from

==========
Removing VCMCodecDataBase::Codec and VideoCodingModule::Codec.

This CL brings us one step closer to removing CodecDatabase and
GenericEncoder, by removing the static VCM::Codec(). Codec specific
methods are moved to video_encoder.cc (they already belonged to this
class) and getting default generic codec settings has been moved to a
test specific file.

This CL also makes video_encoder.h pass style guide and lint checks,
since these checks are triggered with the new video_encoder.cc file.

BUG=webrtc:8064
==========

to

==========
Removing VCMCodecDataBase::Codec and VideoCodingModule::Codec.

This CL brings us one step closer to removing CodecDatabase and
GenericEncoder, by removing the static VCM::Codec(). Codec specific
methods are moved to video_encoder.cc (they already belonged to this
class) and getting default generic codec settings has been moved to a
test specific file.

This CL also makes video_encoder.h pass style guide and lint checks,
since these checks are triggered with the new video_encoder.cc file.

BUG=webrtc:8064

Review-Url: https://codereview.webrtc.org/2993923002
Cr-Commit-Position: refs/heads/master@{#19303}
Committed:
https://chromium.googlesource.com/external/webrtc/+/351424e942323185c24e92516...
==========

[commit-bot: I haz the power, 2017-08-10 09:43:24]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/external/webrtc/+/351424e942323185c24e92516...