MediaRecorder: use VideoTrackRecorder::GetPreferredCodecId() when available

content/renderer/media/video_track_recorder.cc

Index: content/renderer/media/video_track_recorder.cc
diff --git a/content/renderer/media/video_track_recorder.cc b/content/renderer/media/video_track_recorder.cc
index 011c633e3d1e90f8db0bf692b6185ad8c67ff4b1..6f40477193ddfc71a54973b54a076d57b45f8560 100644
--- a/content/renderer/media/video_track_recorder.cc
+++ b/content/renderer/media/video_track_recorder.cc
@@ -1075,6 +1075,30 @@ void H264Encoder::ConfigureEncoderOnEncodingTaskRunner(const gfx::Size& size) {
 
 }  // anonymous namespace
 
+// static
+VideoTrackRecorder::CodecId VideoTrackRecorder::GetPreferredCodec() {
+  CodecId codec_id = CodecId::VP8;
+  while (codec_id != VideoTrackRecorder::CodecId::LAST) {
+    if (CodecIdToVEAProfile(codec_id) != media::VIDEO_CODEC_PROFILE_UNKNOWN)

[emircan, 2017/01/11 18:01:58]

We end up calling CodecIdToVEAProfile() at least 3 times here and I think it is
redundant. Why don't we refactor that CodecIdToVEAProfile() into
GetSupportedCodecsAndProfiles(), where you can just remove the if clause on
l.110 and it would lazy evaluate supported
<content::VideoTrackRecorder::CodecId, media::VideoCodecProfile> pairs. CodecId
is needed by MediaRecorder and VideoCodecProfile by VEA. You can use that
function here, and just keep the if clause on l.110 in CodecIdToVEAProfile().

P.S. We currently always go for the min VideoCodecProfile that matches, i.e.
when H264PROFILE_BASELINE and H264PROFILE_MAIN is supported, we would only go
for baseline. You can also order them if it makes a difference for the
container.

[mcasas, 2017/01/11 21:40:48]

Ok, you convinced me to cut deeper and made a singleton
lazy class encapsulating all these interactions,
Hmm uncertain, really.  This is a fallback path for
when the user has no preference either, so using
the baseline profile sounds good enough.

+      return codec_id;
+
+    switch (codec_id) {

[chfremer, 2017/01/11 17:34:52]

Why not use a for-loop to check a fixed list of candidate codecs instead of a
surprising while-loop with a switch-case for iteration updates and an artifical
CodecID::LAST for termination?

std::vector<CodecId> candidate_codecs({
  CodecId::VP8,
  CodecId::VP9,
  CodecId::H264
});
for (auto codec_id : candidate_codec_ids) {
  if (CodecIdToVEAProfile(codec_id) != media::VIDEO_CODEC_PROFILE_UNKNOWN)
    return codec_id;
}

Could maybe even use std::find_if() instead of for-loop.

[mcasas, 2017/01/11 21:40:48]

I wanted to have a compiler-enforced way to guarantee
that all codecs are iterated for, hence the switch with
no default:

Following the comments of ermican@ above allows to
enforce it via static_assert.

+      case CodecId::VP8:
+        codec_id = CodecId::VP9;
+        break;
+      case CodecId::VP9:
+        codec_id = CodecId::H264;
+        break;
+      case CodecId::H264:
+        codec_id = CodecId::LAST;
+        break;
+      case CodecId::LAST:
+        NOTREACHED();
+    }
+  }
+  return CodecId::VP8;
+}
+
 VideoTrackRecorder::VideoTrackRecorder(
     CodecId codec,
     const blink::WebMediaStreamTrack& track,