Closed Bug 1552717 Opened 5 months ago Closed 5 months ago

[EME] Mp4s that make use of key rotation and start with an unencrypted lead do not surface initDataType correctly

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla69
Tracking Status
firefox-esr60 --- unaffected
firefox67 --- wontfix
firefox68 --- fixed
firefox69 --- fixed

People

(Reporter: bryce, Assigned: bryce)

References

(Regression)

Details

(Keywords: regression)

Attachments

(3 files)

Bug 1487416 updated our mp4 parsing to handle cbcs data. During this I appear to have broken our handling of init data (pssh boxes) in fragments.

In the regressing bug I changed the pssh/init data handling so that init data type is only set if the sample is encrypted. This is incorrect. An example of why this is: Consider a fragment with an unencrypted and an encrypted sample + a pssh box with data relating to the encrypted sample. In this case the first, unencrypted sample should have init data associated with it so the encrypted event can be fired (but the sample should not hold sample specific crypto data).

test_eme_pssh_in_moof tests if our key rotation works correctly. It currently
does so by using a single video with an audio and video track. This patch
refactors the test so that it does the same thing (all going well), but in a
more extensible way.

The changes in this patch seek to lean more heavily on test harness
functionality in manifest.js and eme.js where possible. This cuts down on some
boilerplate, but means we have to make some concessions in a more verbose
expression of our test media list so the eme.js functions work with it.

Add another test case for the mp4 key rotation (pssh in fragments) test. In this
case, the test file has a clear (unencrypted) lead. This test seeks to ensure we
don't regress surfacing of init info even if we encounter it for fragments that
start with unencrypted samples.

Add a further check to the key rotation test to ensure that the initDataType is
being surfaced correctly on the encrypted event.

Media files created with shaka packager via:

packager-win.exe
in=bipbop.mp4,stream=audio,out=bipbop-clearkey-keyrotation-clear-lead-audio.mp4
in=bipbop.mp4,stream=video,out=bipbop-clearkey-keyrotation-clear-lead-video.mp4
--enable_raw_key_encryption --keys
label=:key_id=00112233445566778899AABBCCDDEEFF:key=00112233445566778899AABBCCDDEEFF
--crypto_period_duration 5 --fragment_duration 5 --clear_lead 3

Note that the way shaka packager handles key rotation in this case is just to
left shift the key id and the key. In this case, where crypto_period_duration ==
fragment_duration, a left shift of 1 will take place each time the keys rotate.
This happens once in the test file leaving us with 2 key ids + keys.

Depends on D32750

Unencrypted samples can carry init data and thus we should set the init data
type regardless of if the sample itself is encrypted (otherwise the init data
info is incomplete for unencrypted samples).

Depends on D32751

  • Try run with new test but without patch to correct Index.cpp behaviour.
  • Try run with new test and patch.
Summary: [EME] Key rotation has been broken by mp4 changes for cbcs → [EME] Mp4s that make use of key rotation and start with an unencrypted lead do not sureface initDataType correctly
Summary: [EME] Mp4s that make use of key rotation and start with an unencrypted lead do not sureface initDataType correctly → [EME] Mp4s that make use of key rotation and start with an unencrypted lead do not surface initDataType correctly
Pushed by bvandyk@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/aeb0cd86768f
P1: Refactor test_eme_pssh_in_moof mochitest so it's easier to add further test media. r=cpearce
https://hg.mozilla.org/integration/autoland/rev/71edbdac5e12
P2: Add test media for key rotation with a clear lead. r=cpearce
https://hg.mozilla.org/integration/autoland/rev/267d1abce058
P3: Set mInitDataType on sample info regardless of if the sample is encrypted. r=cpearce
Status: NEW → RESOLVED
Closed: 5 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla69

What's the severity of this issue? Is this something we should consider backporting to Beta ahead of the next ESR or can this just ride the trains normally?

Flags: needinfo?(bvandyk)
Flags: in-testsuite+

(In reply to Ryan VanderMeulen [:RyanVM] from comment #10)

What's the severity of this issue? Is this something we should consider backporting to Beta ahead of the next ESR or can this just ride the trains normally?

That's a good question. We don't have good data on how often we encounter the kind of media that would be impacted by this. I had been thinking about uplifting and I landed this change before some others I have in the works to make it easier to uplift. Since it's relatively simple and since it would be nice to avoid having this regression I will request uplift.

Flags: needinfo?(bvandyk)

Comment on attachment 9067858 [details]
Bug 1552717 - P3: Set mInitDataType on sample info regardless of if the sample is encrypted. r?cpearce

Beta/Release Uplift Approval Request

  • User impact if declined: Sites that serve media that utilize key rotation with a unencrypted lead in will fail to playback media. I don't have any concrete examples of these sites in the wild, but am able to easily create such media with Google's shaka-packager, which means sites could easily do so too.
  • Is this code covered by automated tests?: Yes
  • Has the fix been verified in Nightly?: Yes
  • Needs manual test from QE?: No
  • If yes, steps to reproduce:
  • List of other uplifts needed: None, please only take P3 from this bug.
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): The change is low risk as it's small and has undergone a both automated and manual testing.
  • String changes made/needed:
Attachment #9067858 - Flags: approval-mozilla-beta?

Comment on attachment 9067858 [details]
Bug 1552717 - P3: Set mInitDataType on sample info regardless of if the sample is encrypted. r?cpearce

Beta/Release Uplift Approval Request

  • User impact if declined: Sites that serve media that utilize key rotation with a unencrypted lead in will fail to playback media. I don't have any concrete examples of these sites in the wild, but am able to easily create such media with Google's shaka-packager, which means sites could easily do so too.
  • Is this code covered by automated tests?: Yes
  • Has the fix been verified in Nightly?: Yes
  • Needs manual test from QE?: No
  • If yes, steps to reproduce:
  • List of other uplifts needed: None, please only take P3 from this bug.
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): The change is low risk as it's small and has undergone a both automated and manual testing.
  • String changes made/needed:

Beta/Release Uplift Approval Request

  • User impact if declined: Sites that serve media that utilize key rotation with a unencrypted lead in will fail to playback media. I don't have any concrete examples of these sites in the wild, but am able to easily create such media with Google's shaka-packager, which means sites could easily do so too.
  • Is this code covered by automated tests?: Yes
  • Has the fix been verified in Nightly?: Yes
  • Needs manual test from QE?: No
  • If yes, steps to reproduce:
  • List of other uplifts needed: None, please only take P3 from this bug.
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): The change is low risk as it's small and has undergone a both automated and manual testing.
  • String changes made/needed:

Comment on attachment 9067858 [details]
Bug 1552717 - P3: Set mInitDataType on sample info regardless of if the sample is encrypted. r?cpearce

regression fix, approved for 68.0b9

Attachment #9067858 - Flags: approval-mozilla-beta?
Attachment #9067858 - Flags: approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.