Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1149023002:
Combine 'pssh' parsing routines. (Closed)
|
Created:
5 years, 7 months ago by jrummell Modified:
5 years, 6 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. |
DescriptionCombine CENC 'pssh' box parsing routines.
Also update the routines to ignore 'pssh' boxes with version 2 or
later.
BUG=460359, 460360
TEST=new unittests pass
Committed: https://crrev.com/b666d7874efac44b359a95329f0cb890e97671df
Cr-Commit-Position: refs/heads/master@{#333556}
Committed: https://crrev.com/7a84443597e92a03f90806712f8629df40bf408e
Cr-Commit-Position: refs/heads/master@{#333611}
Committed: https://crrev.com/4d54a811979a0a6e3b5260cb7acd6d4fa1df71de
Cr-Commit-Position: refs/heads/master@{#334416}
Patch Set 1 #
Total comments: 4
Patch Set 2 : changes #Patch Set 3 : rebase #
Total comments: 12
Patch Set 4 : changes #
Total comments: 12
Patch Set 5 : more comments #
Total comments: 4
Patch Set 6 : boxes #
Total comments: 4
Patch Set 7 : changes #Patch Set 8 : fix array init #
Total comments: 4
Patch Set 9 : fix uint #
Total comments: 2
Patch Set 10 : size_t #
Messages
Total messages: 41 (7 generated)
jrummell@chromium.org changed reviewers: + ddorwin@chromium.org, sandersd@chromium.org, xhwang@chromium.org
PTAL. This is a replacement for https://codereview.chromium.org/1145853002/, unless you don't like it.
https://codereview.chromium.org/1149023002/diff/1/media/cdm/cenc_utils.cc File media/cdm/cenc_utils.cc (right): https://codereview.chromium.org/1149023002/diff/1/media/cdm/cenc_utils.cc#new... media/cdm/cenc_utils.cc:75: return false; This pattern (lines 68 to 75) is repeated 3 times. Extract to a helper? https://codereview.chromium.org/1149023002/diff/1/media/formats/mp4/box_reade... File media/formats/mp4/box_reader.cc (right): https://codereview.chromium.org/1149023002/diff/1/media/formats/mp4/box_reade... media/formats/mp4/box_reader.cc:238: MEDIA_LOG(DEBUG, log_cb_) << "Media Source Extensions do not support ISO " This message is specific to MSE, but the flag (!enforce_size_check_) does not imply MSE. Given that the two code paths are quite different, I'd almost rather duplicate the code into separate classes (eg via subclassing). Maybe a middle ground is to move some of the code into a helper that can return a status value: success, failure, need more data. Then the two variants only need to differ in how they handle the last case. (I realize that this doesn't solve the zero-size case, so I'll accept other solutions.)
Updated. https://codereview.chromium.org/1149023002/diff/1/media/cdm/cenc_utils.cc File media/cdm/cenc_utils.cc (right): https://codereview.chromium.org/1149023002/diff/1/media/cdm/cenc_utils.cc#new... media/cdm/cenc_utils.cc:75: return false; On 2015/05/22 19:51:24, sandersd wrote: > This pattern (lines 68 to 75) is repeated 3 times. Extract to a helper? Handling of empty() is different. But extracted the rest. https://codereview.chromium.org/1149023002/diff/1/media/formats/mp4/box_reade... File media/formats/mp4/box_reader.cc (right): https://codereview.chromium.org/1149023002/diff/1/media/formats/mp4/box_reade... media/formats/mp4/box_reader.cc:238: MEDIA_LOG(DEBUG, log_cb_) << "Media Source Extensions do not support ISO " On 2015/05/22 19:51:25, sandersd wrote: > This message is specific to MSE, but the flag (!enforce_size_check_) does not > imply MSE. Given that the two code paths are quite different, I'd almost rather > duplicate the code into separate classes (eg via subclassing). > > Maybe a middle ground is to move some of the code into a helper that can return > a status value: success, failure, need more data. Then the two variants only > need to differ in how they handle the last case. (I realize that this doesn't > solve the zero-size case, so I'll accept other solutions.) Changed the flag name to indicate EOS, added comments, and changed the message since MSE will be obvious.
lgtm
I skimmed at a high-level. Mostly nits. I also slightly modified the first line of the description. https://codereview.chromium.org/1149023002/diff/40001/media/cdm/cenc_utils_un... File media/cdm/cenc_utils_unittest.cc (right): https://codereview.chromium.org/1149023002/diff/40001/media/cdm/cenc_utils_un... media/cdm/cenc_utils_unittest.cc:28: const uint8_t kClearKeyUuid[] = { s/CK/CommonSystem/ https://codereview.chromium.org/1149023002/diff/40001/media/cdm/cenc_utils_un... media/cdm/cenc_utils_unittest.cc:386: TEST_F(CencUtilsTest, GetPsshDataVersion0) { Add an underscore to all of these: GetPsshData_<test-details> https://codereview.chromium.org/1149023002/diff/40001/media/cdm/cenc_utils_un... media/cdm/cenc_utils_unittest.cc:431: TEST_F(CencUtilsTest, GetPsshDataVersion2) { Should test Version2 then 1 and vice versa. https://codereview.chromium.org/1149023002/diff/40001/media/formats/mp4/box_r... File media/formats/mp4/box_reader.h (right): https://codereview.chromium.org/1149023002/diff/40001/media/formats/mp4/box_r... media/formats/mp4/box_reader.h:159: BoxReader(const uint8* buf, const int size, const LogCB& log_cb, bool is_EOS); Should probably explain is_EOS here. https://codereview.chromium.org/1149023002/diff/40001/media/formats/mp4/box_r... media/formats/mp4/box_reader.h:182: // Set to true if the buffer provided to the reader is the complete stream. Drop "Set to " since we don't intend for it to be set other than during construction. https://codereview.chromium.org/1149023002/diff/40001/media/formats/mp4/box_r... media/formats/mp4/box_reader.h:183: bool is_EOS_; const?
Updated. https://codereview.chromium.org/1149023002/diff/40001/media/cdm/cenc_utils_un... File media/cdm/cenc_utils_unittest.cc (right): https://codereview.chromium.org/1149023002/diff/40001/media/cdm/cenc_utils_un... media/cdm/cenc_utils_unittest.cc:28: const uint8_t kClearKeyUuid[] = { On 2015/06/04 18:08:45, ddorwin wrote: > s/CK/CommonSystem/ Done. https://codereview.chromium.org/1149023002/diff/40001/media/cdm/cenc_utils_un... media/cdm/cenc_utils_unittest.cc:386: TEST_F(CencUtilsTest, GetPsshDataVersion0) { On 2015/06/04 18:08:45, ddorwin wrote: > Add an underscore to all of these: GetPsshData_<test-details> Done. https://codereview.chromium.org/1149023002/diff/40001/media/cdm/cenc_utils_un... media/cdm/cenc_utils_unittest.cc:431: TEST_F(CencUtilsTest, GetPsshDataVersion2) { On 2015/06/04 18:08:45, ddorwin wrote: > Should test Version2 then 1 and vice versa. Done. https://codereview.chromium.org/1149023002/diff/40001/media/formats/mp4/box_r... File media/formats/mp4/box_reader.h (right): https://codereview.chromium.org/1149023002/diff/40001/media/formats/mp4/box_r... media/formats/mp4/box_reader.h:159: BoxReader(const uint8* buf, const int size, const LogCB& log_cb, bool is_EOS); On 2015/06/04 18:08:45, ddorwin wrote: > Should probably explain is_EOS here. Done. https://codereview.chromium.org/1149023002/diff/40001/media/formats/mp4/box_r... media/formats/mp4/box_reader.h:182: // Set to true if the buffer provided to the reader is the complete stream. On 2015/06/04 18:08:45, ddorwin wrote: > Drop "Set to " since we don't intend for it to be set other than during > construction. Done. https://codereview.chromium.org/1149023002/diff/40001/media/formats/mp4/box_r... media/formats/mp4/box_reader.h:183: bool is_EOS_; On 2015/06/04 18:08:45, ddorwin wrote: > const? Done.
Thanks. https://codereview.chromium.org/1149023002/diff/60001/media/cdm/cenc_utils.cc File media/cdm/cenc_utils.cc (right): https://codereview.chromium.org/1149023002/diff/60001/media/cdm/cenc_utils.cc... media/cdm/cenc_utils.cc:27: std::vector<mp4::FullProtectionSystemSpecificHeader>* children) { Why is this named |children|? https://codereview.chromium.org/1149023002/diff/60001/media/cdm/cenc_utils.cc... media/cdm/cenc_utils.cc:32: // Verify that |input| contains only 'pssh' boxes. I don't see how we validate this? Does this rely on templates or something? https://codereview.chromium.org/1149023002/diff/60001/media/cdm/cenc_utils.cc... media/cdm/cenc_utils.cc:40: // For each 'pssh' box, parse it into a FullProtectionSystemSpecificHeader. This code is confusing until one really thinks about it. I think there could be better comments that link the parts and use of Full vs. not Full together. For example, first we're going to do X then Y. It's not initially clear whether the mixing of Full and not Full is intentional or why we need to call ReadAllChildren() twice. I initially wondered, "Why doesn't the second call verify that these are all PSSH boxes?" I guess because we "continue" in this loop. Perhaps we could report is_wrong_box_type as an output parameter and avoid the first loop. https://codereview.chromium.org/1149023002/diff/60001/media/cdm/cenc_utils.cc... media/cdm/cenc_utils.cc:44: scoped_ptr<mp4::BoxReader> child_reader( Why isn't this just on the stack? https://codereview.chromium.org/1149023002/diff/60001/media/cdm/cenc_utils_un... File media/cdm/cenc_utils_unittest.cc (right): https://codereview.chromium.org/1149023002/diff/60001/media/cdm/cenc_utils_un... media/cdm/cenc_utils_unittest.cc:28: const uint8_t kCommonEncryptionUuid[] = { nit: Is there a reason "Uuid" is used? That's only used for Android AFAIK. "SystemId" is probably better. https://codereview.chromium.org/1149023002/diff/60001/media/cdm/cenc_utils_un... media/cdm/cenc_utils_unittest.cc:443: // Concatentate box1 onto end of box2. This is a little confusing. box_v1 and box_v2 might be better if that's why they are in this order and named this way.
Updated. https://codereview.chromium.org/1149023002/diff/60001/media/cdm/cenc_utils.cc File media/cdm/cenc_utils.cc (right): https://codereview.chromium.org/1149023002/diff/60001/media/cdm/cenc_utils.cc... media/cdm/cenc_utils.cc:27: std::vector<mp4::FullProtectionSystemSpecificHeader>* children) { On 2015/06/04 22:09:15, ddorwin wrote: > Why is this named |children|? Renamed to |pssh_boxes|. https://codereview.chromium.org/1149023002/diff/60001/media/cdm/cenc_utils.cc... media/cdm/cenc_utils.cc:32: // Verify that |input| contains only 'pssh' boxes. On 2015/06/04 22:09:15, ddorwin wrote: > I don't see how we validate this? Does this rely on templates or something? Yes. Comment expanded. https://codereview.chromium.org/1149023002/diff/60001/media/cdm/cenc_utils.cc... media/cdm/cenc_utils.cc:40: // For each 'pssh' box, parse it into a FullProtectionSystemSpecificHeader. On 2015/06/04 22:09:15, ddorwin wrote: > This code is confusing until one really thinks about it. I think there could be > better comments that link the parts and use of Full vs. not Full together. For > example, first we're going to do X then Y. > > It's not initially clear whether the mixing of Full and not Full is intentional > or why we need to call ReadAllChildren() twice. I initially wondered, "Why > doesn't the second call verify that these are all PSSH boxes?" I guess because > we "continue" in this loop. Perhaps we could report is_wrong_box_type as an > output parameter and avoid the first loop. Done. https://codereview.chromium.org/1149023002/diff/60001/media/cdm/cenc_utils.cc... media/cdm/cenc_utils.cc:44: scoped_ptr<mp4::BoxReader> child_reader( On 2015/06/04 22:09:15, ddorwin wrote: > Why isn't this just on the stack? ReadConcatentatedBoxes() returns a pointer. https://codereview.chromium.org/1149023002/diff/60001/media/cdm/cenc_utils_un... File media/cdm/cenc_utils_unittest.cc (right): https://codereview.chromium.org/1149023002/diff/60001/media/cdm/cenc_utils_un... media/cdm/cenc_utils_unittest.cc:28: const uint8_t kCommonEncryptionUuid[] = { On 2015/06/04 22:09:15, ddorwin wrote: > nit: Is there a reason "Uuid" is used? That's only used for Android AFAIK. > "SystemId" is probably better. Done. https://codereview.chromium.org/1149023002/diff/60001/media/cdm/cenc_utils_un... media/cdm/cenc_utils_unittest.cc:443: // Concatentate box1 onto end of box2. On 2015/06/04 22:09:15, ddorwin wrote: > This is a little confusing. box_v1 and box_v2 might be better if that's why they > are in this order and named this way. Done.
LG % some nits for the parts I reviewed. https://codereview.chromium.org/1149023002/diff/80001/media/cdm/cenc_utils.cc File media/cdm/cenc_utils.cc (right): https://codereview.chromium.org/1149023002/diff/80001/media/cdm/cenc_utils.cc... media/cdm/cenc_utils.cc:60: // Must have successfully parsed at least one 'pssh' box successfully. nit: successfully twice https://codereview.chromium.org/1149023002/diff/80001/media/cdm/cenc_utils_un... File media/cdm/cenc_utils_unittest.cc (right): https://codereview.chromium.org/1149023002/diff/80001/media/cdm/cenc_utils_un... media/cdm/cenc_utils_unittest.cc:437: EXPECT_TRUE(GetPsshData(box_v2, CommonSystemSystemId(), &pssh_data)); nit: A little bit hard to read with the "v2 box" being passed. Even though it's less efficient, this is a test so inserting both into |boxes| would probably be better.
Updated. https://codereview.chromium.org/1149023002/diff/80001/media/cdm/cenc_utils.cc File media/cdm/cenc_utils.cc (right): https://codereview.chromium.org/1149023002/diff/80001/media/cdm/cenc_utils.cc... media/cdm/cenc_utils.cc:60: // Must have successfully parsed at least one 'pssh' box successfully. On 2015/06/08 03:43:23, ddorwin wrote: > nit: successfully twice Done. https://codereview.chromium.org/1149023002/diff/80001/media/cdm/cenc_utils_un... File media/cdm/cenc_utils_unittest.cc (right): https://codereview.chromium.org/1149023002/diff/80001/media/cdm/cenc_utils_un... media/cdm/cenc_utils_unittest.cc:437: EXPECT_TRUE(GetPsshData(box_v2, CommonSystemSystemId(), &pssh_data)); On 2015/06/08 03:43:23, ddorwin wrote: > nit: A little bit hard to read with the "v2 box" being passed. Even though it's > less efficient, this is a test so inserting both into |boxes| would probably be > better. Done.
https://codereview.chromium.org/1149023002/diff/100001/media/cdm/cenc_utils.cc File media/cdm/cenc_utils.cc (right): https://codereview.chromium.org/1149023002/diff/100001/media/cdm/cenc_utils.c... media/cdm/cenc_utils.cc:60: // Must have successfully parsed at least one 'pssh' box successfully twice. I meant you use the word "successfully" twice in this sentence and should remove one. You can probably just drop everything after "box". https://codereview.chromium.org/1149023002/diff/100001/media/cdm/cenc_utils_u... File media/cdm/cenc_utils_unittest.cc (right): https://codereview.chromium.org/1149023002/diff/100001/media/cdm/cenc_utils_u... media/cdm/cenc_utils_unittest.cc:455: // Concatentate the boxes together (v2 first). s/v2/v1/
Updated. https://codereview.chromium.org/1149023002/diff/100001/media/cdm/cenc_utils.cc File media/cdm/cenc_utils.cc (right): https://codereview.chromium.org/1149023002/diff/100001/media/cdm/cenc_utils.c... media/cdm/cenc_utils.cc:60: // Must have successfully parsed at least one 'pssh' box successfully twice. On 2015/06/08 20:57:15, ddorwin wrote: > I meant you use the word "successfully" twice in this sentence and should remove > one. You can probably just drop everything after "box". Done. https://codereview.chromium.org/1149023002/diff/100001/media/cdm/cenc_utils_u... File media/cdm/cenc_utils_unittest.cc (right): https://codereview.chromium.org/1149023002/diff/100001/media/cdm/cenc_utils_u... media/cdm/cenc_utils_unittest.cc:455: // Concatentate the boxes together (v2 first). On 2015/06/08 20:57:15, ddorwin wrote: > s/v2/v1/ Done.
LG2M for what I reviewed.
On 2015/06/09 17:56:37, ddorwin wrote: > LG2M for what I reviewed. And components/cdm/browser/ 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/1149023002/#ps120001 (title: "changes")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1149023002/120001
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/b666d7874efac44b359a95329f0cb890e97671df Cr-Commit-Position: refs/heads/master@{#333556}
Message was sent while issue was closed.
A revert of this CL (patchset #7 id:120001) has been created in https://codereview.chromium.org/1170923005/ by rfevang@chromium.org. The reason for reverting is: Broke Google Chrome Mac build: https://build.chromium.org/p/chromium.chrome/builders/Google%20Chrome%20Mac/b....
Message was sent while issue was closed.
Updated test code so it should compile on Mac.
Fix in PS8 LGTM, assuming the change in data is intentional and okay. https://codereview.chromium.org/1149023002/diff/140001/media/cdm/cenc_utils_u... File media/cdm/cenc_utils_unittest.cc (right): https://codereview.chromium.org/1149023002/diff/140001/media/cdm/cenc_utils_u... media/cdm/cenc_utils_unittest.cc:498: const uint8_t data1_bytes[] = {0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07}; The data changed, including lengths. Is that okay?
Comments only. https://codereview.chromium.org/1149023002/diff/140001/media/cdm/cenc_utils_u... File media/cdm/cenc_utils_unittest.cc (right): https://codereview.chromium.org/1149023002/diff/140001/media/cdm/cenc_utils_u... media/cdm/cenc_utils_unittest.cc:498: const uint8_t data1_bytes[] = {0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07}; On 2015/06/09 22:21:24, ddorwin wrote: > The data changed, including lengths. Is that okay? Yup. With 8 bytes the line is too long and clang-format wants to split it into 1 byte per line which takes up way too much space. We really don't care about the length of the data or what it contains, but just need to make sure it's different in the 2 pssh boxes so we can check we get the right stuff back.
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/1149023002/#ps140001 (title: "fix array init")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1149023002/140001
https://codereview.chromium.org/1149023002/diff/140001/media/cdm/cenc_utils_u... File media/cdm/cenc_utils_unittest.cc (right): https://codereview.chromium.org/1149023002/diff/140001/media/cdm/cenc_utils_u... media/cdm/cenc_utils_unittest.cc:388: AppendData(box, data); Alternatively, you could update AppendData() to take a pointer and size. That would avoid needing two variables per call and hide this complexity in that single location. AppendData() is only called from places you had to make this change, so it does not impact any other code.
https://codereview.chromium.org/1149023002/diff/140001/media/cdm/cenc_utils_u... File media/cdm/cenc_utils_unittest.cc (right): https://codereview.chromium.org/1149023002/diff/140001/media/cdm/cenc_utils_u... media/cdm/cenc_utils_unittest.cc:388: AppendData(box, data); On 2015/06/09 22:48:24, ddorwin wrote: > Alternatively, you could update AppendData() to take a pointer and size. That > would avoid needing two variables per call and hide this complexity in that > single location. > AppendData() is only called from places you had to make this change, so it does > not impact any other code. I tried that, but the |data| is used later to make sure it is returned correctly.
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/7a84443597e92a03f90806712f8629df40bf408e Cr-Commit-Position: refs/heads/master@{#333611}
Message was sent while issue was closed.
A revert of this CL (patchset #8 id:140001) has been created in https://codereview.chromium.org/1165313003/ by kjellander@google.com. The reason for reverting is: Breaks compile on 'Google Chrome Win' bot: https://build.chromium.org/p/chromium.chrome/builders/Google%20Chrome%20Win/b....
Updated once again.
https://codereview.chromium.org/1149023002/diff/160001/media/cdm/cenc_utils_u... File media/cdm/cenc_utils_unittest.cc (right): https://codereview.chromium.org/1149023002/diff/160001/media/cdm/cenc_utils_u... media/cdm/cenc_utils_unittest.cc:108: for (uint32_t i = 0; i < key1.size(); ++i) always use size_t when using or comparing to std:: size().
Updated. https://codereview.chromium.org/1149023002/diff/160001/media/cdm/cenc_utils_u... File media/cdm/cenc_utils_unittest.cc (right): https://codereview.chromium.org/1149023002/diff/160001/media/cdm/cenc_utils_u... media/cdm/cenc_utils_unittest.cc:108: for (uint32_t i = 0; i < key1.size(); ++i) On 2015/06/12 23:34:38, ddorwin wrote: > always use size_t when using or comparing to std:: size(). Done.
PS10 changes 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/1149023002/#ps180001 (title: "size_t")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1149023002/180001
Message was sent while issue was closed.
Committed patchset #10 (id:180001)
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/4d54a811979a0a6e3b5260cb7acd6d4fa1df71de Cr-Commit-Position: refs/heads/master@{#334416} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
