The value of [AVPlayerViewControl isPictureInPicturePossible] is NO in the first attempt to enter PiP
<rdar://problem/57671139>
Created attachment 385086 [details] Patch
Created attachment 385160 [details] Patch (Updated to fix build failures)
Created attachment 393821 [details] Patch
Comment on attachment 393821 [details] Patch r=me. Is there a radar on the weird PiP behavior here? If so, it would be great to have a "FIXME: remove this once XXX is fixed" comment in this area.
Comment on attachment 393821 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=393821&action=review > Source/WebCore/platform/ios/VideoFullscreenInterfaceAVKit.mm:1380 > + [m_playerViewController tryToStartPictureInPicture]; Won't this fail to enter PiP if @pictureInPicturePossible is already true and so doesn't change? Maybe something like this would be better? if ([m_playerViewController isPictureInPicturePossible]) [m_playerViewController startPictureInPicture]; else [m_playerViewController tryToStartPictureInPicture];
(In reply to Jer Noble from comment #5) > Comment on attachment 393821 [details] > Patch > > r=me. Is there a radar on the weird PiP behavior here? If so, it would be > great to have a "FIXME: remove this once XXX is fixed" comment in this area. Thanks for the review. Yes, there is a radar for it, but not for this patch. This patch is a protection after AVKit fixed their issue. We won’t remove it.
Comment on attachment 393821 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=393821&action=review >> Source/WebCore/platform/ios/VideoFullscreenInterfaceAVKit.mm:1380 >> + [m_playerViewController tryToStartPictureInPicture]; > > Won't this fail to enter PiP if @pictureInPicturePossible is already true and so doesn't change? > > Maybe something like this would be better? > > if ([m_playerViewController isPictureInPicturePossible]) > [m_playerViewController startPictureInPicture]; > else > [m_playerViewController tryToStartPictureInPicture]; No it won't fail because of “includeInitialValue:YES”. The initial value is treated as a change for _avPlayerViewControllerObservationController as well. However, I think your suggestion is a great idea to simplify the logic for most cases. So I will update the patch. Thanks!
Created attachment 393874 [details] Patch for landing
Committed r258655: <https://trac.webkit.org/changeset/258655> All reviewed patches have been landed. Closing bug and clearing flags on attachment 393874 [details].
Comment on attachment 393874 [details] Patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=393874&action=review > Source/WebCore/platform/ios/VideoFullscreenInterfaceAVKit.mm:577 > + _fullscreenInterface = nullptr; What does this line of code accomplish?
Comment on attachment 393874 [details] Patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=393874&action=review >> Source/WebCore/platform/ios/VideoFullscreenInterfaceAVKit.mm:577 >> + _fullscreenInterface = nullptr; > > What does this line of code accomplish? Oops, it does nothing useful. Will remove it in another patch. Probably we need a refactoring to replace it with a smart pointer.
Reopen this bug to fix a watch os build failure.
Created attachment 393912 [details] Patch to fix a watch OS build failure
Comment on attachment 393912 [details] Patch to fix a watch OS build failure View in context: https://bugs.webkit.org/attachment.cgi?id=393912&action=review > Source/WebCore/PAL/pal/spi/cocoa/AVKitSPI.h:32 > +#if !PLATFORM(WATCHOS) > #import <AVKit/AVObservationController.h> > +#endif This header is also not present on macOS Mojave (build 18G103, for example). I just hit a build failure on that build of macOS: In file included from OpenSource/Source/WebCore/platform/graphics/avfoundation/objc/MediaPlaybackTargetPickerMac.mm:36: PAL/pal/spi/cocoa/AVKitSPI.h:30:9: fatal error: 'AVKit/AVObservationController.h' file not found #import <AVKit/AVObservationController.h> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 1 error generated.
Comment on attachment 393912 [details] Patch to fix a watch OS build failure View in context: https://bugs.webkit.org/attachment.cgi?id=393912&action=review >> Source/WebCore/PAL/pal/spi/cocoa/AVKitSPI.h:32 >> +#endif > > This header is also not present on macOS Mojave (build 18G103, for example). I just hit a build failure on that build of macOS: > > In file included from OpenSource/Source/WebCore/platform/graphics/avfoundation/objc/MediaPlaybackTargetPickerMac.mm:36: > PAL/pal/spi/cocoa/AVKitSPI.h:30:9: fatal error: 'AVKit/AVObservationController.h' file not found > #import <AVKit/AVObservationController.h> > ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > 1 error generated. Note that the AVObservationController class is available; the header just isn't there: $ nm /System/Library/Frameworks/AVKit.framework/AVKit | grep AVObservationController | grep OBJC_CLASS 0000000000144bd0 S _OBJC_CLASS_$_AVObservationController So maybe you can conditionally include the header here, and then just declare the classes/methods for Mojave in the USE(APPLE_INTERNAL_SDK) section? +@class AVKeyValueChange; + +@interface AVObservationController<Owner> : NSObject +- (instancetype)initWithOwner:(Owner)owner NS_DESIGNATED_INITIALIZER; +- (id)startObserving:(id)object keyPath:(NSString *)keyPath includeInitialValue:(BOOL)shouldIncludeInitialValue observationHandler:(void (^)(Owner owner, id observed, AVKeyValueChange *change))observationHandler; +- (void)stopAllObservation; +@end
Created attachment 393927 [details] patch to fix build failures
Created attachment 393928 [details] patch to fix build failures
Comment on attachment 393912 [details] Patch to fix a watch OS build failure View in context: https://bugs.webkit.org/attachment.cgi?id=393912&action=review >>> Source/WebCore/PAL/pal/spi/cocoa/AVKitSPI.h:32 >>> +#endif >> >> This header is also not present on macOS Mojave (build 18G103, for example). I just hit a build failure on that build of macOS: >> >> In file included from OpenSource/Source/WebCore/platform/graphics/avfoundation/objc/MediaPlaybackTargetPickerMac.mm:36: >> PAL/pal/spi/cocoa/AVKitSPI.h:30:9: fatal error: 'AVKit/AVObservationController.h' file not found >> #import <AVKit/AVObservationController.h> >> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ >> 1 error generated. > > Note that the AVObservationController class is available; the header just isn't there: > > $ nm /System/Library/Frameworks/AVKit.framework/AVKit | grep AVObservationController | grep OBJC_CLASS > 0000000000144bd0 S _OBJC_CLASS_$_AVObservationController > > So maybe you can conditionally include the header here, and then just declare the classes/methods for Mojave in the USE(APPLE_INTERNAL_SDK) section? > > +@class AVKeyValueChange; > + > +@interface AVObservationController<Owner> : NSObject > +- (instancetype)initWithOwner:(Owner)owner NS_DESIGNATED_INITIALIZER; > +- (id)startObserving:(id)object keyPath:(NSString *)keyPath includeInitialValue:(BOOL)shouldIncludeInitialValue observationHandler:(void (^)(Owner owner, id observed, AVKeyValueChange *change))observationHandler; > +- (void)stopAllObservation; > +@end Sorry for the trouble and thanks for the investigation! I updated the patch to only use AVObservationController in iOS for now.
Committed r258682: <https://trac.webkit.org/changeset/258682> All reviewed patches have been landed. Closing bug and clearing flags on attachment 393928 [details].
Comment on attachment 393928 [details] patch to fix build failures View in context: https://bugs.webkit.org/attachment.cgi?id=393928&action=review > Source/WebCore/PAL/pal/spi/cocoa/AVKitSPI.h:57 > +#if HAVE(HAVE_AVOBSERVATIONCONTROLLER) This change turns off the code entirely, because it says HAVE(HAVE_xxx). To turn the code back on, we need to change it all to HAVE(AVOBSERVATIONCONTROLLER). This also shows we have no tests covering this, because they would all fail!
Comment on attachment 393928 [details] patch to fix build failures View in context: https://bugs.webkit.org/attachment.cgi?id=393928&action=review >> Source/WebCore/PAL/pal/spi/cocoa/AVKitSPI.h:57 >> +#if HAVE(HAVE_AVOBSERVATIONCONTROLLER) > > This change turns off the code entirely, because it says HAVE(HAVE_xxx). > > To turn the code back on, we need to change it all to HAVE(AVOBSERVATIONCONTROLLER). > > This also shows we have no tests covering this, because they would all fail! Sorry for the stupid mistake. :-( Will upload a patch to fix it. The patch is to provide extra protection suggested by AVKit after they fixed the bug in AVPlayerViewController. With the current behavior of AVKit, WebKit works fine without this patch.
Reopen this bug to fix mistakes on the macro name.
Created attachment 395529 [details] A follow-up patch to fix mistakes on #if HAVE(HAVE_AVOBSERVATIONCONTROLLER)
Comment on attachment 395529 [details] A follow-up patch to fix mistakes on #if HAVE(HAVE_AVOBSERVATIONCONTROLLER) Any way to add a test for this? I caught it by code inspection, but that’s not a reliable way to check these things.
(In reply to Darin Adler from comment #25) > Any way to add a test for this? I caught it by code inspection, but that’s > not a reliable way to check these things. Oh, I see your comment now. No simple way to test for now.
Created attachment 395539 [details] Fix iOS build failures by adding NS_ASSUME_NONNULL_BEGIN/NS_ASSUME_NONNULL_END
Committed r259561: <https://trac.webkit.org/changeset/259561> All reviewed patches have been landed. Closing bug and clearing flags on attachment 395539 [details].