[webkit-reviews] review granted: [Bug 195124] Call was negotiated with H264 Base Profile 42e01f but encoded in High Profile : [Attachment 371408] Patch

[bugzilla-daemon at webkit.org]

Eric Carlson <eric.carlson at apple.com> has granted youenn fablet
<youennf at gmail.com>'s request for review:
Bug 195124: Call was negotiated with H264 Base Profile 42e01f but encoded in
High Profile
https://bugs.webkit.org/show_bug.cgi?id=195124

Attachment 371408: Patch

https://bugs.webkit.org/attachment.cgi?id=371408&action=review




--- Comment #25 from Eric Carlson <eric.carlson at apple.com> ---
Comment on attachment 371408
  --> https://bugs.webkit.org/attachment.cgi?id=371408
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=371408&action=review

>
Source/ThirdParty/libwebrtc/Source/webrtc/sdk/WebKit/VideoProcessingSoftLink.h:
37
>  #define ENABLE_VCP_ENCODER 0

Nit: not new, but there is an extra space before the "&&" in the line above
this

>
Source/ThirdParty/libwebrtc/Source/webrtc/sdk/WebKit/VideoProcessingSoftLink.h:
39
>  #elif (defined(TARGET_OS_IPHONE)  && TARGET_OS_IPHONE)

Nit: Ditto.

>
Source/ThirdParty/libwebrtc/Source/webrtc/sdk/WebKit/VideoProcessingSoftLink.h:
44
> +//#define ENABLE_VCP_ENCODER __MAC_OS_X_VERSION_MIN_REQUIRED >= 101300
> +//#define ENABLE_VCP_VTB_ENCODER __MAC_OS_X_VERSION_MIN_REQUIRED >= 101500

Oops!

>
Source/ThirdParty/libwebrtc/Source/webrtc/sdk/objc/components/video_codec/RTCVi
deoEncoderH264.mm:321
> +  VTCompressionSessionRef _compressionSession;

Nit: something like "_vtCompressionSession" would be clearer.

>
Source/ThirdParty/libwebrtc/Source/webrtc/sdk/objc/components/video_codec/RTCVi
deoEncoderH264.mm:507
> +    status = 1;

Is it worth logging an error here to make it clear what failed?

>
Source/ThirdParty/libwebrtc/Source/webrtc/sdk/objc/components/video_codec/RTCVi
deoEncoderH264.mm:876
> +	 RTC_LOG(LS_ERROR) << "VTSessionSetProperty failed to set: " <<
kVTCompressionPropertyKey_DataRateLimits

Nit: kVTCompressionPropertyKey_DataRateLimits is a constant, why not hard code
it in the string?

>
Source/ThirdParty/libwebrtc/Source/webrtc/sdk/objc/components/video_codec/helpe
rs.cc:38
> +void SetVTSessionProperty(VTCompressionSessionRef session,
VCPCompressionSessionRef vcpSession,

Nit: something like "vtSession" would be clearer.

>
Source/ThirdParty/libwebrtc/Source/webrtc/sdk/objc/components/video_codec/helpe
rs.cc:48
> +  if (session)
> +    status = VTSessionSetProperty(session, key, cfNum);
> +#if ENABLE_VCP_ENCODER
> +  else
> +    status = webrtc::VCPCompressionSessionSetProperty(vcpSession, key,
cfNum);

If we are going to assume vcpSession is valid when session is NULL, maybe
ASSERT(session || vcpSession)?

>
Source/ThirdParty/libwebrtc/Source/webrtc/sdk/objc/components/video_codec/helpe
rs.cc:70
> +  if (session)
> +    status = VTSessionSetProperty(session, key, cfNum);
> +#if ENABLE_VCP_ENCODER
> +  else
> +    status = webrtc::VCPCompressionSessionSetProperty(vcpSession, key,
cfNum);

Ditto.

>
Source/ThirdParty/libwebrtc/Source/webrtc/sdk/objc/components/video_codec/helpe
rs.cc:88
> +  if (session)
> +    status = VTSessionSetProperty(session, key, cf_bool);
> +#if ENABLE_VCP_ENCODER
> +  else
> +    status = webrtc::VCPCompressionSessionSetProperty(vcpSession, key,
cf_bool);

Ditto.

>
Source/ThirdParty/libwebrtc/Source/webrtc/sdk/objc/components/video_codec/helpe
rs.cc:106
> +  if (session)
> +    status = VTSessionSetProperty(session, key, value);
> +#if ENABLE_VCP_ENCODER
> +  else
> +    status = webrtc::VCPCompressionSessionSetProperty(vcpSession, key,
value);

Ditto