Stop using hardcoded payload types for video codecs

Stop using hardcoded payload types for video codecs

This CL stops using hardcoded payload types for different video codecs
and will dynamically assign them payload types incrementally from 96 to
127 instead.

This CL:
 * Replaces 'std::vector<VideoCodec> DefaultVideoCodecList()' in
   webrtcvideoengine2.cc with an explicit WebRtcVideoEncoderFactory for
   internally supported software codecs instead. The purpose is to
   streamline the payload type assignment in webrtcvideoengine2.cc which
   will now have two encoder factories of the same
   WebRtcVideoEncoderFactory type; one internal and one external.
 * Removes webrtc::VideoEncoder::EncoderType and use cricket::VideoCodec
   instead.
 * Removes 'static VideoEncoder* Create(EncoderType codec_type)' and
   moves the create function to the internal encoder factory instead.
 * Removes video_encoder.cc. webrtc::VideoEncoder is now just an
   interface without any static functions.
 * The function GetSupportedCodecs in webrtcvideoengine2.cc unifies
   the internal and external codecs and assigns them payload types
   incrementally from 96 to 127.
 * Updates webrtcvideoengine2_unittest.cc and removes assumptions about
   what payload types will be used.

BUG=webrtc:6677, webrtc:6705
R=hta@webrtc.org, ossu@webrtc.org, stefan@webrtc.org

Committed: https://crrev.com/42043b95872b51321f508bf255d804ce3dff366b
Cr-Commit-Position: refs/heads/master@{#15135}

[magjed_webrtc, 2016-11-12 10:22:33]

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

[commit-bot: I haz the power, 2016-11-12 10:22:41]

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, 2016-11-12 10:23:50]

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

[commit-bot: I haz the power, 2016-11-12 10:23:50]

Dry run: Try jobs failed on following builders:
  ios32_sim_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios32_sim_dbg/builds/12432)
  ios64_sim_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_dbg/builds/12446)
  ios_arm64_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_dbg/builds/14787)
  ios_arm64_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_rel/builds/14660)
  ios_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_dbg/builds/20015)
  ios_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_rel/builds/18713)
  linux_libfuzzer_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_libfuzzer_rel/bui...)
  presubmit on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/10111)

[magjed_webrtc, 2016-11-12 15:30:22]

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

[commit-bot: I haz the power, 2016-11-12 15:30:27]

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, 2016-11-12 15:31:38]

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

[commit-bot: I haz the power, 2016-11-12 15:31:39]

Dry run: Try jobs failed on following builders:
  android_more_configs on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/android_more_configs/bu...)
  ios32_sim_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios32_sim_dbg/builds/12435)
  ios_arm64_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_dbg/builds/14790)
  ios_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_dbg/builds/20018)
  ios_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_rel/builds/18716)
  presubmit on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/10115)

[magjed_webrtc, 2016-11-13 14:00:03]

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

[commit-bot: I haz the power, 2016-11-13 14:00:12]

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, 2016-11-13 14:01:32]

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

[commit-bot: I haz the power, 2016-11-13 14:01:33]

Dry run: Try jobs failed on following builders:
  android_compile_x86_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/android_compile_x86_rel...)
  android_more_configs on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/android_more_configs/bu...)
  ios32_sim_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios32_sim_dbg/builds/12440)
  ios64_gyp_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios64_gyp_dbg/builds/2319)
  ios64_sim_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_dbg/builds/12454)
  ios_arm64_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_rel/builds/14668)
  ios_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_dbg/builds/20023)
  ios_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_rel/builds/18721)
  linux_libfuzzer_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_libfuzzer_rel/bui...)
  linux_more_configs on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_more_configs/buil...)
  mac_compile_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_compile_dbg/builds/...)
  mac_gyp_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_gyp_dbg/builds/2396)
  mac_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_rel/builds/20072)
  presubmit on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/10123)

[magjed_webrtc, 2016-11-13 15:25:35]

Description was changed from

==========
Internal encoder factory
==========

to

==========
Stop using hardcoded payload types for video codecs

This CL stops using hardcoded payload types for different video codecs
and will assign them payload types incrementally from 96 to 127 instead.

This CL:
 * Replaces 'std::vector<VideoCodec> DefaultVideoCodecList()' in
   webrtcvideoengine2.cc with an explicit WebRtcVideoEncoderFactory for
   internally supported software codecs instead. The purpose is to
   streamline the payload type assignment in webrtcvideoengine2.cc which
   will now have two encoder factories of the same
   WebRtcVideoEncoderFactory type; one internal and one external.
 * Removes webrtc::VideoEncoder::EncoderType and use cricket::VideoCodec
   instead.
 * Removes 'static VideoEncoder* Create(EncoderType codec_type)' and
   moves the create function to the internal encoder factory instead.
 * Removes video_encoder.cc. webrtc::VideoEncoder is now just an
   interface without any static functions.
 * The function GetSupportedCodecs in webrtcvideoengine2.cc unifies
   the internal and external codecs and assigns them payload types
   incrementally from 96 to 127.
 * Updates webrtcvideoengine2_unittest.cc and removes assumptions about
   what payload types will be used.

BUG=webrtc:6677,webrtc:6705
==========

[magjed_webrtc, 2016-11-13 15:25:46]

Patchset #3 (id:40001) has been deleted

[magjed_webrtc, 2016-11-13 15:25:57]

Patchset #2 (id:20001) has been deleted

[magjed_webrtc, 2016-11-13 15:26:05]

Patchset #1 (id:1) has been deleted

[magjed_webrtc, 2016-11-14 08:28:23]

Patchset #1 (id:50001) has been deleted

[magjed_webrtc, 2016-11-14 08:29:12]

magjed@webrtc.org changed reviewers:
+ hta@webrtc.org, stefan@webrtc.org

[magjed_webrtc, 2016-11-14 08:32:07]

magjed@webrtc.org changed reviewers:
+ ossu@webrtc.org

[magjed_webrtc, 2016-11-14 08:33:11]

Description was changed from

==========
Stop using hardcoded payload types for video codecs

This CL stops using hardcoded payload types for different video codecs
and will assign them payload types incrementally from 96 to 127 instead.

This CL:
 * Replaces 'std::vector<VideoCodec> DefaultVideoCodecList()' in
   webrtcvideoengine2.cc with an explicit WebRtcVideoEncoderFactory for
   internally supported software codecs instead. The purpose is to
   streamline the payload type assignment in webrtcvideoengine2.cc which
   will now have two encoder factories of the same
   WebRtcVideoEncoderFactory type; one internal and one external.
 * Removes webrtc::VideoEncoder::EncoderType and use cricket::VideoCodec
   instead.
 * Removes 'static VideoEncoder* Create(EncoderType codec_type)' and
   moves the create function to the internal encoder factory instead.
 * Removes video_encoder.cc. webrtc::VideoEncoder is now just an
   interface without any static functions.
 * The function GetSupportedCodecs in webrtcvideoengine2.cc unifies
   the internal and external codecs and assigns them payload types
   incrementally from 96 to 127.
 * Updates webrtcvideoengine2_unittest.cc and removes assumptions about
   what payload types will be used.

BUG=webrtc:6677,webrtc:6705
==========

to

==========
Stop using hardcoded payload types for video codecs

This CL stops using hardcoded payload types for different video codecs
and will dynamically assign them payload types incrementally from 96 to
127 instead.

This CL:
 * Replaces 'std::vector<VideoCodec> DefaultVideoCodecList()' in
   webrtcvideoengine2.cc with an explicit WebRtcVideoEncoderFactory for
   internally supported software codecs instead. The purpose is to
   streamline the payload type assignment in webrtcvideoengine2.cc which
   will now have two encoder factories of the same
   WebRtcVideoEncoderFactory type; one internal and one external.
 * Removes webrtc::VideoEncoder::EncoderType and use cricket::VideoCodec
   instead.
 * Removes 'static VideoEncoder* Create(EncoderType codec_type)' and
   moves the create function to the internal encoder factory instead.
 * Removes video_encoder.cc. webrtc::VideoEncoder is now just an
   interface without any static functions.
 * The function GetSupportedCodecs in webrtcvideoengine2.cc unifies
   the internal and external codecs and assigns them payload types
   incrementally from 96 to 127.
 * Updates webrtcvideoengine2_unittest.cc and removes assumptions about
   what payload types will be used.

BUG=webrtc:6677,webrtc:6705
==========

[magjed_webrtc, 2016-11-14 08:38:43]

Please take a look.
stefan: ownership of webrtc/video/ and webrtc/api.
hta: I have moved some of your TODOs and I think you might be interested in this
CL in general.
ossu: I have removed the hardcoded internal video payload types from
PayloadTypeMapper. I don't see the need for why they should be in there.

[ossu, 2016-11-14 13:14:20]

On 2016/11/14 08:38:43, magjed_webrtc wrote:
> ossu: I have removed the hardcoded internal video payload types from
> PayloadTypeMapper. I don't see the need for why they should be in there.

These values, as well as the "dynamic" mappings for audio, are only in there as
a temporary measure. I didn't want to change both how payload types were
assigned and the actual assignments in one CL, in case there were things
actually depending on the values here.
In fact, I wrote PayloadTypeMapper with the intent (well, hope) that it would
eventually be used by both audio and video (and data?) to pick RTP payload type
ids. I'm not sure if this is the CL to do that for video as well.
In future, I believe this mapping should be done in the transport, rather than
in the engines. Maybe it's fine to leave that change for then? WDYT?

[stefan-webrtc, 2016-11-15 09:00:08]

lg for webrtc/video

https://codereview.webrtc.org/2493133002/diff/70001/webrtc/api/webrtcsdp_unit...
File webrtc/api/webrtcsdp_unittest.cc (right):

https://codereview.webrtc.org/2493133002/diff/70001/webrtc/api/webrtcsdp_unit...
webrtc/api/webrtcsdp_unittest.cc:2090: video_desc_->AddCodec(h264_codec);
Could you explain why this change is needed? Do I have to review code in
webrtc/media to understand it?

https://codereview.webrtc.org/2493133002/diff/70001/webrtc/video/video_qualit...
File webrtc/video/video_quality_test.cc (right):

https://codereview.webrtc.org/2493133002/diff/70001/webrtc/video/video_qualit...
webrtc/video/video_quality_test.cc:33: #include
"webrtc/modules/video_coding/codecs/h264/include/h264.h"
Sort

[hta-webrtc, 2016-11-15 11:35:00]

I've got a couple of questions, so not LGTMing yet.
Overall looks good.

The helper function may not matter too much, since it's in local anonymous
namespace - we know who's calling it.

https://codereview.webrtc.org/2493133002/diff/70001/webrtc/api/webrtcsdp_unit...
File webrtc/api/webrtcsdp_unittest.cc (right):

https://codereview.webrtc.org/2493133002/diff/70001/webrtc/api/webrtcsdp_unit...
webrtc/api/webrtcsdp_unittest.cc:2086: cricket::VideoCodec h264_codec("H264");
Does this compile & run if you're compiling without H.264 support?

https://codereview.webrtc.org/2493133002/diff/70001/webrtc/api/webrtcsdp_unit...
webrtc/api/webrtcsdp_unittest.cc:2087: h264_codec.SetParam("profile-level-id",
"42e01f");
Can you use the constructor and stringifier for profile-level-id, so that it's
easier to see what you're testing?

https://codereview.webrtc.org/2493133002/diff/70001/webrtc/media/engine/paylo...
File webrtc/media/engine/payload_type_mapper_unittest.cc (right):

https://codereview.webrtc.org/2493133002/diff/70001/webrtc/media/engine/paylo...
webrtc/media/engine/payload_type_mapper_unittest.cc:64: // WebRTC, with their
hard coded values.
Shouldn't there be a test that runs thorugh the codec names and checks that
they're present, non-zero and distinct?

https://codereview.webrtc.org/2493133002/diff/70001/webrtc/media/engine/webrt...
File webrtc/media/engine/webrtcvideoengine2.cc (right):

https://codereview.webrtc.org/2493133002/diff/70001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvideoengine2.cc:562: // |codecs| should be incremental
values of the dynamic payload types [96, 127].
Grammar: I think "incrementing" is better than "incremental".

https://codereview.webrtc.org/2493133002/diff/70001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvideoengine2.cc:571: const int next_payload_type =
codecs.back().id + 1;
This strategy has two weaknesses:

1) If something gets added with a high number by another function, it will leave
a hole of unused numbers behind it.
2) If something gets added out of order, codecs.back().id + 1 may return a
number that is already in use.

If "codecs" had been a datatype that guaranteed that it was ordered (is there a
std::ordered_set?), I would be more happy with this line of code.

https://codereview.webrtc.org/2493133002/diff/70001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvideoengine2.cc:626: if (external_encoder_factory !=
nullptr) {
What is the result of this strategy if the internal and external factories
support exactly the same codec? (vp8, for instance)?
Will both be present with different payload types? Is that a good or a bad
thing?

https://codereview.webrtc.org/2493133002/diff/70001/webrtc/media/engine/webrt...
File webrtc/media/engine/webrtcvideoengine2_unittest.cc (right):

https://codereview.webrtc.org/2493133002/diff/70001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvideoengine2_unittest.cc:502: EXPECT_TRUE(false);
FAIL()?

https://codereview.webrtc.org/2493133002/diff/70001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvideoengine2_unittest.cc:514: // Need to look up the
codec in the engine to get the correct payload type.
Grammar: "We need to look up ", or just "We look up".

https://codereview.webrtc.org/2493133002/diff/70001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvideoengine2_unittest.cc:788: EXPECT_TRUE(false);
FAIL()

https://codereview.webrtc.org/2493133002/diff/70001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvideoengine2_unittest.cc:1488:
parameters.codecs.push_back(vp8_codec_without_feedback_params);
That's three times for this 3-line code block. Time to make a helper function?

[magjed_webrtc, 2016-11-15 16:24:47]

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

[commit-bot: I haz the power, 2016-11-15 16:24:53]

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, 2016-11-15 16:27:43]

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

[commit-bot: I haz the power, 2016-11-15 16:27:44]

Dry run: Try jobs failed on following builders:
  android_more_configs on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/android_more_configs/bu...)

[magjed_webrtc, 2016-11-15 17:05:36]

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

[commit-bot: I haz the power, 2016-11-15 17:05:45]

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, 2016-11-15 17:10:55]

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

[commit-bot: I haz the power, 2016-11-15 17:10:56]

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

[magjed_webrtc, 2016-11-15 17:15:00]

Patchset #2 (id:90001) has been deleted

[magjed_webrtc, 2016-11-15 17:18:26]

> These values, as well as the "dynamic" mappings for audio, are only in there
> as a temporary measure.
Ok, good. Then you're fine with this CL removing the video payload constants?

> In fact, I wrote PayloadTypeMapper with the intent (well, hope) that it would
> eventually be used by both audio and video (and data?) to pick RTP payload
type
> ids. I'm not sure if this is the CL to do that for video as well.
PayloadTypeMapper is hardcoded right now to only work with audio since it uses
webrtc::SdpAudioFormat for the mapping, so it would require rewriting it.

> In future, I believe this mapping should be done in the transport, rather
> than in the engines. Maybe it's fine to leave that change for then? WDYT?
Yeah probably. I think it best to postpone that change and make incremental
improvements like removing hardcoded constants first.

https://codereview.webrtc.org/2493133002/diff/70001/webrtc/api/webrtcsdp_unit...
File webrtc/api/webrtcsdp_unittest.cc (right):

https://codereview.webrtc.org/2493133002/diff/70001/webrtc/api/webrtcsdp_unit...
webrtc/api/webrtcsdp_unittest.cc:2086: cricket::VideoCodec h264_codec("H264");
On 2016/11/15 11:34:59, hta-webrtc wrote:
> Does this compile & run if you're compiling without H.264 support?

Yes, it doesn't use the actual encoder, it just serializes the
cricket::VideoCodec.

https://codereview.webrtc.org/2493133002/diff/70001/webrtc/api/webrtcsdp_unit...
webrtc/api/webrtcsdp_unittest.cc:2087: h264_codec.SetParam("profile-level-id",
"42e01f");
On 2016/11/15 11:34:59, hta-webrtc wrote:
> Can you use the constructor and stringifier for profile-level-id, so that it's
> easier to see what you're testing?

I can, but in the test we are checking:
EXPECT_NE(std::string::npos, fmtp_value.find("profile-level-id=42e01f"));
so maybe it's clearer to use the same string here?

https://codereview.webrtc.org/2493133002/diff/70001/webrtc/api/webrtcsdp_unit...
webrtc/api/webrtcsdp_unittest.cc:2090: video_desc_->AddCodec(h264_codec);
On 2016/11/15 09:00:08, stefan-webrtc (holmer) wrote:
> Could you explain why this change is needed? Do I have to review code in
> webrtc/media to understand it?

This change is needed because cricket::DefaultVideoCodecList() does not exist
anymore. Also, I don't think this unittest should have any dependency to a real
codec implementation. We should only be testing serialization of session
description with H264. So basically, the old 'cricket::DefaultVideoCodecList()'
is faked here with only the H264 codec.

https://codereview.webrtc.org/2493133002/diff/70001/webrtc/media/engine/paylo...
File webrtc/media/engine/payload_type_mapper_unittest.cc (right):

https://codereview.webrtc.org/2493133002/diff/70001/webrtc/media/engine/paylo...
webrtc/media/engine/payload_type_mapper_unittest.cc:64: // WebRTC, with their
hard coded values.
On 2016/11/15 11:34:59, hta-webrtc wrote:
> Shouldn't there be a test that runs thorugh the codec names and checks that
> they're present, non-zero and distinct?

Do you mean instead of the existing test below that checks what the actual
payload types are for the audio codecs? Probably, but I didn't want to touch
more code than necessary. This class is only used for audio codecs so the codec
names for e.g. VP8, VP9, etc are not mapped by this class at all so I just
removed all references to the video codecs here.

https://codereview.webrtc.org/2493133002/diff/70001/webrtc/media/engine/webrt...
File webrtc/media/engine/webrtcvideoengine2.cc (right):

https://codereview.webrtc.org/2493133002/diff/70001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvideoengine2.cc:562: // |codecs| should be incremental
values of the dynamic payload types [96, 127].
On 2016/11/15 11:34:59, hta-webrtc wrote:
> Grammar: I think "incrementing" is better than "incremental".

Done.

https://codereview.webrtc.org/2493133002/diff/70001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvideoengine2.cc:571: const int next_payload_type =
codecs.back().id + 1;
On 2016/11/15 11:34:59, hta-webrtc wrote:
> This strategy has two weaknesses:
> 
> 1) If something gets added with a high number by another function, it will
leave
> a hole of unused numbers behind it.
> 2) If something gets added out of order, codecs.back().id + 1 may return a
> number that is already in use.
> 
> If "codecs" had been a datatype that guaranteed that it was ordered (is there
a
> std::ordered_set?), I would be more happy with this line of code.

Yeah, it was not supposed to be generic. Like the function comment says; it's a
helper function for AppendVideoCodecs and the payload types of |codecs| must be
incremental values. I kept it simple since it's a very simple algorithm that
just assigns the payload types incrementally from 96 to 127.

I changed the code and the comments so that it's actually generic now. The code
is slightly more complex now and we will waste a few more cycles. Let me know if
you prefer this version.

https://codereview.webrtc.org/2493133002/diff/70001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvideoengine2.cc:626: if (external_encoder_factory !=
nullptr) {
On 2016/11/15 11:34:59, hta-webrtc wrote:
> What is the result of this strategy if the internal and external factories
> support exactly the same codec? (vp8, for instance)?
> Will both be present with different payload types? Is that a good or a bad
> thing?

Only one codec of a specific type will be added to the list. For H264, we also
check the profile, so we will only ever have one H264 Baseline for example, but
we can have for example both H264 Baseline and H264 High.

This is enforced by the code:
// Don't add same codec twice.
if (FindMatchingCodec(*unified_codecs, codec))
  continue;
This behavior is unchanged. That check used to be at line 641 and now it's at
593.

https://codereview.webrtc.org/2493133002/diff/70001/webrtc/media/engine/webrt...
File webrtc/media/engine/webrtcvideoengine2_unittest.cc (right):

https://codereview.webrtc.org/2493133002/diff/70001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvideoengine2_unittest.cc:502: EXPECT_TRUE(false);
On 2016/11/15 11:34:59, hta-webrtc wrote:
> FAIL()?

Done.

https://codereview.webrtc.org/2493133002/diff/70001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvideoengine2_unittest.cc:514: // Need to look up the
codec in the engine to get the correct payload type.
On 2016/11/15 11:35:00, hta-webrtc wrote:
> Grammar: "We need to look up ", or just "We look up".

Done.

https://codereview.webrtc.org/2493133002/diff/70001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvideoengine2_unittest.cc:788: EXPECT_TRUE(false);
On 2016/11/15 11:34:59, hta-webrtc wrote:
> FAIL()

Done.

https://codereview.webrtc.org/2493133002/diff/70001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvideoengine2_unittest.cc:1488:
parameters.codecs.push_back(vp8_codec_without_feedback_params);
On 2016/11/15 11:34:59, hta-webrtc wrote:
> That's three times for this 3-line code block. Time to make a helper function?

Yeah, probably. Done.

https://codereview.webrtc.org/2493133002/diff/70001/webrtc/video/video_qualit...
File webrtc/video/video_quality_test.cc (right):

https://codereview.webrtc.org/2493133002/diff/70001/webrtc/video/video_qualit...
webrtc/video/video_quality_test.cc:33: #include
"webrtc/modules/video_coding/codecs/h264/include/h264.h"
On 2016/11/15 09:00:08, stefan-webrtc (holmer) wrote:
> Sort

Done.

[hta-webrtc, 2016-11-15 20:36:25]

lgtm

Thanks for addressing these!

https://codereview.webrtc.org/2493133002/diff/70001/webrtc/api/webrtcsdp_unit...
File webrtc/api/webrtcsdp_unittest.cc (right):

https://codereview.webrtc.org/2493133002/diff/70001/webrtc/api/webrtcsdp_unit...
webrtc/api/webrtcsdp_unittest.cc:2087: h264_codec.SetParam("profile-level-id",
"42e01f");
On 2016/11/15 11:34:59, hta-webrtc wrote:
> Can you use the constructor and stringifier for profile-level-id, so that it's
> easier to see what you're testing?

Acknowledged.

https://codereview.webrtc.org/2493133002/diff/70001/webrtc/media/engine/paylo...
File webrtc/media/engine/payload_type_mapper_unittest.cc (right):

https://codereview.webrtc.org/2493133002/diff/70001/webrtc/media/engine/paylo...
webrtc/media/engine/payload_type_mapper_unittest.cc:64: // WebRTC, with their
hard coded values.
On 2016/11/15 17:18:26, magjed_webrtc wrote:
> On 2016/11/15 11:34:59, hta-webrtc wrote:
> > Shouldn't there be a test that runs thorugh the codec names and checks that
> > they're present, non-zero and distinct?
> 
> Do you mean instead of the existing test below that checks what the actual
> payload types are for the audio codecs? Probably, but I didn't want to touch
> more code than necessary. This class is only used for audio codecs so the
codec
> names for e.g. VP8, VP9, etc are not mapped by this class at all so I just
> removed all references to the video codecs here.

That makes sense. Can you add the word "audio" to the comment above so that it's
obvious?

"...knows about the audio formats we've been using in WebRTC ..."

https://codereview.webrtc.org/2493133002/diff/70001/webrtc/media/engine/webrt...
File webrtc/media/engine/webrtcvideoengine2.cc (right):

https://codereview.webrtc.org/2493133002/diff/70001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvideoengine2.cc:626: if (external_encoder_factory !=
nullptr) {
On 2016/11/15 17:18:26, magjed_webrtc wrote:
> On 2016/11/15 11:34:59, hta-webrtc wrote:
> > What is the result of this strategy if the internal and external factories
> > support exactly the same codec? (vp8, for instance)?
> > Will both be present with different payload types? Is that a good or a bad
> > thing?
> 
> Only one codec of a specific type will be added to the list. For H264, we also
> check the profile, so we will only ever have one H264 Baseline for example,
but
> we can have for example both H264 Baseline and H264 High.
> 
> This is enforced by the code:
> // Don't add same codec twice.
> if (FindMatchingCodec(*unified_codecs, codec))
>   continue;
> This behavior is unchanged. That check used to be at line 641 and now it's at
> 593.

OK, so if something's internally supported, we can't add an external codec for
the same codec and profile. Nice to know.

https://codereview.webrtc.org/2493133002/diff/110001/webrtc/api/android/jni/a...
File webrtc/api/android/jni/androidmediaencoder_jni.cc (right):

https://codereview.webrtc.org/2493133002/diff/110001/webrtc/api/android/jni/a...
webrtc/api/android/jni/androidmediaencoder_jni.cc:409: scale_ =
codec_settings->codecSpecific.VP8.automaticResizeOn;
Sideswipe: This access to codecSpecific shoudldn't exist. Can you change it to
codec_setting->VP8()?

(I thought I'd caught them all, but they seem to crop up like weeds. The CL
making them illegal is blocked on an import error I can't figure out.)

https://codereview.webrtc.org/2493133002/diff/110001/webrtc/media/engine/webr...
File webrtc/media/engine/webrtcvideoengine2.cc (right):

https://codereview.webrtc.org/2493133002/diff/110001/webrtc/media/engine/webr...
webrtc/media/engine/webrtcvideoengine2.cc:581: return rtc::Optional<int>();
Yep, I prefer this version. It's obviously correct.

(This is implementing a size-limited set, which is a construct I've been
searching for in std::, but haven't found... this works.)

[magjed_webrtc, 2016-11-16 13:43:30]

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

[commit-bot: I haz the power, 2016-11-16 13:43:34]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[magjed_webrtc, 2016-11-16 13:44:11]

https://codereview.webrtc.org/2493133002/diff/70001/webrtc/media/engine/paylo...
File webrtc/media/engine/payload_type_mapper_unittest.cc (right):

https://codereview.webrtc.org/2493133002/diff/70001/webrtc/media/engine/paylo...
webrtc/media/engine/payload_type_mapper_unittest.cc:64: // WebRTC, with their
hard coded values.
On 2016/11/15 20:36:24, hta-webrtc wrote:
> On 2016/11/15 17:18:26, magjed_webrtc wrote:
> > On 2016/11/15 11:34:59, hta-webrtc wrote:
> > > Shouldn't there be a test that runs thorugh the codec names and checks
that
> > > they're present, non-zero and distinct?
> > 
> > Do you mean instead of the existing test below that checks what the actual
> > payload types are for the audio codecs? Probably, but I didn't want to touch
> > more code than necessary. This class is only used for audio codecs so the
> codec
> > names for e.g. VP8, VP9, etc are not mapped by this class at all so I just
> > removed all references to the video codecs here.
> 
> That makes sense. Can you add the word "audio" to the comment above so that
it's
> obvious?
> 
> "...knows about the audio formats we've been using in WebRTC ..."

Done.

https://codereview.webrtc.org/2493133002/diff/70001/webrtc/media/engine/webrt...
File webrtc/media/engine/webrtcvideoengine2.cc (right):

https://codereview.webrtc.org/2493133002/diff/70001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvideoengine2.cc:626: if (external_encoder_factory !=
nullptr) {
On 2016/11/15 20:36:24, hta-webrtc wrote:
> On 2016/11/15 17:18:26, magjed_webrtc wrote:
> > On 2016/11/15 11:34:59, hta-webrtc wrote:
> > > What is the result of this strategy if the internal and external factories
> > > support exactly the same codec? (vp8, for instance)?
> > > Will both be present with different payload types? Is that a good or a bad
> > > thing?
> > 
> > Only one codec of a specific type will be added to the list. For H264, we
also
> > check the profile, so we will only ever have one H264 Baseline for example,
> but
> > we can have for example both H264 Baseline and H264 High.
> > 
> > This is enforced by the code:
> > // Don't add same codec twice.
> > if (FindMatchingCodec(*unified_codecs, codec))
> >   continue;
> > This behavior is unchanged. That check used to be at line 641 and now it's
at
> > 593.
> 
> OK, so if something's internally supported, we can't add an external codec for
> the same codec and profile. Nice to know.
> 

Yes, we can't add the codec twice, but we will actually prefer the external
encoder over the internal encoder in WebRtcVideoSendStream::CreateVideoEncoder.
So the internal codecs provide default implementations that you can override
with the external factories. That's how iOS and Android sets up their VP8 and
H264 HW encoders.

https://codereview.webrtc.org/2493133002/diff/110001/webrtc/api/android/jni/a...
File webrtc/api/android/jni/androidmediaencoder_jni.cc (right):

https://codereview.webrtc.org/2493133002/diff/110001/webrtc/api/android/jni/a...
webrtc/api/android/jni/androidmediaencoder_jni.cc:409: scale_ =
codec_settings->codecSpecific.VP8.automaticResizeOn;
On 2016/11/15 20:36:24, hta-webrtc wrote:
> Sideswipe: This access to codecSpecific shoudldn't exist. Can you change it to
> codec_setting->VP8()?
> 
> (I thought I'd caught them all, but they seem to crop up like weeds. The CL
> making them illegal is blocked on an import error I can't figure out.)
> 
> 

Done.

[commit-bot: I haz the power, 2016-11-16 13:47:06]

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

[commit-bot: I haz the power, 2016-11-16 13:47:07]

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

[ossu, 2016-11-16 16:18:32]

On 2016/11/15 17:18:26, magjed_webrtc wrote:
> > These values, as well as the "dynamic" mappings for audio, are only in there
> > as a temporary measure.
> Ok, good. Then you're fine with this CL removing the video payload constants?

I am. In fact, I applaud it!
I'll take a look at removing the fixed "dynamic" parts from the audio side in a
separate CL. At this point, I believe it ShouldJustWork(tm).

> > In fact, I wrote PayloadTypeMapper with the intent (well, hope) that it
would
> > eventually be used by both audio and video (and data?) to pick RTP payload
> type
> > ids. I'm not sure if this is the CL to do that for video as well.
> PayloadTypeMapper is hardcoded right now to only work with audio since it uses
> webrtc::SdpAudioFormat for the mapping, so it would require rewriting it.

Honestly, isn't most code hardcoded? :)

> > In future, I believe this mapping should be done in the transport, rather
> > than in the engines. Maybe it's fine to leave that change for then? WDYT?
> Yeah probably. I think it best to postpone that change and make incremental
> improvements like removing hardcoded constants first.

Agreed.

Here, have another lgtm!

[magjed_webrtc, 2016-11-16 16:31:08]

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

[commit-bot: I haz the power, 2016-11-16 16:31:18]

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, 2016-11-16 16:32:25]

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

[commit-bot: I haz the power, 2016-11-16 16:32:26]

Dry run: Try jobs failed on following builders:
  ios32_sim_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios32_sim_dbg/builds/12559)
  ios64_sim_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_dbg/builds/12576)
  ios_arm64_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_rel/builds/14789)
  ios_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_rel/builds/18842)

[magjed_webrtc, 2016-11-16 18:15:30]

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

[commit-bot: I haz the power, 2016-11-16 18:15:33]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[magjed_webrtc, 2016-11-16 18:15:47]

Patchset #4 (id:150001) has been deleted

[commit-bot: I haz the power, 2016-11-16 18:18:23]

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

[commit-bot: I haz the power, 2016-11-16 18:18:24]

Dry run: Try jobs failed on following builders:
  linux_baremetal on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_baremetal/builds/...)
  linux_tsan2 on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_tsan2/builds/17031)
  mac_baremetal on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_baremetal/builds/16286)

[magjed_webrtc, 2016-11-17 13:19:13]

Stefan - ping.

[stefan-webrtc, 2016-11-17 14:01:25]

lgtm

[magjed_webrtc, 2016-11-17 14:25:18]

The CQ bit was checked by magjed@webrtc.org

[magjed_webrtc, 2016-11-17 14:25:18]

The patchset sent to the CQ was uploaded after l-g-t-m from hta@webrtc.org,
ossu@webrtc.org, stefan@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2493133002/#ps190001 (title:
"Rebase")

[commit-bot: I haz the power, 2016-11-17 14:25:24]

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, 2016-11-17 14:50:47]

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

[commit-bot: I haz the power, 2016-11-17 14:50:49]

Failed to apply patch for webrtc/video/video_quality_test.cc:
While running git apply --index -p1;
  error: patch failed: webrtc/video/video_quality_test.cc:28
  error: webrtc/video/video_quality_test.cc: patch does not apply

Patch:       webrtc/video/video_quality_test.cc
Index: webrtc/video/video_quality_test.cc
diff --git a/webrtc/video/video_quality_test.cc
b/webrtc/video/video_quality_test.cc
index
b48c69237695f6c5e31213113cf731014599e875..9b0eb8a90512884a6f2aa2f13bd87de5b3260501
100644
--- a/webrtc/video/video_quality_test.cc
+++ b/webrtc/video/video_quality_test.cc
@@ -28,6 +28,9 @@
 #include "webrtc/logging/rtc_event_log/rtc_event_log.h"
 #include "webrtc/modules/rtp_rtcp/include/rtp_header_parser.h"
 #include "webrtc/modules/rtp_rtcp/source/rtp_utility.h"
+#include "webrtc/modules/video_coding/codecs/h264/include/h264.h"
+#include "webrtc/modules/video_coding/codecs/vp8/include/vp8.h"
+#include "webrtc/modules/video_coding/codecs/vp9/include/vp9.h"
 #include "webrtc/system_wrappers/include/cpu_info.h"
 #include "webrtc/test/gtest.h"
 #include "webrtc/test/layer_filtering_transport.h"
@@ -1005,13 +1008,13 @@ void VideoQualityTest::SetupVideo(Transport*
send_transport,
 
   int payload_type;
   if (params_.video.codec == "H264") {
-    video_encoder_.reset(VideoEncoder::Create(VideoEncoder::kH264));
+    video_encoder_.reset(H264Encoder::Create());
     payload_type = kPayloadTypeH264;
   } else if (params_.video.codec == "VP8") {
-    video_encoder_.reset(VideoEncoder::Create(VideoEncoder::kVp8));
+    video_encoder_.reset(VP8Encoder::Create());
     payload_type = kPayloadTypeVP8;
   } else if (params_.video.codec == "VP9") {
-    video_encoder_.reset(VideoEncoder::Create(VideoEncoder::kVp9));
+    video_encoder_.reset(VP9Encoder::Create());
     payload_type = kPayloadTypeVP9;
   } else {
     RTC_NOTREACHED() << "Codec not supported!";

[magjed_webrtc, 2016-11-17 15:07:44]

The CQ bit was checked by magjed@webrtc.org

[magjed_webrtc, 2016-11-17 15:07:45]

The patchset sent to the CQ was uploaded after l-g-t-m from hta@webrtc.org,
stefan@webrtc.org, ossu@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2493133002/#ps210001 (title:
"Rebase again")

[magjed_webrtc, 2016-11-17 15:07:48]

The CQ bit was unchecked by magjed@webrtc.org

[magjed_webrtc, 2016-11-17 15:08:52]

Description was changed from

==========
Stop using hardcoded payload types for video codecs

This CL stops using hardcoded payload types for different video codecs
and will dynamically assign them payload types incrementally from 96 to
127 instead.

This CL:
 * Replaces 'std::vector<VideoCodec> DefaultVideoCodecList()' in
   webrtcvideoengine2.cc with an explicit WebRtcVideoEncoderFactory for
   internally supported software codecs instead. The purpose is to
   streamline the payload type assignment in webrtcvideoengine2.cc which
   will now have two encoder factories of the same
   WebRtcVideoEncoderFactory type; one internal and one external.
 * Removes webrtc::VideoEncoder::EncoderType and use cricket::VideoCodec
   instead.
 * Removes 'static VideoEncoder* Create(EncoderType codec_type)' and
   moves the create function to the internal encoder factory instead.
 * Removes video_encoder.cc. webrtc::VideoEncoder is now just an
   interface without any static functions.
 * The function GetSupportedCodecs in webrtcvideoengine2.cc unifies
   the internal and external codecs and assigns them payload types
   incrementally from 96 to 127.
 * Updates webrtcvideoengine2_unittest.cc and removes assumptions about
   what payload types will be used.

BUG=webrtc:6677,webrtc:6705
==========

to

==========
Stop using hardcoded payload types for video codecs

This CL stops using hardcoded payload types for different video codecs
and will dynamically assign them payload types incrementally from 96 to
127 instead.

This CL:
 * Replaces 'std::vector<VideoCodec> DefaultVideoCodecList()' in
   webrtcvideoengine2.cc with an explicit WebRtcVideoEncoderFactory for
   internally supported software codecs instead. The purpose is to
   streamline the payload type assignment in webrtcvideoengine2.cc which
   will now have two encoder factories of the same
   WebRtcVideoEncoderFactory type; one internal and one external.
 * Removes webrtc::VideoEncoder::EncoderType and use cricket::VideoCodec
   instead.
 * Removes 'static VideoEncoder* Create(EncoderType codec_type)' and
   moves the create function to the internal encoder factory instead.
 * Removes video_encoder.cc. webrtc::VideoEncoder is now just an
   interface without any static functions.
 * The function GetSupportedCodecs in webrtcvideoengine2.cc unifies
   the internal and external codecs and assigns them payload types
   incrementally from 96 to 127.
 * Updates webrtcvideoengine2_unittest.cc and removes assumptions about
   what payload types will be used.

BUG=webrtc:6677,webrtc:6705
R=hta@webrtc.org, ossu@webrtc.org, stefan@webrtc.org

Committed:
https://chromium.googlesource.com/external/webrtc/+/42043b95872b51321f508bf25...
==========

[commit-bot: I haz the power, 2016-11-17 15:08:52]

Description was changed from

==========
Stop using hardcoded payload types for video codecs

This CL stops using hardcoded payload types for different video codecs
and will dynamically assign them payload types incrementally from 96 to
127 instead.

This CL:
 * Replaces 'std::vector<VideoCodec> DefaultVideoCodecList()' in
   webrtcvideoengine2.cc with an explicit WebRtcVideoEncoderFactory for
   internally supported software codecs instead. The purpose is to
   streamline the payload type assignment in webrtcvideoengine2.cc which
   will now have two encoder factories of the same
   WebRtcVideoEncoderFactory type; one internal and one external.
 * Removes webrtc::VideoEncoder::EncoderType and use cricket::VideoCodec
   instead.
 * Removes 'static VideoEncoder* Create(EncoderType codec_type)' and
   moves the create function to the internal encoder factory instead.
 * Removes video_encoder.cc. webrtc::VideoEncoder is now just an
   interface without any static functions.
 * The function GetSupportedCodecs in webrtcvideoengine2.cc unifies
   the internal and external codecs and assigns them payload types
   incrementally from 96 to 127.
 * Updates webrtcvideoengine2_unittest.cc and removes assumptions about
   what payload types will be used.

BUG=webrtc:6677,webrtc:6705
==========

to

==========
Stop using hardcoded payload types for video codecs

This CL stops using hardcoded payload types for different video codecs
and will dynamically assign them payload types incrementally from 96 to
127 instead.

This CL:
 * Replaces 'std::vector<VideoCodec> DefaultVideoCodecList()' in
   webrtcvideoengine2.cc with an explicit WebRtcVideoEncoderFactory for
   internally supported software codecs instead. The purpose is to
   streamline the payload type assignment in webrtcvideoengine2.cc which
   will now have two encoder factories of the same
   WebRtcVideoEncoderFactory type; one internal and one external.
 * Removes webrtc::VideoEncoder::EncoderType and use cricket::VideoCodec
   instead.
 * Removes 'static VideoEncoder* Create(EncoderType codec_type)' and
   moves the create function to the internal encoder factory instead.
 * Removes video_encoder.cc. webrtc::VideoEncoder is now just an
   interface without any static functions.
 * The function GetSupportedCodecs in webrtcvideoengine2.cc unifies
   the internal and external codecs and assigns them payload types
   incrementally from 96 to 127.
 * Updates webrtcvideoengine2_unittest.cc and removes assumptions about
   what payload types will be used.

BUG=webrtc:6677,webrtc:6705
R=hta@webrtc.org, ossu@webrtc.org, stefan@webrtc.org

Committed: https://crrev.com/42043b95872b51321f508bf255d804ce3dff366b
Cr-Commit-Position: refs/heads/master@{#15135}
==========

[commit-bot: I haz the power, 2016-11-17 15:08:53]

Patchset 6 (id:??) landed as
https://crrev.com/42043b95872b51321f508bf255d804ce3dff366b
Cr-Commit-Position: refs/heads/master@{#15135}

[magjed_webrtc, 2016-11-17 15:08:54]

Committed patchset #6 (id:210001) manually as
42043b95872b51321f508bf255d804ce3dff366b (presubmit successful).

[magjed_webrtc, 2016-11-17 16:51:40]

A revert of this CL (patchset #6 id:210001) has been created in
https://codereview.webrtc.org/2513633002/ by magjed@webrtc.org.

The reason for reverting is: Breaks chromium.fyi test:
WebRtcBrowserTest.NegotiateUnsupportedVideoCodec.