Bug 130855 – Add support for AirPlay picker in WK2 for iOS

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

[Enrica Casucci]

This tracks the work required to support the airplay picker on WK2.

<rdar://problem/15349859>

[Enrica Casucci]

Created attachment 227970 [details]
Patch

[Eric Carlson]

Comment on attachment 227970 [details]
Patch

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

r=me on the WebCore changes

> Source/WebCore/html/HTMLMediaSession.cpp:213
> -    MediaSessionManager::sharedManager().showPlaybackTargetPicker();
> +#if PLATFORM(IOS)
> +    element.document().frame()->page()->chrome().client().showAirPlayRoutePicker(element.hasVideo());
> +#endif

MediaSessionManager::showPlaybackTargetPicker doesn't have any purpose now, it should be removed.

> Source/WebCore/page/ChromeClient.h:260
> +    virtual void showAirPlayRoutePicker(bool hasVideo) = 0;

This name shouldn't be be AirPlay specific, maybe showPlaybackTargetPicker?

> Source/WebCore/platform/audio/MediaSessionManager.cpp:280
> +    // FIXME: Add implementation.

It would be good to have a call to notImplemented() here.

> Source/WebCore/platform/audio/ios/MediaSessionManagerIOS.mm:76
> +SOFT_LINK_CLASS(MediaPlayer, MPVolumeView)

Nit: we usually group all of the SOFT_LINK_CLASS() macros together.

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:2077
> -    m_allowsWirelessVideoPlayback = ![m_avPlayer.get() allowsExternalPlayback];
> +    m_allowsWirelessVideoPlayback = [m_avPlayer.get() allowsExternalPlayback];

:-O

[Enrica Casucci]

thanks for the review.
> MediaSessionManager::showPlaybackTargetPicker doesn't have any purpose now, it should be removed.
I'll remove it.
> 
> > Source/WebCore/page/ChromeClient.h:260
> > +    virtual void showAirPlayRoutePicker(bool hasVideo) = 0;
> 
> This name shouldn't be be AirPlay specific, maybe showPlaybackTargetPicker?
Good idea, I'll change it.
> 
> > Source/WebCore/platform/audio/MediaSessionManager.cpp:280
> > +    // FIXME: Add implementation.
> 
> It would be good to have a call to notImplemented() here.
Will do.
> 
> > Source/WebCore/platform/audio/ios/MediaSessionManagerIOS.mm:76
> > +SOFT_LINK_CLASS(MediaPlayer, MPVolumeView)
> 
> Nit: we usually group all of the SOFT_LINK_CLASS() macros together.
Ok.

[Joseph Pecoraro]

Comment on attachment 227970 [details]
Patch

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

>> Source/WebCore/page/ChromeClient.h:260
>> +    virtual void showAirPlayRoutePicker(bool hasVideo) = 0;
> 
> This name shouldn't be be AirPlay specific, maybe showPlaybackTargetPicker?

Does this need to be pure virtual? Can we provide a stub empty implementation so ports not interested do not need to include it?

I would also expect, because this is in PLATFORM(IOS) that it be behind ENABLE(IOS_AIRPLAY) as well. But maybe that ENABLE flag is not as important now.

> Source/WebCore/platform/audio/ios/MediaSessionManagerIOS.mm:257
> +    _airPlayPresenceRoutingController = [[getMPAVRoutingControllerClass() alloc] initWithName:@"WebCore - HTML media element checking for AirPlay route presence"];

I think this should adoptNS(...), otherwise we will leak.

> Source/WebKit2/Configurations/WebKit2.xcconfig:33
> +FRAMEWORK_AND_LIBRARY_LDFLAGS_iphonesimulator = -lobjc -framework CFNetwork -framework CoreFoundation -framework CoreGraphics -framework CorePDF -framework CoreText -framework Foundation -framework GraphicsServices -framework ImageIO -framework UIKit -framework MediaPlayer -framework OpenGLES -framework MobileAsset -framework WebKit -lMobileGestalt -lassertion_extension;

Would it help performance to soft link MediaPlayer framework?

> Source/WebKit2/UIProcess/ios/PageClientImplIOS.h:116
> +    virtual void showAirPlayRoutePicker(bool hasVideo, const WebCore::IntRect& elementRect);

Nit: override. In fact, these should all probably have override but at least add it for the new functions.

> Source/WebKit2/UIProcess/ios/forms/WKAirPlayRoutePicker.mm:39
> +#import <MediaPlayer/MPAudioVideoRoutingPopoverController.h>
> +#import <MediaPlayer/MPAudioVideoRoutingActionSheet.h>
> +#import <MediaPlayer/MPAudioVideoRoutingPopoverController.h>
> +#import <MediaPlayer/MPAudioVideoRoutingActionSheet.h>

Duplicate includes.

> Source/WebKit2/UIProcess/ios/forms/WKAirPlayRoutePicker.mm:47
> +    RetainPtr<id<WKFormControl>> _control;

This does not appear to be used. Can we remove it?

> Source/WebKit2/UIProcess/ios/forms/WKAirPlayRoutePicker.mm:50
> +    RetainPtr<MPAVRoutingController> _airPlayPickerRoutingController;
> +    RetainPtr<MPAudioVideoRoutingPopoverController> _airPlayPopoverController;  // iPad
> +    RetainPtr<MPAudioVideoRoutingActionSheet> _airPlayActionSheet;              // iPhone

These all have "_airPlay" in the name, but that is redundant considering the class name. Maybe you can drop the prefix, and just have _popoverController, _actionSheet, and _routingController. Much easier to distinguish down below.

> Source/WebKit2/UIProcess/ios/forms/WKAirPlayRoutePicker.mm:96
> +    [self _dismissAirPlayRoutePickerIPad];

Shouldn't this re-present the picker (-show:fromRect:), instead of dismissing?

> Source/WebKit2/UIProcess/ios/forms/WKAirPlayRoutePicker.mm:162
> +#endif  // PLATFORM(IOS)

Weird double space here.

> Source/WebKit/ios/WebCoreSupport/WebChromeClientIOS.h:86
> +    virtual void showAirPlayRoutePicker(bool hasVideo) override;

If this is stubbed in ChromeClient.h, we don't even need to update WebKit1. Unless we really will need to fix this later, then I would suggest not stubbing it.

[Enrica Casucci]

> > Source/WebCore/platform/audio/ios/MediaSessionManagerIOS.mm:257
> > +    _airPlayPresenceRoutingController = [[getMPAVRoutingControllerClass() alloc] initWithName:@"WebCore - HTML media element checking for AirPlay route presence"];
> 
> I think this should adoptNS(...), otherwise we will leak.
OOPS! good catch!
> 
> > Source/WebKit2/Configurations/WebKit2.xcconfig:33
> > +FRAMEWORK_AND_LIBRARY_LDFLAGS_iphonesimulator = -lobjc -framework CFNetwork -framework CoreFoundation -framework CoreGraphics -framework CorePDF -framework CoreText -framework Foundation -framework GraphicsServices -framework ImageIO -framework UIKit -framework MediaPlayer -framework OpenGLES -framework MobileAsset -framework WebKit -lMobileGestalt -lassertion_extension;
> 
> Would it help performance to soft link MediaPlayer framework?
I don't know.
> 
> > Source/WebKit2/UIProcess/ios/PageClientImplIOS.h:116
> > +    virtual void showAirPlayRoutePicker(bool hasVideo, const WebCore::IntRect& elementRect);
> 
> Nit: override. In fact, these should all probably have override but at least add it for the new functions.
> 
I'll fix it.
> > Source/WebKit2/UIProcess/ios/forms/WKAirPlayRoutePicker.mm:39
> > +#import <MediaPlayer/MPAudioVideoRoutingPopoverController.h>
> > +#import <MediaPlayer/MPAudioVideoRoutingActionSheet.h>
> > +#import <MediaPlayer/MPAudioVideoRoutingPopoverController.h>
> > +#import <MediaPlayer/MPAudioVideoRoutingActionSheet.h>
> 
> Duplicate includes.
> 
> > Source/WebKit2/UIProcess/ios/forms/WKAirPlayRoutePicker.mm:47
> > +    RetainPtr<id<WKFormControl>> _control;
> 
> This does not appear to be used. Can we remove it?
> 
OOPS leftover from cut&paste :-)


> These all have "_airPlay" in the name, but that is redundant considering the class name. Maybe you can drop the prefix, and just have _popoverController, _actionSheet, and _routingController. Much easier to distinguish down below.
> 
Sure.
> > Source/WebKit2/UIProcess/ios/forms/WKAirPlayRoutePicker.mm:96
> > +    [self _dismissAirPlayRoutePickerIPad];
> 
No, we decided to remove it upon rotation.

> Shouldn't this re-present the picker (-show:fromRect:), instead of dismissing?
> 
> > Source/WebKit2/UIProcess/ios/forms/WKAirPlayRoutePicker.mm:162
> > +#endif  // PLATFORM(IOS)
> 
> Weird double space here.
> 

> > Source/WebKit/ios/WebCoreSupport/WebChromeClientIOS.h:86
> > +    virtual void showAirPlayRoutePicker(bool hasVideo) override;
> 
> If this is stubbed in ChromeClient.h, we don't even need to update WebKit1. Unless we really will need to fix this later, then I would suggest not stubbing it.
I'll post the patch for WK1 shortly.

thanks for the comments!

[Benjamin Poulain]

Comment on attachment 227970 [details]
Patch

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

> Source/WebCore/html/HTMLMediaSession.cpp:212
> +    element.document().frame()->page()->chrome().client().showAirPlayRoutePicker(element.hasVideo());

Is it safe to assume the element has a document/frame etc?
Can't the JavaScript use the API from a detached element? Or an element into a detached frame?

> Source/WebKit2/Configurations/WebKit2.xcconfig:33
> +FRAMEWORK_AND_LIBRARY_LDFLAGS_iphonesimulator = -lobjc -framework CFNetwork -framework CoreFoundation -framework CoreGraphics -framework CorePDF -framework CoreText -framework Foundation -framework GraphicsServices -framework ImageIO -framework UIKit -framework MediaPlayer -framework OpenGLES -framework MobileAsset -framework WebKit -lMobileGestalt -lassertion_extension;

Maybe we should soft link?

> Source/WebKit2/UIProcess/ios/forms/WKAirPlayRoutePicker.h:34
> +- (void)show:(BOOL)hasVideo fromRect:(CGRect)elementRect;

This is a little confusing. For audio, you would call show:NO, which makes it looks like you want to hide the picker.
Probably better to used a type enum and do something like showMedia:(typed enum) fromRect:()...

> Source/WebKit2/UIProcess/ios/forms/WKAirPlayRoutePicker.mm:51
> +    WKContentView* _view;   // Weak reference.

Would __weak work?

> Source/WebKit2/UIProcess/ios/forms/WKAirPlayRoutePicker.mm:65
> +    if (_airPlayActionSheet) {

You don't need the if().

> Source/WebKit2/UIProcess/ios/forms/WKAirPlayRoutePicker.mm:70
> +    if (_airPlayPopoverController)

Maybe ditto here.

> Source/WebKit2/UIProcess/ios/forms/WKAirPlayRoutePicker.mm:118
> +    if (_airPlayPopoverController)
> +        return;

Shouldn't we check for _airPlayPickerRoutingController in show:fromRect: instead?

> Source/WebKit2/WebProcess/WebPage/WebPage.h:1129
> +    WebCore::IntPoint m_interactionLocation;

Maybe m_lastInteractionLocation?

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:307
> +    m_interactionLocation = point;

I think this should use the roundedAdjustedPoint.

[Enrica Casucci]

(In reply to comment #6)
> (From update of attachment 227970 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=227970&action=review
> 
> > Source/WebCore/html/HTMLMediaSession.cpp:212
> > +    element.document().frame()->page()->chrome().client().showAirPlayRoutePicker(element.hasVideo());
> 
> Is it safe to assume the element has a document/frame etc?
> Can't the JavaScript use the API from a detached element? Or an element into a detached frame?
> 
I believe the answer is no, but I'd like Eric to comment on this. 

> This is a little confusing. For audio, you would call show:NO, which makes it looks like you want to hide the picker.
> Probably better to used a type enum and do something like showMedia:(typed enum) fromRect:()...
> 
At the call site this looks like show:hasVideo, I think it is ok.

I've addressed all your other comments. thanks!

[Enrica Casucci]

Committed revision 166384.

[Eric Carlson]

(In reply to comment #7)
> (In reply to comment #6)
> > (From update of attachment 227970 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=227970&action=review
> > 
> > > Source/WebCore/html/HTMLMediaSession.cpp:212
> > > +    element.document().frame()->page()->chrome().client().showAirPlayRoutePicker(element.hasVideo());
> > 
> > Is it safe to assume the element has a document/frame etc?
> > Can't the JavaScript use the API from a detached element? Or an element into a detached frame?
> > 
> I believe the answer is no, but I'd like Eric to comment on this. 
> 
Oops, it looks like I forgot to save my comment about guarding against a NULL frame!

It is safe to assume there is a document because there is an element. showingPlaybackTargetPickerPermitted (called earlier in the method) returns false if there isn't a page, but it is probably worth adding something like ASSERT(element.document().page()).