1513651 - Collect telemetry on sample description box entries when parsing mp4s

Status: RESOLVED FIXED

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

Description

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

The changes in bug 1513042 expose further information from the an mp4's sample description box (stsd) to Firefox's mp4 parsing code. Implementing these changes highlighted a number of cases that appear possible within the specs, but that we do not handle well. These cases are assumed to be rare or non-existent in the wild, but it would be useful to gather telemetry to verify this, and so we can dedicate engineering time if our assumptions are proven incorrect.

These cases are:
- A track may contain multiple sample description entries (we use the term 'sample info' in much of our code). Much of our code assumes we'll encounter only one entry. It would be useful to know how often we 1, 2, or more.
- In the case where a track contains more than one sample description entry, those entries may not have the same codec. Our code rejects such cases. How often do we see 1, 2, or more codecs?
- In the case where a track contains more than one sample description entry, those entries may specify different crypto information. Our code rejects such cases. How often do we see 0, 1, 2, or more sets of crypto information?

Comment 1

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

Attachment: DataCollectionReviewForm.txt

Comment 2

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

Attachment: Bug 1513651 - Collect telemetry on sample description box entries when parsing mp4s. r?jya,janerik

This patch adds telemetry to help us determine if mp4s with certain structures
can occur in the wild. Specifically:
- How often do mp4s have multiple entries in the sample description box?
- Do we ever see mp4s with multiple codecs in the sample description box?
- Do we ever see mp4s with multiple sets of crypto info in the sample
  description box?

This information is collected each time we parse mp4 metadata.

Comment 3

[Chris H-C :chutten]

Comment on attachment 9031131 [details]
DataCollectionReviewForm.txt

Preliminary note:
For "what is the opt-out mechanism for users?" the answer for Firefox Telemetry is almost always "The Data Choices setting in Firefox's Preferences" (I keep meaning to add that to the template (Issue #11) but I never seem to get to it)

Also, while I'm here, did you consider only collecting these for an initial six months to ensure that they're meeting your needs appropriately? Having a specific future date where collection needs to be reviewed is sometimes a helpful thing.

Also, permanent collections often benefit from automated tests. Without an expiry & renewal cycle keeping an eye on them you probably want a test ensuring no one breaks your collections.

DATA COLLECTION REVIEW RESPONSE:

    Is there or will there be documentation that describes the schema for the ultimate data set available publicly, complete and accurate? 

Yes. These collections are Telemetry so are documented in their definitions file (Histograms.json), the Probe Dictionary, and on telemetry.mozilla.org's Measurement Dashboards.

    Is there a control mechanism that allows the user to turn the data collection on and off?

Yes. These collections are Telemetry so are controlled by Firefox's Preferences.

    If the request is for permanent data collection, is there someone who will monitor the data over time?

Yes. :bryce will do so.

    Using the category system of data types on the Mozilla wiki, what collection type of data do the requested measurements fall under?

Category 2, Interaction. 

    Is the data collection request for default-on or default-off?

Default on for all channels.

    Does the instrumentation include the addition of any new identifiers (whether anonymous or otherwise; e.g., username, random IDs, etc. See the appendix for more details)?

No.

    Is the data collection covered by the existing Firefox privacy notice? 

Yes.

    Does there need to be a check-in in the future to determine whether to renew the data? 

No. These collections are permanent.

---
Result: datareview+

Comment 4

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

It sounds like adding some tests here would be sensible. I'm trying to clear some other bugs in time for the holidays, but hope to return to this to address :jya's comments as well as add some tests based on the above.

Comment 5

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

Attachment: Bug 1513651 - Add gtest for MP4 metadata telemetry probes. r?jya,chutten

Add a gtest to cover the new telemetry probes which collect info on sample
description entries.

Add a new init segment to those already in our gtests. This segment has data for
one video track, and covers the case where multiple sample description entries
are present for a single track. In this case, because separate sample
description entries are used for unencrypted and encrypted samples.

For reference, the test init segment was created from our existing bipbop using
shaka-packager via the following command (non-init segments were discarded and
the output was renamed):

packager-win.exe
in=bipbop.mp4,stream=video,init_segment=bipbop_cbcs_video_init.mp4,segment_template=bipbop_cbcs_video_$Number$.m4s
--protection_scheme cbcs --enable_raw_key_encryption --keys
label=:key_id=7e571d047e571d047e571d047e571d21:key=7e5744447e5744447e5744447e574421
--iv 11223344556677889900112233445566 --generate_static_mpd --mpd_output
bipbop_cbcs.mpd

Depends on D14426

Comment 6

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

Attachment: Bug 1513651 - Add test file with 2 sample description entries to MP4 parser general gtests. r?jya

Depends on D19386

Comment 7

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

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

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c387900cf31b
Collect telemetry on sample description box entries when parsing mp4s. r=jya
https://hg.mozilla.org/integration/autoland/rev/946f649535c1
Add gtest for MP4 metadata telemetry probes. r=chutten
https://hg.mozilla.org/integration/autoland/rev/fdf2d461ceb2
Add test file with 2 sample description entries to MP4 parser general gtests. r=jya

Comment 9

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/c387900cf31b
https://hg.mozilla.org/mozilla-central/rev/946f649535c1
https://hg.mozilla.org/mozilla-central/rev/fdf2d461ceb2