Bug 65400 – Adopt AVCF media back end on Windows

• Component: Media
• Status: RESOLVED FIXED

[Jeff Miller]

Adopt AVFoundation / AVCF media back end on Windows.

[Jeff Miller]

<rdar://problem/9083559>

[Jeff Miller]

Created attachment 102409 [details]
Make MediaPlayerPrivateAVFoundation::setDelayCallbacks() const

Required by the Windows subclass of MediaPlayerPrivateAVFoundation.

[Jeff Miller]

Created attachment 102412 [details]
Prepare WebCore.vcproj to support AVCF media back end on Windows

[Jeff Miller]

Comment on attachment 102412 [details]
Prepare WebCore.vcproj to support AVCF media back end on Windows

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

> Source/WebCore/ChangeLog:11
> +         No changes to functionality so no new tests.

I've already removed the extra leading space.

[Darin Adler]

Comment on attachment 102412 [details]
Prepare WebCore.vcproj to support AVCF media back end on Windows

Thanks. Seems fine to add these files like this and then fill them in later.

[Darin Adler]

Comment on attachment 102409 [details]
Make MediaPlayerPrivateAVFoundation::setDelayCallbacks() const

I guess this is OK, but it makes me wonder why a const function could do something that could trigger a callback. Maybe such a function shouldn’t really be const!

[Jeff Miller]

(In reply to comment #6)
> (From update of attachment 102409 [details])
> I guess this is OK, but it makes me wonder why a const function could do something that could trigger a callback. Maybe such a function shouldn’t really be const!

Yes, that was the other option that Eric and I considered, and Eric felt that using mutable was a suitable solution.  Thanks for the review!

[Jeff Miller]

Patch to make MediaPlayerPrivateAVFoundation::setDelayCallbacks() const landed in <http://trac.webkit.org/changeset/92034>.

[Jeff Miller]

Patch to prepare WebCore.vcproj to support AVCF media back end on Windows landed in <http://trac.webkit.org/changeset/92035>

[Jeff Miller]

Created attachment 102865 [details]
First cut at implementing AVCF support on Windows.

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundationCF.h:47:  The parameter name "player" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundationCF.cpp:34:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundationCF.cpp:45:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundationCF.cpp:47:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundationCF.cpp:48:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundationCF.cpp:85:  The parameter name "rect" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundationCF.cpp:134:  Missing space inside { }.  [whitespace/braces] [5]
Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundationCF.cpp:885:  One line control clauses should not use braces.  [whitespace/braces] [4]
Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundationCF.cpp:1016:  One line control clauses should not use braces.  [whitespace/braces] [4]
Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundationCF.cpp:1057:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundationCF.cpp:1107:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundationCF.cpp:1156:  Extra space before )  [whitespace/parens] [2]
Total errors found: 12 in 8 files


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

[Brian Weinstein]

Comment on attachment 102865 [details]
First cut at implementing AVCF support on Windows.

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

> Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundation.cpp:782
> +         contentsNeedsDisplay();

Spacing is a little bit weird here.

> Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundation.h:284
> +#endif // MediaPlayerPrivateAVFoundation_h

A blank line between these end ifs would be nice.

[Jeff Miller]

Note this is now <rdar://problem/9894105>, not <rdar://problem/9083559>

[Darin Adler]

Comment on attachment 102865 [details]
First cut at implementing AVCF support on Windows.

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

Looks good.

> Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundation.cpp:782
> +         contentsNeedsDisplay();

Indented here by an extra space.

> Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundationCF.cpp:176
> +    static CFArrayRef keys;
> +    if (!keys) {
> +        static const CFStringRef keyNames[] = {
> +            AVCFAssetPropertyDuration,
> +            AVCFAssetPropertyNaturalSize,
> +            AVCFAssetPropertyPreferredTransform,
> +            AVCFAssetPropertyPreferredRate,
> +            AVCFAssetPropertyPlayable,
> +            AVCFAssetPropertyTracks 
> +        };
> +        keys = CFArrayCreate(0, (const void**)keyNames, sizeof(keyNames) / sizeof(keyNames[0]), &kCFTypeArrayCallBacks);
> +    }
> +    return keys;

I suggest putting the creation of the array in a separate create function, then you can just do it like this:

    static CFArrayRef keys = createMetadataKeysArray();
    return keys;

> Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundationCF.cpp:613
> +    return static_cast<unsigned>(totalMediaSize);

What makes it safe to cast this to unsigned?

>> Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundationCF.cpp:1057
>> +    AVCFAssetLoadValuesAsynchronouslyForProperties(avAsset(), metadataKeyNames(), loadMetadataCompletionCallback, this);	
> 
> Tab found; better to use spaces  [whitespace/tab] [1]

Tab is at the end of this line.

[Darin Adler]

Comment on attachment 102865 [details]
First cut at implementing AVCF support on Windows.

Please fix the things the style bot complained about.

[Jeff Miller]

(In reply to comment #15)
> (From update of attachment 102865 [details])
> Please fix the things the style bot complained about.

Yup, working on that now, as well as Brian's comments.

[Jeff Miller]

Comment on attachment 102865 [details]
First cut at implementing AVCF support on Windows.

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

>>> Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundation.cpp:782
>>> +         contentsNeedsDisplay();
>> 
>> Spacing is a little bit weird here.
> 
> Indented here by an extra space.

Fixed.

>> Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundation.h:284
>> +#endif // MediaPlayerPrivateAVFoundation_h
> 
> A blank line between these end ifs would be nice.

Fixed, also added a comment on the #endif for namespace WebCore above these two

>> Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundationCF.cpp:34
>> +#include "FloatConversion.h"
> 
> Alphabetical sorting problem.  [build/include_order] [4]

Fixed.

>> Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundationCF.cpp:45
>> +#include <wtf/Threading.h>
> 
> Alphabetical sorting problem.  [build/include_order] [4]

Fixed.

>> Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundationCF.cpp:47
>> +#include <AVFoundationCF/AVFoundationCF.h>
> 
> Alphabetical sorting problem.  [build/include_order] [4]

Fixed.

>> Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundationCF.cpp:48
>> +#include <AVFoundationCF/AVCFPlayerLayer.h>
> 
> Alphabetical sorting problem.  [build/include_order] [4]

Fixed.

>> Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundationCF.cpp:85
>> +    RetainPtr<CGImageRef> createImageForTimeInRect(float time, const IntRect& rect);
> 
> The parameter name "rect" adds no information, so it should be removed.  [readability/parameter_name] [5]

Fixed.

>> Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundationCF.cpp:134
>> +    LayerClient(AVFWrapper* parent) : m_parent(parent) {}
> 
> Missing space inside { }.  [whitespace/braces] [5]

Fixed.

>> Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundationCF.cpp:176
>> +    return keys;
> 
> I suggest putting the creation of the array in a separate create function, then you can just do it like this:
> 
>     static CFArrayRef keys = createMetadataKeysArray();
>     return keys;

Sounds good, will do.

>> Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundationCF.cpp:613
>> +    return static_cast<unsigned>(totalMediaSize);
> 
> What makes it safe to cast this to unsigned?

This isn't a good answer, but we're already doing the same thing on the Mac in MediaPlayerPrivateAVFoundationObjC::totalBytes().  I will add a FIXME for now and talk to Eric about this tomorrow.

>> Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundationCF.cpp:885
>> +        }
> 
> One line control clauses should not use braces.  [whitespace/braces] [4]

Fixed.

>> Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundationCF.cpp:1016
>> +    }
> 
> One line control clauses should not use braces.  [whitespace/braces] [4]

Fixed.

>>> Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundationCF.cpp:1057
>>> +    AVCFAssetLoadValuesAsynchronouslyForProperties(avAsset(), metadataKeyNames(), loadMetadataCompletionCallback, this);	
>> 
>> Tab found; better to use spaces  [whitespace/tab] [1]
> 
> Tab is at the end of this line.

Fixed.

>> Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundationCF.cpp:1107
>> +	CACFLayerInsertSublayer(m_videoLayerWrapper->platformLayer(), m_caVideoLayer.get(), 0);
> 
> Tab found; better to use spaces  [whitespace/tab] [1]

Fixed.

>> Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundationCF.cpp:1156
>> +    AVCFAssetImageGeneratorSetApertureMode(m_imageGenerator.get(), AVCFAssetImageGeneratorApertureModeCleanAperture );
> 
> Extra space before )  [whitespace/parens] [2]

Fixed.

>> Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundationCF.h:47
>> +    static PassOwnPtr<MediaPlayerPrivateInterface> create(MediaPlayer* player);
> 
> The parameter name "player" adds no information, so it should be removed.  [readability/parameter_name] [5]

Fixed.

[Adam Roben (:aroben)]

Comment on attachment 102865 [details]
First cut at implementing AVCF support on Windows.

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

> Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundationCF.cpp:58
> +#pragma comment(lib, "AVFoundationCF.lib")
> +#pragma comment(lib, "CoreMedia.lib")
> +#pragma comment(lib, "libdispatch.lib")
> +
> +// Use the soft link macros so we can easily test for the existence of the dlls.
> +SOFT_LINK_LIBRARY(AVFoundationCF)
> +SOFT_LINK_LIBRARY(CoreMedia)
> +SOFT_LINK_LIBRARY(libdispatch)

It doesn't seem right to both link against and delay-load these DLLs. Also, we should be linking/loading the _debug variants in Debug_All builds.

> Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundationCF.cpp:60
> +using namespace WTF;

Is this really needed?

> Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundationCF.cpp:66
> +class LayerClient;
> +class AVFWrapper {

Missing a blank line here.

> Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundationCF.cpp:69
> +public:
> +
> +    AVFWrapper(MediaPlayerPrivateAVFoundationCF*);

Extra blank line here.

> Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundationCF.cpp:195
> +static const CFStringRef CMTimeRangeStartKey()
> +{
> +    DEFINE_STATIC_LOCAL(CFStringRef, key, (CFSTR("start")));
> +    return key;
> +}
> +
> +static const CFStringRef CMTimeRangeDurationKey()
> +{
> +    DEFINE_STATIC_LOCAL(CFStringRef, key, (CFSTR("duration")));
> +    return key;
> +}
> +
> +static const CFStringRef CACFContextNeedsFlushNotification()
> +{
> +    DEFINE_STATIC_LOCAL(CFStringRef, name, (CFSTR("kCACFContextNeedsFlushNotification")));
> +    return name;
> +}

I don't think the "const"s here have any effect.

Should we have FIXMEs to use real constants for these strings (ideally provided by CoreMedia and AVCF)?

> Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundationCF.cpp:725
> +bool MediaPlayerPrivateAVFoundationCF::isAvailable()
> +{
> +    return AVFoundationCFLibrary() && CoreMediaLibrary() && libdispatchLibrary();
> +}

Do we really expect this code to ever be called when these DLLs aren't available?

[Eric Carlson]

Comment on attachment 102865 [details]
First cut at implementing AVCF support on Windows.

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

>> Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundationCF.cpp:58
>> +SOFT_LINK_LIBRARY(libdispatch)
> 
> It doesn't seem right to both link against and delay-load these DLLs. Also, we should be linking/loading the _debug variants in Debug_All builds.

I think it is worth soft linking with the dlls so they aren't loaded until a page used a media element.

>> Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundationCF.cpp:725
>> +}
> 
> Do we really expect this code to ever be called when these DLLs aren't available?

I think we might as well do this as long as we soft link the dlls. It doesn't cost anything and it will prevent Safari from crashing on a system where one or more of the dlls isn't available for whatever reason.

[Jeff Miller]

Comment on attachment 102865 [details]
First cut at implementing AVCF support on Windows.

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

>>> Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundationCF.cpp:58
>>> +SOFT_LINK_LIBRARY(libdispatch)
>> 
>> It doesn't seem right to both link against and delay-load these DLLs. Also, we should be linking/loading the _debug variants in Debug_All builds.
> 
> I think it is worth soft linking with the dlls so they aren't loaded until a page used a media element.

I will leave the soft linking in, but use the debug versions when appropriate.

>> Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundationCF.cpp:60
>> +using namespace WTF;
> 
> Is this really needed?

No, I removed it.

>> Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundationCF.cpp:66
>> +class AVFWrapper {
> 
> Missing a blank line here.

Fixed.

>> Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundationCF.cpp:69
>> +    AVFWrapper(MediaPlayerPrivateAVFoundationCF*);
> 
> Extra blank line here.

Removed.

>> Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundationCF.cpp:195
>> +}
> 
> I don't think the "const"s here have any effect.
> 
> Should we have FIXMEs to use real constants for these strings (ideally provided by CoreMedia and AVCF)?

Removed the const's and added FIXMEs.

>>> Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundationCF.cpp:725
>>> +}
>> 
>> Do we really expect this code to ever be called when these DLLs aren't available?
> 
> I think we might as well do this as long as we soft link the dlls. It doesn't cost anything and it will prevent Safari from crashing on a system where one or more of the dlls isn't available for whatever reason.

I'm going to leave this in, based on Eric's comments.

[Jeff Miller]

AVCF support on Windows landed in <http://trac.webkit.org/changeset/92404>