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: port media/formats/webm to CMake #1147

Merged
merged 6 commits into from
Dec 17, 2022

Conversation

cadubentzen
Copy link
Contributor

@cadubentzen cadubentzen commented Nov 22, 2022

Port media/formats/webm to CMake.

  • Compiles
  • Tests compile and pass

Depends on #1143
Related #1047

@google-cla
Copy link

google-cla bot commented Nov 22, 2022

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

Will review in detail after #1143 lands. Thanks!

@cadubentzen cadubentzen force-pushed the cmake-media-formats-webm branch 4 times, most recently from 4079dd0 to 96a3cb4 Compare December 16, 2022 15:18
@cadubentzen cadubentzen force-pushed the cmake-media-formats-webm branch from 96a3cb4 to aded5be Compare December 16, 2022 15:21
@cadubentzen cadubentzen force-pushed the cmake-media-formats-webm branch from bc1fa14 to 756a99d Compare December 16, 2022 16:10
@joeyparrish
Copy link
Member

formats_webm.lib(mkv_writer.obj) : error LNK2019: unresolved external symbol "public: static __int64 __cdecl shaka::File::CopyFileA(class shaka::File *,class shaka::File *,__int64)" (?CopyFileA@File@shaka@@SA_JPEAV12@0_J@Z) referenced in function "public: __int64 __cdecl shaka::media::MkvWriter::WriteFromFile(class shaka::File *,__int64)" (?WriteFromFile@MkvWriter@media@shaka@@QEAA_JPEAVFile@3@_J@Z) [D:\a\shaka-packager\shaka-packager\build\packager\media\formats\webm\webm_unittest.vcxproj]

I'm guessing this is a Windows macro gone wrong. CopyFileA smacks of the days of Microsoft API variants for ASCII (A) and wide characters (W).

There's a macro in file/file.cc that deals with this, but it may need to be moved to the file.h header instead.

@cadubentzen cadubentzen force-pushed the cmake-media-formats-webm branch from 7dd5823 to 63ebdba Compare December 16, 2022 19:27
@cadubentzen cadubentzen marked this pull request as ready for review December 16, 2022 22:06
@cadubentzen
Copy link
Contributor Author

cadubentzen commented Dec 16, 2022

@joeyparrish CI is finally green, please take a look 🙂

@joeyparrish
Copy link
Member

Will do. Thanks!

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.

Looks like maybe one mistake. The rest looks good.

packager/media/formats/webm/webm_cluster_parser.cc Outdated Show resolved Hide resolved
@joeyparrish joeyparrish merged commit 56d3304 into shaka-project:cmake Dec 17, 2022
@cadubentzen cadubentzen deleted the cmake-media-formats-webm branch December 17, 2022 10:48
@github-actions github-actions bot added the status: archived Archived and locked; will not be updated label Jul 24, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 24, 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.

2 participants