1220885 - Add telemetry reporting for rust mp4parse success

Status: RESOLVED FIXED

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

Description

[Ralph Giles (:rillian)]

We'd like to have some measure of how successful the rust mp4 parser is with files in the wild as we transition to it.

This bug is about adding a telemetry probe for this.

Comment 1

[Ralph Giles (:rillian)]

Attachment: Add telemetry probe for rust mp4parse success

Add a boolean to record whether mp4parse successfully parsed an initialization segment, or returned an error.

Adding :ally for data collection review.

NB this is my first telemetry probe. I can't tell if it is working or not. The patch compiles, but in my test build, about:telemetry doesn't show anything matching the key in the Histograms section.

Comment 2

[Allison Naaktgeboren :ally]

Comment on attachment 8684367 [details] [diff] [review]
Add telemetry probe for rust mp4parse success

Review of attachment 8684367 [details] [diff] [review]:
-----------------------------------------------------------------

p=ally with change for data collection. 

If the probe hasn't been triggered, there won't be any record. You'll need to trigger the accumulate() then if you don't see it in the payload, there's a problem. If you're not confident your patch is correct, please hope into #telemetry and ask for assistance.

::: toolkit/components/telemetry/Histograms.json
@@ +6261,5 @@
>    },
> +  "MEDIA_RUST_MP4PARSE_SUCCESS": {
> +    "expires_in_version": "50",
> +    "kind": "boolean",
> +    "description": "Whether the rust mp4 demuxer successfully parsed a stream segment.",

Please add the bug number to the description.

Comment 3

[Matthew Gregan [:kinetik]]

Comment on attachment 8684367 [details] [diff] [review]
Add telemetry probe for rust mp4parse success

Review of attachment 8684367 [details] [diff] [review]:
-----------------------------------------------------------------

Don't we want to record the result of the existing parser somehow for comparison? What we need to know is that the new Rust parser has similar behaviour to the existing parser, I think.

Comment 4

[Ralph Giles (:rillian)]

Attachment: Add telemetry probe for rust mp4parse success v2

Add bug number to the histogram description as requested. Carrying p=ally.

Kinetik said,

> Don't we want to record the result of the existing parser somehow for comparison? What we need to know is that the new Rust parser has similar behaviour to the existing parser, I think.

That was the original idea, but I thought this was a useful starting point which we could build on once your track query changes are ready.

In particular, stagefright doesn't propagate parse failure the way we do. It just doesn't report any tracks it wasn't able to parse. We treat the file as unplayable if there are no audio or video tracks. The next step would be to record whether the audio and video track counts match, but wouldn't we still want to distinguish "mp4parse-rust failed" from "mp4parse-rust and stagefright both found no audio or video tracks"?

Would flattening that into an enum be a better representation to work with? If not, we might as well add this one now and either redefine 'success' later to include track counts matching, etc., or add additional keys for new checks.

Comment 5

[Matthew Gregan [:kinetik]]

Comment on attachment 8685069 [details] [diff] [review]
Add telemetry probe for rust mp4parse success v2

Review of attachment 8685069 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks, sounds okay.

Comment 6

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/890596bd4b01

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/890596bd4b01