Bug 141655 – Consolidate soft-linked CMTimeMakeWithSeconds() function

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[David Kilzer (:ddkilzer)]

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.

[David Kilzer (:ddkilzer)]

Created attachment 246668 [details]
Feedback on approach and test compile v1

[WebKit Commit Bot]

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 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)]

(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 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)]

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)]

Created attachment 246673 [details]
Feedback on approach and test compile v2

[David Kilzer (:ddkilzer)]

(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]

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)]

(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)]

Created attachment 246686 [details]
Feedback on approach and test compile v3

[WebKit Commit Bot]

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)]

(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 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 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)]

(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)]

(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)]

Created attachment 246698 [details]
Feedback on approach and test compile v4

[WebKit Commit Bot]

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]

This looks good on Windows now!

[Brent Fulgham]

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 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)]

Created attachment 246825 [details]
Patch v5

[WebKit Commit Bot]

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)]

Created attachment 246830 [details]
Patch v6

[WebKit Commit Bot]

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)]

(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 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)]

Created attachment 246833 [details]
Patch v7

[WebKit Commit Bot]

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)]

(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 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)]

Created attachment 246837 [details]
Patch v8

[Brent Fulgham]

Comment on attachment 246837 [details]
Patch v8

This version of the patch builds for me correctly on Windows.

[WebKit Commit Bot]

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)]

Committed r180287: <http://trac.webkit.org/changeset/180287>

[Alexey Proskuryakov]

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 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. :-)