1249706 - More detailed telemetry on the proportion of frames dropped.

Status: RESOLVED WONTFIX

(Core :: Audio/Video: Playback, defect, P3)

Description

[Louis Christie [:lchristie]]

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.

Comment 1

[Louis Christie [:lchristie]]

Attachment: bug-1249706-fix.patch

Comment 2

[Louis Christie [:lchristie]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7d2c7b2e6473

Comment 3

[Louis Christie [:lchristie]]

Attachment: bug-1249706-fix.patch

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1342ba3a1dfa

Comment 4

[Chris Pearce [:cpearce (Not reading bugmail)]]

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": ["[e-mail]", "[e-mail]"],
> +    "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%.

Comment 5

[Jean-Yves Avenard [:jya]]

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.

Comment 6

[Louis Christie [:lchristie]]

(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

[Jean-Yves Avenard [:jya]]

(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 8

[Louis Christie [:lchristie]]

Attachment: bug-1249706-fix.patch

https://treeherder.mozilla.org/#/jobs?repo=try&revision=276b85062ca3

Comment 9

[Jean-Yves Avenard [:jya]]

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

Comment 10

[Louis Christie [:lchristie]]

Attachment: bug-1249706-fix.patch

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d68cc04ec465

Comment 11

[Jean-Yves Avenard [:jya]]

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)

Comment 12

[Louis Christie [:lchristie]]

Attachment: bug-1249706-fix.patch

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a085ea2d24bb

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a085ea2d24bb

Comment 15

[Chris Pearce [:cpearce (Not reading bugmail)]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8fe22dd4fc8a753f9b4c479b773af8ce03f4dae7
Bug 1249706 - Backout a085ea2d24bb for blowing telemetry server's mind. r=backout

Comment 16

[Chris Pearce [:cpearce (Not reading bugmail)]]

(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

[Chris Pearce [:cpearce (Not reading bugmail)]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1253710274373ffe0928ea7378ef2a60921d84a7
Bug 1249706 - Fix 8fe22dd4fc8a (backout of a085ea2d24bb). r=bustage

Comment 18

[Jean-Yves Avenard [:jya]]

Should have kept the original WMFVideoMFTManager::GetDescriptionName()

it was showing the type of D3D decoder used..

Comment 19

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Mass change P2 -> P3

Comment 20

[BugBot [:suhaib / :marco/ :calixte]]

The leave-open keyword is there and there is no activity for 6 months.
:drno, maybe it's time to close this bug?

Comment 21

[Byron Campen [:bwc]]

Is this still something we want?

Comment 22

[Jean-Yves Avenard [:jya]]

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