To help tweak the decoder-suspend functionality, we want to measure inter-keyframe timings (average and maximum times between successive keyframes, per video), to get an idea of how long typical post-suspend recovery should take on average, and in the worst case. Telemetry will be keyed by audio presence ('AV' with audio, 'V' video only), and by resolution ranges (e.g. '720<h<=1080'). An 'All' key will also accumulate all numbers.

Upcoming patches are based on bug 1289674 (not yet landed at this time).

Review commit: https://reviewboard.mozilla.org/r/67572/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/67572/

Review commit: https://reviewboard.mozilla.org/r/67574/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/67574/

Move FrameStatistics' data into separate struct, so that it can more easily be changed and passed around, outside of the lock-controlled FrameStatistics object. Review commit: https://reviewboard.mozilla.org/r/67576/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/67576/

Decoders uses 64-bit values to count frames, so we should use the same type in FrameStatistics. Because VideoPlaybackQuality can only use 32 bits (as defined in W3C specs), we need to ensure that imported FrameStatistics numbers can fit in 32 bits, while keeping their ratios the same. Review commit: https://reviewboard.mozilla.org/r/67578/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/67578/

Decoders now use FrameStatisticsData to gather data for their frame-related notifications. This will ease introducing new members later on. Review commit: https://reviewboard.mozilla.org/r/67580/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/67580/

HTMLVideoElement can expose its thread-safe FrameStatistics object, so that HTMLMediaElement can access more adequate data for its telemetry, without having to use an intermediary (and potentially less accurate) VideoPlaybackQuality object. This will also help with accessing other/new FrameStatistics members later on. Review commit: https://reviewboard.mozilla.org/r/67582/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/67582/

FrameStatisticsData can now store inter-keyframe information, which is provided by the MediaFormatReader (based on live decoding). Review commit: https://reviewboard.mozilla.org/r/67584/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/67584/

r?francois for telemetry-peer feedback through mozreview. The goal of this telemetry is to gather statistics about inter-keyframe timings of played videos, so we have a better understanding of typical values in the field. This inter-keyframe time is important for the background-suspend functionality, as it will impact the recovery time (i.e., when moving a video back to the foreground, what is the average & max time to the next random-access point?) Review commit: https://reviewboard.mozilla.org/r/67586/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/67586/

Use the new FrameStatistics members to report telemetry about inter-keyframe timings. Review commit: https://reviewboard.mozilla.org/r/67588/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/67588/

Comment on attachment 8775397 [details] Bug 1289668 - Refactor FrameStatistics writers to use Data struct - Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67580/diff/1-2/

Comment on attachment 8775398 [details] Bug 1289668 - HTMLMediaElement uses video decoder's frame statistics - Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67582/diff/1-2/

Comment on attachment 8775399 [details] Bug 1289668 - Record inter-keyframe statistics - Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67584/diff/1-2/

Comment on attachment 8775400 [details] Bug 1289668 - Inter-keyframe telemetry histograms - Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67586/diff/1-2/

Comment on attachment 8775401 [details] Bug 1289668 - Report inter-keyframe telemetry - Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67588/diff/1-2/

Comment on attachment 8775393 [details] Bug 1289668 - VideoPlaybackQuality style - https://reviewboard.mozilla.org/r/67572/#review64894 

Comment on attachment 8775394 [details] Bug 1289668 - FrameStatistics style - https://reviewboard.mozilla.org/r/67574/#review64898 

Comment on attachment 8775400 [details] Bug 1289668 - Inter-keyframe telemetry histograms - https://reviewboard.mozilla.org/r/67586/#review64902 datareview+

Comment on attachment 8775395 [details] Bug 1289668 - Refactor FrameStatistics - https://reviewboard.mozilla.org/r/67576/#review64900 ::: dom/html/HTMLVideoElement.cpp:245 (Diff revision 1) > creationTime = perf->Now(); > } > } > > if (mDecoder) { > - FrameStatistics& stats = mDecoder->GetFrameStatistics(); > + FrameStatisticsData stats = This should be a reference, right? We don't need a copy of the struct on the stack. ::: dom/media/FrameStatistics.h:44 (Diff revision 1) > - , mDroppedFrames(0) > {} > > + // Returns a copy of all frame statistics data. > + // Can be called on any thread. > + FrameStatisticsData GetFrameStatisticsData() const Why return a copy? Won't const FrameStatisticsData& GetFrameStatisticsData() const suffice?

Comment on attachment 8775395 [details] Bug 1289668 - Refactor FrameStatistics - https://reviewboard.mozilla.org/r/67576/#review64912 Ship it!

Comment on attachment 8775398 [details] Bug 1289668 - HTMLMediaElement uses video decoder's frame statistics - https://reviewboard.mozilla.org/r/67582/#review64914 Ship It! ::: dom/html/HTMLMediaElement.cpp:3090 (Diff revision 2) > - uint32_t totalFrames = quality->TotalVideoFrames(); > - if (totalFrames) { > - uint32_t droppedFrames = quality->DroppedVideoFrames(); > - MOZ_ASSERT(droppedFrames <= totalFrames); > + if (stats) { > + FrameStatisticsData data = stats->GetFrameStatisticsData(); > + if (data.mParsedFrames) { > + MOZ_ASSERT(data.mDroppedFrames <= data.mParsedFrames); > - // Dropped frames <= total frames, so 'percentage' cannot be higher than > + // Dropped frames <= total frames, so 'percentage' cannot be higher than > - // 100 and therefore can fit in a uint32_t (that Telemetry takes). > + // 100 and therefore can fit in a uint32_t (that Telemetry takes). I know you didn't add this comment in this change, but I'm a bit confused by it's origin. Of course [0..100] fits in a uint32_t. It fits in a byte!

Comment on attachment 8775396 [details] Bug 1289668 - Use 64 bits to store FrameStatistics values - https://reviewboard.mozilla.org/r/67578/#review64922 OK, because roc said. ;-)

https://reviewboard.mozilla.org/r/67582/#review64914 > I know you didn't add this comment in this change, but I'm a bit confused by it's origin. Of course [0..100] fits in a uint32_t. It fits in a byte! I did write it in a previous patch. I just wanted to make it clear that the numbers generated [0..100] do fit in the uint32_t that the teleletry function expects, which I thought was not obvious at a glance since the statement uses 64-bit types.

Comment on attachment 8775397 [details] Bug 1289668 - Refactor FrameStatistics writers to use Data struct - https://reviewboard.mozilla.org/r/67580/#review64926 Ship It! ::: dom/media/MediaFormatReader.cpp:1494 (Diff revision 2) > } > } > decoder.mOutput.Clear(); > decoder.mSizeOfQueue -= lengthDecodedQueue; > if (aTrack == TrackInfo::kVideoTrack && mDecoder) { > - mDecoder->NotifyDecodedFrames(0, 0, lengthDecodedQueue); > + mDecoder->NotifyDecodedFrames({ 0, 0, lengthDecodedQueue }); Nice

Comment on attachment 8775399 [details] Bug 1289668 - Record inter-keyframe statistics - https://reviewboard.mozilla.org/r/67584/#review64934 Ship It!

Comment on attachment 8775400 [details] Bug 1289668 - Inter-keyframe telemetry histograms - https://reviewboard.mozilla.org/r/67586/#review64936 Ship It!

Comment on attachment 8775401 [details] Bug 1289668 - Report inter-keyframe telemetry - https://reviewboard.mozilla.org/r/67588/#review64942  ::: dom/html/HTMLMediaElement.cpp:3150 (Diff revision 2) > LOG(LogLevel::Debug, ("%p VIDEO_HIDDEN_PLAY_TIME_PERCENTAGE = %u, keys: '%s' and 'All'", > this, hiddenPercentage, key.get())); > + > + if (data.mInterKeyframeCount != 0) { > + uint32_t average_ms = > + uint32_t(std::min<uint64_t>(double(data.mInterKeyframeSum_us) So many conversions.

Pushed by gsquelart@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/32a2d4eb5941 VideoPlaybackQuality style - r=kamidphish https://hg.mozilla.org/integration/autoland/rev/442f12220074 FrameStatistics style - r=kamidphish https://hg.mozilla.org/integration/autoland/rev/86cec74f5da6 Refactor FrameStatistics - r=kamidphish https://hg.mozilla.org/integration/autoland/rev/98c9b3317cbf Use 64 bits to store FrameStatistics values - r=kamidphish https://hg.mozilla.org/integration/autoland/rev/12a97f1e4297 Refactor FrameStatistics writers to use Data struct - r=kamidphish https://hg.mozilla.org/integration/autoland/rev/fff6018d7bba HTMLMediaElement uses video decoder's frame statistics - r=kamidphish https://hg.mozilla.org/integration/autoland/rev/a3bd0fb7c269 Record inter-keyframe statistics - r=kamidphish https://hg.mozilla.org/integration/autoland/rev/e39471da882b Inter-keyframe telemetry histograms - r=francois,kamidphish https://hg.mozilla.org/integration/autoland/rev/124d1ee0513a Report inter-keyframe telemetry - r=kamidphish

https://hg.mozilla.org/mozilla-central/rev/32a2d4eb5941 https://hg.mozilla.org/mozilla-central/rev/442f12220074 https://hg.mozilla.org/mozilla-central/rev/86cec74f5da6 https://hg.mozilla.org/mozilla-central/rev/98c9b3317cbf https://hg.mozilla.org/mozilla-central/rev/12a97f1e4297 https://hg.mozilla.org/mozilla-central/rev/fff6018d7bba https://hg.mozilla.org/mozilla-central/rev/a3bd0fb7c269 https://hg.mozilla.org/mozilla-central/rev/e39471da882b https://hg.mozilla.org/mozilla-central/rev/124d1ee0513a