Closed
Bug 1249706
Opened 8 years ago
Closed 5 years ago
More detailed telemetry on the proportion of frames dropped.
Categories
(Core :: Audio/Video: Playback, defect, P3)
Core
Audio/Video: Playback
Tracking
()
RESOLVED
WONTFIX
mozilla47
Tracking | Status | |
---|---|---|
firefox47 | --- | fixed |
People
(Reporter: lchristie, Unassigned)
References
Details
Attachments
(1 file, 4 obsolete files)
9.96 KB,
patch
|
Details | Diff | Splinter Review |
Following up on bug 1238433, we should add a keyed linear histogram to report the portion of frame drops based on the Mime type, resolution and hardware acceleration.
Reporter | ||
Updated•8 years ago
|
Assignee: nobody → lchristie
Reporter | ||
Comment 1•8 years ago
|
||
Attachment #8722321 -
Flags: review?(cpearce)
Reporter | ||
Comment 2•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7d2c7b2e6473
Reporter | ||
Updated•8 years ago
|
Attachment #8722321 -
Flags: review?(cpearce)
Reporter | ||
Comment 3•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1342ba3a1dfa
Attachment #8722321 -
Attachment is obsolete: true
Attachment #8723262 -
Flags: review?(cpearce)
Comment 4•8 years ago
|
||
Comment on attachment 8723262 [details] [diff] [review] bug-1249706-fix.patch Review of attachment 8723262 [details] [diff] [review]: ----------------------------------------------------------------- jya should also look at this because he understands the MediaFormatReader better than anyone else here. ::: dom/media/MediaFormatReader.cpp @@ +1708,5 @@ > > +void > +MediaFormatReader::ReportDroppedFramesTelemetry() > +{ > + if (!mVideo.mInfo || !mVideo.mDecoder) { MOZ_ASSERT(OnTaskQueue()); at the start of the function please. @@ +1736,5 @@ > + uint32_t percentage = > + 100 * mVideo.mNumSamplesSkippedTotalSinceTelemetry / > + mVideo.mNumSamplesOutputTotalSinceTelemetry; > + LOG("Reporting telemetry DROPPED_FRAMES_IN_VIDEO_PLAYBACK"); > + Telemetry::Accumulate(Telemetry::VIDEO_DETAILED_DROPPED_FRAMES_PROPORTION, Telemetry isn't thread-safe, so you must accumulate telemetry on the main thread only. So you'll need to dispatch a task to the main thread to call Telemetry::Accumulate(). For example, like how I did in OggReader::~OggReader(): http://mxr.mozilla.org/mozilla-central/source/dom/media/ogg/OggReader.cpp#158 ::: dom/media/platforms/wmf/WMFMediaDataDecoder.h @@ +48,5 @@ > // Destroys all resources. > virtual void Shutdown() = 0; > > virtual bool IsHardwareAccelerated(nsACString& aFailureReason) const { return false; } > + virtual nsCString GetHardwareAccelerator() const { return nsCString(); } return EmptyCString(); ::: dom/media/platforms/wmf/WMFVideoMFTManager.cpp @@ +655,5 @@ > +WMFVideoMFTManager::GetHardwareAccelerator() const > +{ > + if (mDecoder && mUseHwAccel) { > + if (mDXVA2Manager->IsD3D11()) { > + return nsCString("D3D11_"); Use two spaces per indent. I don't think you need the '_' at the end of the string? If you did need that '_' at the call site, you should add it at the call site since it's the callers responsibility to format the returned value, and this functions responsibility to report its value in a way that potentially could be useful to other callers. @@ +660,5 @@ > + } else { > + return nsCString("D3D9_"); > + } > + } else { > + return nsCString(); EmptyCString() ::: dom/media/platforms/wrappers/H264Converter.cpp @@ +135,5 @@ > +H264Converter::GetHardwareAccelerator() const > +{ > + if (mDecoder) { > + return mDecoder->GetHardwareAccelerator(); > + } } else { @@ +137,5 @@ > + if (mDecoder) { > + return mDecoder->GetHardwareAccelerator(); > + } > + else { > + return nsCString(); EmptyCString() @@ +138,5 @@ > + return mDecoder->GetHardwareAccelerator(); > + } > + else { > + return nsCString(); > + } Or, you could go: return mDecoder ? mDecoder->GetHardwareAccelerator() : EmptyCString(); ::: toolkit/components/telemetry/Histograms.json @@ +10324,5 @@ > + "alert_emails": ["lchristie@mozilla.com", "cpearce@mozilla.com"], > + "expires_in_version": "55", > + "kind": "linear", > + "high": 100, > + "n_buckets": 50, "n_buckets": 101 So we get accuracy down to a percentage point, and include 0 and 100%.
Attachment #8723262 -
Flags: review?(jyavenard)
Attachment #8723262 -
Flags: review?(cpearce)
Attachment #8723262 -
Flags: review-
Comment 5•8 years ago
|
||
Comment on attachment 8723262 [details] [diff] [review] bug-1249706-fix.patch Review of attachment 8723262 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/media/MediaFormatReader.cpp @@ +1245,5 @@ > MOZ_ASSERT(OnTaskQueue()); > LOGV(""); > > + if (HasVideo()) { > + ReportDroppedFramesTelemetry(); why report it whenever ResetDecode() is called? this called when seeking. why not accumulate the data, and report it at the end only? that way you don't have to worry about threading safety, you're not impairing on speed. It certainly isn't the place to hook it up anyway, as when changing resolution in MSE this isn't called. @@ +1715,5 @@ > + nsCString keyPhrase = nsCString("MimeType="); > + keyPhrase.Append(mVideo.mInfo->mMimeType); > + keyPhrase.Append("; "); > + > + const VideoInfo* info = mVideo.mInfo->GetAsVideoInfo(); this will not work outside of MSE. mVideo.mInfo is only an override to mInfo when a content change is detected. So you want something like mVideo.mInfo ? mVideo.mInfo->mMimeType : mInfo.mVideo.mMimeType or: const VideoInfo* info = mVideo.mInfo ? mVideo.mInfo->GetAsVideoInfo() : &mInfo.mVideo @@ +1734,5 @@ > + > + if (mVideo.mNumSamplesOutputTotalSinceTelemetry) { > + uint32_t percentage = > + 100 * mVideo.mNumSamplesSkippedTotalSinceTelemetry / > + mVideo.mNumSamplesOutputTotalSinceTelemetry; ResetDecode() can be called multiple times , and before a video is decoded. so it's entirely possible mVideo.mNumSamplesOutputTotalSinceTelemetry could be 0. then boom ::: dom/media/platforms/PlatformDecoderModule.h @@ +211,5 @@ > virtual bool IsHardwareAccelerated(nsACString& aFailureReason) const { return false; } > > + // Provides information about the specific type of hardware acceleration for > + // use in telemetry (See bug 1249706) > + virtual nsCString GetHardwareAccelerator() const { return nsCString(); } that information is already reported by GetDescriptionName() the name itself will say if it's HW or not you don't need this.
Attachment #8723262 -
Flags: review?(jyavenard) → review-
Reporter | ||
Comment 6•8 years ago
|
||
(In reply to Jean-Yves Avenard [:jya] from comment #5) > Comment on attachment 8723262 [details] [diff] [review] > bug-1249706-fix.patch > > Review of attachment 8723262 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/media/MediaFormatReader.cpp > @@ +1245,5 @@ > > MOZ_ASSERT(OnTaskQueue()); > > LOGV(""); > > > > + if (HasVideo()) { > > + ReportDroppedFramesTelemetry(); > > why report it whenever ResetDecode() is called? this called when seeking. > > why not accumulate the data, and report it at the end only? > > that way you don't have to worry about threading safety, you're not > impairing on speed. > > It certainly isn't the place to hook it up anyway, as when changing > resolution in MSE this isn't called. This was because of cpearce's comment on IRC about reporting it when the resolution is changed. Where would you suggest we report it? > @@ +1734,5 @@ > > + > > + if (mVideo.mNumSamplesOutputTotalSinceTelemetry) { > > + uint32_t percentage = > > + 100 * mVideo.mNumSamplesSkippedTotalSinceTelemetry / > > + mVideo.mNumSamplesOutputTotalSinceTelemetry; > > ResetDecode() can be called multiple times , and before a video is decoded. > so it's entirely possible mVideo.mNumSamplesOutputTotalSinceTelemetry could > be 0. > > then boom Hence the check that its non-zero before reporting ... > ::: dom/media/platforms/PlatformDecoderModule.h > @@ +211,5 @@ > > virtual bool IsHardwareAccelerated(nsACString& aFailureReason) const { return false; } > > > > + // Provides information about the specific type of hardware acceleration for > > + // use in telemetry (See bug 1249706) > > + virtual nsCString GetHardwareAccelerator() const { return nsCString(); } > > that information is already reported by GetDescriptionName() > > the name itself will say if it's HW or not > > you don't need this. WFTVideoMFTManager::GetDescriptionName will return whether it's Hardware or Software decoded. Should I change it to report if the hardware decoder is D3D9 or D3D11 there instead?
Comment 7•8 years ago
|
||
(In reply to Louis Christie [:lchristie] from comment #6) > > Hence the check that its non-zero before reporting ... fair comment :) this is what happens when cpearce ask for a review to be done immediately ! > > > ::: dom/media/platforms/PlatformDecoderModule.h > > @@ +211,5 @@ > > > virtual bool IsHardwareAccelerated(nsACString& aFailureReason) const { return false; } > > > > > > + // Provides information about the specific type of hardware acceleration for > > > + // use in telemetry (See bug 1249706) > > > + virtual nsCString GetHardwareAccelerator() const { return nsCString(); } > > > > that information is already reported by GetDescriptionName() > > > > the name itself will say if it's HW or not > > > > you don't need this. > > WFTVideoMFTManager::GetDescriptionName will return whether it's Hardware or > Software decoded. Should I change it to report if the hardware decoder is > D3D9 or D3D11 there instead? you could. As to when reporting, I would accumulate on the resolution+mimetype and upload it at the end. but if you want to report when the resolution changes, ResetDecode() isn't the place to use as this is only called when seeking. if the use plays the video from beginning to the end without seeking, ResetDecode() will be called at the end and that's it. I would report it there: https://dxr.mozilla.org/mozilla-central/source/dom/media/MediaFormatReader.cpp#910 and upon a call to Shutdown() to cater for plain media playback, and the last resolution used for MSE
Reporter | ||
Comment 8•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=276b85062ca3
Attachment #8723262 -
Attachment is obsolete: true
Attachment #8723357 -
Flags: review?(jyavenard)
Comment 9•8 years ago
|
||
Comment on attachment 8723357 [details] [diff] [review] bug-1249706-fix.patch Review of attachment 8723357 [details] [diff] [review]: ----------------------------------------------------------------- r=me with requested changes addressed ::: dom/media/MediaFormatReader.cpp @@ +914,5 @@ > decoder.mNextStreamSourceID.ref() != info->GetID()) { > LOG("%s stream id has changed from:%d to:%d, draining decoder.", > TrackTypeToStr(aTrack), decoder.mLastStreamSourceID, > info->GetID()); > + if (HasVideo()) { if (aTrack == TrackType::kVideoTrack) @@ +1717,5 @@ > + if (!mVideo.mInfo || !mVideo.mDecoder) { > + return; > + } > + nsCString keyPhrase = nsCString("MimeType="); > + keyPhrase.Append(mVideo.mInfo->mMimeType); move that below and use info->mMimeType, so it reports the right mimetype, and not the one of the first decoder seen ::: dom/media/platforms/wmf/WMFVideoMFTManager.cpp @@ +654,5 @@ > +const char* > +WMFVideoMFTManager::GetDescriptionName() const > +{ > + if (mDecoder && mUseHwAccel && mDXVA2Manager) { > + return (mDXVA2Manager->IsD3D11()) ? "D3D11" : "D3D9"; please return something like D3D11/D3D9 hardware video decoder" mDXVA2Manager->IsD3D11() isn't thread-safe really... but i guess how it's used it doesn't really matter. And I have no good suggestions on how to make it proper
Attachment #8723357 -
Flags: review?(jyavenard) → review+
Reporter | ||
Comment 10•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d68cc04ec465
Attachment #8723357 -
Attachment is obsolete: true
Comment 11•8 years ago
|
||
Comment on attachment 8723747 [details] [diff] [review] bug-1249706-fix.patch Review of attachment 8723747 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/media/MediaFormatReader.cpp @@ +1713,5 @@ > +MediaFormatReader::ReportDroppedFramesTelemetry() > +{ > + MOZ_ASSERT(OnTaskQueue()); > + > + if (!mVideo.mInfo || !mVideo.mDecoder) { oh and you don't want to test for mVideo.mInfo here, as it will be nullptr for non-MSE. You can move that test below the declaration of info and test: if (!info || !mVideo.mDecoder)
Reporter | ||
Comment 12•8 years ago
|
||
Attachment #8723747 -
Attachment is obsolete: true
Reporter | ||
Updated•8 years ago
|
Keywords: checkin-needed
Comment 13•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/a085ea2d24bb
Keywords: checkin-needed
Comment 14•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/a085ea2d24bb
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox47:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Comment 15•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/8fe22dd4fc8a753f9b4c479b773af8ce03f4dae7 Bug 1249706 - Backout a085ea2d24bb for blowing telemetry server's mind. r=backout
Comment 16•8 years ago
|
||
(In reply to Chris Pearce (:cpearce) from comment #15) > https://hg.mozilla.org/integration/mozilla-inbound/rev/ > 8fe22dd4fc8a753f9b4c479b773af8ce03f4dae7 > Bug 1249706 - Backout a085ea2d24bb for blowing telemetry server's mind. > r=backout Backed out. We needed data collection peer review here.
Comment 17•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/1253710274373ffe0928ea7378ef2a60921d84a7 Bug 1249706 - Fix 8fe22dd4fc8a (backout of a085ea2d24bb). r=bustage
Comment 18•8 years ago
|
||
Should have kept the original WMFVideoMFTManager::GetDescriptionName() it was showing the type of D3D decoder used..
Updated•8 years ago
|
Priority: -- → P2
Mass change P2 -> P3
Priority: P2 → P3
Comment 20•5 years ago
|
||
The leave-open keyword is there and there is no activity for 6 months. :drno, maybe it's time to close this bug?
Flags: needinfo?(drno)
Comment 21•5 years ago
|
||
Is this still something we want?
Assignee: louis → nobody
Flags: needinfo?(drno) → needinfo?(jyavenard)
Comment 22•5 years ago
|
||
not sure what this is about, we have such telemetry already being reported
Status: REOPENED → RESOLVED
Closed: 8 years ago → 5 years ago
Flags: needinfo?(jyavenard)
Resolution: --- → WONTFIX
Updated•5 years ago
|
Keywords: leave-open
You need to log in
before you can comment on or make changes to this bug.
Description
•