All users were logged out of Bugzilla on October 13th, 2018

[EME] Implement WebM subsample encryption

RESOLVED FIXED in Firefox 54

Status

()

P3
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: cpearce, Assigned: jay.harris)

Tracking

(Blocks: 1 bug)

unspecified
mozilla54
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox54 fixed)

Details

(URL)

Attachments

(4 attachments)

(Assignee)

Updated

2 years ago
Assignee: nobody → jharris
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 5

2 years ago
mozreview-review
Comment on attachment 8835168 [details]
Bug 1306477 - Updates to latest upstream version of nestegg.

https://reviewboard.mozilla.org/r/110844/#review112192
Attachment #8835168 - Flags: review?(kinetik) → review+
(Reporter)

Comment 7

2 years ago
mozreview-review
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.
Attachment #8835170 - Flags: review?(cpearce) → review-
(Reporter)

Comment 8

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

https://reviewboard.mozilla.org/r/110850/#review112590
Attachment #8835171 - Flags: review?(cpearce) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Reporter)

Comment 15

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

https://reviewboard.mozilla.org/r/110848/#review112670
Attachment #8835170 - Flags: review?(cpearce) → review+
(Reporter)

Comment 16

2 years ago
mozreview-review
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.
Attachment #8835169 - Flags: review?(cpearce) → review-
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Reporter)

Comment 20

2 years ago
mozreview-review
Comment on attachment 8835169 [details]
Bug 1306477 - Adds support for subsample encrypted WebMs

https://reviewboard.mozilla.org/r/110846/#review113474
Attachment #8835169 - Flags: review?(cpearce) → review+
(Assignee)

Updated

2 years ago
Keywords: checkin-needed
Autoland couldn't rebase this for pushing.
Keywords: checkin-needed
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Keywords: checkin-needed
(Assignee)

Comment 26

2 years ago
(rebased)

Comment 27

2 years ago
Pushed by ihsiao@mozilla.com:
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
Keywords: checkin-needed

Comment 28

2 years ago
bugherder
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
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in before you can comment on or make changes to this bug.