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(sdk): Add support for MP3 without ID3 header #588

Closed
wants to merge 1 commit into from

Conversation

shc261392
Copy link
Contributor

Changes in this pull request

MP3 files does not necessarily have ID3 header and the current version will always raise "unsupported type" error when an file is MP3 format but has no ID3 header. Getting the ID3 header in MP3 file handler only serves the purpose of skipping the size of the header, hence when the MP3 file does not have an ID3 header, we could check for the MP3 header sync word (11 bits of 1) to make sure it is a valid MP3 file instead and skip 0 bytes as there is no ID3 header to skip.

This change would enable c2pa-rs to work with MP3 files that lack ID3 headers.

Checklist

  • [v] This PR represents a single feature, fix, or change.
  • [v] All applicable changes have been documented.
  • [v] Any TO DO items (or similar) have been entered as GitHub issues and the link to that issue has been included in a comment.

@mauricefisher64
Copy link
Collaborator

Looks good. If you bring the branch up to date with main I will merge.

Copy link
Collaborator

@mauricefisher64 mauricefisher64 left a comment

Choose a reason for hiding this comment

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

Thanks for the update

@mauricefisher64
Copy link
Collaborator

Can you please sign our contributor agreement:

https://github.com/contentauth/c2pa-rs/pull/588/checks?check_run_id=30903968016

@shc261392 shc261392 closed this Oct 2, 2024
@shc261392 shc261392 reopened this Oct 2, 2024
@shc261392
Copy link
Contributor Author

Can you please sign our contributor agreement:

https://github.com/contentauth/c2pa-rs/pull/588/checks?check_run_id=30903968016

@mauricefisher64 I've signed it and closed/reopened the pull request to trigger the check again.

@scouten-adobe scouten-adobe changed the title feat(sdk/src/asset_handlers/mp3_io.rs): add support for mp3 without ID3 header feat(sdk): Add support for MP3 without ID3 header Oct 7, 2024
@scouten-adobe
Copy link
Collaborator

@shc261392 would you please enable maintainers' contributions? This would allow us to update to base branch and merge when we have a moment. Otherwise, we'll keep asking you to update and hope that we notice at the right time. 😐

@shc261392
Copy link
Contributor Author

@shc261392 would you please enable maintainers' contributions? This would allow us to update to base branch and merge when we have a moment. Otherwise, we'll keep asking you to update and hope that we notice at the right time. 😐

Sadly seems the option is still not available for organization forks. Seems I need to manually keep it updated.

…D3 header

Signed-off-by: James Chien <james@numbersprotocol.io>
@shc261392
Copy link
Contributor Author

@mauricefisher64 Would you please check again to see if it could be merged?

@shc261392
Copy link
Contributor Author

@mauricefisher64 @scouten-adobe I've created a pull request using personal repo fork instead so the maintainer editing option is enabled. It should make things easier: #652

@shc261392
Copy link
Contributor Author

Close to switch to personal fork for maintaining editing option. #652

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants