Bug 179076

Summary: Use VCP H264 encoder for platforms supporting it
Product: WebKit Reporter: youenn fablet <youennf>
Component: WebRTCAssignee: youenn fablet <youennf>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, eric.carlson, jlewis3, jonlee, ryanhaddad, webkit-bug-importer, youennf
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=180858
Bug Depends on: 179274, 179335    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch for landing
none
Build fix
none
Build fix
none
Build fix
none
Trying to fix build again
none
Patch
none
Patch
none
Patch none

Description youenn fablet 2017-10-31 13:59:54 PDT
This will be more efficient
Comment 1 youenn fablet 2017-10-31 14:01:27 PDT
Created attachment 325487 [details]
Patch
Comment 2 Eric Carlson 2017-11-01 09:50:38 PDT
Comment on attachment 325487 [details]
Patch

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

> Source/ThirdParty/libwebrtc/Configurations/libwebrtc.xcconfig:33
> +

Nit: extra blank line

> Source/ThirdParty/libwebrtc/Source/webrtc/sdk/objc/Framework/Classes/VideoProcessing/encoder_vcp.h:8
> + *  Copyright (c) 2015 The WebRTC project authors. All Rights Reserved.
> + *
> + *  Use of this source code is governed by a BSD-style license
> + *  that can be found in the LICENSE file in the root of the source
> + *  tree. An additional intellectual property rights grant can be found
> + *  in the file PATENTS.  All contributing project authors may
> + *  be found in the AUTHORS file in the root of the source tree.

Is this the correct header?

> Source/ThirdParty/libwebrtc/Source/webrtc/sdk/objc/Framework/Classes/VideoProcessing/encoder_vcp.h:43
> +// This file provides a H264 encoder implementation using the VideoToolbox
> +// APIs. Since documentation is almost non-existent, this is largely based on
> +// the information in the VideoToolbox header files, a talk from WWDC 2014 and
> +// experimentation.

This is incorrect.
Comment 3 youenn fablet 2017-11-02 15:06:37 PDT
Created attachment 325773 [details]
Patch
Comment 4 Jon Lee 2017-11-02 17:06:20 PDT
Comment on attachment 325773 [details]
Patch

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

> Source/ThirdParty/libwebrtc/ChangeLog:4
> +        https://bugs.webkit.org/show_bug.cgi?id=179076

please add reference to radar.
Comment 5 Jon Lee 2017-11-02 17:06:34 PDT
rdar://problem/35180773
Comment 6 youenn fablet 2017-11-02 17:38:55 PDT
Created attachment 325803 [details]
Patch
Comment 7 youenn fablet 2017-11-02 17:39:26 PDT
(In reply to youenn fablet from comment #6)
> Created attachment 325803 [details]
> Patch

Fixing macOS version.
Need to handle iOS sim/iOS missing private framework.
Comment 8 youenn fablet 2017-11-03 09:49:59 PDT
Created attachment 325908 [details]
Patch
Comment 9 youenn fablet 2017-11-03 10:46:03 PDT
Comment on attachment 325908 [details]
Patch

Patch only activates VCP for MacOS right now.
It should be extended to iOS at some point.
Comment 10 Eric Carlson 2017-11-03 12:05:55 PDT
Comment on attachment 325908 [details]
Patch

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

> Source/ThirdParty/libwebrtc/Source/webrtc/sdk/objc/Framework/Classes/VideoProcessing/encoder_vcp.h:3
> + *  Copyright (c) 2017 Apple INC. All Rights Reserved.

Nit: Apple INC => Apple Inc

> Source/ThirdParty/libwebrtc/Source/webrtc/sdk/objc/Framework/Classes/VideoProcessing/encoder_vcp.mm:3
> + *  Copyright (c) 2017 Apple INC. All Rights Reserved.

Ditto.
Comment 11 youenn fablet 2017-11-03 13:53:19 PDT
Created attachment 325956 [details]
Patch for landing
Comment 12 youenn fablet 2017-11-03 13:53:53 PDT
Thanks for the review.
Patch updated accordingly.
Comment 13 WebKit Commit Bot 2017-11-03 14:13:39 PDT
Comment on attachment 325956 [details]
Patch for landing

Clearing flags on attachment: 325956

Committed r224428: <https://trac.webkit.org/changeset/224428>
Comment 14 WebKit Commit Bot 2017-11-03 14:13:41 PDT
All reviewed patches have been landed.  Closing bug.
Comment 15 youenn fablet 2017-11-03 14:47:40 PDT
Reopening to attach new patch.
Comment 16 youenn fablet 2017-11-03 14:47:40 PDT
Created attachment 325966 [details]
Build fix
Comment 17 WebKit Commit Bot 2017-11-03 15:03:34 PDT
Comment on attachment 325966 [details]
Build fix

Clearing flags on attachment: 325966

Committed r224435: <https://trac.webkit.org/changeset/224435>
Comment 18 WebKit Commit Bot 2017-11-03 15:03:36 PDT
All reviewed patches have been landed.  Closing bug.
Comment 19 youenn fablet 2017-11-03 15:37:15 PDT
Reopening to attach new patch.
Comment 20 youenn fablet 2017-11-03 15:37:16 PDT
Created attachment 325974 [details]
Build fix
Comment 21 youenn fablet 2017-11-03 15:48:19 PDT
Created attachment 325977 [details]
Build fix
Comment 22 WebKit Commit Bot 2017-11-03 16:21:51 PDT
Comment on attachment 325977 [details]
Build fix

Clearing flags on attachment: 325977

Committed r224440: <https://trac.webkit.org/changeset/224440>
Comment 23 WebKit Commit Bot 2017-11-03 16:21:52 PDT
All reviewed patches have been landed.  Closing bug.
Comment 24 WebKit Commit Bot 2017-11-03 16:30:17 PDT
Re-opened since this is blocked by bug 179274
Comment 25 youenn fablet 2017-11-03 17:18:36 PDT
Created attachment 325992 [details]
Trying to fix build again
Comment 26 WebKit Commit Bot 2017-11-06 09:48:49 PST
Comment on attachment 325992 [details]
Trying to fix build again

Clearing flags on attachment: 325992

Committed r224497: <https://trac.webkit.org/changeset/224497>
Comment 27 WebKit Commit Bot 2017-11-06 09:48:50 PST
All reviewed patches have been landed.  Closing bug.
Comment 28 WebKit Commit Bot 2017-11-06 11:45:57 PST
Re-opened since this is blocked by bug 179335
Comment 29 youenn fablet 2017-12-11 13:16:38 PST
Created attachment 329025 [details]
Patch
Comment 30 youenn fablet 2017-12-11 14:07:58 PST
Created attachment 329032 [details]
Patch
Comment 31 youenn fablet 2017-12-11 15:03:54 PST
Created attachment 329040 [details]
Patch
Comment 32 WebKit Commit Bot 2017-12-11 15:37:42 PST
Comment on attachment 329040 [details]
Patch

Clearing flags on attachment: 329040

Committed r225761: <https://trac.webkit.org/changeset/225761>
Comment 33 WebKit Commit Bot 2017-12-11 15:37:44 PST
All reviewed patches have been landed.  Closing bug.
Comment 34 Matt Lewis 2017-12-12 10:47:04 PST
This caused an API failure with test ContentFiltering.LazilyLoadPlatformFrameworks

https://build.webkit.org/builders/Apple%20High%20Sierra%20Release%20WK2%20%28Tests%29/builds/1689
https://build.webkit.org/builders/Apple%20High%20Sierra%20Release%20WK2%20%28Tests%29/builds/1689/steps/run-api-tests/logs/stdio

Failure:


FAIL ContentFiltering.LazilyLoadPlatformFrameworks

/Volumes/Data/slave/highsierra-release/build/Tools/TestWebKitAPI/Tests/WebKitCocoa/ContentFiltering.mm:366
Value of: static_cast<bool>(networkExtensionLoaded)
  Actual: true
Expected: static_cast<bool>(networkExtensionShouldBeLoaded)
Which is: false


/Volumes/Data/slave/highsierra-release/build/Tools/TestWebKitAPI/Tests/WebKitCocoa/ContentFiltering.mm:366
Value of: static_cast<bool>(networkExtensionLoaded)
  Actual: true
Expected: static_cast<bool>(networkExtensionShouldBeLoaded)
Which is: false


/Volumes/Data/slave/highsierra-release/build/Tools/TestWebKitAPI/Tests/WebKitCocoa/ContentFiltering.mm:366
Value of: static_cast<bool>(networkExtensionLoaded)
  Actual: true
Expected: static_cast<bool>(networkExtensionShouldBeLoaded)
Which is: false


/Volumes/Data/slave/highsierra-release/build/Tools/TestWebKitAPI/Tests/WebKitCocoa/ContentFiltering.mm:366
Value of: static_cast<bool>(networkExtensionLoaded)
  Actual: true
Expected: static_cast<bool>(networkExtensionShouldBeLoaded)
Which is: false


/Volumes/Data/slave/highsierra-release/build/Tools/TestWebKitAPI/Tests/WebKitCocoa/ContentFiltering.mm:366
Value of: static_cast<bool>(networkExtensionLoaded)
  Actual: true
Expected: static_cast<bool>(networkExtensionShouldBeLoaded)
Which is: false