Bug 231945 - MediaPlayerAVFoundation should support rvfc
Summary: MediaPlayerAVFoundation should support rvfc
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: youenn fablet
URL:
Keywords: InRadar
Depends on: 231803
Blocks: 211945 233186
  Show dependency treegraph
 
Reported: 2021-10-19 04:26 PDT by youenn fablet
Modified: 2021-11-21 02:19 PST (History)
16 users (show)

See Also:


Attachments
Patch (16.15 KB, patch)
2021-10-19 04:28 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (33.78 KB, patch)
2021-10-21 05:10 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (33.76 KB, patch)
2021-10-21 09:00 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (46.38 KB, patch)
2021-11-15 04:12 PST, youenn fablet
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (48.13 KB, patch)
2021-11-15 04:29 PST, youenn fablet
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (48.17 KB, patch)
2021-11-15 04:56 PST, youenn fablet
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (48.25 KB, patch)
2021-11-15 05:29 PST, youenn fablet
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (48.78 KB, patch)
2021-11-15 06:10 PST, youenn fablet
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (43.18 KB, patch)
2021-11-15 06:38 PST, youenn fablet
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (43.21 KB, patch)
2021-11-15 08:27 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (52.34 KB, patch)
2021-11-16 00:16 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (52.73 KB, patch)
2021-11-17 00:09 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (52.63 KB, patch)
2021-11-17 00:56 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Patch for landing (52.69 KB, patch)
2021-11-17 23:36 PST, youenn fablet
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch for landing (52.58 KB, patch)
2021-11-18 01:47 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Fixing generator tests (2.75 KB, patch)
2021-11-18 08:51 PST, youenn fablet
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description youenn fablet 2021-10-19 04:26:20 PDT
MediaPlayerAVFoundation should support rvfc
Comment 1 youenn fablet 2021-10-19 04:28:11 PDT
Created attachment 441711 [details]
Patch
Comment 2 youenn fablet 2021-10-21 05:10:36 PDT
Created attachment 442011 [details]
Patch
Comment 3 EWS Watchlist 2021-10-21 05:11:32 PDT
This patch modifies the imported WPT tests. Please ensure that any changes on the tests (not coming from a WPT import) are exported to WPT. Please see https://trac.webkit.org/wiki/WPTExportProcess
Comment 4 youenn fablet 2021-10-21 05:17:03 PDT
Submitted web-platform-tests pull request: https://github.com/web-platform-tests/wpt/pull/31343
Comment 5 Eric Carlson 2021-10-21 08:11:12 PDT
Comment on attachment 442011 [details]
Patch

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

> Source/WebCore/html/HTMLMediaElement.cpp:6911
>      m_player = MediaPlayer::create(*this);
> +    createdNewPlayer();
> +
>      m_player->setBufferingPolicy(m_bufferingPolicy);
>      m_player->setPreferredDynamicRangeMode(m_overrideDynamicRangeMode.value_or(preferredDynamicRangeMode(document().view())));
>      m_player->setMuted(effectiveMuted());

Maybe put the player creation and initialization into the base createdNewPlayer() and HTMLVideoElement can override and call it?

> Source/WebCore/platform/graphics/MediaPlayer.h:289
> +    virtual void onNewVideoFrameMetadata(VideoFrameMetadata&&) { ASSERT_NOT_REACHED(); }

This will assert when media in the GPUP is not enabled because HTMLMediaElement doesn't implement it.

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:1442
> +    m_videoFrameMetadataGatheringObserver = [m_avPlayer addPeriodicTimeObserverForInterval:PAL::CMTimeMake(1, 60) queue:dispatch_get_main_queue() usingBlock:[weakThis = WeakPtr { *this }](CMTime) {

Is 60fps always necessary?

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:1452
> +    CMTime currentTime = [m_avPlayerItem currentTime];

This is unnecessary (and potentially expensive), `addPeriodicTimeObserverForInterval` passes the current time to the callback.
Comment 6 youenn fablet 2021-10-21 09:00:42 PDT
Created attachment 442034 [details]
Patch
Comment 7 Jer Noble 2021-10-21 14:18:29 PDT
Comment on attachment 442034 [details]
Patch

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

> Source/WebCore/html/HTMLAudioElement.h:36
> +struct VideoFrameMetadata;

Why is this needed?

> Source/WebCore/html/HTMLVideoElement.cpp:651
> +void HTMLVideoElement::createdNewPlayer()
> +{
> +    if (!m_videoFrameRequests.isEmpty())
> +        player()->startVideoFrameMetadataGathering();

This doesn't seem correct. Just because we have a MediaPlayer doesn't mean we have a MediaPlayerPrivate. In fact, this `createdNewPlayer()` method seems to be totally unnecessary, and this could go into `mediaPlayerEngineUpdated()`.

> Source/WebCore/platform/graphics/MediaPlayer.h:289
> +    virtual void onNewVideoFrameMetadata(VideoFrameMetadata&&) { ASSERT_NOT_REACHED(); }

MediaPlayerClient methods should be prepended with `mediaPlayer...`.

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:1459
> +    if (![m_videoOutput hasNewPixelBufferForItemTime:currentTime])
> +        return;
> +
> +    VideoFrameMetadata metadata;
> +    metadata.width = m_cachedPresentationSize.width();
> +    metadata.height = m_cachedPresentationSize.height();
> +    metadata.presentedFrames = ++m_sampleCount;

This will give incorrect answers.  If this is being called 60 times a second, it'll imply a 60fps video. -hasNewPixelBufferForItemTime: will continuously return YES for the same or similar media time until that pixelBuffer is dequeued.  So as written, this will just increment m_sampleCount every time `checkNewVideoFrameMetadata()` is called.  And if this is called less often than the native frame rate of the video, it will also miss frames.  If a client is painting at the same time as calling rvaf, this call will also miss frames.
Comment 8 Radar WebKit Bug Importer 2021-10-26 04:27:15 PDT
<rdar://problem/84657372>
Comment 9 youenn fablet 2021-11-15 04:12:12 PST
Created attachment 444237 [details]
Patch
Comment 10 youenn fablet 2021-11-15 04:29:06 PST
Created attachment 444238 [details]
Patch
Comment 11 youenn fablet 2021-11-15 04:56:08 PST
Created attachment 444239 [details]
Patch
Comment 12 youenn fablet 2021-11-15 05:29:37 PST
Created attachment 444240 [details]
Patch
Comment 13 youenn fablet 2021-11-15 06:10:51 PST
Created attachment 444243 [details]
Patch
Comment 14 youenn fablet 2021-11-15 06:38:36 PST
Created attachment 444251 [details]
Patch
Comment 15 youenn fablet 2021-11-15 08:27:49 PST
Created attachment 444258 [details]
Patch
Comment 16 youenn fablet 2021-11-16 00:16:53 PST
Created attachment 444350 [details]
Patch
Comment 17 youenn fablet 2021-11-16 06:31:59 PST
Comment on attachment 444350 [details]
Patch

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

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:1474
> +        [m_avPlayer removeTimeObserver:m_videoFrameMetadataGatheringObserver.get()];

I should probably call stopVideoFrameMetadataGathering() in destructor or when player is destroyed in cancelLoad.
Comment 18 Philippe Normand 2021-11-16 09:49:42 PST
Comment on attachment 444350 [details]
Patch

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

> Source/WebCore/platform/graphics/MediaPlayerPrivate.h:327
> +    virtual void startVideoFrameMetadataGathering() { }
> +    virtual void stopVideoFrameMetadataGathering() { }

I don't think I'll make use of these in the GStreamer player. I will likely use pad probes for the measurement, those are dynamically added when the pipeline is being built, but IIUC startVideoFrameMetadataGathering() can in theory be called after the pipeline has been built, so not very convenient for us.
Comment 19 youenn fablet 2021-11-16 10:01:01 PST
> I don't think I'll make use of these in the GStreamer player. I will likely
> use pad probes for the measurement, those are dynamically added when the
> pipeline is being built, but IIUC startVideoFrameMetadataGathering() can in
> theory be called after the pipeline has been built, so not very convenient
> for us.

requestVideoFrameCallback can be called after the pipeline is set so you might need to add probes in advance.

Also, please look at the changes in DumpRenderTree/TestController, which might affect GTK/WPE results for those tests that are feature detecting RVFC.
Comment 20 Philippe Normand 2021-11-16 10:30:48 PST
(In reply to youenn fablet from comment #19)
> > I don't think I'll make use of these in the GStreamer player. I will likely
> > use pad probes for the measurement, those are dynamically added when the
> > pipeline is being built, but IIUC startVideoFrameMetadataGathering() can in
> > theory be called after the pipeline has been built, so not very convenient
> > for us.
> 
> requestVideoFrameCallback can be called after the pipeline is set so you
> might need to add probes in advance.
> 

Yeah that's what I implied, I need the probes in any case.

> Also, please look at the changes in DumpRenderTree/TestController, which
> might affect GTK/WPE results for those tests that are feature detecting RVFC.

Right this will likely need some gardening. Maybe you can skip the rvfc tests in glib ports for now? I started a branch for this but I'll likely rework it a bit after this patch lands. Thanks!
Comment 21 youenn fablet 2021-11-17 00:09:23 PST
Created attachment 444489 [details]
Patch
Comment 22 youenn fablet 2021-11-17 00:56:21 PST
Created attachment 444492 [details]
Patch
Comment 23 Eric Carlson 2021-11-17 10:20:14 PST
Comment on attachment 444492 [details]
Patch

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

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:1468
> +    metadata.presentationTime = MonotonicTime::now().secondsSinceEpoch().seconds();
> +    metadata.expectedDisplayTime = metadata.presentationTime;

Maybe add a FIXME here as these probably aren't always equivalent
Comment 24 youenn fablet 2021-11-17 23:36:27 PST
Created attachment 444642 [details]
Patch for landing
Comment 25 youenn fablet 2021-11-18 01:47:38 PST
Created attachment 444649 [details]
Patch for landing
Comment 26 EWS 2021-11-18 02:06:08 PST
Found 1 new test failure: imported/w3c/web-platform-tests/video-rvfc/request-video-frame-callback-repeating.html
Comment 27 EWS 2021-11-18 02:34:29 PST
Committed r285993 (244390@main): <https://commits.webkit.org/244390@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 444649 [details].
Comment 28 Chris Dumez 2021-11-18 08:43:22 PST
I think you may need to `python Source/WebKit/Scripts/webkit/messages_unittest.py --reset-results`:
```
diff --git a/Source/WebKit/Scripts/webkit/tests/MessageArgumentDescriptions.cpp b/Source/WebKit/Scripts/webkit/tests/MessageArgumentDescriptions.cpp
index 630a865b13a6..52e3c7638390 100644
--- a/Source/WebKit/Scripts/webkit/tests/MessageArgumentDescriptions.cpp
+++ b/Source/WebKit/Scripts/webkit/tests/MessageArgumentDescriptions.cpp
@@ -127,6 +127,9 @@
 #endif
 #include "TestWithCVPixelBufferMessages.h"
 #if USE(AVFOUNDATION)
+#include <WebCore/CVUtilities.h>
+#endif
+#if USE(AVFOUNDATION)
 #include <wtf/RetainPtr.h>
 #endif
 
diff --git a/Source/WebKit/Scripts/webkit/tests/TestWithCVPixelBufferMessageReceiver.cpp b/Source/WebKit/Scripts/webkit/tests/TestWithCVPixelBufferMessageReceiver.cpp
index ee7defb81297..56496ddba5ea 100644
--- a/Source/WebKit/Scripts/webkit/tests/TestWithCVPixelBufferMessageReceiver.cpp
+++ b/Source/WebKit/Scripts/webkit/tests/TestWithCVPixelBufferMessageReceiver.cpp
@@ -32,6 +32,9 @@
 #include "HandleMessage.h"
 #include "TestWithCVPixelBufferMessages.h"
 #if USE(AVFOUNDATION)
+#include <WebCore/CVUtilities.h>
+#endif
+#if USE(AVFOUNDATION)
 #include <wtf/RetainPtr.h>
 #endif
 
diff --git a/Source/WebKit/Scripts/webkit/tests/TestWithCVPixelBufferMessages.h b/Source/WebKit/Scripts/webkit/tests/TestWithCVPixelBufferMessages.h
index 1675bea59439..326500eb0a0e 100644
--- a/Source/WebKit/Scripts/webkit/tests/TestWithCVPixelBufferMessages.h
+++ b/Source/WebKit/Scripts/webkit/tests/TestWithCVPixelBufferMessages.h
@@ -28,6 +28,9 @@
 #include "Connection.h"
 #include "MessageNames.h"
 #include "TestWithCVPixelBufferMessagesReplies.h"
+#if PLATFORM(COCOA)
+#include <WebCore/CVUtilities.h>
+#endif
 #include <wtf/Forward.h>
 #include <wtf/RetainPtr.h>
 #include <wtf/ThreadSafeRefCounted.h>
```
Comment 29 youenn fablet 2021-11-18 08:51:03 PST
Reopening to attach new patch.
Comment 30 youenn fablet 2021-11-18 08:51:07 PST
Created attachment 444684 [details]
Fixing generator tests
Comment 31 EWS 2021-11-18 08:54:00 PST
ChangeLog entry in Source/WebKit/ChangeLog contains OOPS!.
Comment 32 EWS 2021-11-18 09:21:31 PST
Committed r286005 (244402@main): <https://commits.webkit.org/244402@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 444684 [details].
Comment 33 Philippe Normand 2021-11-21 02:19:24 PST
Comment on attachment 444649 [details]
Patch for landing

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

> Source/WebCore/platform/graphics/MediaPlayer.h:291
> +#if PLATFORM(COCOA)
> +    virtual void mediaPlayerOnNewVideoFrameMetadata(VideoFrameMetadata&&, RetainPtr<CVPixelBufferRef>&&) { }
> +#endif

Would it be possible to use a MediaSample here?