Bug 204979 – The value of [AVPlayerViewController isPictureInPicturePossible] is NO in the first attempt to enter PiP

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Peng Liu]

The value of [AVPlayerViewControl isPictureInPicturePossible] is NO in the first attempt to enter PiP

[Peng Liu]

<rdar://problem/57671139>

[Peng Liu]

Created attachment 385086 [details]
Patch

[Peng Liu]

Created attachment 385160 [details]
Patch (Updated to fix build failures)

[Peng Liu]

Created attachment 393821 [details]
Patch

[Jer Noble]

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.

[Eric Carlson]

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];

[Peng Liu]

(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.

[Peng Liu]

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!

[Peng Liu]

Created attachment 393874 [details]
Patch for landing

[EWS]

Committed r258655: <https://trac.webkit.org/changeset/258655>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 393874 [details].

[Darin Adler]

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?

[Peng Liu]

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.

[Peng Liu]

Reopen this bug to fix a watch os build failure.

[Peng Liu]

Created attachment 393912 [details]
Patch to fix a watch OS build failure

[David Kilzer (:ddkilzer)]

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.

[David Kilzer (:ddkilzer)]

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

[Peng Liu]

Created attachment 393927 [details]
patch to fix build failures

[Peng Liu]

Created attachment 393928 [details]
patch to fix build failures

[Peng Liu]

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.

[EWS]

Committed r258682: <https://trac.webkit.org/changeset/258682>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 393928 [details].

[Darin Adler]

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!

[Peng Liu]

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.

[Peng Liu]

Reopen this bug to fix mistakes on the macro name.

[Peng Liu]

Created attachment 395529 [details]
A follow-up patch to fix mistakes on #if HAVE(HAVE_AVOBSERVATIONCONTROLLER)

[Darin Adler]

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.

[Darin Adler]

(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.

[Peng Liu]

Created attachment 395539 [details]
Fix iOS build failures by adding NS_ASSUME_NONNULL_BEGIN/NS_ASSUME_NONNULL_END

[EWS]

Committed r259561: <https://trac.webkit.org/changeset/259561>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 395539 [details].