Bug 231945 – MediaPlayerAVFoundation should support rvfc

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

[youenn fablet]

MediaPlayerAVFoundation should support rvfc

[youenn fablet]

Created attachment 441711 [details]
Patch

[youenn fablet]

Created attachment 442011 [details]
Patch

[EWS Watchlist]

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

[youenn fablet]

Submitted web-platform-tests pull request: https://github.com/web-platform-tests/wpt/pull/31343

[Eric Carlson]

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.

[youenn fablet]

Created attachment 442034 [details]
Patch

[Jer Noble]

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.

[Radar WebKit Bug Importer]

<rdar://problem/84657372>

[youenn fablet]

Created attachment 444237 [details]
Patch

[youenn fablet]

Created attachment 444238 [details]
Patch

[youenn fablet]

Created attachment 444239 [details]
Patch

[youenn fablet]

Created attachment 444240 [details]
Patch

[youenn fablet]

Created attachment 444243 [details]
Patch

[youenn fablet]

Created attachment 444251 [details]
Patch

[youenn fablet]

Created attachment 444258 [details]
Patch

[youenn fablet]

Created attachment 444350 [details]
Patch

[youenn fablet]

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.

[Philippe Normand]

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.

[youenn fablet]

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

[Philippe Normand]

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

[youenn fablet]

Created attachment 444489 [details]
Patch

[youenn fablet]

Created attachment 444492 [details]
Patch

[Eric Carlson]

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

[youenn fablet]

Created attachment 444642 [details]
Patch for landing

[youenn fablet]

Created attachment 444649 [details]
Patch for landing

[EWS]

Found 1 new test failure: imported/w3c/web-platform-tests/video-rvfc/request-video-frame-callback-repeating.html

[EWS]

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

[Chris Dumez]

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>
```

[youenn fablet]

Reopening to attach new patch.

[youenn fablet]

Created attachment 444684 [details]
Fixing generator tests

[EWS]

ChangeLog entry in Source/WebKit/ChangeLog contains OOPS!.

[EWS]

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

[Philippe Normand]

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?