Bug 130275 – Provide preference to enable additional AVFoundation options

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

[Brent Fulgham]

Add a new preference and attribute to allow WebKit to tell AVFoundation to turn on additional processing during playback.

[Brent Fulgham]

Created attachment 226802 [details]
Patch

[Brent Fulgham]

Comment on attachment 226802 [details]
Patch

Initial WIP. Checking Windows build.

[Brent Fulgham]

Created attachment 226810 [details]
Patch

[Jon Lee]

Comment on attachment 226810 [details]
Patch

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

Where's WebKit2?

> Source/WebCore/html/HTMLMediaElement.cpp:5986
> +    return true;

This logic, while correct, seems a bit odd to me. Why check for the attribute first before the setting, rather than the other way around?

> Source/WebCore/platform/graphics/avfoundation/cf/MediaPlayerPrivateAVFoundationCF.cpp:2
> + * Copyright (C) 2011-2014 Apple Inc. All rights reserved.

I think proper style was to list individual years separately than a range…?

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:589
> +    

Please remove.

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:617
> +        [options.get() setObject: [NSNumber numberWithBool: TRUE] forKey: AVURLAssetInheritURIQueryComponentFromReferencingURIKey];

no space after ":"

[Brent Fulgham]

Created attachment 226824 [details]
Updated to handle Windows.

[Brent Fulgham]

Comment on attachment 226810 [details]
Patch

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

>> Source/WebCore/html/HTMLMediaElement.cpp:5986
>> +    return true;
> 
> This logic, while correct, seems a bit odd to me. Why check for the attribute first before the setting, rather than the other way around?

After talking with Jer, we decided to make this a generic method that could check for arbitrary attributes, not just this specific case we are dealing with today. If the attribute doesn't exist, it doesn't really matter if the setting is turned off since the result is the same. But if the setting does exist, we only need to check the setting for this specific URI case.

I'm not strongly attached to this; I could switch the order if you prefer.

>> Source/WebCore/platform/graphics/avfoundation/cf/MediaPlayerPrivateAVFoundationCF.cpp:2
>> + * Copyright (C) 2011-2014 Apple Inc. All rights reserved.
> 
> I think proper style was to list individual years separately than a range…?

Darin told me in a recent review that we should use ranges.

> Source/WebCore/platform/graphics/avfoundation/cf/MediaPlayerPrivateAVFoundationCF.cpp:255
> +static CFStringRef AVCFURLAssetInheritURIQueryComponentFromReferencingURIKey()

Note: This isn't needed and is removed in an updated patch.

>> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:589
>> +    
> 
> Please remove.

Will do.

>> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:617
>> +        [options.get() setObject: [NSNumber numberWithBool: TRUE] forKey: AVURLAssetInheritURIQueryComponentFromReferencingURIKey];
> 
> no space after ":"

Will do.

[Brent Fulgham]

(In reply to comment #4)
> (From update of attachment 226810 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=226810&action=review
> 
> Where's WebKit2?

I'll review this on Mac after the servers are back up. I believe the preference logic is auto generated based on the Settings.in file.

[Brent Fulgham]

Created attachment 226875 [details]
Patch

[Brent Fulgham]

Created attachment 226878 [details]
Patch

[Eric Carlson]

Comment on attachment 226878 [details]
Patch

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

> Source/WebCore/platform/graphics/MediaPlayer.h:251
> +    virtual bool doesHaveAttribute(const QualifiedName&) const { return false; }

This is a layering violation, QualifiedName is in WebCore/dom.

> Source/WebCore/platform/graphics/avfoundation/cf/MediaPlayerPrivateAVFoundationCF.cpp:39
> +#include "HTMLNames.h"

This can't be included from platform/

> Source/WebCore/platform/graphics/avfoundation/cf/MediaPlayerPrivateAVFoundationCF.cpp:1339
> +        bool value = true;

Nit: why not reuse inheritURI?

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:43
> +#import "HTMLNames.h"

Layering.

[Brent Fulgham]

Created attachment 226935 [details]
Patch

[Brent Fulgham]

Comment on attachment 226935 [details]
Patch

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

WIP patch to move changes from Mac to Windows.

> Source/WebCore/platform/graphics/MediaPlayer.h:142
> +class QualifiedName;

Whoops -- need to remove this.

> Source/WebKit/win/WebView.cpp:5054
> +    hr = prefsPrivate->isInheritURIQueryComponentEnabled(&enabled); // OOPS! Don't checkin

Oops 2.

[Eric Carlson]

Comment on attachment 226935 [details]
Patch

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

> Source/WebCore/html/HTMLMediaElement.cpp:5989
> +    

Nit: unnecessary white space.

> Source/WebCore/platform/graphics/MediaPlayer.cpp:1414
> +    

Ditto.

> Source/WebCore/platform/graphics/MediaPlayer.h:303
> +    

Ditto.

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:615
> +    

Ditto.

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:618
> +    

Ditto.

> Source/WebKit2/Shared/WebPreferencesStore.h:218
> +    

Ditto.

> Source/WebKit2/UIProcess/API/C/WKPreferencesPrivate.h:324
> +    

Ditto.

> Source/WebKit2/UIProcess/API/C/WKPreferencesPrivate.h:328
> +    

Ditto.

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:2628
> +    

Ditto.

> Source/WebKit/mac/WebView/WebView.mm:2394
> +    

Ditto.

[Brent Fulgham]

Created attachment 226945 [details]
Finalized after testing.

[Brent Fulgham]

Comment on attachment 226935 [details]
Patch

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

>> Source/WebCore/html/HTMLMediaElement.cpp:5989
>> +    
> 
> Nit: unnecessary white space.

Looks like I have a bad Xcode editor preference set. I'll fix these when landing.

[Brent Fulgham]

Committed r165753: <http://trac.webkit.org/changeset/165753>