MediaRecorder: use VideoTrackRecorder::GetPreferredCodecId() when available

content/renderer/media/video_track_recorder.cc

 // Copyright 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "content/renderer/media/video_track_recorder.h"
 
 #include <utility>
 
 #include "base/bind.h"
 #include "base/logging.h"
@@ skipping 1057 matching lines @@
     return;
   }
 
   int pixel_format = EVideoFormatType::videoFormatI420;
   openh264_encoder_->SetOption(ENCODER_OPTION_DATAFORMAT, &pixel_format);
 }
 #endif //#if BUILDFLAG(RTC_USE_H264)
 
 }  // 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,
     const OnEncodedVideoCB& on_encoded_video_callback,
     int32_t bits_per_second)
     : track_(track),
       paused_before_init_(false),
       weak_ptr_factory_(this) {
   DCHECK(main_render_thread_checker_.CalledOnValidThread());
   DCHECK(!track_.isNull());
@@ skipping 83 matching lines @@
     encoder_->SetPaused(paused_before_init_);
 
   // StartFrameEncode() will be called on Render IO thread.
   MediaStreamVideoSink::ConnectToTrack(
       track_,
       base::Bind(&VideoTrackRecorder::Encoder::StartFrameEncode, encoder_),
       false);
 }
 
 }  // namespace content