More detailed telemetry on the proportion of frames dropped.

RESOLVED WONTFIX

Status

()

defect
P3
normal
RESOLVED WONTFIX
3 years ago
3 months ago

People

(Reporter: lchristie, Unassigned)

Tracking

unspecified
mozilla47
Points:
---

Firefox Tracking Flags

(firefox47 fixed)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

3 years ago
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

3 years ago
Assignee: nobody → lchristie
(Reporter)

Comment 1

3 years ago
Posted patch bug-1249706-fix.patch (obsolete) — Splinter Review
Attachment #8722321 - Flags: review?(cpearce)
(Reporter)

Updated

3 years ago
Attachment #8722321 - Flags: review?(cpearce)
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 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

3 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?
(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
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+
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

3 years ago
Attachment #8723747 - Attachment is obsolete: true
(Reporter)

Updated

3 years ago
Keywords: checkin-needed

Comment 14

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/a085ea2d24bb
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
(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.
Status: RESOLVED → REOPENED
Keywords: leave-open
Resolution: FIXED → ---
Should have kept the original WMFVideoMFTManager::GetDescriptionName()

it was showing the type of D3D decoder used..
Mass change P2 -> P3
Priority: P2 → P3
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)

Is this still something we want?

Assignee: louis → nobody
Flags: needinfo?(drno) → needinfo?(jyavenard)

not sure what this is about, we have such telemetry already being reported

Status: REOPENED → RESOLVED
Last Resolved: 3 years ago3 months ago
Flags: needinfo?(jyavenard)
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.