Closed Bug 1306477 Opened 8 years ago Closed 8 years ago

[EME] Implement WebM subsample encryption

Categories

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

Product:
Core
Component:
Audio/Video: Playback
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla54
Tracking Flags:
Tracking Status
firefox54 --- fixed

People

(Reporter: cpearce, Assigned: jay.harris)

References

(Blocks 1 open bug,
URL
)

Details

Attachments

(4 files)

Bug 1306477 - Updates to latest upstream version of nestegg.
59 bytes, text/x-review-board-request
kinetik
: review+
Details
Bug 1306477 - Adds support for subsample encrypted WebMs
59 bytes, text/x-review-board-request
cpearce
: review+
Details
Bug 1306477 - Updates an assert in Clearkey to deal with unencrypted samples.
59 bytes, text/x-review-board-request
cpearce
: review+
Details
Bug 1306477 - Adds a simple test to confirm WebM Subsample Encryption works
59 bytes, text/x-review-board-request
cpearce
: review+
Details
Assignee: nobody → jharris
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+
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-
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 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+
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 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+
Keywords: checkin-needed
Autoland couldn't rebase this for pushing.
Keywords: checkin-needed
Keywords: checkin-needed
(rebased)
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
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: