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.

Sign up for GitHub

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

Jump to bottom

feat: Parse ID3 metadata #4409

Merged
merged 38 commits into from
Oct 11, 2022
Merged

feat: Parse ID3 metadata #4409

merged 38 commits into from
Oct 11, 2022

Conversation

avelad
Copy link
Member

@avelad avelad commented Aug 16, 2022

No description provided.

@avelad avelad marked this pull request as draft August 16, 2022 11:00
@avelad avelad added the type: enhancement New feature or request label Aug 16, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Aug 17, 2022 •
edited
Loading

Incremental code coverage: 92.63%

@avelad avelad added this to the v4.3 milestone Aug 17, 2022
joeyparrish
joeyparrish reviewed Aug 19, 2022
View reviewed changes
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.

Seems like a good start. The links to the ID3v2 spec were very helpful in understanding the parser.

lib/util/id3_utils.js Outdated Show resolved Hide resolved
lib/util/id3_utils.js Show resolved Hide resolved
lib/util/id3_utils.js Show resolved Hide resolved
lib/util/id3_utils.js Outdated Show resolved Hide resolved
lib/util/id3_utils.js Outdated Show resolved Hide resolved
lib/media/media_source_engine.js Outdated Show resolved Hide resolved
@avelad avelad marked this pull request as ready for review August 30, 2022 06:26
@avelad
Copy link
Member Author

avelad commented Aug 30, 2022

All changes have been tested with multiple streams (including DAI). I wait for a revision to add the tests and not waste time if there are big architectural changes (I hope not...)

joeyparrish
joeyparrish requested changes Oct 10, 2022
View reviewed changes
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.

Just one small nit in a test. Looks great!

test/test/util/id3_generator.js
const Id3Generator = shaka.test.Id3Generator;
const Uint8ArrayUtils = shaka.util.Uint8ArrayUtils;
const result = Uint8ArrayUtils.concat(
Id3Generator.stringToInts_(type),
Copy link
Member

Choose a reason for hiding this comment

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

I would like to see an assertion that type is 4 characters long. That would cause a test case to fail if there is a typo in the frame type when calling generateId3Frame.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done!

@joeyparrish joeyparrish merged commit 95bbf72 into shaka-project:main Oct 11, 2022
@github-actions github-actions bot mentioned this pull request Oct 11, 2022
Merged
@avelad
Copy link
Member Author

avelad commented Oct 11, 2022

Thanks @joeyparrish !

@avelad avelad deleted the id3 branch October 11, 2022 16:38
@joeyparrish joeyparrish mentioned this pull request Oct 31, 2022
Closed
@github-actions github-actions bot added the status: archived Archived and locked; will not be updated label Jul 25, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 25, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@joeyparrish joeyparrish joeyparrish approved these changes

@theodab theodab Awaiting requested review from theodab

@JulianDomingo JulianDomingo Awaiting requested review from JulianDomingo

@littlespex littlespex Awaiting requested review from littlespex

Assignees
No one assigned
Labels
priority: P1 Big impact or workaround impractical; resolve before feature release status: archived Archived and locked; will not be updated type: enhancement New feature or request
Projects
None yet
Milestone
v4.3
Development

Successfully merging this pull request may close these issues.
2 participants
@avelad @joeyparrish