[webkit-changes] [280723] trunk/Source

Fri, 06 Aug 2021 08:10:12 -0700

Title: [280723] trunk/Source
Revision
280723
Author
jer.no...@apple.com
Date
2021-08-06 08:09:20 -0700 (Fri, 06 Aug 2021)

Log Message

[Cocoa] Remove support for AVAssetImageGenerator
https://bugs.webkit.org/show_bug.cgi?id=228560
<rdar://problem/81336280>

Reviewed by Eric Carlson.

Source/WebCore:

A much more minimal approach to removing support for AVAssetImageGenerator.

The only time we use an AVAssetImageGenerator (as opposed to an AVPlayerItemVideoOutput)
is when the latter does not currently have an available image enqueued. Because painting
is a synchronous operation, we use a synchronous API (the generator) to create an image
for that operation. However, this can create deadlocks if (for example) the resource needs
to load data on the main thread in order to complete the painting operation.

Instead, allow the main runloop to spin while waiting (up to 1_s) for the video output
to receive a decoded frame.

Drive-by fixes:
- Don't create an AVPlayerLayer at AVPlayer-creation; this causes the AVPlayerItemVideoOutput
  to never receive a decoded frambe (as the layer is not in a CALayer-heirarchy).
- preferredRenderingMode() shouldn't be "none" when the page isn't visible. We already
  just mark the layer as "hidden" in that case.
- Don't tear down the AVPlayerItemVideoOutput when creating an AVPlayerLayer; it'll just
  get re-created anyway.

* platform/graphics/avfoundation/MediaPlayerPrivateAVFoundation.cpp:
(WebCore::MediaPlayerPrivateAVFoundation::preferredRenderingMode const):
(WebCore::MediaPlayerPrivateAVFoundation::setUpVideoRendering):
* platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.h:
* platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:
(WebCore::MediaPlayerPrivateAVFoundationObjC::createAVPlayer):
(WebCore::MediaPlayerPrivateAVFoundationObjC::paintCurrentFrameInContext):
(WebCore::MediaPlayerPrivateAVFoundationObjC::createVideoOutput):
(WebCore::MediaPlayerPrivateAVFoundationObjC::paintWithVideoOutput):
(WebCore::MediaPlayerPrivateAVFoundationObjC::waitForVideoOutputMediaDataWillChange):
(WebCore::MediaPlayerPrivateAVFoundationObjC::outputMediaDataWillChange):
(-[WebCoreAVFPullDelegate outputMediaDataWillChange:]):
(-[WebCoreAVFPullDelegate setParent:]):

Source/WebKit:

Drive-by fix: we're passing the wrong value into acceleratedRenderingStateChanged(), and
we're not setting the correct initial value on MediaPlayerPrivateRemote creation.

* GPUProcess/media/RemoteMediaPlayerProxy.h:
* WebProcess/GPU/media/MediaPlayerPrivateRemote.cpp:
(WebKit::MediaPlayerPrivateRemote::MediaPlayerPrivateRemote):
(WebKit::MediaPlayerPrivateRemote::acceleratedRenderingStateChanged):

Modified Paths

Diff

Modified: trunk/Source/WebCore/ChangeLog (280722 => 280723)


--- trunk/Source/WebCore/ChangeLog	2021-08-06 15:04:13 UTC (rev 280722)
+++ trunk/Source/WebCore/ChangeLog	2021-08-06 15:09:20 UTC (rev 280723)
@@ -1,3 +1,44 @@
+2021-08-06  Jer Noble  <jer.no...@apple.com>
+
+        [Cocoa] Remove support for AVAssetImageGenerator
+        https://bugs.webkit.org/show_bug.cgi?id=228560
+        <rdar://problem/81336280>
+
+        Reviewed by Eric Carlson.
+
+        A much more minimal approach to removing support for AVAssetImageGenerator.
+
+        The only time we use an AVAssetImageGenerator (as opposed to an AVPlayerItemVideoOutput)
+        is when the latter does not currently have an available image enqueued. Because painting
+        is a synchronous operation, we use a synchronous API (the generator) to create an image
+        for that operation. However, this can create deadlocks if (for example) the resource needs
+        to load data on the main thread in order to complete the painting operation.
+
+        Instead, allow the main runloop to spin while waiting (up to 1_s) for the video output
+        to receive a decoded frame.
+
+        Drive-by fixes:
+        - Don't create an AVPlayerLayer at AVPlayer-creation; this causes the AVPlayerItemVideoOutput
+          to never receive a decoded frambe (as the layer is not in a CALayer-heirarchy).
+        - preferredRenderingMode() shouldn't be "none" when the page isn't visible. We already
+          just mark the layer as "hidden" in that case.
+        - Don't tear down the AVPlayerItemVideoOutput when creating an AVPlayerLayer; it'll just
+          get re-created anyway.
+
+        * platform/graphics/avfoundation/MediaPlayerPrivateAVFoundation.cpp:
+        (WebCore::MediaPlayerPrivateAVFoundation::preferredRenderingMode const):
+        (WebCore::MediaPlayerPrivateAVFoundation::setUpVideoRendering):
+        * platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.h:
+        * platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:
+        (WebCore::MediaPlayerPrivateAVFoundationObjC::createAVPlayer):
+        (WebCore::MediaPlayerPrivateAVFoundationObjC::paintCurrentFrameInContext):
+        (WebCore::MediaPlayerPrivateAVFoundationObjC::createVideoOutput):
+        (WebCore::MediaPlayerPrivateAVFoundationObjC::paintWithVideoOutput):
+        (WebCore::MediaPlayerPrivateAVFoundationObjC::waitForVideoOutputMediaDataWillChange):
+        (WebCore::MediaPlayerPrivateAVFoundationObjC::outputMediaDataWillChange):
+        (-[WebCoreAVFPullDelegate outputMediaDataWillChange:]):
+        (-[WebCoreAVFPullDelegate setParent:]):
+
 2021-08-06  Antti Koivisto  <an...@apple.com>
 
         REGRESSION (r274038): Keyframe animation with top/left with percentages fails to animate

Modified: trunk/Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundation.cpp (280722 => 280723)


--- trunk/Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundation.cpp	2021-08-06 15:04:13 UTC (rev 280722)
+++ trunk/Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundation.cpp	2021-08-06 15:09:20 UTC (rev 280723)
@@ -105,7 +105,7 @@
 
 MediaPlayerPrivateAVFoundation::MediaRenderingMode MediaPlayerPrivateAVFoundation::preferredRenderingMode() const
 {
-    if (!m_visible || assetStatus() == MediaPlayerAVAssetStatusUnknown)
+    if (assetStatus() == MediaPlayerAVAssetStatusUnknown)
         return MediaRenderingNone;
 
     if (supportsAcceleratedRendering() && m_player->renderingCanBeAccelerated())
@@ -122,18 +122,16 @@
     MediaRenderingMode currentMode = currentRenderingMode();
     MediaRenderingMode preferredMode = preferredRenderingMode();
 
-    if (preferredMode == MediaRenderingNone)
-        preferredMode = MediaRenderingToContext;
-
     if (currentMode == preferredMode && currentMode != MediaRenderingNone)
         return;
 
-    if (currentMode != MediaRenderingNone)
+    switch (preferredMode) {
+    case MediaRenderingNone:
         tearDownVideoRendering();
+        break;
 
-    switch (preferredMode) {
-    case MediaRenderingNone:
     case MediaRenderingToContext:
+        destroyVideoLayer();
         createContextVideoRenderer();
         break;

Modified: trunk/Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.h (280722 => 280723)


--- trunk/Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.h	2021-08-06 15:04:13 UTC (rev 280722)
+++ trunk/Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.h	2021-08-06 15:09:20 UTC (rev 280723)
@@ -117,6 +117,7 @@
     void outputObscuredDueToInsufficientExternalProtectionChanged(bool);
 
     MediaTime currentMediaTime() const final;
+    void outputMediaDataWillChange();
 
 private:
 #if ENABLE(ENCRYPTED_MEDIA)
@@ -435,6 +436,7 @@
     mutable bool m_allowsWirelessVideoPlayback { true };
     bool m_shouldPlayToPlaybackTarget { false };
 #endif
+    bool m_runningModalPaint { false };
 };
 
 }

Modified: trunk/Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm (280722 => 280723)


--- trunk/Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm	2021-08-06 15:04:13 UTC (rev 280722)
+++ trunk/Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm	2021-08-06 15:09:20 UTC (rev 280723)
@@ -196,6 +196,7 @@
 @interface WebCoreAVFPullDelegate : NSObject<AVPlayerItemOutputPullDelegate> {
     BinarySemaphore m_semaphore;
 }
+- (id)initWithPlayer:(WeakPtr<MediaPlayerPrivateAVFoundationObjC>&&)player;
 - (void)outputMediaDataWillChange:(AVPlayerItemOutput *)sender;
 - (void)outputSequenceWasFlushed:(AVPlayerItemOutput *)output;
 
@@ -1070,9 +1071,6 @@
 #endif
     }
 
-    if (player()->isVideoPlayer())
-        createAVPlayerLayer();
-
     if (m_avPlayerItem)
         setAVPlayerItem(m_avPlayerItem.get());
 
@@ -1802,19 +1800,7 @@
     setDelayCallbacks(true);
     BEGIN_BLOCK_OBJC_EXCEPTIONS
 
-    // Callers of this will often call copyVideoTextureToPlatformTexture first,
-    // which calls updateLastPixelBuffer, which clears m_lastImage whenever the
-    // video delivers a new frame. This breaks videoOutputHasAvailableFrame's
-    // short-circuiting when m_lastImage is non-null, but the video often
-    // doesn't have a new frame to deliver since the last time
-    // hasNewPixelBufferForItemTime was called against m_videoOutput. To avoid
-    // changing the semantics of videoOutputHasAvailableFrame in ways that might
-    // break other callers, look for production of a recent pixel buffer from
-    // the video output, too.
-    if (videoOutputHasAvailableFrame() || (m_videoOutput && m_lastPixelBuffer))
-        paintWithVideoOutput(context, rect);
-    else
-        paintWithImageGenerator(context, rect);
+    paintWithVideoOutput(context, rect);
 
     END_BLOCK_OBJC_EXCEPTIONS
     setDelayCallbacks(false);
@@ -2439,7 +2425,7 @@
         return;
     }
 
-    m_videoOutputDelegate = adoptNS([[WebCoreAVFPullDelegate alloc] init]);
+    m_videoOutputDelegate = adoptNS([[WebCoreAVFPullDelegate alloc] initWithPlayer:makeWeakPtr(*this)]);
     [m_videoOutput setDelegate:m_videoOutputDelegate.get() queue:globalPullDelegateQueue()];
 
     [m_avPlayerItem.get() addOutput:m_videoOutput.get()];
@@ -2523,11 +2509,7 @@
 
 void MediaPlayerPrivateAVFoundationObjC::paintWithVideoOutput(GraphicsContext& context, const FloatRect& outputRect)
 {
-    // It's crucial to not wait synchronously for the next image. Videos that
-    // come down this path are performing slow-case software uploads, and such
-    // videos may not return metadata in a timely fashion. Use the most recently
-    // available pixel buffer, if any.
-    updateLastImage();
+    updateLastImage(UpdateType::UpdateSynchronously);
     if (!m_lastImage)
         return;
 
@@ -2567,11 +2549,30 @@
     [m_videoOutput requestNotificationOfMediaDataChangeWithAdvanceInterval:0];
 
     // Wait for 1 second.
-    bool satisfied = [m_videoOutputDelegate semaphore].waitFor(1_s);
+    MonotonicTime start = MonotonicTime::now();
+
+    RunLoop::Timer<MediaPlayerPrivateAVFoundationObjC> timeoutTimer { RunLoop::main(), [] {
+        RunLoop::main().stop();
+    } };
+    timeoutTimer.startOneShot(1_s);
+
+    m_runningModalPaint = true;
+    RunLoop::run();
+    m_runningModalPaint = false;
+
+    bool satisfied = timeoutTimer.isActive();
     if (!satisfied)
         ERROR_LOG(LOGIDENTIFIER, "timed out");
+    else
+        INFO_LOG(LOGIDENTIFIER, "waiting for videoOutput took ", (MonotonicTime::now() - start).seconds());
 }
 
+void MediaPlayerPrivateAVFoundationObjC::outputMediaDataWillChange()
+{
+    if (m_runningModalPaint)
+        RunLoop::main().stop();
+}
+
 #if ENABLE(LEGACY_ENCRYPTED_MEDIA)
 
 RetainPtr<AVAssetResourceLoadingRequest> MediaPlayerPrivateAVFoundationObjC::takeRequestForKeyURI(const String& keyURI)
@@ -3899,14 +3900,29 @@
 
 @end
 
-@implementation WebCoreAVFPullDelegate
+@implementation WebCoreAVFPullDelegate {
+    WeakPtr<WebCore::MediaPlayerPrivateAVFoundationObjC> _player;
+}
 
 @synthesize semaphore = m_semaphore;
 
+- (id)initWithPlayer:(WeakPtr<MediaPlayerPrivateAVFoundationObjC>&&)player
+{
+    self = [super init];
+    if (!self)
+        return nil;
+    _player = WTFMove(player);
+    return self;
+}
+
 - (void)outputMediaDataWillChange:(AVPlayerItemVideoOutput *)output
 {
     UNUSED_PARAM(output);
     m_semaphore.signal();
+    RunLoop::main().dispatch([player = _player] {
+        if (player)
+            player->outputMediaDataWillChange();
+    });
 }
 
 - (void)outputSequenceWasFlushed:(AVPlayerItemVideoOutput *)output

Modified: trunk/Source/WebKit/ChangeLog (280722 => 280723)


--- trunk/Source/WebKit/ChangeLog	2021-08-06 15:04:13 UTC (rev 280722)
+++ trunk/Source/WebKit/ChangeLog	2021-08-06 15:09:20 UTC (rev 280723)
@@ -1,3 +1,19 @@
+2021-08-06  Jer Noble  <jer.no...@apple.com>
+
+        [Cocoa] Remove support for AVAssetImageGenerator
+        https://bugs.webkit.org/show_bug.cgi?id=228560
+        <rdar://problem/81336280>
+
+        Reviewed by Eric Carlson.
+
+        Drive-by fix: we're passing the wrong value into acceleratedRenderingStateChanged(), and
+        we're not setting the correct initial value on MediaPlayerPrivateRemote creation.
+
+        * GPUProcess/media/RemoteMediaPlayerProxy.h:
+        * WebProcess/GPU/media/MediaPlayerPrivateRemote.cpp:
+        (WebKit::MediaPlayerPrivateRemote::MediaPlayerPrivateRemote):
+        (WebKit::MediaPlayerPrivateRemote::acceleratedRenderingStateChanged):
+
 2021-08-06  Eric Carlson  <eric.carl...@apple.com>
 
         REGRESSION: ASSERTION FAILED: !DeprecatedGlobalSettings::shouldManageAudioSessionCategory() || AudioSession::sharedSession().category() == AudioSession::CategoryType::PlayAndRecord

Modified: trunk/Source/WebKit/GPUProcess/media/RemoteMediaPlayerProxy.h (280722 => 280723)


--- trunk/Source/WebKit/GPUProcess/media/RemoteMediaPlayerProxy.h	2021-08-06 15:04:13 UTC (rev 280722)
+++ trunk/Source/WebKit/GPUProcess/media/RemoteMediaPlayerProxy.h	2021-08-06 15:09:20 UTC (rev 280723)
@@ -353,7 +353,7 @@
     float m_videoContentScale { 1.0 };
 
     bool m_bufferedChanged { true };
-    bool m_renderingCanBeAccelerated { true };
+    bool m_renderingCanBeAccelerated { false };
 
 #if ENABLE(LEGACY_ENCRYPTED_MEDIA) && ENABLE(ENCRYPTED_MEDIA)
     bool m_shouldContinueAfterKeyNeeded { false };

Modified: trunk/Source/WebKit/WebProcess/GPU/media/MediaPlayerPrivateRemote.cpp (280722 => 280723)


--- trunk/Source/WebKit/WebProcess/GPU/media/MediaPlayerPrivateRemote.cpp	2021-08-06 15:04:13 UTC (rev 280722)
+++ trunk/Source/WebKit/WebProcess/GPU/media/MediaPlayerPrivateRemote.cpp	2021-08-06 15:09:20 UTC (rev 280723)
@@ -114,6 +114,8 @@
     , m_documentSecurityOrigin(player->documentSecurityOrigin())
 {
     INFO_LOG(LOGIDENTIFIER);
+
+    acceleratedRenderingStateChanged();
 }
 #endif
 
@@ -455,7 +457,7 @@
 void MediaPlayerPrivateRemote::acceleratedRenderingStateChanged()
 {
     if (auto player = makeRefPtr(m_player.get()))
-        connection().send(Messages::RemoteMediaPlayerProxy::AcceleratedRenderingStateChanged(player->supportsAcceleratedRendering()), m_id);
+        connection().send(Messages::RemoteMediaPlayerProxy::AcceleratedRenderingStateChanged(player->renderingCanBeAccelerated()), m_id);
 }
 
 #if ENABLE(WIRELESS_PLAYBACK_TARGET)

_______________________________________________
webkit-changes mailing list
webkit-changes@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-changes

Reply via email to