WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
188745
Update libwebrtc up to 984f1a80c0
https://bugs.webkit.org/show_bug.cgi?id=188745
Summary
Update libwebrtc up to 984f1a80c0
youenn fablet
Reported
2018-08-20 08:54:47 PDT
This revision is not stable but has support for all the new features we actually want to use.
Attachments
WIP
(29.71 MB, patch)
2018-08-20 10:40 PDT
,
youenn fablet
no flags
Details
Formatted Diff
Diff
WIP
(29.43 MB, patch)
2018-08-20 10:57 PDT
,
youenn fablet
no flags
Details
Formatted Diff
Diff
WIP
(62.97 MB, patch)
2018-08-20 11:01 PDT
,
youenn fablet
no flags
Details
Formatted Diff
Diff
WIP
(62.36 MB, patch)
2018-08-20 16:41 PDT
,
youenn fablet
no flags
Details
Formatted Diff
Diff
WIP
(62.97 MB, patch)
2018-08-20 21:21 PDT
,
youenn fablet
no flags
Details
Formatted Diff
Diff
WIP
(62.60 MB, patch)
2018-08-21 13:26 PDT
,
youenn fablet
no flags
Details
Formatted Diff
Diff
patch
(56.02 MB, patch)
2018-08-21 14:35 PDT
,
youenn fablet
no flags
Details
Formatted Diff
Diff
patch
(56.02 MB, patch)
2018-08-21 17:06 PDT
,
youenn fablet
no flags
Details
Formatted Diff
Diff
patch
(56.04 MB, patch)
2018-08-21 22:50 PDT
,
youenn fablet
no flags
Details
Formatted Diff
Diff
WIP GTK compilation patch
(44.75 KB, patch)
2018-08-22 08:20 PDT
,
Alejandro G. Castro
no flags
Details
Formatted Diff
Diff
patch
(56.08 MB, patch)
2018-08-22 09:40 PDT
,
youenn fablet
eric.carlson
: review+
Details
Formatted Diff
Diff
reduced patch that contains only the changes after resyncing libwebrtc
(844.46 KB, patch)
2018-08-22 13:35 PDT
,
youenn fablet
no flags
Details
Formatted Diff
Diff
Show Obsolete
(10)
View All
Add attachment
proposed patch, testcase, etc.
youenn fablet
Comment 1
2018-08-20 10:40:36 PDT
Created
attachment 347508
[details]
WIP
youenn fablet
Comment 2
2018-08-20 10:57:38 PDT
Created
attachment 347510
[details]
WIP
youenn fablet
Comment 3
2018-08-20 11:01:57 PDT
Created
attachment 347512
[details]
WIP
youenn fablet
Comment 4
2018-08-20 16:41:59 PDT
Created
attachment 347568
[details]
WIP
EWS Watchlist
Comment 5
2018-08-20 18:22:05 PDT
Attachment 347568
[details]
did not pass style-queue: Traceback (most recent call last): File "/Volumes/Data/StyleQueue/WebKit/Tools/Scripts/webkit-patch", line 84, in <module> main() File "/Volumes/Data/StyleQueue/WebKit/Tools/Scripts/webkit-patch", line 79, in main WebKitPatch(os.path.abspath(__file__)).main() File "/Volumes/Data/StyleQueue/WebKit/Tools/Scripts/webkitpy/tool/multicommandtool.py", line 305, in main result = command.check_arguments_and_execute(options, args, self) File "/Volumes/Data/StyleQueue/WebKit/Tools/Scripts/webkitpy/tool/multicommandtool.py", line 123, in check_arguments_and_execute return self.execute(options, args, tool) or 0 File "/Volumes/Data/StyleQueue/WebKit/Tools/Scripts/webkitpy/tool/commands/abstractsequencedcommand.py", line 55, in execute self._sequence.run_and_handle_errors(tool, options, state) File "/Volumes/Data/StyleQueue/WebKit/Tools/Scripts/webkitpy/tool/commands/stepsequence.py", line 73, in run_and_handle_errors self._run(tool, options, state) File "/Volumes/Data/StyleQueue/WebKit/Tools/Scripts/webkitpy/tool/commands/stepsequence.py", line 67, in _run step(tool, options).run(state) File "/Volumes/Data/StyleQueue/WebKit/Tools/Scripts/webkitpy/tool/steps/checkstyle.py", line 66, in run self._tool.executive.run_and_throw_if_fail(self._tool.deprecated_port().check_webkit_style_command() + args, cwd=self._tool.scm().checkout_root) File "/Volumes/Data/StyleQueue/WebKit/Tools/Scripts/webkitpy/common/system/executive.py", line 135, in run_and_throw_if_fail exit_code = self._run_command_with_teed_output(args, child_stdout, **kwargs) File "/Volumes/Data/StyleQueue/WebKit/Tools/Scripts/webkitpy/common/system/executive.py", line 107, in _run_command_with_teed_output **kwargs) File "/Volumes/Data/StyleQueue/WebKit/Tools/Scripts/webkitpy/common/system/executive.py", line 481, in popen return subprocess.Popen(string_args, **kwargs) File "/System/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/subprocess.py", line 710, in __init__ errread, errwrite) File "/System/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/subprocess.py", line 1335, in _execute_child raise child_exception OSError: [Errno 7] Argument list too long If any of these errors are false positives, please file a bug against check-webkit-style.
Radar WebKit Bug Importer
Comment 6
2018-08-20 18:22:59 PDT
<
rdar://problem/43539177
>
youenn fablet
Comment 7
2018-08-20 21:21:03 PDT
Created
attachment 347607
[details]
WIP
youenn fablet
Comment 8
2018-08-21 13:26:11 PDT
Created
attachment 347679
[details]
WIP
youenn fablet
Comment 9
2018-08-21 14:35:07 PDT
Created
attachment 347698
[details]
patch
EWS Watchlist
Comment 10
2018-08-21 15:20:51 PDT
Attachment 347698
[details]
did not pass style-queue: Traceback (most recent call last): File "/Volumes/Data/StyleQueue/WebKit/Tools/Scripts/webkit-patch", line 84, in <module> main() File "/Volumes/Data/StyleQueue/WebKit/Tools/Scripts/webkit-patch", line 79, in main WebKitPatch(os.path.abspath(__file__)).main() File "/Volumes/Data/StyleQueue/WebKit/Tools/Scripts/webkitpy/tool/multicommandtool.py", line 305, in main result = command.check_arguments_and_execute(options, args, self) File "/Volumes/Data/StyleQueue/WebKit/Tools/Scripts/webkitpy/tool/multicommandtool.py", line 123, in check_arguments_and_execute return self.execute(options, args, tool) or 0 File "/Volumes/Data/StyleQueue/WebKit/Tools/Scripts/webkitpy/tool/commands/abstractsequencedcommand.py", line 55, in execute self._sequence.run_and_handle_errors(tool, options, state) File "/Volumes/Data/StyleQueue/WebKit/Tools/Scripts/webkitpy/tool/commands/stepsequence.py", line 73, in run_and_handle_errors self._run(tool, options, state) File "/Volumes/Data/StyleQueue/WebKit/Tools/Scripts/webkitpy/tool/commands/stepsequence.py", line 67, in _run step(tool, options).run(state) File "/Volumes/Data/StyleQueue/WebKit/Tools/Scripts/webkitpy/tool/steps/checkstyle.py", line 66, in run self._tool.executive.run_and_throw_if_fail(self._tool.deprecated_port().check_webkit_style_command() + args, cwd=self._tool.scm().checkout_root) File "/Volumes/Data/StyleQueue/WebKit/Tools/Scripts/webkitpy/common/system/executive.py", line 135, in run_and_throw_if_fail exit_code = self._run_command_with_teed_output(args, child_stdout, **kwargs) File "/Volumes/Data/StyleQueue/WebKit/Tools/Scripts/webkitpy/common/system/executive.py", line 107, in _run_command_with_teed_output **kwargs) File "/Volumes/Data/StyleQueue/WebKit/Tools/Scripts/webkitpy/common/system/executive.py", line 481, in popen return subprocess.Popen(string_args, **kwargs) File "/System/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/subprocess.py", line 710, in __init__ errread, errwrite) File "/System/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/subprocess.py", line 1335, in _execute_child raise child_exception OSError: [Errno 7] Argument list too long If any of these errors are false positives, please file a bug against check-webkit-style.
youenn fablet
Comment 11
2018-08-21 17:06:30 PDT
Created
attachment 347735
[details]
patch
EWS Watchlist
Comment 12
2018-08-21 17:52:01 PDT
Attachment 347735
[details]
did not pass style-queue: Traceback (most recent call last): File "/Volumes/Data/StyleQueue/WebKit/Tools/Scripts/webkit-patch", line 84, in <module> main() File "/Volumes/Data/StyleQueue/WebKit/Tools/Scripts/webkit-patch", line 79, in main WebKitPatch(os.path.abspath(__file__)).main() File "/Volumes/Data/StyleQueue/WebKit/Tools/Scripts/webkitpy/tool/multicommandtool.py", line 305, in main result = command.check_arguments_and_execute(options, args, self) File "/Volumes/Data/StyleQueue/WebKit/Tools/Scripts/webkitpy/tool/multicommandtool.py", line 123, in check_arguments_and_execute return self.execute(options, args, tool) or 0 File "/Volumes/Data/StyleQueue/WebKit/Tools/Scripts/webkitpy/tool/commands/abstractsequencedcommand.py", line 55, in execute self._sequence.run_and_handle_errors(tool, options, state) File "/Volumes/Data/StyleQueue/WebKit/Tools/Scripts/webkitpy/tool/commands/stepsequence.py", line 73, in run_and_handle_errors self._run(tool, options, state) File "/Volumes/Data/StyleQueue/WebKit/Tools/Scripts/webkitpy/tool/commands/stepsequence.py", line 67, in _run step(tool, options).run(state) File "/Volumes/Data/StyleQueue/WebKit/Tools/Scripts/webkitpy/tool/steps/checkstyle.py", line 66, in run self._tool.executive.run_and_throw_if_fail(self._tool.deprecated_port().check_webkit_style_command() + args, cwd=self._tool.scm().checkout_root) File "/Volumes/Data/StyleQueue/WebKit/Tools/Scripts/webkitpy/common/system/executive.py", line 135, in run_and_throw_if_fail exit_code = self._run_command_with_teed_output(args, child_stdout, **kwargs) File "/Volumes/Data/StyleQueue/WebKit/Tools/Scripts/webkitpy/common/system/executive.py", line 107, in _run_command_with_teed_output **kwargs) File "/Volumes/Data/StyleQueue/WebKit/Tools/Scripts/webkitpy/common/system/executive.py", line 481, in popen return subprocess.Popen(string_args, **kwargs) File "/System/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/subprocess.py", line 710, in __init__ errread, errwrite) File "/System/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/subprocess.py", line 1335, in _execute_child raise child_exception OSError: [Errno 7] Argument list too long If any of these errors are false positives, please file a bug against check-webkit-style.
youenn fablet
Comment 13
2018-08-21 22:50:26 PDT
Created
attachment 347769
[details]
patch
EWS Watchlist
Comment 14
2018-08-21 23:38:06 PDT
Attachment 347769
[details]
did not pass style-queue: Traceback (most recent call last): File "/Volumes/Data/StyleQueue/WebKit/Tools/Scripts/webkit-patch", line 84, in <module> main() File "/Volumes/Data/StyleQueue/WebKit/Tools/Scripts/webkit-patch", line 79, in main WebKitPatch(os.path.abspath(__file__)).main() File "/Volumes/Data/StyleQueue/WebKit/Tools/Scripts/webkitpy/tool/multicommandtool.py", line 305, in main result = command.check_arguments_and_execute(options, args, self) File "/Volumes/Data/StyleQueue/WebKit/Tools/Scripts/webkitpy/tool/multicommandtool.py", line 123, in check_arguments_and_execute return self.execute(options, args, tool) or 0 File "/Volumes/Data/StyleQueue/WebKit/Tools/Scripts/webkitpy/tool/commands/abstractsequencedcommand.py", line 55, in execute self._sequence.run_and_handle_errors(tool, options, state) File "/Volumes/Data/StyleQueue/WebKit/Tools/Scripts/webkitpy/tool/commands/stepsequence.py", line 73, in run_and_handle_errors self._run(tool, options, state) File "/Volumes/Data/StyleQueue/WebKit/Tools/Scripts/webkitpy/tool/commands/stepsequence.py", line 67, in _run step(tool, options).run(state) File "/Volumes/Data/StyleQueue/WebKit/Tools/Scripts/webkitpy/tool/steps/checkstyle.py", line 66, in run self._tool.executive.run_and_throw_if_fail(self._tool.deprecated_port().check_webkit_style_command() + args, cwd=self._tool.scm().checkout_root) File "/Volumes/Data/StyleQueue/WebKit/Tools/Scripts/webkitpy/common/system/executive.py", line 135, in run_and_throw_if_fail exit_code = self._run_command_with_teed_output(args, child_stdout, **kwargs) File "/Volumes/Data/StyleQueue/WebKit/Tools/Scripts/webkitpy/common/system/executive.py", line 107, in _run_command_with_teed_output **kwargs) File "/Volumes/Data/StyleQueue/WebKit/Tools/Scripts/webkitpy/common/system/executive.py", line 481, in popen return subprocess.Popen(string_args, **kwargs) File "/System/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/subprocess.py", line 710, in __init__ errread, errwrite) File "/System/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/subprocess.py", line 1335, in _execute_child raise child_exception OSError: [Errno 7] Argument list too long If any of these errors are false positives, please file a bug against check-webkit-style.
Alejandro G. Castro
Comment 15
2018-08-22 08:20:58 PDT
Created
attachment 347795
[details]
WIP GTK compilation patch I made the compilation work for GTK with this patch. It applies over the 347769 patch, I guess we can merge them.
youenn fablet
Comment 16
2018-08-22 08:59:49 PDT
(In reply to Alejandro G. Castro from
comment #15
)
> Created
attachment 347795
[details]
> WIP GTK compilation patch > > I made the compilation work for GTK with this patch. It applies over the > 347769 patch, I guess we can merge them.
Nice!
youenn fablet
Comment 17
2018-08-22 09:02:00 PDT
Comment on
attachment 347795
[details]
WIP GTK compilation patch View in context:
https://bugs.webkit.org/attachment.cgi?id=347795&action=review
> Source/WTF/wtf/DisallowCType.h:43 > +#if !(OS(DARWIN) && PLATFORM(GTK)) && !PLATFORM(WPE) && !PLATFORM(GTK) && !defined(_LIBCPP_VERSION) && defined(__GLIBC__)
Is this change needed? PLATFORM(GTK) appears twice also, I guess it could be simplified.
youenn fablet
Comment 18
2018-08-22 09:40:00 PDT
Created
attachment 347808
[details]
patch
youenn fablet
Comment 19
2018-08-22 09:41:45 PDT
(In reply to youenn fablet from
comment #18
)
> Created
attachment 347808
[details]
> patch
Merged GTK changes, let's see what will happen...
EWS Watchlist
Comment 20
2018-08-22 10:27:40 PDT
Attachment 347808
[details]
did not pass style-queue: Traceback (most recent call last): File "/Volumes/Data/StyleQueue/WebKit/Tools/Scripts/webkit-patch", line 84, in <module> main() File "/Volumes/Data/StyleQueue/WebKit/Tools/Scripts/webkit-patch", line 79, in main WebKitPatch(os.path.abspath(__file__)).main() File "/Volumes/Data/StyleQueue/WebKit/Tools/Scripts/webkitpy/tool/multicommandtool.py", line 305, in main result = command.check_arguments_and_execute(options, args, self) File "/Volumes/Data/StyleQueue/WebKit/Tools/Scripts/webkitpy/tool/multicommandtool.py", line 123, in check_arguments_and_execute return self.execute(options, args, tool) or 0 File "/Volumes/Data/StyleQueue/WebKit/Tools/Scripts/webkitpy/tool/commands/abstractsequencedcommand.py", line 55, in execute self._sequence.run_and_handle_errors(tool, options, state) File "/Volumes/Data/StyleQueue/WebKit/Tools/Scripts/webkitpy/tool/commands/stepsequence.py", line 73, in run_and_handle_errors self._run(tool, options, state) File "/Volumes/Data/StyleQueue/WebKit/Tools/Scripts/webkitpy/tool/commands/stepsequence.py", line 67, in _run step(tool, options).run(state) File "/Volumes/Data/StyleQueue/WebKit/Tools/Scripts/webkitpy/tool/steps/checkstyle.py", line 66, in run self._tool.executive.run_and_throw_if_fail(self._tool.deprecated_port().check_webkit_style_command() + args, cwd=self._tool.scm().checkout_root) File "/Volumes/Data/StyleQueue/WebKit/Tools/Scripts/webkitpy/common/system/executive.py", line 135, in run_and_throw_if_fail exit_code = self._run_command_with_teed_output(args, child_stdout, **kwargs) File "/Volumes/Data/StyleQueue/WebKit/Tools/Scripts/webkitpy/common/system/executive.py", line 107, in _run_command_with_teed_output **kwargs) File "/Volumes/Data/StyleQueue/WebKit/Tools/Scripts/webkitpy/common/system/executive.py", line 481, in popen return subprocess.Popen(string_args, **kwargs) File "/System/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/subprocess.py", line 710, in __init__ errread, errwrite) File "/System/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/subprocess.py", line 1335, in _execute_child raise child_exception OSError: [Errno 7] Argument list too long If any of these errors are false positives, please file a bug against check-webkit-style.
youenn fablet
Comment 21
2018-08-22 13:35:46 PDT
Created
attachment 347839
[details]
reduced patch that contains only the changes after resyncing libwebrtc Might help doing the review
Alejandro G. Castro
Comment 22
2018-08-23 01:36:32 PDT
Comment on
attachment 347839
[details]
reduced patch that contains only the changes after resyncing libwebrtc View in context:
https://bugs.webkit.org/attachment.cgi?id=347839&action=review
> Source/WebCore/Modules/mediastream/libwebrtc/LibWebRTCDataChannelHandler.h:33 > +#pragma clang diagnostic push > +#pragma clang diagnostic ignored "-Wunused-parameter"
Is this required? We should use something that can be used for gcc or control it is just clang using it, it causes a ton of warnings in the gcc compilation. I wonder if it is cleaner for us to modify the include in the library.
> Source/WebCore/Modules/mediastream/libwebrtc/LibWebRTCMediaEndpoint.h:37 > +#pragma clang diagnostic push > +#pragma clang diagnostic ignored "-Wunused-parameter"
Ditto
> Source/WebCore/platform/mediastream/RealtimeIncomingAudioSource.h:39 > + > +#pragma clang diagnostic push > +#pragma clang diagnostic ignored "-Wunused-parameter"
Ditto
> Source/WebCore/platform/mediastream/RealtimeIncomingVideoSource.h:39 > +#pragma clang diagnostic push > +#pragma clang diagnostic ignored "-Wunused-parameter"
Ditto
> Source/WebCore/platform/mediastream/RealtimeOutgoingAudioSource.h:39 > +#pragma clang diagnostic push > +#pragma clang diagnostic ignored "-Wunused-parameter"
Ditto
> Source/WebCore/platform/mediastream/RealtimeOutgoingVideoSource.h:38 > +#pragma clang diagnostic push > +#pragma clang diagnostic ignored "-Wunused-parameter"
Ditto
> Source/WebCore/platform/mediastream/libwebrtc/LibWebRTCAudioModule.h:33 > +#pragma clang diagnostic push > +#pragma clang diagnostic ignored "-Wunused-parameter"
Ditto
> Source/WebCore/platform/mediastream/libwebrtc/LibWebRTCProvider.cpp:35 > +#pragma clang diagnostic push > +#pragma clang diagnostic ignored "-Wunused-parameter"
Ditto
> Source/WebCore/platform/mediastream/libwebrtc/LibWebRTCProvider.h:39 > +#pragma clang diagnostic push > +#pragma clang diagnostic ignored "-Wunused-parameter"
Ditto
> Source/WebCore/platform/mediastream/mac/RealtimeIncomingVideoSourceCocoa.mm:38 > +#pragma clang diagnostic push > +#pragma clang diagnostic ignored "-Wunused-parameter"
Ditto
> Source/WebCore/platform/mediastream/mac/RealtimeOutgoingVideoSourceCocoa.cpp:34 > +#pragma clang diagnostic push > +#pragma clang diagnostic ignored "-Wunused-parameter"
Ditto
> Source/WebCore/testing/MockLibWebRTCPeerConnection.h:32 > +#pragma clang diagnostic push > +#pragma clang diagnostic ignored "-Wunused-parameter"
Ditto
Alejandro G. Castro
Comment 23
2018-08-23 02:09:20 PDT
(In reply to youenn fablet from
comment #17
)
> Comment on
attachment 347795
[details]
> WIP GTK compilation patch > > View in context: >
https://bugs.webkit.org/attachment.cgi?id=347795&action=review
> > > Source/WTF/wtf/DisallowCType.h:43 > > +#if !(OS(DARWIN) && PLATFORM(GTK)) && !PLATFORM(WPE) && !PLATFORM(GTK) && !defined(_LIBCPP_VERSION) && defined(__GLIBC__) > > Is this change needed? > PLATFORM(GTK) appears twice also, I guess it could be simplified.
Unfortunately it is needed because stringutils.h inside libwebrtc uses tolower. I wonder why it does not happen for mac ports ... I forgot to review the change I used initially to test the compilation. I think we can add a comment and a more accurate condition like this: diff --git a/Source/WTF/wtf/DisallowCType.h b/Source/WTF/wtf/DisallowCType.h index 30544e729fd..de71b79155d 100644 --- a/Source/WTF/wtf/DisallowCType.h +++ b/Source/WTF/wtf/DisallowCType.h @@ -39,8 +39,9 @@ // Also generates errors on wx on Windows, presumably because these functions // are used from wx headers. On GTK+ for Mac many GTK+ files include <libintl.h> // or <glib/gi18n-lib.h>, which in turn include <xlocale/_ctype.h> which uses -// isacii(). -#if !(OS(DARWIN) && PLATFORM(GTK)) && !defined(_LIBCPP_VERSION) && defined(__GLIBC__) +// isacii(). This breaks the compilation for libwebrtc in GTK and WPE, it +// includes a stringutils.h header that uses tolower. +#if !(OS(DARWIN) && PLATFORM(GTK)) && !(USE(LIBWEBRTC) && (PLATFORM(WPE) || PLATFORM(GTK))) && !defined(_LIBCPP_VERSION) && defined(__GLIBC__) #include <ctype.h> The other option is to remove that function in libwebrtc header stringutils.h, it is not used. Both options are ugly, maybe we can find a different option.
Alejandro G. Castro
Comment 24
2018-08-23 02:29:43 PDT
(In reply to Alejandro G. Castro from
comment #23
) >
> [...]
>
> The other option is to remove that function in libwebrtc header > stringutils.h, it is not used. > > Both options are ugly, maybe we can find a different option.
With removing I meant something like this: diff --git a/Source/ThirdParty/libwebrtc/Source/webrtc/rtc_base/stringutils.h b/Source/ThirdParty/libwebrtc/Source/webrtc/rtc_base/stringutils.h index d92ba0260f9..883b0584e8e 100644 --- a/Source/ThirdParty/libwebrtc/Source/webrtc/rtc_base/stringutils.h +++ b/Source/ThirdParty/libwebrtc/Source/webrtc/rtc_base/stringutils.h @@ -50,6 +50,7 @@ bool string_match(const char* target, const char* pattern); } // namespace rtc +#if !defined(WEBRTC_WEBKIT_BUILD) /////////////////////////////////////////////////////////////////////////////// // Rename a few common string functions so they are consistent across platforms. // tolowercase is like tolower, but not compatible with end-of-file value @@ -61,6 +62,7 @@ bool string_match(const char* target, const char* pattern); inline char tolowercase(char c) { return static_cast<char>(tolower(c)); } +#endif #if defined(WEBRTC_WIN)
youenn fablet
Comment 25
2018-08-23 08:36:19 PDT
> > Source/WebCore/Modules/mediastream/libwebrtc/LibWebRTCDataChannelHandler.h:33 > > +#pragma clang diagnostic push > > +#pragma clang diagnostic ignored "-Wunused-parameter" > > Is this required? We should use something that can be used for gcc or > control it is just clang using it, it causes a ton of warnings in the gcc > compilation. I wonder if it is cleaner for us to modify the include in the > library.
WebCore on Mac/iOS ports is using -Werror and -Wunused-parameter. Previously, we were updating the libwebrtc source files but this is tedious and creates a lot of conflicts every time we update libwebrtc. The ideal solution would be to progressively migrate code including these headers in libwebrtc and create new libwebrtc headers matching WebCore/WebKit constraints. I would like to land it as is and we could improve with a follow-up patch to fix GCC warnings, maybe adding macros to support easily CLANG and GCC. (In reply to Alejandro G. Castro from
comment #24
)
> (In reply to Alejandro G. Castro from
comment #23
) > > > > [...] > > > > The other option is to remove that function in libwebrtc header > > stringutils.h, it is not used.
The ideal solution would be to not include stringutils.h in WebCore/WebKit code. Doing a grep, tolowercase does not seem used at all except in unit tests so it might be the best short term solution. I'll do that.
youenn fablet
Comment 26
2018-08-23 10:58:34 PDT
https://trac.webkit.org/r235230
Alejandro G. Castro
Comment 27
2018-08-24 01:53:43 PDT
(In reply to youenn fablet from
comment #25
)
> > > Source/WebCore/Modules/mediastream/libwebrtc/LibWebRTCDataChannelHandler.h:33 > > > +#pragma clang diagnostic push > > > +#pragma clang diagnostic ignored "-Wunused-parameter" > > > > Is this required? We should use something that can be used for gcc or > > control it is just clang using it, it causes a ton of warnings in the gcc > > compilation. I wonder if it is cleaner for us to modify the include in the > > library. > > WebCore on Mac/iOS ports is using -Werror and -Wunused-parameter. > Previously, we were updating the libwebrtc source files but this is tedious > and creates a lot of conflicts every time we update libwebrtc. > > The ideal solution would be to progressively migrate code including these > headers in libwebrtc and create new libwebrtc headers matching > WebCore/WebKit constraints. >
Yep, it seems the solution also for the stringutils.h issue, which we are not including directly it is the main peerconnectioninterface.h in the api that causes the include.
> I would like to land it as is and we could improve with a follow-up patch to > fix GCC warnings, maybe adding macros to support easily CLANG and GCC. >
Ok, I'll look into this.
> (In reply to Alejandro G. Castro from
comment #24
) > > (In reply to Alejandro G. Castro from
comment #23
) > > > > > > [...] > > > > > > The other option is to remove that function in libwebrtc header > > > stringutils.h, it is not used. > > The ideal solution would be to not include stringutils.h in WebCore/WebKit > code. > > Doing a grep, tolowercase does not seem used at all except in unit tests so > it might be the best short term solution. > I'll do that.
The problem is that we are not including it directly it is the main peerconnectioninterface.h in the api that causes the problem.
Truitt Savell
Comment 28
2018-09-05 09:55:33 PDT
Looks like
https://trac.webkit.org/changeset/235230/webkit
Has caused test webrtc/video-rotation.html to become flaky the failure is easily reproducible with command: run-webkit-tests webrtc/video-rotation.html --iterations 500 -f When this command is run using
r235229
no failures occur, when using
r235230
the failure is intermittent. History:
http://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=webrtc%2Fvideo-rotation.html
Diff: --- /Volumes/Data/slave/highsierra-release-tests-wk2/build/layout-test-results/webrtc/video-rotation-expected.txt +++ /Volumes/Data/slave/highsierra-release-tests-wk2/build/layout-test-results/webrtc/video-rotation-actual.txt @@ -2,5 +2,5 @@ PASS Setting video exchange PASS Track is enabled, video should not be black -PASS Track is enabled and rotated, video should not be black and should change size +FAIL Track is enabled and rotated, video should not be black and should change size promise_test: Unhandled rejection with value: "waitForVideoSize timed out, expected 240x320 but got 320x240"
youenn fablet
Comment 29
2018-09-07 12:47:32 PDT
(In reply to Truitt Savell from
comment #28
)
> Looks like
https://trac.webkit.org/changeset/235230/webkit
> > Has caused test webrtc/video-rotation.html to become flaky > > the failure is easily reproducible with command: > run-webkit-tests webrtc/video-rotation.html --iterations 500 -f > > When this command is run using
r235229
no failures occur, when using
r235230
> the failure is intermittent. > > History: >
http://webkit-test-results.webkit.org/dashboards/flakiness_dashboard
. > html#showAllRuns=true&tests=webrtc%2Fvideo-rotation.html > > Diff: > --- > /Volumes/Data/slave/highsierra-release-tests-wk2/build/layout-test-results/ > webrtc/video-rotation-expected.txt > +++ > /Volumes/Data/slave/highsierra-release-tests-wk2/build/layout-test-results/ > webrtc/video-rotation-actual.txt > @@ -2,5 +2,5 @@ > > PASS Setting video exchange > PASS Track is enabled, video should not be black > -PASS Track is enabled and rotated, video should not be black and should > change size > +FAIL Track is enabled and rotated, video should not be black and should > change size promise_test: Unhandled rejection with value: "waitForVideoSize > timed out, expected 240x320 but got 320x240"
https://bugs.webkit.org/show_bug.cgi?id=189427
should fix this.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug