1366639 - Add telemetry to track max number of shmems used by PChromiumCDM

Status: RESOLVED FIXED

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

Description

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

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 1

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

Attachment: Bug 1366639 - Add telemetry to track max number of PChromiumCDM video frame shmems.

This will enable us to pre-allocate the correct number of shared memory buffers
that we pre-allocate for sending video frames between the CDM and Gecko. That
means we won't need to take the slow path to recover from underestimating how
many shmems we need.

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

Comment 2

[François Marier [: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/#review144982

::: toolkit/components/telemetry/Histograms.json:8341
(Diff revision 1)
> +    "alert_emails": ["[e-mail]"],
> +    "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?

Comment 3

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

Comment on attachment 8869901 [details]
Bug 1366639 - Add telemetry to track max number of PChromiumCDM video frame shmems.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/141446/diff/1-2/

Comment 4

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

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

Comment 5

[François Marier [: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/#review145308

datareview+

Comment 6

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

Comment on attachment 8869901 [details]
Bug 1366639 - Add telemetry to track max number of PChromiumCDM video frame shmems.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/141446/diff/2-3/

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9bc41c308992
Add telemetry to track max number of PChromiumCDM video frame shmems. r=francois,gerald

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9bc41c308992