Bug 103770 – [Win] support in-band text tracks

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[Eric Carlson]

In-band caption track support requires changes in the media engine.

[Brent Fulgham]

<rdar://problem/13011371>

[Brent Fulgham]

Created attachment 206829 [details]
Patch

[WebKit Commit Bot]

Attachment 206829 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/WebCore.vcxproj/WebCore.vcxproj', u'Source/WebCore/WebCore.vcxproj/WebCore.vcxproj.filters', u'Source/WebCore/config.h', u'Source/WebCore/platform/graphics/avfoundation/InbandTextTrackPrivateAVF.cpp', u'Source/WebCore/platform/graphics/avfoundation/InbandTextTrackPrivateAVF.h', u'Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundation.cpp', u'Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundation.h', u'Source/WebCore/platform/graphics/avfoundation/cf/AVFoundationCFSoftLinking.h', u'Source/WebCore/platform/graphics/avfoundation/cf/CoreMediaSoftLinking.h', u'Source/WebCore/platform/graphics/avfoundation/cf/InbandTextTrackPrivateAVCF.cpp', u'Source/WebCore/platform/graphics/avfoundation/cf/InbandTextTrackPrivateAVCF.h', u'Source/WebCore/platform/graphics/avfoundation/cf/InbandTextTrackPrivateLegacyAVCF.cpp', u'Source/WebCore/platform/graphics/avfoundation/cf/InbandTextTrackPrivateLegacyAVCF.h', u'Source/WebCore/platform/graphics/avfoundation/cf/MediaPlayerPrivateAVFoundationCF.cpp', u'Source/WebCore/platform/graphics/avfoundation/cf/MediaPlayerPrivateAVFoundationCF.h', u'Tools/ChangeLog', u'Tools/WinLauncher/WinLauncher.cpp', u'Tools/WinLauncher/WinLauncher.vcxproj/WinLauncherCF.props', u'Tools/WinLauncher/WinLauncher.vcxproj/WinLauncherCFLite.props', u'Tools/WinLauncher/WinLauncher.vcxproj/WinLauncherLib.vcxproj']" exit_code: 1
Source/WebCore/platform/graphics/avfoundation/cf/MediaPlayerPrivateAVFoundationCF.cpp:56:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebCore/platform/graphics/avfoundation/cf/InbandTextTrackPrivateLegacyAVCF.cpp:41:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebCore/platform/graphics/avfoundation/cf/InbandTextTrackPrivateAVCF.cpp:43:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebCore/platform/graphics/avfoundation/cf/InbandTextTrackPrivateAVCF.h:33:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 4 in 20 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Brent Fulgham]

Created attachment 206830 [details]
Patch

[WebKit Commit Bot]

Attachment 206830 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/WebCore.vcxproj/WebCore.vcxproj', u'Source/WebCore/WebCore.vcxproj/WebCore.vcxproj.filters', u'Source/WebCore/config.h', u'Source/WebCore/platform/graphics/avfoundation/InbandTextTrackPrivateAVF.cpp', u'Source/WebCore/platform/graphics/avfoundation/InbandTextTrackPrivateAVF.h', u'Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundation.cpp', u'Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundation.h', u'Source/WebCore/platform/graphics/avfoundation/cf/AVFoundationCFSoftLinking.h', u'Source/WebCore/platform/graphics/avfoundation/cf/CoreMediaSoftLinking.h', u'Source/WebCore/platform/graphics/avfoundation/cf/InbandTextTrackPrivateAVCF.cpp', u'Source/WebCore/platform/graphics/avfoundation/cf/InbandTextTrackPrivateAVCF.h', u'Source/WebCore/platform/graphics/avfoundation/cf/InbandTextTrackPrivateLegacyAVCF.cpp', u'Source/WebCore/platform/graphics/avfoundation/cf/InbandTextTrackPrivateLegacyAVCF.h', u'Source/WebCore/platform/graphics/avfoundation/cf/MediaPlayerPrivateAVFoundationCF.cpp', u'Source/WebCore/platform/graphics/avfoundation/cf/MediaPlayerPrivateAVFoundationCF.h', u'Tools/ChangeLog', u'Tools/WinLauncher/WinLauncher.cpp', u'Tools/WinLauncher/WinLauncher.vcxproj/WinLauncherCF.props', u'Tools/WinLauncher/WinLauncher.vcxproj/WinLauncherCFLite.props', u'Tools/WinLauncher/WinLauncher.vcxproj/WinLauncherLib.vcxproj']" exit_code: 1
Source/WebCore/platform/graphics/avfoundation/cf/MediaPlayerPrivateAVFoundationCF.cpp:56:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebCore/platform/graphics/avfoundation/cf/InbandTextTrackPrivateLegacyAVCF.cpp:41:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebCore/platform/graphics/avfoundation/cf/InbandTextTrackPrivateAVCF.cpp:43:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebCore/platform/graphics/avfoundation/cf/InbandTextTrackPrivateAVCF.h:33:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 4 in 20 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Eric Carlson]

Comment on attachment 206830 [details]
Patch

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

> Source/WebCore/ChangeLog:15
> +        * platform/graphics/avfoundation/InbandTextTrackPrivateAVF.cpp: Update
> +        to support Windows platform.

What happened to the list of methods that were changed (and associated comments)?

> Source/WebCore/ChangeLog:30
> +        * platform/graphics/avfoundation/cf/MediaPlayerPrivateAVFoundationCF.cpp:
> +        Update to support LegibleOutput features for the Windows port.

Ditto.

> Source/WebCore/platform/graphics/avfoundation/InbandTextTrackPrivateAVF.cpp:88
> +SOFT_LINK_LIBRARY(CoreMedia)
> +
> +SOFT_LINK_VARIABLE_DLL_IMPORT_OPTIONAL(CoreMedia, kCMTextMarkupAttribute_Alignment, CFStringRef)
> +SOFT_LINK_VARIABLE_DLL_IMPORT_OPTIONAL(CoreMedia, kCMTextMarkupAlignmentType_Start, CFStringRef)
> +SOFT_LINK_VARIABLE_DLL_IMPORT_OPTIONAL(CoreMedia, kCMTextMarkupAlignmentType_Middle, CFStringRef)

Instead of having duplicates of every import, could we create new macros to be used in this file? Something like:

#if !PLATFORM(WIN)
    #define SOFT_LINK_AVF_POINTER(_lib, _name, _type) SOFT_LINK_POINTER_OPTIONAL(_lib, _name, _type)
#else
    #define SOFT_LINK_AVF_POINTER(_lib, _name, _type) SOFT_LINK_VARIABLE_DLL_IMPORT_OPTIONAL(_lib, _name, _type)
#endif

SOFT_LINK_AVF_POINTER(CoreMedia, kCMTextMarkupAttribute_Alignment, CFStringRef)

etc?

> Source/WebCore/platform/graphics/avfoundation/cf/InbandTextTrackPrivateAVCF.cpp:148
> +    CFIndex titlesCount = CFArrayGetCount(titles.get());
> +    if (titlesCount) {
> +        // If possible, return a title in one of the user's preferred languages.

I see that you are just following the pattern (that I created) in InbandTextTrackPrivateAVFObjC::label(), but you can use an early return here.

> Source/WebCore/platform/graphics/avfoundation/cf/MediaPlayerPrivateAVFoundationCF.cpp:208
> +        // AVCFAssetPropertyPreferredVolume,    // FIXME: Should be exported by AVCF. We can use AVCFAssetTrackPropertyPreferredVolume for now.

This can be removed because neither is currently used.

> Source/WebCore/platform/graphics/avfoundation/cf/MediaPlayerPrivateAVFoundationCF.cpp:535
> +        return max(narrowPrecisionToFloat(CMTimeGetSeconds(itemTime)), 0.0f);

Nit: is the "f" necessary, does "0" cause a compiler warning/error?

> Source/WebCore/platform/graphics/avfoundation/cf/MediaPlayerPrivateAVFoundationCF.cpp:797
> +    CFArrayRef foo = AVCFURLAssetCopyAudiovisualMIMETypes();
> +    ASSERT(CFArrayGetTypeID() == CFGetTypeID(foo)); 

Won't this cause an unused variable error in a release build? If it is necessary, I'll bet you can come up with a more descriptive name than "foo" ;-)

> Source/WebCore/platform/graphics/avfoundation/cf/MediaPlayerPrivateAVFoundationCF.cpp:866
> -        setHasAudio(CFArrayGetCount(captionTracks.get()));
> +        hasCaptions = CFArrayGetCount(captionTracks.get());

Oops :-O

> Source/WebCore/platform/graphics/avfoundation/cf/MediaPlayerPrivateAVFoundationCF.cpp:1017
> +    // FIXME: Lift to parent class
> +    if (removedTextTracks.size()) {

Please add a bug number for this FIXME.

> Source/WebCore/platform/graphics/avfoundation/cf/MediaPlayerPrivateAVFoundationCF.cpp:1549
> +    AVFWrapper* self = avfWrapperForCallbackContext(context);
> +    if (!self) {
> +        LOG(Media, "AVFWrapper::legibleOutputCallback invoked for deleted AVFWrapper %d", reinterpret_cast<uintptr_t>(context));
> +        return;
> +    }
> +

It might be worth adding something like "ASSERT(legibleOutput ==  self->legibleOutput())"

[Brent Fulgham]

Comment on attachment 206830 [details]
Patch

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

>> Source/WebCore/platform/graphics/avfoundation/cf/InbandTextTrackPrivateAVCF.cpp:148
>> +        // If possible, return a title in one of the user's preferred languages.
> 
> I see that you are just following the pattern (that I created) in InbandTextTrackPrivateAVFObjC::label(), but you can use an early return here.

Done!

>> Source/WebCore/platform/graphics/avfoundation/cf/MediaPlayerPrivateAVFoundationCF.cpp:208
>> +        // AVCFAssetPropertyPreferredVolume,    // FIXME: Should be exported by AVCF. We can use AVCFAssetTrackPropertyPreferredVolume for now.
> 
> This can be removed because neither is currently used.

Done!

>> Source/WebCore/platform/graphics/avfoundation/cf/MediaPlayerPrivateAVFoundationCF.cpp:535
>> +        return max(narrowPrecisionToFloat(CMTimeGetSeconds(itemTime)), 0.0f);
> 
> Nit: is the "f" necessary, does "0" cause a compiler warning/error?

Yes -- the compiler can't (won't?) assume a 0.0 (or 0) argument is a float to match the return from narrowPrecisionToFloat.  I could write this as max<float>, or just use the 0.0f notation.

>> Source/WebCore/platform/graphics/avfoundation/cf/MediaPlayerPrivateAVFoundationCF.cpp:797
>> +    ASSERT(CFArrayGetTypeID() == CFGetTypeID(foo)); 
> 
> Won't this cause an unused variable error in a release build? If it is necessary, I'll bet you can come up with a more descriptive name than "foo" ;-)

Doh!  Stupid debugging code.  It was worse than that -- it was an unreleased variable each time this was called.

>> Source/WebCore/platform/graphics/avfoundation/cf/MediaPlayerPrivateAVFoundationCF.cpp:1549
>> +
> 
> It might be worth adding something like "ASSERT(legibleOutput ==  self->legibleOutput())"

Good idea.

[Brent Fulgham]

Created attachment 206907 [details]
Patch

[WebKit Commit Bot]

Attachment 206907 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/WebCore.vcxproj/WebCore.vcxproj', u'Source/WebCore/WebCore.vcxproj/WebCore.vcxproj.filters', u'Source/WebCore/config.h', u'Source/WebCore/platform/graphics/avfoundation/InbandTextTrackPrivateAVF.cpp', u'Source/WebCore/platform/graphics/avfoundation/InbandTextTrackPrivateAVF.h', u'Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundation.cpp', u'Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundation.h', u'Source/WebCore/platform/graphics/avfoundation/cf/AVFoundationCFSoftLinking.h', u'Source/WebCore/platform/graphics/avfoundation/cf/CoreMediaSoftLinking.h', u'Source/WebCore/platform/graphics/avfoundation/cf/InbandTextTrackPrivateAVCF.cpp', u'Source/WebCore/platform/graphics/avfoundation/cf/InbandTextTrackPrivateAVCF.h', u'Source/WebCore/platform/graphics/avfoundation/cf/InbandTextTrackPrivateLegacyAVCF.cpp', u'Source/WebCore/platform/graphics/avfoundation/cf/InbandTextTrackPrivateLegacyAVCF.h', u'Source/WebCore/platform/graphics/avfoundation/cf/MediaPlayerPrivateAVFoundationCF.cpp', u'Source/WebCore/platform/graphics/avfoundation/cf/MediaPlayerPrivateAVFoundationCF.h', u'Tools/ChangeLog', u'Tools/WinLauncher/WinLauncher.cpp', u'Tools/WinLauncher/WinLauncher.vcxproj/WinLauncherCF.props', u'Tools/WinLauncher/WinLauncher.vcxproj/WinLauncherCFLite.props', u'Tools/WinLauncher/WinLauncher.vcxproj/WinLauncherLib.vcxproj']" exit_code: 1
Source/WebCore/platform/graphics/avfoundation/cf/MediaPlayerPrivateAVFoundationCF.cpp:55:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebCore/platform/graphics/avfoundation/cf/InbandTextTrackPrivateLegacyAVCF.cpp:41:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebCore/platform/graphics/avfoundation/cf/InbandTextTrackPrivateAVCF.cpp:43:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebCore/platform/graphics/avfoundation/cf/InbandTextTrackPrivateAVCF.h:33:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 4 in 20 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Eric Carlson]

Comment on attachment 206907 [details]
Patch

r=me, although I would still prefer to see the SOFT_LINK macros combined if possible, either in this patch or in a follow up.

[Brent Fulgham]

Created attachment 206918 [details]
Patch with SoftLink changes

[WebKit Commit Bot]

Attachment 206918 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/WebCore.vcxproj/WebCore.vcxproj', u'Source/WebCore/WebCore.vcxproj/WebCore.vcxproj.filters', u'Source/WebCore/config.h', u'Source/WebCore/page/CaptionUserPreferencesMediaAF.cpp', u'Source/WebCore/platform/graphics/avfoundation/InbandTextTrackPrivateAVF.cpp', u'Source/WebCore/platform/graphics/avfoundation/InbandTextTrackPrivateAVF.h', u'Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundation.cpp', u'Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundation.h', u'Source/WebCore/platform/graphics/avfoundation/cf/AVFoundationCFSoftLinking.h', u'Source/WebCore/platform/graphics/avfoundation/cf/CoreMediaSoftLinking.h', u'Source/WebCore/platform/graphics/avfoundation/cf/InbandTextTrackPrivateAVCF.cpp', u'Source/WebCore/platform/graphics/avfoundation/cf/InbandTextTrackPrivateAVCF.h', u'Source/WebCore/platform/graphics/avfoundation/cf/InbandTextTrackPrivateLegacyAVCF.cpp', u'Source/WebCore/platform/graphics/avfoundation/cf/InbandTextTrackPrivateLegacyAVCF.h', u'Source/WebCore/platform/graphics/avfoundation/cf/MediaPlayerPrivateAVFoundationCF.cpp', u'Source/WebCore/platform/graphics/avfoundation/cf/MediaPlayerPrivateAVFoundationCF.h', u'Source/WebCore/platform/win/SoftLinking.h', u'Tools/ChangeLog', u'Tools/WinLauncher/WinLauncher.cpp', u'Tools/WinLauncher/WinLauncher.vcxproj/WinLauncherCF.props', u'Tools/WinLauncher/WinLauncher.vcxproj/WinLauncherCFLite.props', u'Tools/WinLauncher/WinLauncher.vcxproj/WinLauncherLib.vcxproj']" exit_code: 1
Source/WebCore/platform/graphics/avfoundation/cf/MediaPlayerPrivateAVFoundationCF.cpp:55:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebCore/platform/graphics/avfoundation/cf/InbandTextTrackPrivateLegacyAVCF.cpp:41:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebCore/platform/graphics/avfoundation/cf/InbandTextTrackPrivateAVCF.cpp:43:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebCore/platform/graphics/avfoundation/cf/InbandTextTrackPrivateAVCF.h:33:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 4 in 22 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Eric Carlson]

Comment on attachment 206918 [details]
Patch with SoftLink changes

r=me, but please fix the alphabetical sorting problems before committing

[Brent Fulgham]

Created attachment 207011 [details]
Updated to resolve alphabetical ordering

[WebKit Commit Bot]

Attachment 207011 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/WebCore.vcxproj/WebCore.vcxproj', u'Source/WebCore/WebCore.vcxproj/WebCore.vcxproj.filters', u'Source/WebCore/config.h', u'Source/WebCore/page/CaptionUserPreferencesMediaAF.cpp', u'Source/WebCore/platform/graphics/avfoundation/InbandTextTrackPrivateAVF.cpp', u'Source/WebCore/platform/graphics/avfoundation/InbandTextTrackPrivateAVF.h', u'Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundation.cpp', u'Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundation.h', u'Source/WebCore/platform/graphics/avfoundation/cf/AVFoundationCFSoftLinking.h', u'Source/WebCore/platform/graphics/avfoundation/cf/CoreMediaSoftLinking.h', u'Source/WebCore/platform/graphics/avfoundation/cf/InbandTextTrackPrivateAVCF.cpp', u'Source/WebCore/platform/graphics/avfoundation/cf/InbandTextTrackPrivateAVCF.h', u'Source/WebCore/platform/graphics/avfoundation/cf/InbandTextTrackPrivateLegacyAVCF.cpp', u'Source/WebCore/platform/graphics/avfoundation/cf/InbandTextTrackPrivateLegacyAVCF.h', u'Source/WebCore/platform/graphics/avfoundation/cf/MediaPlayerPrivateAVFoundationCF.cpp', u'Source/WebCore/platform/graphics/avfoundation/cf/MediaPlayerPrivateAVFoundationCF.h', u'Source/WebCore/platform/win/SoftLinking.h', u'Tools/ChangeLog', u'Tools/WinLauncher/WinLauncher.cpp', u'Tools/WinLauncher/WinLauncher.vcxproj/WinLauncherCF.props', u'Tools/WinLauncher/WinLauncher.vcxproj/WinLauncherCFLite.props', u'Tools/WinLauncher/WinLauncher.vcxproj/WinLauncherLib.vcxproj']" exit_code: 1
Source/WebCore/platform/graphics/avfoundation/cf/InbandTextTrackPrivateLegacyAVCF.cpp:41:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebCore/platform/graphics/avfoundation/cf/InbandTextTrackPrivateAVCF.cpp:43:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 2 in 22 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Brent Fulgham]

(In reply to comment #15)
> Attachment 207011 [details] did not pass style-queue:
> 
> Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', > Source/WebCore/platform/graphics/avfoundation/cf/InbandTextTrackPrivateLegacyAVCF.cpp:41:  Alphabetical sorting problem.  [build/include_order] [4]
> Source/WebCore/platform/graphics/avfoundation/cf/InbandTextTrackPrivateAVCF.cpp:43:  Alphabetical sorting problem.  [build/include_order] [4]
> Total errors found: 2 in 22 files

These includes cannot be rearranged. The soft linking headers need to see the original AVFoundationCF headers to work properly.

[Brent Fulgham]

Committed revision 152861.

http://trac.webkit.org/changeset/152861