Adopt AVFoundation / AVCF media back end on Windows.
<rdar://problem/9083559>
Created attachment 102409 [details] Make MediaPlayerPrivateAVFoundation::setDelayCallbacks() const Required by the Windows subclass of MediaPlayerPrivateAVFoundation.
Created attachment 102412 [details] Prepare WebCore.vcproj to support AVCF media back end on Windows
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.
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.
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!
(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!
Patch to make MediaPlayerPrivateAVFoundation::setDelayCallbacks() const landed in <http://trac.webkit.org/changeset/92034>.
Patch to prepare WebCore.vcproj to support AVCF media back end on Windows landed in <http://trac.webkit.org/changeset/92035>
Created attachment 102865 [details] First cut at implementing AVCF support on Windows.
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.
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.
Note this is now <rdar://problem/9894105>, not <rdar://problem/9083559>
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.
Comment on attachment 102865 [details] First cut at implementing AVCF support on Windows. Please fix the things the style bot complained about.
(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.
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.
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?
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.
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.
AVCF support on Windows landed in <http://trac.webkit.org/changeset/92404>