RtcVideoEncoder: determine video codec profile at InitEncode time.

content/renderer/media/rtc_video_encoder.cc

Index: content/renderer/media/rtc_video_encoder.cc
diff --git a/content/renderer/media/rtc_video_encoder.cc b/content/renderer/media/rtc_video_encoder.cc
index 99e963a96b27fe7ff7532447e20541ca50c12669..95779ae3af8fb47eaaa0834c73d149941b8ac7f1 100644
--- a/content/renderer/media/rtc_video_encoder.cc
+++ b/content/renderer/media/rtc_video_encoder.cc
@@ -62,6 +62,20 @@ bool GetRTPFragmentationHeaderH264(webrtc::RTPFragmentationHeader* header,
   return true;
 }
 
+// Maps webrtc::VideoCodecProfile to media::VideoCodecProfile for H264 codec.

[Pawel Osciak, 2014/10/20 06:28:43]

Could we instead have a method that takes both profile and type from webrtc and
converts to media::VideoCodecProfile and unify with the existing method in
RtcVEFactory?

Or actually, since we should move type/codec to InitEncode entirely, probably
unify it here and remove from factory.

[hshi1, 2014/10/20 16:55:50]

The |profile_| is of media::VideoCodecProfile type; it is internal to chrome
media code and not recognized by WebRTC.

The reason for RtcVE to retain a member |profile_| at its constructor is because
that is the only place where RtcVEFactory interacts with the RtcVE instance. If
we don't save |profile| there it would be lost, as webRTC code has no idea about
its existence.

[Pawel Osciak, 2014/10/21 13:41:46]

Yes. My suggestion though was that it would be nice to try to delay translation
from WebRTC's [codec,profile] pair into media::VCP until InitEncode, when we
have both, instead of translating from codec and then doing a fixup in
InitEncode and do it in RTCVE::InitEncode. Especially since RTCVE constructor
doesn't really do anything with it.
Yes, although could we maybe change the factory to do what I'm suggesting in
comment below, unless I'm missing something?

[hshi1, 2014/10/22 19:01:16]

Done.

+media::VideoCodecProfile GetH264CodecProfile(
+    webrtc::VideoCodecProfile profile) {
+  switch (profile) {
+    case webrtc::kProfileBase:
+      return media::H264PROFILE_BASELINE;

[Pawel Osciak, 2014/10/20 06:28:43]

Is this actually baseline or constrained baseline?

[hshi1, 2014/10/20 16:55:50]

This is the actual baseline, however as I commented in the bug, the HW encoder
is free to implement a proper subset of the requested profile (e.g.
ConstrainedBaseline) and still be compliant.

[Pawel Osciak, 2014/10/21 13:41:46]

The issue is, what if an encoder does non-constrained and receiver can't decode
it... There is no way to pass this information/negotiate through media::VCP, or
WebRTC, or between devices, as far as I can tell? No matter whether it's CBP or
BP, we always say BP... It's true that it's a superset so technically ok, but we
may lose opportunity to decode if it's CBP and we treat it as BP, or may fail
unexpectedly if we always treat as CBP.

[hshi1, 2014/10/22 19:01:16]

But there's no constrained baseline profile definition in media code, so even if
we want to pass CBP down to GpuVEA there's no enums defined at the moment (see
media/base/video_decoder_config.h)

[Pawel Osciak, 2014/10/25 01:29:57]

Right, but that's a bug (crbug.com/345569) so perhaps it might be a good time to
consider looking into it (separately from this CL). I'm just explaining
consequences here and in crbug.com/424828 and hoping whomever is more
knowledgeable of what we should be expecting from decoders/encoders we use for
this to weigh the priority of looking into it.

+    case webrtc::kProfileMain:
+      return media::H264PROFILE_MAIN;
+    default:
+      DLOG(ERROR) << "Unrecognized H264 video codec profile";
+  }
+  return media::H264PROFILE_BASELINE;

[Pawel Osciak, 2014/10/20 06:28:43]

notreached.

[hshi1, 2014/10/20 16:55:50]

Acknowledged.

+}
+
 }  // namespace
 
 // This private class of RTCVideoEncoder does the actual work of communicating
@@ -574,6 +588,11 @@ int32_t RTCVideoEncoder::InitEncode(const webrtc::VideoCodec* codec_settings,
   DCHECK(thread_checker_.CalledOnValidThread());
   DCHECK(!impl_.get());
 
+  if (codec_settings->codecType == webrtc::kVideoCodecH264) {

[Pawel Osciak, 2014/10/20 06:28:43]

If we can't determine profile on construction, can we remove it from the
constructor?

[hshi1, 2014/10/20 16:55:50]

No we can't because of the way webRTC defines the VideoEncoder interface. webRTC
does not explicitly define video codec profiles (the H264 profile enum is only
added by holmer@ very recently, and it is part of the codecSpecific struct, not
a general concept).

[Pawel Osciak, 2014/10/21 13:41:46]

We could perhaps store webrtc codec in RTCVE constructor and translate only once
we get webrtc profile here from both (or just codec for VP8), and not in the
factory?

[hshi1, 2014/10/22 19:01:16]

Done.

+    video_codec_profile_ =
+        GetH264CodecProfile(codec_settings->codecSpecific.H264.profile);
+  }
+
   weak_factory_.InvalidateWeakPtrs();
   impl_ = new Impl(weak_factory_.GetWeakPtr(), gpu_factories_);
   base::WaitableEvent initialization_waiter(true, false);