Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Cmake port of Media crypto #1221

Merged
merged 11 commits into from
Jul 14, 2023

Conversation

cosmin
Copy link
Collaborator

@cosmin cosmin commented Jun 28, 2023

Rebasing #1148 on top of current cmake branch

Related to #1047

@google-cla
Copy link

google-cla bot commented Jun 28, 2023

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@joeyparrish
Copy link
Member

@cosmin, could you fix the linter errors?

To run the linter against the changes since your local cmake branch, run:

python3 packager/tools/git/check_formatting.py cmake

To fix the same errors, run:

git clang-format --style Chromium cmake

For both of those, make sure your local cmake branch is up-to-date with the upstream cmake branch.

Thanks!

packager/media/base/aes_encryptor.cc Outdated Show resolved Hide resolved
packager/media/base/aes_encryptor.cc Show resolved Hide resolved
packager/media/base/aes_encryptor.cc Outdated Show resolved Hide resolved
packager/media/base/aes_encryptor.cc Outdated Show resolved Hide resolved
packager/media/crypto/encryption_handler.cc Outdated Show resolved Hide resolved
packager/media/crypto/encryption_handler.cc Outdated Show resolved Hide resolved
@joeyparrish joeyparrish added the status: waiting on response Waiting on a response from the reporter(s) of the issue label Jul 5, 2023
@joeyparrish joeyparrish removed the status: waiting on response Waiting on a response from the reporter(s) of the issue label Jul 7, 2023
@cosmin cosmin requested a review from joeyparrish July 11, 2023 10:53
packager/media/base/aes_encryptor.h Outdated Show resolved Hide resolved
packager/third_party/mbedtls/CMakeLists.txt Outdated Show resolved Hide resolved
@cosmin cosmin requested a review from joeyparrish July 12, 2023 19:37
@cosmin
Copy link
Collaborator Author

cosmin commented Jul 12, 2023

Now that I reverted the change with dealt with calling CryptInternal with an insufficiently large ciphertext size I'm seeing a test failure related to this. Taking a look at that.

@cosmin
Copy link
Collaborator Author

cosmin commented Jul 12, 2023

https://github.com/cosmin/shaka-packager/blob/75987351976cd4a84b24cf1590c77482aab9250a/packager/media/crypto/encryption_handler.cc#L302

ok, I think this is an example of a callsite that sets up the cipher text buffer to be the same size as the plaintext, but when using AesCbcEncryptor the required ciphertext size is larger due to padding. This then fails the check, which the earlier commit worked around by re-allocating a larger buffer.

@cosmin
Copy link
Collaborator Author

cosmin commented Jul 12, 2023

@joeyparrish take a look at the most recent commit, the only way I could think to workaround this issue is to introduce a function to return the expected ciphertext size so the caller can allocate a properly sized buffer for it. Open to suggestions though if there's a better way to solve this.

Copy link
Member

@joeyparrish joeyparrish left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Such an improvement, thanks!

packager/media/base/aes_encryptor.cc Show resolved Hide resolved
packager/media/crypto/encryption_handler.cc Outdated Show resolved Hide resolved
@joeyparrish
Copy link
Member

I have now fixed the HTTP tests, so I'm going to merge your branch with cmake and update the PR again. Then we should see a fairly straightforward test pass and know that the crypto tests enabled by this PR are passing.

@joeyparrish
Copy link
Member

Build failure on Windows:

packager\media\crypto\subsample_generator.cc(273,28): error C2220: the following warning is treated as an error
packager\media\crypto\subsample_generator.cc(273,28): warning C4457: declaration of 'frame' hides function parameter
packager\media\crypto\subsample_generator.cc(262,20): message : see declaration of 'frame'

That sounds like it could be a real bug.

@cosmin
Copy link
Collaborator Author

cosmin commented Jul 14, 2023

As far as I can tell this code has not changed recently. I don't think it's a bug in this case because by the time

  for (const VPxFrameInfo& frame : vpx_frames) {

happens which shadows the frame function parameter it was already parsed into vpx_frames by

  if (!vpx_parser_->Parse(frame, frame_size, &vpx_frames))

however it's trivial to fix while I'm at it.

@cosmin
Copy link
Collaborator Author

cosmin commented Jul 14, 2023

All tests seem to pass now except for Arch which should be fixed by #1233

@joeyparrish joeyparrish merged commit 86bf6cf into shaka-project:cmake Jul 14, 2023
30 of 31 checks passed
@github-actions github-actions bot added the status: archived Archived and locked; will not be updated label Sep 12, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 12, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
status: archived Archived and locked; will not be updated
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants