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..dcea89e61e6596bcfc2755e466ab8a97377c4d96 100644
--- a/content/renderer/media/video_track_recorder.cc
+++ b/content/renderer/media/video_track_recorder.cc
@@ -60,60 +60,98 @@ const int kVEADefaultBitratePerPixel = 2;
 // encoders.
 const int kVEAEncoderOutputBufferCount = 4;
 
-static struct {
-  VideoTrackRecorder::CodecId codec_id;
-  media::VideoCodecProfile min_profile;
-  media::VideoCodecProfile max_profile;
-} const kSupportedVideoCodecIdToProfile[] = {
-    {VideoTrackRecorder::CodecId::VP8,
-     media::VP8PROFILE_MIN,
-     media::VP8PROFILE_MAX},
-    {VideoTrackRecorder::CodecId::VP9,
-     media::VP9PROFILE_MIN,
-     media::VP9PROFILE_MAX},
-    {VideoTrackRecorder::CodecId::H264,
-     media::H264PROFILE_MIN,
-     media::H264PROFILE_MAX}};
-
-// Returns the corresponding codec profile from VEA supported codecs. If no
-// profile is found, returns VIDEO_CODEC_PROFILE_UNKNOWN.
-media::VideoCodecProfile CodecIdToVEAProfile(
-    content::VideoTrackRecorder::CodecId codec) {
-  // See https://crbug.com/616659.
+using CodecId = VideoTrackRecorder::CodecId;
+
+// Class to encapsulate the enumeration of CodecIds/VideoCodecProfiles supported

[chfremer, 2017/01/11 23:24:55]

Nit: "Encapsulates the ..." 

[mcasas, 2017/01/12 00:20:54]

"to encapsulate" is correct, right? 

+// by the VEA underlying platform. Provides methods to query the preferred
+// CodecId and to check if a given CodecId is supported.
+class CodecEnumerator {
+ public:
+  CodecEnumerator();
+  ~CodecEnumerator() = default;
+
+  // Get the first CodecId that has an associated VEA VideoCodecProfile, or VP8.

[chfremer, 2017/01/11 23:24:55]

Nit: "Returns the ..."

[mcasas, 2017/01/12 00:20:55]

Done.

+  CodecId GetPreferredCodecId();
+
+  // Returns the VEA VideoCodedProfile for a given CodecId, if supported, or
+  // VIDEO_CODEC_PROFILE_UNKNOWN otherwise.
+  media::VideoCodecProfile CodecIdToVEAProfile(CodecId codec);
+
+ private:
+  static const struct CodecIdAndVEAProfile {
+    CodecId codec_id;
+    media::VideoCodecProfile min_profile;
+    media::VideoCodecProfile max_profile;
+  } constexpr kCodecIdAndVEAProfiles[] = {
+      {CodecId::VP8, media::VP8PROFILE_MIN, media::VP8PROFILE_MAX},
+      {CodecId::VP9, media::VP9PROFILE_MIN, media::VP9PROFILE_MAX},
+      {CodecId::H264, media::H264PROFILE_MIN, media::H264PROFILE_MAX}};
+
+  // A map of VEA-supported CodecId-and-VEA-profile pairs.
+  std::map<CodecId, media::VideoCodecProfile> codec_id_to_profile_;
+
+  DISALLOW_COPY_AND_ASSIGN(CodecEnumerator);
+};
+
+constexpr CodecEnumerator::CodecIdAndVEAProfile
+    CodecEnumerator::kCodecIdAndVEAProfiles[];
+
+static base::LazyInstance<CodecEnumerator>::Leaky g_codec_enumerator =
+    LAZY_INSTANCE_INITIALIZER;
+
+CodecEnumerator::CodecEnumerator() {
+  static_assert(
+      arraysize(kCodecIdAndVEAProfiles) == static_cast<int>(CodecId::LAST),
+      "|kCodecIdAndVEAProfiles| should consider all CodecIds");
 #if defined(OS_CHROMEOS)
-  return media::VIDEO_CODEC_PROFILE_UNKNOWN;
-#endif  // defined(OS_CHROMEOS)
+  // See https://crbug.com/616659.
+  return;
+#endif
 
-// See https://crbug.com/653864.
 #if defined(OS_ANDROID)
-  return media::VIDEO_CODEC_PROFILE_UNKNOWN;
-#endif  // defined(OS_ANDROID)
+  // See https://crbug.com/653864.
+  return;
+#endif
 
   content::RenderThreadImpl* const render_thread_impl =
       content::RenderThreadImpl::current();
   if (!render_thread_impl) {
-    DVLOG(3) << "Couldn't access the render thread";
-    return media::VIDEO_CODEC_PROFILE_UNKNOWN;
+    DVLOG(2) << "Couldn't access the render thread";
+    return;
   }
 
   media::GpuVideoAcceleratorFactories* const gpu_factories =
       render_thread_impl->GetGpuFactories();
   if (!gpu_factories || !gpu_factories->IsGpuVideoAcceleratorEnabled()) {
-    DVLOG(3) << "Couldn't initialize GpuVideoAcceleratorFactories";
-    return media::VIDEO_CODEC_PROFILE_UNKNOWN;
+    DVLOG(2) << "Couldn't initialize GpuVideoAcceleratorFactories";
+    return;
   }
 
   const media::VideoEncodeAccelerator::SupportedProfiles& vea_profiles =

[chfremer, 2017/01/11 23:24:55]

Nit: During my read-through, I would have found it more useful if the variable
was called |supported_vea_profiles|, because in l.132 it didn't help me much
that the type name |SupportedProfiles| contained the hint that these are the
supported profiles. (I guess I didn't read the code strictly from top to bottom,
... or I have an extremely short memory)

[mcasas, 2017/01/12 00:20:54]

Done.

       gpu_factories->GetVideoEncodeAcceleratorSupportedProfiles();
   for (const auto& vea_profile : vea_profiles) {
-    for (const auto& supported_profile : kSupportedVideoCodecIdToProfile) {
-      if (codec == supported_profile.codec_id &&
-          vea_profile.profile >= supported_profile.min_profile &&
-          vea_profile.profile <= supported_profile.max_profile)
-        return vea_profile.profile;
+    for (auto& codec_id_and_profile : kCodecIdAndVEAProfiles) {
+      if (vea_profile.profile >= codec_id_and_profile.min_profile &&
+          vea_profile.profile <= codec_id_and_profile.max_profile) {
+        DVLOG(2) << media::GetProfileName(vea_profile.profile) << " supported";
+        codec_id_to_profile_.emplace(codec_id_and_profile.codec_id,
+                                     vea_profile.profile);

[chfremer, 2017/01/11 23:24:55]

Can it happen that more than one entry is emplaced for the same codec_id?

[mcasas, 2017/01/12 00:20:54]

Yeah, as emircan@ commented, subsequent ones will be
ignored, so if we have e.g. 
  H264PROFILE_MAIN = 1,
  H264PROFILE_EXTENDED = 2,
  H264PROFILE_HIGH = 3,
 
only the first one would be in |codec_id_to_profile_|.
At this point that's fine, and is the current behaviour
for CodecIdToVEAProfile().

+      }
     }
   }
-  return media::VIDEO_CODEC_PROFILE_UNKNOWN;
+}
+
+CodecId CodecEnumerator::GetPreferredCodecId() {
+  if (codec_id_to_profile_.empty())
+    return CodecId::VP8;
+  return codec_id_to_profile_.begin()->first;

[chfremer, 2017/01/11 23:24:55]

Hmm, so if multiple codecs have VEA profiles, the order in
|kCodecIdAndVEAProfiles| determines which one is preferred. 
If that is true and intended, could we express that more explicitly?
Best would be in the variable name. Second best may be in a comment at the
declaration of |kCodecIdAndVEAProfiles|.

[mcasas, 2017/01/12 00:20:54]

It's good that you take a fresh look at this!
s/kCodecIdAndVEAProfiles/kPreferredCodecAndProfiles/.

+}
+
+media::VideoCodecProfile CodecEnumerator::CodecIdToVEAProfile(CodecId codec) {
+  const auto profile = codec_id_to_profile_.find(codec);
+  return profile == codec_id_to_profile_.end()
+             ? media::VIDEO_CODEC_PROFILE_UNKNOWN
+             : profile->second;
 }
 
 }  // anonymous namespace
@@ -1075,6 +1113,11 @@ void H264Encoder::ConfigureEncoderOnEncodingTaskRunner(const gfx::Size& size) {
 
 }  // anonymous namespace
 
+// static
+VideoTrackRecorder::CodecId VideoTrackRecorder::GetPreferredCodecId() {
+  return g_codec_enumerator.Get().GetPreferredCodecId();
+}
+
 VideoTrackRecorder::VideoTrackRecorder(
     CodecId codec,
     const blink::WebMediaStreamTrack& track,
@@ -1143,7 +1186,8 @@ void VideoTrackRecorder::InitializeEncoder(
   MediaStreamVideoSink::DisconnectFromTrack();
 
   const gfx::Size& input_size = frame->visible_rect().size();
-  const auto& vea_supported_profile = CodecIdToVEAProfile(codec);
+  const auto& vea_supported_profile =
+      g_codec_enumerator.Get().CodecIdToVEAProfile(codec);
   if (vea_supported_profile != media::VIDEO_CODEC_PROFILE_UNKNOWN &&
       input_size.width() >= kVEAEncoderMinResolutionWidth &&
       input_size.height() >= kVEAEncoderMinResolutionHeight) {