If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Add telemetry reporting for rust mp4parse success

RESOLVED FIXED in Firefox 45

Status

()

Core
Audio/Video: Playback
P2
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: rillian, Assigned: rillian)

Tracking

(Blocks: 1 bug)

Trunk
mozilla45
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox45 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

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.
(Assignee)

Updated

2 years ago
Priority: -- → P2
(Assignee)

Comment 1

2 years ago
Created attachment 8684367 [details] [diff] [review]
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.
Attachment #8684367 - Flags: review?(kinetik)
Attachment #8684367 - Flags: feedback?(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.
Attachment #8684367 - Flags: feedback?(ally) → feedback+
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.
Attachment #8684367 - Flags: review?(kinetik)
(Assignee)

Comment 4

2 years ago
Created attachment 8685069 [details] [diff] [review]
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.
Attachment #8684367 - Attachment is obsolete: true
Attachment #8685069 - Flags: review?(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.
Attachment #8685069 - Flags: review?(kinetik) → review+

Comment 6

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

Comment 7

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/890596bd4b01
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox45: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
You need to log in before you can comment on or make changes to this bug.