RESOLVED FIXED 125380
[MSE] Add support for VideoPlaybackMetrics.
https://bugs.webkit.org/show_bug.cgi?id=125380
Summary [MSE] Add support for VideoPlaybackMetrics.
Jer Noble
Reported 2013-12-06 22:10:11 PST
[MSE] Add support for VideoPlaybackMetrics.
Attachments
Patch (45.97 KB, patch)
2013-12-06 23:01 PST, Jer Noble
no flags
Patch (47.92 KB, patch)
2013-12-07 22:58 PST, Jer Noble
no flags
Patch (47.95 KB, patch)
2013-12-08 11:17 PST, Jer Noble
no flags
Patch (47.99 KB, patch)
2013-12-08 12:49 PST, Jer Noble
no flags
Patch (48.85 KB, patch)
2013-12-09 08:18 PST, Jer Noble
no flags
Patch (49.28 KB, patch)
2013-12-09 08:57 PST, Jer Noble
no flags
Patch (50.66 KB, patch)
2013-12-09 13:54 PST, Jer Noble
no flags
Jer Noble
Comment 1 2013-12-06 23:01:44 PST
Sam Weinig
Comment 2 2013-12-07 17:48:47 PST
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
Comment 3 2013-12-07 22:58:58 PST
EFL EWS Bot
Comment 4 2013-12-07 23:04:03 PST
kov's GTK+ EWS bot
Comment 5 2013-12-07 23:05:42 PST
EFL EWS Bot
Comment 6 2013-12-07 23:07:09 PST
Build Bot
Comment 7 2013-12-07 23:49:12 PST
Jer Noble
Comment 8 2013-12-08 11:17:25 PST
Created attachment 218699 [details] Patch Protect getVideoPlaybackQuality with ENABLE guards.
EFL EWS Bot
Comment 9 2013-12-08 11:23:06 PST
EFL EWS Bot
Comment 10 2013-12-08 11:30:27 PST
kov's GTK+ EWS bot
Comment 11 2013-12-08 12:09:32 PST
Build Bot
Comment 12 2013-12-08 12:10:50 PST
Jer Noble
Comment 13 2013-12-08 12:49:47 PST
Created attachment 218706 [details] Patch Also protect the m_droppedVideoFrames initializer.
kov's GTK+ EWS bot
Comment 14 2013-12-08 12:58:17 PST
Jer Noble
Comment 15 2013-12-09 08:18:41 PST
Created attachment 218765 [details] Patch Add new files to cmakelists.txt
kov's GTK+ EWS bot
Comment 16 2013-12-09 08:47:30 PST
Jer Noble
Comment 17 2013-12-09 08:57:53 PST
Created attachment 218768 [details] Patch Add VideoPlaybackQuality.idl to GNUMakefile.list.am.
kov's GTK+ EWS bot
Comment 18 2013-12-09 09:09:05 PST
Eric Carlson
Comment 19 2013-12-09 12:28:24 PST
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 20 2013-12-09 12:32:26 PST
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
Comment 21 2013-12-09 13:54:59 PST
Created attachment 218797 [details] Patch Addressed review comments, and yet another attempt to fix the GTK bot.
WebKit Commit Bot
Comment 22 2013-12-09 15:33:56 PST
Comment on attachment 218797 [details] Patch Clearing flags on attachment: 218797 Committed r160336: <http://trac.webkit.org/changeset/160336>
WebKit Commit Bot
Comment 23 2013-12-09 15:34:01 PST
All reviewed patches have been landed. Closing bug.
Jer Noble
Comment 24 2013-12-09 16:33:15 PST
(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
Comment 25 2013-12-09 16:34:40 PST
Re-opening.
Jer Noble
Comment 26 2013-12-09 16:38:40 PST
Actually, I'll address the follow up patch in a new bug: http://webkit.org/b/125474.
Note You need to log in before you can comment on or make changes to this bug.