1491117 - Samples from WebMs that have the signal byte present, but no encryption should be marked as unencrypted for the Widevine CDM

Status: RESOLVED FIXED

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

Description

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

Background:
WebM encryption[0] involves the usage of a signal byte that precedes sample data. Within this byte bits are used to signal the presence of encryption as well as other information for the following sample.

Bug:
Existing code[1] populates our encryption data structures when we receive a packet containing this signal byte. In the case that the byte indicates the following sample is not encrypted, we indicate this by specifying encryption info on the sample that indicates 0 encrypted bytes in the sample, and n clear bytes, where n is the sample length. I.e. we have encryption metadata on the sample indicating it is all clear bytes, contrast this with if we did not include any encryption metadata.

Including this metadata is suitable for Widevine CDM versions up to 1.4.9.x. However, starting at CDM versions 4.10.x, packets that have the signal bit but no encrypted data need to be annotated as unencrypted and should not carry encryption information.

Part of this is that the new CDM interface expects an explicit enum to be set indicating the encryption of input data. Previously, it appears the CDM would use the number of clear and encrypted bytes given to it to determine if a sample was unencrypted. If we do not include any encryption data, our code the prepares samples for the CDM can handle such samples accordingly.

Proposed solution:
If a packet is from an encrypted WebM stream, but that packet's sample is not encrypted, we should not populate any encryption data on the sample we create. This will appease both the new and current CDM based on my testing.

We could alternatively have the CDM sample handling code check if a sample is all clear bytes and handle accordingly, but I think the above solution is simpler.

[0]: https://www.webmproject.org/docs/webm-encryption/
[1]: https://searchfox.org/mozilla-central/rev/a23c3959b62cd3f616435e02810988ef5bac9031/dom/media/webm/WebMDemuxer.cpp#780

Comment 1

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

Attachment: Bug 1491117 - Do not add crypto info to unencrypted samples from encrypted WebM tracks. r=jya

WebMs with encrypted tracks may have unencrypted samples in these tracks.
Previously we would populate some of the crypto metadata on these samples. This
data was correct, but it was potentially misleading to include crypto metadata on
clear samples.

This changeset alters the behaviour so that we do not populate any such data for
unencrypted packets. This should not alter existing behaviour, notably the
Widevine CDM version 9 should continue to work. However, this change makes our
samples easier to feed to version 10 of the CDM. Without this change, we would
need to do extra conversion steps to appease the new CDM.

Comment 2

[Jean-Yves Avenard [:jya]]

Comment on attachment 9010014 [details]
Bug 1491117 - Do not add crypto info to unencrypted samples from encrypted WebM tracks. r=jya

Jean-Yves Avenard [:jya] has approved the revision.

Comment 3

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

Following further discussion with :jya, I've decided to delve deeper on what exactly causes problems with the newer (4.10.x) version of the CDM before proceeding here.

Before feeding data into the CDM we pack it into a buffer structure used by the CDM. Starting at CDM10 this structure has some new fields[0]:
- encryption_scheme - an enum specifying the encryption scheme used on the input sample.
- pattern - this is used to support pattern encryption. We don't need to worry about this for our current usage.

`encryption_scheme` *must* be set to un-encrypted if a sample is clear. This is the case even if we set up the subsample data to indicate that the the sample is entirely clear (which works with CDMs prior to 10 where there is no scheme enum). I have not exhaustively tested, but it appears that so long as `encryption_scheme` is un-encrypted, the CDM doesn't check other encryption metadata -- filling other metadata with bogus value did not lead to issue for unencrypted samples.

Currently, we pack encryption data for the CDM if the buffer sent to the GMP indicates via an enum that it holds encrypted data[1] -- it is here my work in progress sets the encryption_scheme. This decision is based on if the crypto member on the sample specifies if it is valid or not via the mValid member[2].

My current patch means that mValid will be false for clear WebM samples. I've been looking into the mp4 case, and for media I've tested it appears that we end up with mValid being false for clear samples. I'm not as familiar with mp4, but discussion with :jya and looking at the code[3] suggests it may be possible to have mValid = true for clear samples in mp4 -- this would happen for the first sample in a moof with a pssh box but where that sample was entirely in the clear (if that is possible). I'll tinker with test files further to see if I can get a file to prove this case with.

Given that we've had a convention of setting crypto data with mValid = true on clear samples from encrypted tracks, an alternative is that we could have a bool on our sample to indicate if they're clear. This could better communicate the idea that a sample is from an encrypted track, but itself is no encrypted.

[0]: https://searchfox.org/mozilla-central/rev/a0333927deabfe980094a14d0549b589f34cbe49/dom/media/gmp/widevine-adapter/content_decryption_module.h#177
[1]: https://searchfox.org/mozilla-central/rev/a0333927deabfe980094a14d0549b589f34cbe49/dom/media/gmp/ChromiumCDMChild.cpp#665
[2]: https://searchfox.org/mozilla-central/rev/a0333927deabfe980094a14d0549b589f34cbe49/dom/media/gmp/ChromiumCDMParent.cpp#257
[3]: https://searchfox.org/mozilla-central/rev/a0333927deabfe980094a14d0549b589f34cbe49/dom/media/mp4/Index.cpp#125

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/01b434fc5683
Do not add crypto info to unencrypted samples from encrypted WebM tracks. r=jya

Comment 5

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

Going ahead with landing as is. When implementing CDM10 I plan on putting in place some defensive checks to help catch cases where mValid = true put samples are clear so we can catch such issues.

Comment 6

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/01b434fc5683

Comment 7

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

Comment on attachment 9010014 [details]
Bug 1491117 - Do not add crypto info to unencrypted samples from encrypted WebM tracks. r=jya

[ESR Uplift Approval Request]

If this is not a sec:{high,crit} bug, please state case for ESR consideration: Please see Bug 1518525 for details and motivation.

Further notes: this is the second bug in a series tracked by bug 1518525. Please see https://treeherder.mozilla.org/#/jobs?repo=try&revision=b9976fb4de94bf0933f1faa8118154961fac20fb&selectedJob=220243631 for a try push where these patches have been grafted onto esr60.

User impact if declined: Failure to playback premium media.

Fix Landed on Version: 64

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): This patch has landed and baked since 64. I have manually verified the graft works on esr60.

String or UUID changes made by this patch: None.

Comment 8

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9010014 [details]
Bug 1491117 - Do not add crypto info to unencrypted samples from encrypted WebM tracks. r=jya

Needed for ESR60 to continue to support Widevine video playback in the very near future. Approved for 60.5.0esr.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr60/rev/900f94556211