Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2174533002:
Implement WebM subsample support (Closed)
DescriptionImplement WebM subsample support according to the specification at http://wiki.webmproject.org/encryption/webm-subsample-encryption.
BUG=630344
Committed: https://crrev.com/f35464f359937e4f2ad362cd7919223b012dfd3f
Cr-Commit-Position: refs/heads/master@{#418705}
Patch Set 1 #Patch Set 2 : update with new partition meaning #Patch Set 3 : Update with new meaning of partition #Patch Set 4 : Rewrite the code to make it clearer #
Total comments: 9
Patch Set 5 : Address review comments #
Total comments: 2
Patch Set 6 : Rebased #
Messages
Total messages: 28 (14 generated)
Description was changed from ========== Implement WebM subsample support BUG=630344 ========== to ========== Implement WebM subsample support BUG=630344 ==========
kqyang@chromium.org changed reviewers: + tinskip@chromium.org
kqyang@chromium.org changed reviewers: + tinskip@google.com
kqyang@chromium.org changed reviewers: + chcunningham@chromium.org, ddorwin@chromium.org
chcunningham@chromium.org: Please review changes in ddorwin@chromium.org: Please review changes in
I'm a little confused on the partition offset parsing. I thought N = N+1 comment would imply that for 2 partitions, N = 1. But your tests show that in this scenario N=2 and appears to be counting the number of sections. https://codereview.chromium.org/2174533002/diff/60001/media/formats/webm/webm... File media/formats/webm/webm_crypto_helpers.cc (right): https://codereview.chromium.org/2174533002/diff/60001/media/formats/webm/webm... media/formats/webm/webm_crypto_helpers.cc:48: ? frame_data_size I don't follow this bit. Is the offset for the last partition not expected to be saved as an integer like the previous partition? Frame data size is the size of the whole frame (parition offsets + all sample data) right? Wouldn't this put you at the very end of the buffer? https://codereview.chromium.org/2174533002/diff/60001/media/formats/webm/webm... media/formats/webm/webm_crypto_helpers.cc:126: frame_offset += kWebMEncryptedFramePartitionSize * num_partitions; kWebMEncryptedFramePartitionSize ... might be confused with the actual size of the partition. Really this is the partition_offset_size. I know the name is long already though... I'm fine if you shorten Encrypted to Enc or drop Frame https://codereview.chromium.org/2174533002/diff/60001/media/formats/webm/webm... File media/formats/webm/webm_crypto_helpers_unittest.cc (right): https://codereview.chromium.org/2174533002/diff/60001/media/formats/webm/webm... media/formats/webm/webm_crypto_helpers_unittest.cc:64: 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, Just curious, why do we extend to 16 bytes?
LGTM, but David may also want to take a look before you submit. https://codereview.chromium.org/2174533002/diff/60001/media/formats/webm/webm... File media/formats/webm/webm_crypto_helpers.cc (right): https://codereview.chromium.org/2174533002/diff/60001/media/formats/webm/webm... media/formats/webm/webm_crypto_helpers.cc:48: ? frame_data_size On 2016/07/29 21:32:41, chcunningham wrote: > I don't follow this bit. Is the offset for the last partition not expected to be > saved as an integer like the previous partition? Frame data size is the size of > the whole frame (parition offsets + all sample data) right? Wouldn't this put > you at the very end of the buffer? We chatted, and this works, I was just confused about the meaning of each offset (and what the offset is relative to). Offsets are relative to the start of the actual frame data. The offset of the first clear section is implicit (0). The first offset read from the buffer is for the wall dividing the first clear and encrypted sections. Thanks for agreeing to document this :) https://codereview.chromium.org/2174533002/diff/60001/media/formats/webm/webm... File media/formats/webm/webm_crypto_helpers_unittest.cc (right): https://codereview.chromium.org/2174533002/diff/60001/media/formats/webm/webm... media/formats/webm/webm_crypto_helpers_unittest.cc:5: #include "media/formats/webm/webm_crypto_helpers.h" Thanks for writing great tests.
PTAL https://codereview.chromium.org/2174533002/diff/60001/media/formats/webm/webm... File media/formats/webm/webm_crypto_helpers.cc (right): https://codereview.chromium.org/2174533002/diff/60001/media/formats/webm/webm... media/formats/webm/webm_crypto_helpers.cc:48: ? frame_data_size On 2016/07/29 22:26:22, chcunningham wrote: > On 2016/07/29 21:32:41, chcunningham wrote: > > I don't follow this bit. Is the offset for the last partition not expected to > be > > saved as an integer like the previous partition? Frame data size is the size > of > > the whole frame (parition offsets + all sample data) right? Wouldn't this put > > you at the very end of the buffer? > > We chatted, and this works, I was just confused about the meaning of each offset > (and what the offset is relative to). Offsets are relative to the start of the > actual frame data. The offset of the first clear section is implicit (0). The > first offset read from the buffer is for the wall dividing the first clear and > encrypted sections. > > Thanks for agreeing to document this :) Done. https://codereview.chromium.org/2174533002/diff/60001/media/formats/webm/webm... media/formats/webm/webm_crypto_helpers.cc:126: frame_offset += kWebMEncryptedFramePartitionSize * num_partitions; On 2016/07/29 21:32:41, chcunningham wrote: > kWebMEncryptedFramePartitionSize ... might be confused with the actual size of > the partition. Really this is the partition_offset_size. I know the name is long > already though... I'm fine if you shorten Encrypted to Enc or drop Frame Renamed to kWebMEncryptedFramePartitionOffsetSize https://codereview.chromium.org/2174533002/diff/60001/media/formats/webm/webm... File media/formats/webm/webm_crypto_helpers_unittest.cc (right): https://codereview.chromium.org/2174533002/diff/60001/media/formats/webm/webm... media/formats/webm/webm_crypto_helpers_unittest.cc:5: #include "media/formats/webm/webm_crypto_helpers.h" On 2016/07/29 22:26:22, chcunningham wrote: > Thanks for writing great tests. :) https://codereview.chromium.org/2174533002/diff/60001/media/formats/webm/webm... media/formats/webm/webm_crypto_helpers_unittest.cc:64: 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, On 2016/07/29 21:32:42, chcunningham wrote: > Just curious, why do we extend to 16 bytes? Some CDMs expect IV to be 16 bytes: https://cs.chromium.org/chromium/src/media/cdm/aes_decryptor.cc?rcl=147006465...
ddorwin@chromium.org changed reviewers: - ddorwin@chromium.org
You don't need my review. However, see my questions in the bug. Specifically, is this stable before we enable it by default?
chcunningham@google.com changed reviewers: + chcunningham@google.com
LGTM
lgtm https://codereview.chromium.org/2174533002/diff/80001/media/formats/webm/webm... File media/formats/webm/webm_crypto_helpers.cc (right): https://codereview.chromium.org/2174533002/diff/80001/media/formats/webm/webm... media/formats/webm/webm_crypto_helpers.cc:27: uint32_t ReadInteger(const uint8_t* buf, int size) { Does not exists elsewhere? inline?
Thanks for the review! https://codereview.chromium.org/2174533002/diff/80001/media/formats/webm/webm... File media/formats/webm/webm_crypto_helpers.cc (right): https://codereview.chromium.org/2174533002/diff/80001/media/formats/webm/webm... media/formats/webm/webm_crypto_helpers.cc:27: uint32_t ReadInteger(const uint8_t* buf, int size) { On 2016/08/17 19:28:25, tinskip1 wrote: > Does not exists elsewhere? There is a BufferReader in src/media/formats/mp4/box_reader.h; It is not a good idea to depend on an implementation in mp4 from webm. We can move the BufferReader implementation out to media/formats/common if necessary - that should be done in a separate cleanup cl. > > inline? Chrome does not like inline codes except for simple getters and setters: https://www.chromium.org/developers/coding-style/cpp-dos-and-donts
Description was changed from ========== Implement WebM subsample support BUG=630344 ========== to ========== Implement WebM subsample support according to the specification at http://wiki.webmproject.org/encryption/webm-subsample-encryption. BUG=630344 ==========
The WebM subsample encryption spec has been published in http://wiki.webmproject.org/encryption/webm-subsample-encryption. I am planning to submit this cl tomorrow afternoon. Please let me know if there is any concerns. Thanks.
The CQ bit was checked by kqyang@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from chcunningham@chromium.org Link to the patchset: https://codereview.chromium.org/2174533002/#ps80001 (title: "Address review comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by kqyang@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tinskip@chromium.org, chcunningham@chromium.org, chcunningham@google.com Link to the patchset: https://codereview.chromium.org/2174533002/#ps100001 (title: "Rebased")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Implement WebM subsample support according to the specification at http://wiki.webmproject.org/encryption/webm-subsample-encryption. BUG=630344 ========== to ========== Implement WebM subsample support according to the specification at http://wiki.webmproject.org/encryption/webm-subsample-encryption. BUG=630344 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Implement WebM subsample support according to the specification at http://wiki.webmproject.org/encryption/webm-subsample-encryption. BUG=630344 ========== to ========== Implement WebM subsample support according to the specification at http://wiki.webmproject.org/encryption/webm-subsample-encryption. BUG=630344 Committed: https://crrev.com/f35464f359937e4f2ad362cd7919223b012dfd3f Cr-Commit-Position: refs/heads/master@{#418705} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/f35464f359937e4f2ad362cd7919223b012dfd3f Cr-Commit-Position: refs/heads/master@{#418705} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
