Bug 195124 - Call was negotiated with H264 Base Profile 42e01f but encoded in High Profile
Summary: Call was negotiated with H264 Base Profile 42e01f but encoded in High Profile
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: Safari 12
Hardware: Macintosh macOS 10.14
: P2 Critical
Assignee: youenn fablet
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-02-27 14:43 PST by Long Cheng
Modified: 2019-06-10 08:59 PDT (History)
6 users (show)

See Also:


Attachments
SDP Offer from Safari 12.1 (51.39 KB, image/png)
2019-04-12 08:19 PDT, Long Cheng
no flags Details
SDP Answer from our product (47.08 KB, image/png)
2019-04-12 08:19 PDT, Long Cheng
no flags Details
Patch (31.62 KB, patch)
2019-06-04 09:51 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews215 for win-future (13.68 MB, application/zip)
2019-06-04 11:03 PDT, Build Bot
no flags Details
Patch (33.22 KB, patch)
2019-06-04 11:10 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (33.21 KB, patch)
2019-06-04 11:11 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (33.24 KB, patch)
2019-06-04 11:12 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (32.68 KB, patch)
2019-06-04 15:51 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews107 for mac-highsierra-wk2 (3.43 MB, application/zip)
2019-06-04 17:37 PDT, Build Bot
no flags Details
Patch (32.80 KB, patch)
2019-06-04 20:04 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (32.93 KB, patch)
2019-06-05 09:25 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch for landing (33.61 KB, patch)
2019-06-06 15:57 PDT, youenn fablet
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Long Cheng 2019-02-27 14:43:05 PST
Problem:

  In a WebRTC call, the local SDP from Safari claims supporting video codec H264 with profile-level-id 42e01f which is the Base Profile. The remote SDP also supports video codec H264 with profile-level-id 42c01f which is also the Base Profile. In this case, both sides should agree on H264 Base Profile. However, we observed the Safari browser is sending High Profile stream instead.

Expected:

  H264 Base Profile stream is important for many video systems. Please fix this issue by sending Base Profile stream according to the negotiated profile-level-id. Or please let us know how to configure Safari browser to send Base Profile stream only.
Comment 1 Radar WebKit Bug Importer 2019-02-27 15:19:53 PST
<rdar://problem/48453085>
Comment 2 Long Cheng 2019-03-04 14:13:13 PST
Hello, 

Do you need any further information? Did you successfully reproduce this issue? When should I anticipate a fix on this issue? Please let me know if there is any progress. Thank you.

Thanks,
Long Cheng
Comment 3 Long Cheng 2019-03-15 08:45:53 PDT
Hello, 

Do you need any further information? 

Thanks,
Long Cheng
Comment 4 youenn fablet 2019-03-15 08:48:46 PDT
Hi Long Chen,
Thanks for the report and sorry for the delay.
I think we have sufficient information now.
Please feel free to share additional information here or privately like repro cases or services/devices that are affected.
Comment 5 Long Cheng 2019-03-15 08:53:59 PDT
Thanks for your reply. 

May I ask for the status of this bug? Is it in progress of a fix? 

Thanks,
Long Cheng
Comment 6 youenn fablet 2019-03-15 09:03:53 PDT
This is under investigation.
Any information that may help us prioritizing this would be useful.
Comment 7 Long Cheng 2019-03-18 10:34:17 PDT
Hello Youenn,

Thanks for your reply. Please investigate the issue and keep us updated. Appreciate your help. 

We are providing video conference services here in Compunetix. One major job we do is to mix all media streams from a variety of devices (including legacy video room terminals). We are excited to see a great progress of WebRTC implementation in WebKit in the recent two years. There are many customers prefer to use Safari on iOS and Mac. However, due to this issue, these customers have to use Chrome instead.

Here is a test conference you may use to reproduce the issue as well as verify the fix.
https://evergreen.compunetix.com/cx-full/conference/join?autoJoinPasscode=35678554

Thanks,
Long Cheng
Comment 8 youenn fablet 2019-04-08 14:42:54 PDT
Safari now supports VP8, H264 base profile and H264 high profile.
The default codec is H264 high profile, which might be your issue here.

Selection of the base profile can be forced by munging SDP to remove VP8 and H264 high profile.
Or it should be selected if the other party only supports H264 base profile.

I put up https://youennf.github.io/webrtc-tests/src/content/peerconnection/pc-h264-baseline/ which shows that baseline can be forced by munging SDP.

Can you verify on your side whether munging SDP would fix your issue?

I am marking this issue as Invalid for now.
Please reopen the bug if needed.
Comment 9 Long Cheng 2019-04-12 08:19:12 PDT
Created attachment 367321 [details]
SDP Offer from Safari 12.1
Comment 10 Long Cheng 2019-04-12 08:19:43 PDT
Created attachment 367322 [details]
SDP Answer from our product
Comment 11 Long Cheng 2019-04-12 08:25:46 PDT
Comment from our team member:

Hello Youenn,

We are currently already doing what you suggest. If you examine the offer and answer images we have attached you will see that each side supports H.264 base profile and neither supports H.264 high profile. What we have observed is that even with this SDP exchange we are receiving an H.264 RTP stream that contains a profile-level-id set to high profile in the H.264 header and features only available in high profile are present in the video stream. This should not be allowed based on the SDP exchange. Can you please examine this issue from this perspective and let us know if you need anything from us to expedite this? This was tested on a brand new and fully updated Mac Book Pro running Mojave and the latest version of Safari.
Comment 12 Long Cheng 2019-05-13 08:16:25 PDT
Hello Youenn,

Have you successfully verified the H.264 stream headers and features? We believe with the SDP forcing base profile, the H.264 RTP stream still contains a profile-level-id set to high profile in the H.264 header and features only available in high profile are present in the video stream. 

Thanks,
Long Cheng
Comment 13 youenn fablet 2019-05-31 14:36:05 PDT
When using VTB, it seems to work fine with the hardware H264 encoder on my MacBookPro.
This does not work when not using the hardware H264 encoder.
When using VCP, this does not work properly, no matter whether hardware or not.
kVTCompressionPropertyKey_ProfileLevel does not seem to be respected.
Comment 14 youenn fablet 2019-06-04 09:51:11 PDT
Created attachment 371293 [details]
Patch
Comment 15 Build Bot 2019-06-04 11:03:55 PDT
Comment on attachment 371293 [details]
Patch

Attachment 371293 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/12374570

New failing tests:
fast/block/float/float-with-anonymous-previous-sibling.html
Comment 16 Build Bot 2019-06-04 11:03:58 PDT
Created attachment 371302 [details]
Archive of layout-test-results from ews215 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews215  Port: win-future  Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit
Comment 17 youenn fablet 2019-06-04 11:10:18 PDT
Created attachment 371305 [details]
Patch
Comment 18 youenn fablet 2019-06-04 11:11:32 PDT
Created attachment 371306 [details]
Patch
Comment 19 youenn fablet 2019-06-04 11:12:38 PDT
Created attachment 371307 [details]
Patch
Comment 20 youenn fablet 2019-06-04 15:51:31 PDT
Created attachment 371345 [details]
Patch
Comment 21 Build Bot 2019-06-04 17:37:01 PDT
Comment on attachment 371345 [details]
Patch

Attachment 371345 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/12378704

Number of test failures exceeded the failure limit.
Comment 22 Build Bot 2019-06-04 17:37:03 PDT
Created attachment 371358 [details]
Archive of layout-test-results from ews107 for mac-highsierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-highsierra-wk2  Platform: Mac OS X 10.13.6
Comment 23 youenn fablet 2019-06-04 20:04:01 PDT
Created attachment 371365 [details]
Patch
Comment 24 youenn fablet 2019-06-05 09:25:09 PDT
Created attachment 371408 [details]
Patch
Comment 25 Eric Carlson 2019-06-06 05:58:11 PDT
Comment on attachment 371408 [details]
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/RTCVideoEncoderH264.mm:321
> +  VTCompressionSessionRef _compressionSession;

Nit: something like "_vtCompressionSession" would be clearer.

> Source/ThirdParty/libwebrtc/Source/webrtc/sdk/objc/components/video_codec/RTCVideoEncoderH264.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/RTCVideoEncoderH264.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/helpers.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/helpers.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/helpers.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/helpers.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/helpers.cc:106
> +  if (session)
> +    status = VTSessionSetProperty(session, key, value);
> +#if ENABLE_VCP_ENCODER
> +  else
> +    status = webrtc::VCPCompressionSessionSetProperty(vcpSession, key, value);

Ditto
Comment 26 youenn fablet 2019-06-06 15:55:27 PDT
Thanks for the review.

(In reply to Eric Carlson from comment #25)
> Comment on attachment 371408 [details]
> 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

OK

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

OK

> > 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!
> 

Thanks!
 Source/ThirdParty/libwebrtc/Source/webrtc/sdk/objc/components/video_codec/RTCVideoEncoderH264.mm:321
> > +  VTCompressionSessionRef _compressionSession;
> 
> Nit: something like "_vtCompressionSession" would be clearer.

OK

> > Source/ThirdParty/libwebrtc/Source/webrtc/sdk/objc/components/video_codec/RTCVideoEncoderH264.mm:507
> > +    status = 1;
> 
> Is it worth logging an error here to make it clear what failed?

It is logged below.
 
> > Source/ThirdParty/libwebrtc/Source/webrtc/sdk/objc/components/video_codec/RTCVideoEncoderH264.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?

I prefer keeping it this way to limit the changes compared to upstream.
Admittedly, we have already branched quite a bit...

> > Source/ThirdParty/libwebrtc/Source/webrtc/sdk/objc/components/video_codec/helpers.cc:38
> > +void SetVTSessionProperty(VTCompressionSessionRef session, VCPCompressionSessionRef vcpSession,
> 
> Nit: something like "vtSession" would be clearer.

OK
Comment 27 youenn fablet 2019-06-06 15:57:10 PDT
Created attachment 371533 [details]
Patch for landing
Comment 28 WebKit Commit Bot 2019-06-10 08:59:20 PDT
Comment on attachment 371533 [details]
Patch for landing

Clearing flags on attachment: 371533

Committed r246263: <https://trac.webkit.org/changeset/246263>
Comment 29 WebKit Commit Bot 2019-06-10 08:59:22 PDT
All reviewed patches have been landed.  Closing bug.