Closed Bug 1366639 Opened 2 years ago Closed 2 years ago

Add telemetry to track max number of shmems used by PChromiumCDM

Categories

(Core :: Audio/Video: GMP, enhancement, P1)

enhancement

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: cpearce, Assigned: cpearce)

References

Details

Attachments

(1 file)

The PChromiumCDM protocol has some fancy footwork to pre-allocate the number of shmems it estimate it will need, and to recover should the estimate be wrong (bug 1358373). It would be great if we could actually get the estimate right, then we'd not take the slow path when we're recovering from estimating wrong. So we should add telemetry to report the high-watermark for number of shmems allocated.
Comment on attachment 8869901 [details]
Bug 1366639 - Add telemetry to track max number of PChromiumCDM video frame shmems.

https://reviewboard.mozilla.org/r/141446/#review144982

::: toolkit/components/telemetry/Histograms.json:8341
(Diff revision 1)
> +    "alert_emails": ["cpearce@mozilla.com"],
> +    "bug_numbers": [1366639],
> +    "expires_in_version": "60",
> +    "kind": "enumerated",
> +    "n_values": 50,
> +    "description": "Counts of the maximum number of shared memory buffers used for transfering video frames between the CDM and Gecko processes during playback of DRM protected video."

The probe is sent from the CDM destructor.

Does that mean the probe keeps track of the max number _per video_ or do we instantiate the CDM process once _per browser session_ and reuse it across many videos?
Attachment #8869901 - Flags: review?(francois)
Comment on attachment 8869901 [details]
Bug 1366639 - Add telemetry to track max number of PChromiumCDM video frame shmems.

https://reviewboard.mozilla.org/r/141446/#review145288

r+ with nits:

::: dom/media/gmp/ChromiumCDMParent.cpp:702
(Diff revision 2)
>      }
>      mVideoShmemsActive++;
>    }
> +
> +  mMaxVideoShmemsActive =
> +    std::max<uint32_t>(mMaxVideoShmemsActive, mVideoShmemsActive);

Both variables are of the same type (and should be!), so there is no need to specify the type for max.
And I'd argue it's better to omit the type, in case you change one type later on but forget the other one, this would catch the mismatch.

::: toolkit/components/telemetry/Histograms.json:8341
(Diff revision 2)
> +    "alert_emails": ["cpearce@mozilla.com"],
> +    "bug_numbers": [1366639],
> +    "expires_in_version": "60",
> +    "kind": "enumerated",
> +    "n_values": 50,
> +    "description": "Counts of the maximum number of shared memory buffers used for transfering video frames between the CDM and Gecko processes during playback of DRM'd video. Reported once per CDMVideoDecoder instance, i.e. once per SourceBuffer JS during playback of video using EME."

"transferring" (double r)

Comma after "i.e." (American English style)

And do you mean "JS SourceBuffer"? (reversed words)
Attachment #8869901 - Flags: review?(gsquelart) → review+
Comment on attachment 8869901 [details]
Bug 1366639 - Add telemetry to track max number of PChromiumCDM video frame shmems.

https://reviewboard.mozilla.org/r/141446/#review145308

datareview+
Attachment #8869901 - Flags: review?(francois) → review+
Pushed by cpearce@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9bc41c308992
Add telemetry to track max number of PChromiumCDM video frame shmems. r=francois,gerald
Rank: 15
Priority: -- → P1
https://hg.mozilla.org/mozilla-central/rev/9bc41c308992
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
See Also: → 1430517
You need to log in before you can comment on or make changes to this bug.