Bug 125380 – [MSE] Add support for VideoPlaybackMetrics.

• Component: New Bugs
• Status: RESOLVED FIXED

[Jer Noble]

[MSE] Add support for VideoPlaybackMetrics.

[Jer Noble]

Created attachment 218653 [details]
Patch

[Sam Weinig]

Comment on attachment 218653 [details]
Patch

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

> Source/WebCore/Modules/mediasource/VideoPlaybackQuality.idl:28
> +    NoInterfaceObject,

Why no NoInterfaceObject?

> Source/WebCore/Modules/mediasource/VideoPlaybackQuality.idl:31
> +    readonly attribute double creationTime;

The spec says this should be a DOMHighResTimeStamp? Does the generator not support that?

> Source/WebCore/html/HTMLMediaElement.cpp:5250
> +#if ENABLE(WEB_TIMING)
> +    DOMWindow* domWindow = document().domWindow();
> +    Performance* performance = domWindow ? domWindow->performance() : nullptr;
> +    double now = performance ? performance->now() : 0;
> +#else
> +    double now = monotonicallyIncreasingTime();
> +#endif

I believe performance()->now() is implemented as:

1000.0 * document()->loader()->timing()->monotonicTimeToZeroBasedDocumentTime(monotonicallyIncreasingTime())

So you should probably match that in the non-WEB_TIMING enabled case.

[Jer Noble]

Created attachment 218678 [details]
Patch

[EFL EWS Bot]

Comment on attachment 218678 [details]
Patch

Attachment 218678 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/46948080

[kov's GTK+ EWS bot]

Comment on attachment 218678 [details]
Patch

Attachment 218678 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/47008090

[EFL EWS Bot]

Comment on attachment 218678 [details]
Patch

Attachment 218678 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/46948081

[Build Bot]

Comment on attachment 218678 [details]
Patch

Attachment 218678 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/46938079

[Jer Noble]

Created attachment 218699 [details]
Patch

Protect getVideoPlaybackQuality with ENABLE guards.

[EFL EWS Bot]

Comment on attachment 218699 [details]
Patch

Attachment 218699 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/47138059

[EFL EWS Bot]

Comment on attachment 218699 [details]
Patch

Attachment 218699 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/47188069

[kov's GTK+ EWS bot]

Comment on attachment 218699 [details]
Patch

Attachment 218699 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/46908082

[Build Bot]

Comment on attachment 218699 [details]
Patch

Attachment 218699 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/46638075

[Jer Noble]

Created attachment 218706 [details]
Patch

Also protect the m_droppedVideoFrames initializer.

[kov's GTK+ EWS bot]

Comment on attachment 218706 [details]
Patch

Attachment 218706 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/47128078

[Jer Noble]

Created attachment 218765 [details]
Patch

Add new files to cmakelists.txt

[kov's GTK+ EWS bot]

Comment on attachment 218765 [details]
Patch

Attachment 218765 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/45898143

[Jer Noble]

Created attachment 218768 [details]
Patch

Add VideoPlaybackQuality.idl to GNUMakefile.list.am.

[kov's GTK+ EWS bot]

Comment on attachment 218768 [details]
Patch

Attachment 218768 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/46278167

[Eric Carlson]

Comment on attachment 218768 [details]
Patch

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

> Source/WebCore/ChangeLog:52
> +        (WebCore::SourceBuffer::TrackBuffer::TrackBuffer): needsRandomAccessFlag defaulst to true.

Nit: "defaulst".

> LayoutTests/media/media-source/media-source-video-playback-quality.html:17
> +    function runTest() {

Nit: a function's opening brace should be on a new line.

[Eric Carlson]

Comment on attachment 218768 [details]
Patch

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

> Source/WebCore/html/HTMLMediaElement.cpp:5252
> +#if ENABLE(WEB_TIMING)
> +    DOMWindow* domWindow = document().domWindow();
> +    Performance* performance = domWindow ? domWindow->performance() : nullptr;
> +    double now = performance ? performance->now() : 0;
> +#else
> +    double now = monotonicallyIncreasingTime();
> +#endif

In an earlier review, Sam said:

I believe performance()->now() is implemented as:

1000.0 * document()->loader()->timing()->monotonicTimeToZeroBasedDocumentTime(monotonicallyIncreasingTime())

So you should probably match that in the non-WEB_TIMING enabled case.

[Jer Noble]

Created attachment 218797 [details]
Patch

Addressed review comments, and yet another attempt to fix the GTK bot.

[WebKit Commit Bot]

Comment on attachment 218797 [details]
Patch

Clearing flags on attachment: 218797

Committed r160336: <http://trac.webkit.org/changeset/160336>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Jer Noble]

(In reply to comment #2)
> (From update of attachment 218653 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=218653&action=review

Sam, sorry this got lost in the blizzard of build failure messages.

> > Source/WebCore/Modules/mediasource/VideoPlaybackQuality.idl:28
> > +    NoInterfaceObject,
> 
> Why no NoInterfaceObject?

I had thought this meant no window-level constructors, but that's not correct.  It means no object prototype, and therefore the object can't be extended by page authors.

I'll put up a follow-up patch to remove this.

> > Source/WebCore/Modules/mediasource/VideoPlaybackQuality.idl:31
> > +    readonly attribute double creationTime;
> 
> The spec says this should be a DOMHighResTimeStamp? Does the generator not support that?

The spec says a DOMHighResTimeStamp is a double.  Since that's what the Performance.idl file lists as the return value as "now", that's what I used here.  And now that I look, there doesn't seem to be support in the generator for that type.

> > Source/WebCore/html/HTMLMediaElement.cpp:5250
> > +#if ENABLE(WEB_TIMING)
> > +    DOMWindow* domWindow = document().domWindow();
> > +    Performance* performance = domWindow ? domWindow->performance() : nullptr;
> > +    double now = performance ? performance->now() : 0;
> > +#else
> > +    double now = monotonicallyIncreasingTime();
> > +#endif
> 
> I believe performance()->now() is implemented as:
> 
> 1000.0 * document()->loader()->timing()->monotonicTimeToZeroBasedDocumentTime(monotonicallyIncreasingTime())
> 
> So you should probably match that in the non-WEB_TIMING enabled case.

Okay, I did this in the committed patch.  I'll post a follow up patch to address the NoInterfaceObject change above.

[Jer Noble]

Re-opening.

[Jer Noble]

Actually, I'll address the follow up patch in a new bug: http://webkit.org/b/125474.