Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1199643002:
Have GetKeyIds() return the key IDs from the first matching 'pssh' box (Closed)
|
Created:
5 years, 6 months ago by jrummell Modified:
5 years, 5 months ago CC:
chromium-reviews, eme-reviews_chromium.org, feature-media-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionHave GetKeyIds() return the key IDs from the first matching 'pssh' box
Rather than returning the key IDs from all matching 'pssh' boxes,
only return the key IDs from the first matching one. Also rename
GetKeyIdsForCommonSystemId() to GetKeyIds() and make it support
any system identifier.
BUG=
TEST=updated tests pass
Committed: https://crrev.com/9a33878f50db3506d97d1cddc5bf5c339132b1c4
Cr-Commit-Position: refs/heads/master@{#336835}
Patch Set 1 #
Total comments: 4
Patch Set 2 : different systemid #Patch Set 3 : new test #
Total comments: 18
Patch Set 4 : changes #
Total comments: 4
Patch Set 5 : back to GetKeyIdsForCommonSystemId #
Messages
Total messages: 18 (4 generated)
jrummell@chromium.org changed reviewers: + ddorwin@chromium.org, sandersd@chromium.org
PTAL.
https://codereview.chromium.org/1199643002/diff/1/media/cdm/cenc_utils_unitte... File media/cdm/cenc_utils_unittest.cc (right): https://codereview.chromium.org/1199643002/diff/1/media/cdm/cenc_utils_unitte... media/cdm/cenc_utils_unittest.cc:184: GetKeyIds(std::vector<uint8_t>(), CommonSystemSystemId(), &key_ids)); Add a test using a different key system ID?
Updated test. https://codereview.chromium.org/1199643002/diff/1/media/cdm/cenc_utils_unitte... File media/cdm/cenc_utils_unittest.cc (right): https://codereview.chromium.org/1199643002/diff/1/media/cdm/cenc_utils_unitte... media/cdm/cenc_utils_unittest.cc:184: GetKeyIds(std::vector<uint8_t>(), CommonSystemSystemId(), &key_ids)); On 2015/06/22 20:17:20, sandersd wrote: > Add a test using a different key system ID? There is one (line 295), but added a second variation.
https://codereview.chromium.org/1199643002/diff/1/media/cdm/cenc_utils_unitte... File media/cdm/cenc_utils_unittest.cc (right): https://codereview.chromium.org/1199643002/diff/1/media/cdm/cenc_utils_unitte... media/cdm/cenc_utils_unittest.cc:184: GetKeyIds(std::vector<uint8_t>(), CommonSystemSystemId(), &key_ids)); On 2015/06/22 21:01:18, jrummell wrote: > On 2015/06/22 20:17:20, sandersd wrote: > > Add a test using a different key system ID? > > There is one (line 295), but added a second variation. I would also like to see a test were a |key_system_id| that is not CommonSystemSystemId() matches successfully, because the ability to do that was one of the key new features in this CL.
Patchset #3 (id:40001) has been deleted
Added new test. https://codereview.chromium.org/1199643002/diff/1/media/cdm/cenc_utils_unitte... File media/cdm/cenc_utils_unittest.cc (right): https://codereview.chromium.org/1199643002/diff/1/media/cdm/cenc_utils_unitte... media/cdm/cenc_utils_unittest.cc:184: GetKeyIds(std::vector<uint8_t>(), CommonSystemSystemId(), &key_ids)); On 2015/06/22 22:23:29, sandersd wrote: > On 2015/06/22 21:01:18, jrummell wrote: > > On 2015/06/22 20:17:20, sandersd wrote: > > > Add a test using a different key system ID? > > > > There is one (line 295), but added a second variation. > > I would also like to see a test were a |key_system_id| that is not > CommonSystemSystemId() matches successfully, because the ability to do that was > one of the key new features in this CL. Added.
lgtm
High-level review. https://codereview.chromium.org/1199643002/diff/60001/media/cdm/aes_decryptor.cc File media/cdm/aes_decryptor.cc (right): https://codereview.chromium.org/1199643002/diff/60001/media/cdm/aes_decryptor... media/cdm/aes_decryptor.cc:32: const uint8_t kCommonSystemId[] = {0x10, 0x77, 0xef, 0xec, 0xc0, 0xb2, This needs some qualification since it is CENC-specific but this file is not. I suggest including "CENC" in line 30 and "Cenc" after the 'k' in the name. Or better yet, hide this detail again. See my comment in the implementation of GetKeyIds(). https://codereview.chromium.org/1199643002/diff/60001/media/cdm/aes_decryptor... media/cdm/aes_decryptor.cc:293: } break; Is this clang format? It looks weird. https://codereview.chromium.org/1199643002/diff/60001/media/cdm/cenc_utils.cc File media/cdm/cenc_utils.cc (right): https://codereview.chromium.org/1199643002/diff/60001/media/cdm/cenc_utils.cc... media/cdm/cenc_utils.cc:18: static bool ReadAllPsshBoxes( Document this function. For example, returns true if |input| contains only valid 'pssh' boxes. https://codereview.chromium.org/1199643002/diff/60001/media/cdm/cenc_utils.cc... media/cdm/cenc_utils.cc:67: const std::vector<uint8_t>& system_id, It's fine to have this generic implementation internally, but would we ever want to call this with any other system_id? Exposing it sort of implies there is a reason to be looking at the key IDs for other systems. https://codereview.chromium.org/1199643002/diff/60001/media/cdm/cenc_utils.h File media/cdm/cenc_utils.h (right): https://codereview.chromium.org/1199643002/diff/60001/media/cdm/cenc_utils.h#... media/cdm/cenc_utils.h:21: // or more concatenated 'pssh' boxes. Returns true if such a box is found and "such" is ambiguous, especially as it relates to v0 boxes. Also, will boxes without key IDs be skipped until one for the system that has key IDs is found? #1 below would seem to indicate no. That's fine, but we should be clear. Depending on the desired behavior, this comment should be updated. https://codereview.chromium.org/1199643002/diff/60001/media/cdm/cenc_utils.h#... media/cdm/cenc_utils.h:26: // 2. Only PSSH boxes are allowed in |input|. Will boxes after a supported box still cause an error? (I'm not sure how to best document the answer.) https://codereview.chromium.org/1199643002/diff/60001/media/cdm/cenc_utils.h#... media/cdm/cenc_utils.h:27: MEDIA_EXPORT bool GetKeyIds(const std::vector<uint8_t>& input, GetKeyIdsFromPsshBoxes (There is no context at the call site, as in the previous non-CENC-specific file.) https://codereview.chromium.org/1199643002/diff/60001/media/cdm/cenc_utils.h#... media/cdm/cenc_utils.h:27: MEDIA_EXPORT bool GetKeyIds(const std::vector<uint8_t>& input, s/input/pssh_boxes/?
Updated. https://codereview.chromium.org/1199643002/diff/60001/media/cdm/aes_decryptor.cc File media/cdm/aes_decryptor.cc (right): https://codereview.chromium.org/1199643002/diff/60001/media/cdm/aes_decryptor... media/cdm/aes_decryptor.cc:32: const uint8_t kCommonSystemId[] = {0x10, 0x77, 0xef, 0xec, 0xc0, 0xb2, On 2015/06/25 21:18:12, ddorwin wrote: > This needs some qualification since it is CENC-specific but this file is not. I > suggest including "CENC" in line 30 and "Cenc" after the 'k' in the name. > > Or better yet, hide this detail again. See my comment in the implementation of > GetKeyIds(). Done. Also made dependent on USE_PROPRIETARY_CODECS since the compilers will complain about unused variable. https://codereview.chromium.org/1199643002/diff/60001/media/cdm/aes_decryptor... media/cdm/aes_decryptor.cc:293: } break; On 2015/06/25 21:18:12, ddorwin wrote: > Is this clang format? It looks weird. Yup. https://codereview.chromium.org/1199643002/diff/60001/media/cdm/cenc_utils.cc File media/cdm/cenc_utils.cc (right): https://codereview.chromium.org/1199643002/diff/60001/media/cdm/cenc_utils.cc... media/cdm/cenc_utils.cc:18: static bool ReadAllPsshBoxes( On 2015/06/25 21:18:12, ddorwin wrote: > Document this function. For example, returns true if |input| contains only valid > 'pssh' boxes. Done. https://codereview.chromium.org/1199643002/diff/60001/media/cdm/cenc_utils.cc... media/cdm/cenc_utils.cc:67: const std::vector<uint8_t>& system_id, On 2015/06/25 21:18:12, ddorwin wrote: > It's fine to have this generic implementation internally, but would we ever want > to call this with any other system_id? Exposing it sort of implies there is a > reason to be looking at the key IDs for other systems. Added in case some future implementation wants to pass the key IDs in the KIDs field rather than the data. https://codereview.chromium.org/1199643002/diff/60001/media/cdm/cenc_utils.h File media/cdm/cenc_utils.h (right): https://codereview.chromium.org/1199643002/diff/60001/media/cdm/cenc_utils.h#... media/cdm/cenc_utils.h:21: // or more concatenated 'pssh' boxes. Returns true if such a box is found and On 2015/06/25 21:18:12, ddorwin wrote: > "such" is ambiguous, especially as it relates to v0 boxes. Also, will boxes > without key IDs be skipped until one for the system that has key IDs is found? > #1 below would seem to indicate no. That's fine, but we should be clear. > > Depending on the desired behavior, this comment should be updated. Done. https://codereview.chromium.org/1199643002/diff/60001/media/cdm/cenc_utils.h#... media/cdm/cenc_utils.h:26: // 2. Only PSSH boxes are allowed in |input|. On 2015/06/25 21:18:12, ddorwin wrote: > Will boxes after a supported box still cause an error? (I'm not sure how to best > document the answer.) Updated comment. https://codereview.chromium.org/1199643002/diff/60001/media/cdm/cenc_utils.h#... media/cdm/cenc_utils.h:27: MEDIA_EXPORT bool GetKeyIds(const std::vector<uint8_t>& input, On 2015/06/25 21:18:12, ddorwin wrote: > GetKeyIdsFromPsshBoxes > > (There is no context at the call site, as in the previous non-CENC-specific > file.) Done. https://codereview.chromium.org/1199643002/diff/60001/media/cdm/cenc_utils.h#... media/cdm/cenc_utils.h:27: MEDIA_EXPORT bool GetKeyIds(const std::vector<uint8_t>& input, On 2015/06/25 21:18:12, ddorwin wrote: > s/input/pssh_boxes/? Done.
See reply in previous PS. https://codereview.chromium.org/1199643002/diff/60001/media/cdm/cenc_utils.cc File media/cdm/cenc_utils.cc (right): https://codereview.chromium.org/1199643002/diff/60001/media/cdm/cenc_utils.cc... media/cdm/cenc_utils.cc:67: const std::vector<uint8_t>& system_id, On 2015/06/25 22:35:58, jrummell wrote: > On 2015/06/25 21:18:12, ddorwin wrote: > > It's fine to have this generic implementation internally, but would we ever > want > > to call this with any other system_id? Exposing it sort of implies there is a > > reason to be looking at the key IDs for other systems. > > Added in case some future implementation wants to pass the key IDs in the KIDs > field rather than the data. I think this adds the risk of YAGNI and misuse, both of which seem more likely. As I said, I think this internal refactoring is fine, but we should probably make this method static and expose GetKeyIdsForCencCommonSystem() or something like that. _If_ we need the generic functionality, we can just make this not static. Also, aes_decryptor should not need to know the Common SystemID as it does currently in this CL. https://codereview.chromium.org/1199643002/diff/80001/media/cdm/cenc_utils.cc File media/cdm/cenc_utils.cc (right): https://codereview.chromium.org/1199643002/diff/80001/media/cdm/cenc_utils.cc... media/cdm/cenc_utils.cc:78: if (!ReadAllPsshBoxes(pssh_boxes, &children)) We now have |pssh_boxes| passed to |input| AND |pssh_boxes| returned as |children|. I'm not sure how to fix that, though. https://codereview.chromium.org/1199643002/diff/80001/media/cdm/cenc_utils_un... File media/cdm/cenc_utils_unittest.cc (right): https://codereview.chromium.org/1199643002/diff/80001/media/cdm/cenc_utils_un... media/cdm/cenc_utils_unittest.cc:496: // GetKeyIdsFromPsshBoxes() should return the single key from the v1 nit: "... v1 box." should fit. Below too.
Updated. https://codereview.chromium.org/1199643002/diff/60001/media/cdm/cenc_utils.cc File media/cdm/cenc_utils.cc (right): https://codereview.chromium.org/1199643002/diff/60001/media/cdm/cenc_utils.cc... media/cdm/cenc_utils.cc:67: const std::vector<uint8_t>& system_id, On 2015/06/25 22:49:23, ddorwin wrote: > On 2015/06/25 22:35:58, jrummell wrote: > > On 2015/06/25 21:18:12, ddorwin wrote: > > > It's fine to have this generic implementation internally, but would we ever > > want > > > to call this with any other system_id? Exposing it sort of implies there is > a > > > reason to be looking at the key IDs for other systems. > > > > Added in case some future implementation wants to pass the key IDs in the KIDs > > field rather than the data. > > I think this adds the risk of YAGNI and misuse, both of which seem more likely. > As I said, I think this internal refactoring is fine, but we should probably > make this method static and expose GetKeyIdsForCencCommonSystem() or something > like that. _If_ we need the generic functionality, we can just make this not > static. > > Also, aes_decryptor should not need to know the Common SystemID as it does > currently in this CL. Done. https://codereview.chromium.org/1199643002/diff/80001/media/cdm/cenc_utils.cc File media/cdm/cenc_utils.cc (right): https://codereview.chromium.org/1199643002/diff/80001/media/cdm/cenc_utils.cc... media/cdm/cenc_utils.cc:78: if (!ReadAllPsshBoxes(pssh_boxes, &children)) On 2015/06/25 22:49:23, ddorwin wrote: > We now have |pssh_boxes| passed to |input| AND |pssh_boxes| returned as > |children|. I'm not sure how to fix that, though. Acknowledged. https://codereview.chromium.org/1199643002/diff/80001/media/cdm/cenc_utils_un... File media/cdm/cenc_utils_unittest.cc (right): https://codereview.chromium.org/1199643002/diff/80001/media/cdm/cenc_utils_un... media/cdm/cenc_utils_unittest.cc:496: // GetKeyIdsFromPsshBoxes() should return the single key from the v1 On 2015/06/25 22:49:23, ddorwin wrote: > nit: "... v1 box." should fit. Below too. Back to the old name, need the wrap.
lgtm
The CQ bit was checked by jrummell@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sandersd@chromium.org Link to the patchset: https://codereview.chromium.org/1199643002/#ps100001 (title: "back to GetKeyIdsForCommonSystemId")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1199643002/100001
Message was sent while issue was closed.
Committed patchset #5 (id:100001)
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/9a33878f50db3506d97d1cddc5bf5c339132b1c4 Cr-Commit-Position: refs/heads/master@{#336835} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
