RESOLVED FIXED 141655
Consolidate soft-linked CMTimeMakeWithSeconds() function
https://bugs.webkit.org/show_bug.cgi?id=141655
Summary Consolidate soft-linked CMTimeMakeWithSeconds() function
David Kilzer (:ddkilzer)
Reported 2015-02-16 11:07:21 PST
Consolidate soft-linked CMTimeMakeWithSeconds() function into a single header/source file. Advantages: - In the future, using a soft-linked function that is already defined is as easy as including a new header file in any source file. - Reduces duplicate code for soft-linking the same framework and the same method in multiple source files. - Reduces static variables that cause dirty pages for soft-linking the same framework and the same method in multiple source files. - New code puts static soft-link methods into the WebCore namespace. - Inline method moved to header, making it more likely to be inlined and thus fixing weak external symbol issues worked around in Bug 141607. Disadvantages: - This strategy requires separate macros for header (declaration) and source (implementation) files, but this is mitigated by extra declarations which will cause the compile to fail if they don't match.
Attachments
Feedback on approach and test compile v1 (22.86 KB, patch)
2015-02-16 12:52 PST, David Kilzer (:ddkilzer)
no flags
Feedback on approach and test compile v2 (23.50 KB, patch)
2015-02-16 13:49 PST, David Kilzer (:ddkilzer)
no flags
Feedback on approach and test compile v3 (23.29 KB, patch)
2015-02-16 15:02 PST, David Kilzer (:ddkilzer)
no flags
Feedback on approach and test compile v4 (23.25 KB, patch)
2015-02-16 16:26 PST, David Kilzer (:ddkilzer)
no flags
Patch v5 (23.36 KB, patch)
2015-02-18 10:07 PST, David Kilzer (:ddkilzer)
no flags
Patch v6 (23.58 KB, patch)
2015-02-18 11:37 PST, David Kilzer (:ddkilzer)
no flags
Patch v7 (23.60 KB, patch)
2015-02-18 12:01 PST, David Kilzer (:ddkilzer)
no flags
Patch v8 (23.59 KB, patch)
2015-02-18 12:10 PST, David Kilzer (:ddkilzer)
bfulgham: review+
David Kilzer (:ddkilzer)
Comment 1 2015-02-16 12:52:58 PST
Created attachment 246668 [details] Feedback on approach and test compile v1
WebKit Commit Bot
Comment 2 2015-02-16 12:56:00 PST
Attachment 246668 [details] did not pass style-queue: ERROR: Source/WebCore/platform/mac/SoftLinking.h:117: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebCore/platform/mac/SoftLinking.h:130: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebCore/platform/mac/SoftLinking.h:135: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebCore/platform/win/SoftLinking.h:138: Extra space before ( in function call [whitespace/parens] [4] Total errors found: 4 in 12 files If any of these errors are false positives, please file a bug against check-webkit-style.
Alex Christensen
Comment 3 2015-02-16 12:57:39 PST
Comment on attachment 246668 [details] Feedback on approach and test compile v1 View in context: https://bugs.webkit.org/attachment.cgi?id=246668&action=review > Source/WebCore/Configurations/WebCore.unexp:-55 > -_CMTimeMakeWithSeconds Why just this one symbol? There are logs of CoreMedia symbols here. > Source/WebCore/WebCore.vcxproj/WebCore.vcxproj:8229 > + <ClCompile Include="..\platform\cocoa\CoreMediaSoftLink.cpp" /> Windows probably shouldn't include things in the cocoa directory.
David Kilzer (:ddkilzer)
Comment 4 2015-02-16 13:03:42 PST
(In reply to comment #3) > Comment on attachment 246668 [details] > Feedback on approach and test compile > > View in context: > https://bugs.webkit.org/attachment.cgi?id=246668&action=review > > > Source/WebCore/Configurations/WebCore.unexp:-55 > > -_CMTimeMakeWithSeconds > > Why just this one symbol? There are logs of CoreMedia symbols here. Yes, there are a lot of symbols. I want to get just one working with this new strategy before moving more (all) of them. That should result in fewer broken builds. As for why CMTimeMakeWithSeconds(), I'm targeting one of the "highest value" symbols (which are duplicated the most) per Bug 141625 Comment #0 and which was worked around for Bug 141607. > > Source/WebCore/WebCore.vcxproj/WebCore.vcxproj:8229 > > + <ClCompile Include="..\platform\cocoa\CoreMediaSoftLink.cpp" /> > > Windows probably shouldn't include things in the cocoa directory. Correct. I suppose "cf" is the best existing subdirectory. I wanted to get a first pass at a patch up for the EWS bots to chew on, since I'll undoubtedly have Windows fixes to make, so I didn't fix this in the first patch.
David Kilzer (:ddkilzer)
Comment 5 2015-02-16 13:22:26 PST
Comment on attachment 246668 [details] Feedback on approach and test compile v1 View in context: https://bugs.webkit.org/attachment.cgi?id=246668&action=review > Source/WebCore/platform/graphics/avfoundation/cf/MediaPlayerPrivateAVFoundationCF.cpp:78 > +#include "CoreMediaSoftLink.h" > #include "CoreMediaSoftLinking.h" If the strategy in this patch is what we want to use going forward, we should rename CoreMediaSoftLinking.h to CoreMediaSoftLinkWin.h (to disambiguate it from CoreMediaSoftLink.h).
David Kilzer (:ddkilzer)
Comment 6 2015-02-16 13:32:52 PST
Errors with patch v1: CoreMediaSoftLink.cpp 1>c:\cygwin\home\buildbot\webkit\source\webcore\platform\cocoa\CoreMediaSoftLink.h(42): warning C4273: 'CMTimeMakeWithSeconds' : inconsistent dll linkage (..\platform\cocoa\CoreMediaSoftLink.cpp) C:\cygwin\home\buildbot\WebKit\WebKitLibraries\win\include\CoreMedia/CMTime.h(63) : see previous definition of 'CMTimeMakeWithSeconds' 1>..\platform\cocoa\CoreMediaSoftLink.cpp(48): error C2732: linkage specification contradicts earlier specification for 'softLink_CMTimeMakeWithSeconds' ..\platform\cocoa\CoreMediaSoftLink.cpp(48) : see declaration of 'softLink_CMTimeMakeWithSeconds' 1>..\platform\cocoa\CoreMediaSoftLink.cpp(48): warning C4211: nonstandard extension used : redefined extern to static 1>..\platform\graphics\avfoundation\MediaTimeAVFoundation.cpp(31): fatal error C1083: Cannot open include file: 'CoreMediaSoftLink.h': No such file or directory 1>..\platform\graphics\avfoundation\cf\MediaPlayerPrivateAVFoundationCF.cpp(77): fatal error C1083: Cannot open include file: 'CoreMediaSoftLink.h': No such file or directory
David Kilzer (:ddkilzer)
Comment 7 2015-02-16 13:49:03 PST
Created attachment 246673 [details] Feedback on approach and test compile v2
David Kilzer (:ddkilzer)
Comment 8 2015-02-16 13:50:12 PST
(In reply to comment #7) > Created attachment 246673 [details] > Feedback on approach and test compile v2 - Attempted to fix Windows compiler errors. - Moved CoreMediaSoftLink.{cpp,h} from WebCore/platform/cocoa to WebCore/platform/cf.
WebKit Commit Bot
Comment 9 2015-02-16 13:51:47 PST
Attachment 246673 [details] did not pass style-queue: ERROR: Source/WebCore/platform/mac/SoftLinking.h:117: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebCore/platform/mac/SoftLinking.h:130: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebCore/platform/mac/SoftLinking.h:135: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebCore/platform/win/SoftLinking.h:138: Extra space before ( in function call [whitespace/parens] [4] Total errors found: 4 in 13 files If any of these errors are false positives, please file a bug against check-webkit-style.
David Kilzer (:ddkilzer)
Comment 10 2015-02-16 14:32:24 PST
(In reply to comment #7) > Created attachment 246673 [details] > Feedback on approach and test compile v2 More Windows build errors: CoreMediaSoftLink.cpp 1>c:\cygwin\home\buildbot\webkit\source\webcore\platform\cf\CoreMediaSoftLink.h(42): warning C4273: 'CMTimeMakeWithSeconds' : inconsistent dll linkage (..\platform\cf\CoreMediaSoftLink.cpp) C:\cygwin\home\buildbot\WebKit\WebKitLibraries\win\include\CoreMedia/CMTime.h(63) : see previous definition of 'CMTimeMakeWithSeconds' 1>..\platform\cf\CoreMediaSoftLink.cpp(48): error C2732: linkage specification contradicts earlier specification for 'softLink_CMTimeMakeWithSeconds' ..\platform\cf\CoreMediaSoftLink.cpp(48) : see declaration of 'softLink_CMTimeMakeWithSeconds' 1>..\platform\cf\CoreMediaSoftLink.cpp(48): error C2086: 'CMTime (__cdecl *__cdecl WebCore::softLinkCMTimeMakeWithSeconds)(Float64,int32_t)' : redefinition c:\cygwin\home\buildbot\webkit\source\webcore\platform\cf\CoreMediaSoftLink.h(42) : see declaration of 'WebCore::softLinkCMTimeMakeWithSeconds' MediaTimeAVFoundation.cpp 1>C:\cygwin\home\buildbot\WebKit\Source\WebCore\platform\cf\CoreMediaSoftLink.h(42): warning C4273: 'CMTimeMakeWithSeconds' : inconsistent dll linkage (..\platform\graphics\avfoundation\MediaTimeAVFoundation.cpp) C:\cygwin\home\buildbot\WebKit\WebKitLibraries\win\include\CoreMedia/CMTime.h(63) : see previous definition of 'CMTimeMakeWithSeconds' MediaPlayerPrivateAVFoundationCF.cpp 1>C:\cygwin\home\buildbot\WebKit\Source\WebCore\platform\cf\CoreMediaSoftLink.h(42): warning C4273: 'CMTimeMakeWithSeconds' : inconsistent dll linkage (..\platform\graphics\avfoundation\cf\MediaPlayerPrivateAVFoundationCF.cpp) C:\cygwin\home\buildbot\WebKit\WebKitLibraries\win\include\CoreMedia/CMTime.h(63) : see previous definition of 'CMTimeMakeWithSeconds'
David Kilzer (:ddkilzer)
Comment 11 2015-02-16 15:02:06 PST
Created attachment 246686 [details] Feedback on approach and test compile v3
WebKit Commit Bot
Comment 12 2015-02-16 15:04:00 PST
Attachment 246686 [details] did not pass style-queue: ERROR: Source/WebCore/platform/mac/SoftLinking.h:117: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebCore/platform/mac/SoftLinking.h:130: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebCore/platform/mac/SoftLinking.h:135: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebCore/platform/win/SoftLinking.h:132: Extra space before ( in function call [whitespace/parens] [4] Total errors found: 4 in 13 files If any of these errors are false positives, please file a bug against check-webkit-style.
David Kilzer (:ddkilzer)
Comment 13 2015-02-16 15:05:56 PST
(In reply to comment #11) > Created attachment 246686 [details] > Feedback on approach and test compile v3 * Removed the additional type checks for the soft-linked method in both SOFT_LINK_DLL_IMPORT_FUNCTION_DECL() and SOFT_LINK_DLL_IMPORT_FUNCTION_IMPL(): - WTF_EXTERN_C_BEGIN \ - resultType functionName parameterDeclarations; \ - WTF_EXTERN_C_END \ * Removed 'static' from the declaration of the softLink##functionName declaration in SOFT_LINK_DLL_IMPORT_FUNCTION_DECL().
Brent Fulgham
Comment 14 2015-02-16 16:03:13 PST
Comment on attachment 246686 [details] Feedback on approach and test compile v3 View in context: https://bugs.webkit.org/attachment.cgi?id=246686&action=review > Source/WebCore/platform/win/SoftLinking.h:129 > + static resultType(callingConvention*softLink##functionName) parameterDeclarations = init##functionName; \ I don't think this should be static..
Brent Fulgham
Comment 15 2015-02-16 16:05:47 PST
Comment on attachment 246686 [details] Feedback on approach and test compile v3 View in context: https://bugs.webkit.org/attachment.cgi?id=246686&action=review > Source/WebCore/platform/cf/CoreMediaSoftLink.h:40 > +#include <CoreMedia/CoreMedia.h> You can write this as <CoreMedia/CMTime.h> on Windows, too. I think using the "CoreMedia.h" header is a holdover from older versions of our support libraries.
David Kilzer (:ddkilzer)
Comment 16 2015-02-16 16:17:12 PST
(In reply to comment #13) > (In reply to comment #11) > > Created attachment 246686 [details] > > Feedback on approach and test compile v3 > > * Removed the additional type checks for the soft-linked method in both > SOFT_LINK_DLL_IMPORT_FUNCTION_DECL() and > SOFT_LINK_DLL_IMPORT_FUNCTION_IMPL(): > - WTF_EXTERN_C_BEGIN \ > - resultType functionName parameterDeclarations; \ > - WTF_EXTERN_C_END \ > > * Removed 'static' from the declaration of the softLink##functionName > declaration in SOFT_LINK_DLL_IMPORT_FUNCTION_DECL(). Errors in this patch: CoreMediaSoftLink.cpp 1>..\platform\cf\CoreMediaSoftLink.cpp(48): error C2370: 'WebCore::softLinkCMTimeMakeWithSeconds' : redefinition; different storage class c:\cygwin\home\buildbot\webkit\source\webcore\platform\cf\CoreMediaSoftLink.h(42) : see declaration of 'WebCore::softLinkCMTimeMakeWithSeconds' 1>..\platform\cf\CoreMediaSoftLink.cpp(48): error C3861: 'softLinkCMTimeMakeWithSeconds': identifier not found
David Kilzer (:ddkilzer)
Comment 17 2015-02-16 16:17:50 PST
(In reply to comment #14) > Comment on attachment 246686 [details] > Feedback on approach and test compile v3 > > View in context: > https://bugs.webkit.org/attachment.cgi?id=246686&action=review > > > Source/WebCore/platform/win/SoftLinking.h:129 > > + static resultType(callingConvention*softLink##functionName) parameterDeclarations = init##functionName; \ > > I don't think this should be static.. Yes, just like the Mac version. Not sure why it's so hard to see that when looking at the macro. Thanks!
David Kilzer (:ddkilzer)
Comment 18 2015-02-16 16:26:31 PST
Created attachment 246698 [details] Feedback on approach and test compile v4
WebKit Commit Bot
Comment 19 2015-02-16 16:28:00 PST
Attachment 246698 [details] did not pass style-queue: ERROR: Source/WebCore/platform/mac/SoftLinking.h:117: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebCore/platform/mac/SoftLinking.h:130: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebCore/platform/mac/SoftLinking.h:135: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebCore/platform/win/SoftLinking.h:132: Extra space before ( in function call [whitespace/parens] [4] Total errors found: 4 in 13 files If any of these errors are false positives, please file a bug against check-webkit-style.
Brent Fulgham
Comment 20 2015-02-17 10:08:59 PST
This looks good on Windows now!
Brent Fulgham
Comment 21 2015-02-17 10:12:31 PST
Comment on attachment 246698 [details] Feedback on approach and test compile v4 View in context: https://bugs.webkit.org/attachment.cgi?id=246698&action=review Looks great! r=me > Source/WebCore/platform/cf/CoreMediaSoftLink.cpp:44 > +SOFT_LINK_LIBRARY(CoreMedia) I wonder if this DEBUG_ALL case could be handled inside the Macro definition somehow, so we could just call it SOFT_LINK_FRAMEWORK on both platforms? > Source/WebCore/platform/cf/CoreMediaSoftLink.cpp:48 > +SOFT_LINK_DLL_IMPORT_FUNCTION_IMPL(CoreMedia, CMTimeMakeWithSeconds, CMTime, __cdecl, (Float64 seconds, int32_t preferredTimeScale), (seconds, preferredTimeScale)) It's a shame we need two different macros for this. I wonder if they could be consolidated. > Source/WebCore/platform/win/SoftLinking.h:117 > +#define SOFT_LINK_DLL_IMPORT_FUNCTION_DECL(functionName, resultType, callingConvention, parameterDeclarations, parameterNames) \ I don't think we ever have a calling convention other than "__cdecl". We might be able to just have that in the implementation, which would give the same signature on Windows and Mac.
David Kilzer (:ddkilzer)
Comment 22 2015-02-17 15:09:03 PST
Comment on attachment 246698 [details] Feedback on approach and test compile v4 Based on an in-person discussion with Brent, I'm going to make mac/SoftLinking.h and win/SoftLinking.h so similar that I won't have to have separate #if PLATFORM(COCOA)/#elif PLATFORM(WIN)/#endif statements in the CoreMediaSoftLink.{cpp,h} files.
David Kilzer (:ddkilzer)
Comment 23 2015-02-18 10:07:24 PST
Created attachment 246825 [details] Patch v5
WebKit Commit Bot
Comment 24 2015-02-18 10:08:55 PST
Attachment 246825 [details] did not pass style-queue: ERROR: Source/WebCore/platform/mac/SoftLinking.h:117: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebCore/platform/mac/SoftLinking.h:130: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebCore/platform/mac/SoftLinking.h:135: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebCore/platform/win/SoftLinking.h:134: Extra space before ( in function call [whitespace/parens] [4] Total errors found: 4 in 13 files If any of these errors are false positives, please file a bug against check-webkit-style.
David Kilzer (:ddkilzer)
Comment 25 2015-02-18 11:37:37 PST
Created attachment 246830 [details] Patch v6
WebKit Commit Bot
Comment 26 2015-02-18 11:39:31 PST
Attachment 246830 [details] did not pass style-queue: ERROR: Source/WebCore/platform/mac/SoftLinking.h:117: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebCore/platform/mac/SoftLinking.h:130: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebCore/platform/mac/SoftLinking.h:135: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebCore/platform/win/SoftLinking.h:134: Extra space before ( in function call [whitespace/parens] [4] Total errors found: 4 in 13 files If any of these errors are false positives, please file a bug against check-webkit-style.
David Kilzer (:ddkilzer)
Comment 27 2015-02-18 11:40:04 PST
(In reply to comment #25) > Created attachment 246830 [details] > Patch v6 This patch attempts to fix the weak external symbol by not reimplementing the extern "C" function as an inline function. The iOS/Mac port now behaves identically to the Windows port by prepending "softLink_" to the inline function name.
David Kilzer (:ddkilzer)
Comment 28 2015-02-18 11:58:30 PST
Comment on attachment 246830 [details] Patch v6 Oops. I see the issue here. We now can't include CoreMediaSoftLink.h in CoreMediaSoftLink.cpp because the method will be redefined. New patch forthcoming.
David Kilzer (:ddkilzer)
Comment 29 2015-02-18 12:01:02 PST
Created attachment 246833 [details] Patch v7
WebKit Commit Bot
Comment 30 2015-02-18 12:02:56 PST
Attachment 246833 [details] did not pass style-queue: ERROR: Source/WebCore/platform/mac/SoftLinking.h:117: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebCore/platform/mac/SoftLinking.h:130: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebCore/platform/mac/SoftLinking.h:135: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebCore/platform/cf/CoreMediaSoftLink.cpp:29: Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] ERROR: Source/WebCore/platform/win/SoftLinking.h:134: Extra space before ( in function call [whitespace/parens] [4] Total errors found: 5 in 13 files If any of these errors are false positives, please file a bug against check-webkit-style.
David Kilzer (:ddkilzer)
Comment 31 2015-02-18 12:06:16 PST
(In reply to comment #30) > Attachment 246833 [details] did not pass style-queue: > > ERROR: Source/WebCore/platform/cf/CoreMediaSoftLink.cpp:29: Found other > header before a header this file implements. Should be: config.h, primary > header, blank line, and then alphabetically sorted. [build/include_order] > [4] This is what caused Patch v6 to fail. Will need to update style checker to ignore this rule for these special *SoftLink.cpp files.
Brent Fulgham
Comment 32 2015-02-18 12:07:32 PST
Comment on attachment 246833 [details] Patch v7 View in context: https://bugs.webkit.org/attachment.cgi?id=246833&action=review > Source/WebCore/platform/win/SoftLinking.h:128 > +#define SOFT_LINK_FUNCTION_IMPL(library, functionName, resultType, __cdecl, parameterDeclarations, parameterNames) \ The '__cdecl' here is getting treated like a parameter name, and breaking the build on Windows! Please just get rid of the '__cdecl' value and we should be all set.
David Kilzer (:ddkilzer)
Comment 33 2015-02-18 12:10:15 PST
Created attachment 246837 [details] Patch v8
Brent Fulgham
Comment 34 2015-02-18 12:11:24 PST
Comment on attachment 246837 [details] Patch v8 This version of the patch builds for me correctly on Windows.
WebKit Commit Bot
Comment 35 2015-02-18 12:13:17 PST
Attachment 246837 [details] did not pass style-queue: ERROR: Source/WebCore/platform/mac/SoftLinking.h:117: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebCore/platform/mac/SoftLinking.h:130: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebCore/platform/mac/SoftLinking.h:135: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebCore/platform/cf/CoreMediaSoftLink.cpp:29: Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] ERROR: Source/WebCore/platform/win/SoftLinking.h:134: Extra space before ( in function call [whitespace/parens] [4] Total errors found: 5 in 13 files If any of these errors are false positives, please file a bug against check-webkit-style.
David Kilzer (:ddkilzer)
Comment 36 2015-02-18 13:14:58 PST
Alexey Proskuryakov
Comment 37 2015-02-19 09:22:51 PST
Comment on attachment 246837 [details] Patch v8 View in context: https://bugs.webkit.org/attachment.cgi?id=246837&action=review > Source/WebCore/platform/graphics/avfoundation/cf/MediaPlayerPrivateAVFoundationCF.cpp:78 > +#include "CoreMediaSoftLink.h" > #include "CoreMediaSoftLinking.h" :-(
Brent Fulgham
Comment 38 2015-02-19 09:50:25 PST
Comment on attachment 246837 [details] Patch v8 View in context: https://bugs.webkit.org/attachment.cgi?id=246837&action=review >> Source/WebCore/platform/graphics/avfoundation/cf/MediaPlayerPrivateAVFoundationCF.cpp:78 >> #include "CoreMediaSoftLinking.h" > > :-( It's just a growing pain until this work is complete. :-)
Note You need to log in before you can comment on or make changes to this bug.