1306477 - [EME] Implement WebM subsample encryption

Status: RESOLVED FIXED

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

Description

[Chris Pearce [:cpearce (Not reading bugmail)]]

The WebM encryption spec now has subsample encryption:

https://www.webmproject.org/docs/webm-encryption/#46-subsample-encrypted-block-format

This has been implemented in Chromium.

Public chromium.org PSA: https://groups.google.com/a/chromium.org/forum/#!searchin/chromium-dev/subsample/chromium-dev/HvGava_WUjE/N9BDDgbYCQAJ
Related chromium bug: https://bugs.chromium.org/p/chromium/issues/detail?id=630344

Comment 1

[jay.harris]

Attachment: Bug 1306477 - Updates to latest upstream version of nestegg.

Review commit: https://reviewboard.mozilla.org/r/110844/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/110844/

Comment 2

[jay.harris]

Attachment: Bug 1306477 - Adds support for subsample encrypted WebMs

Review commit: https://reviewboard.mozilla.org/r/110846/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/110846/

Comment 3

[jay.harris]

Attachment: Bug 1306477 - Updates an assert in Clearkey to deal with unencrypted samples.

Review commit: https://reviewboard.mozilla.org/r/110848/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/110848/

Comment 4

[jay.harris]

Attachment: Bug 1306477 - Adds a simple test to confirm WebM Subsample Encryption works

Review commit: https://reviewboard.mozilla.org/r/110850/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/110850/

Comment 5

[Matthew Gregan [:kinetik]]

Comment on attachment 8835168 [details]
Bug 1306477 - Updates to latest upstream version of nestegg.

https://reviewboard.mozilla.org/r/110844/#review112192

Comment 6

[jay.harris]

Currently running try build https://treeherder.mozilla.org/#/jobs?repo=try&revision=9dd6184d2b6cbf64d60f588e4c16688e925b2e25&selectedJob=75720462

Comment 7

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8835170 [details]
Bug 1306477 - Updates an assert in Clearkey to deal with unencrypted samples.

https://reviewboard.mozilla.org/r/110848/#review112588

::: media/gmp-clearkey/0.1/ClearKeyDecryptionManager.cpp:28
(Diff revision 1)
>  #include <vector>
> +#include <algorithm>
>  
>  using namespace cdm;
>  
> +bool all_zero(std::vector<uint32_t> bytes)

This creates a copy of the vector you're passing in. This also doesn't follow Mozilla's C++ coding style.

So make this:

static bool
AllZero(const std::vector<uint32_t>& aBytes)
{
  //...
}

::: media/gmp-clearkey/0.1/ClearKeyDecryptionManager.cpp:235
(Diff revision 1)
>      tmp.resize((size_t)(iter - &tmp[0]));
>    } else {
>      memcpy(&tmp[0], aBuffer, aBufferSize);
>    }
>  
> -  assert(aMetadata.mIV.size() == 8 || aMetadata.mIV.size() == 16);
> +  assert(aMetadata.mIV.size() == 8 \

You should add a comment here explaining why you assert this.

Comment 8

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8835171 [details]
Bug 1306477 - Adds a simple test to confirm WebM Subsample Encryption works

https://reviewboard.mozilla.org/r/110850/#review112590

Comment 9

[jay.harris]

Comment on attachment 8835169 [details]
Bug 1306477 - Adds support for subsample encrypted WebMs

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/110846/diff/1-2/

Comment 10

[jay.harris]

Comment on attachment 8835170 [details]
Bug 1306477 - Updates an assert in Clearkey to deal with unencrypted samples.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/110848/diff/1-2/

Comment 11

[jay.harris]

Comment on attachment 8835171 [details]
Bug 1306477 - Adds a simple test to confirm WebM Subsample Encryption works

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/110850/diff/1-2/

Comment 12

[jay.harris]

Comment on attachment 8835169 [details]
Bug 1306477 - Adds support for subsample encrypted WebMs

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/110846/diff/2-3/

Comment 13

[jay.harris]

Comment on attachment 8835170 [details]
Bug 1306477 - Updates an assert in Clearkey to deal with unencrypted samples.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/110848/diff/2-3/

Comment 14

[jay.harris]

Comment on attachment 8835171 [details]
Bug 1306477 - Adds a simple test to confirm WebM Subsample Encryption works

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/110850/diff/2-3/

Comment 15

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8835170 [details]
Bug 1306477 - Updates an assert in Clearkey to deal with unencrypted samples.

https://reviewboard.mozilla.org/r/110848/#review112670

Comment 16

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8835169 [details]
Bug 1306477 - Adds support for subsample encrypted WebMs

https://reviewboard.mozilla.org/r/110846/#review113456

::: dom/media/webm/WebMDemuxer.cpp:743
(Diff revision 3)
> +        } else if (packetEncryption == NESTEGG_PACKET_HAS_SIGNAL_BYTE_PARTITIONED) {
> +          uint8_t numPartitions = 0;
> +          const uint32_t* partitions = NULL;
> +          nestegg_packet_offsets(holder->Packet(), &partitions, &numPartitions);
> +
> +          // WebM stores a number of partitions whereas we expect the lengths

The comments don't make it clear why you do the encrypted=!encrypted assignment below.

If you described what WebM stores, it would be clear.

WebM stores the number of partitions, and stores the start offset of each partition, starting with the offset of the first encrypted partition.

You should also comment that there is an implicit "first" partition which is unencrypted.

::: dom/media/webm/WebMDemuxer.cpp:775
(Diff revision 3)
> +            writer->mCrypto.mPlainSizes.AppendElement(length - lastOffset);
> +          }
> +
> +          // Make sure we have an equal number of encrypted and plain sizes (GMP
> +          // expects this).
> +          if (numPartitions % 2 == 0) {

It's not obvious from the code why you can get away with only appending to the encrypted size, so you should have a comment that explains why you do this.

Comment 17

[jay.harris]

Comment on attachment 8835169 [details]
Bug 1306477 - Adds support for subsample encrypted WebMs

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/110846/diff/3-4/

Comment 18

[jay.harris]

Comment on attachment 8835170 [details]
Bug 1306477 - Updates an assert in Clearkey to deal with unencrypted samples.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/110848/diff/3-4/

Comment 19

[jay.harris]

Comment on attachment 8835171 [details]
Bug 1306477 - Adds a simple test to confirm WebM Subsample Encryption works

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/110850/diff/3-4/

Comment 20

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8835169 [details]
Bug 1306477 - Adds support for subsample encrypted WebMs

https://reviewboard.mozilla.org/r/110846/#review113474

Comment 21

[Ryan VanderMeulen [:RyanVM]]

Autoland couldn't rebase this for pushing.

Comment 22

[jay.harris]

Comment on attachment 8835168 [details]
Bug 1306477 - Updates to latest upstream version of nestegg.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/110844/diff/1-2/

Comment 23

[jay.harris]

Comment on attachment 8835169 [details]
Bug 1306477 - Adds support for subsample encrypted WebMs

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/110846/diff/4-5/

Comment 24

[jay.harris]

Comment on attachment 8835170 [details]
Bug 1306477 - Updates an assert in Clearkey to deal with unencrypted samples.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/110848/diff/4-5/

Comment 25

[jay.harris]

Comment on attachment 8835171 [details]
Bug 1306477 - Adds a simple test to confirm WebM Subsample Encryption works

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/110850/diff/4-5/

Comment 26

[jay.harris]

(rebased)

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0fd35cf8df2a
Updates to latest upstream version of nestegg. r=kinetik
https://hg.mozilla.org/integration/autoland/rev/4ba776d3f2d3
Adds support for subsample encrypted WebMs r=cpearce
https://hg.mozilla.org/integration/autoland/rev/dd00f20399f3
Updates an assert in Clearkey to deal with unencrypted samples. r=cpearce
https://hg.mozilla.org/integration/autoland/rev/ef6e1bab2baf
Adds a simple test to confirm WebM Subsample Encryption works r=cpearce

Comment 28

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0fd35cf8df2a
https://hg.mozilla.org/mozilla-central/rev/4ba776d3f2d3
https://hg.mozilla.org/mozilla-central/rev/dd00f20399f3
https://hg.mozilla.org/mozilla-central/rev/ef6e1bab2baf