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

Make Extractor/Mp4 Atom Parsing code reusable by external custom mp4 … #1607

Merged
merged 6 commits into from
Aug 20, 2024

Conversation

colinkho
Copy link
Contributor

@colinkho colinkho commented Aug 9, 2024

…extractors

@icbaker icbaker self-assigned this Aug 12, 2024
@icbaker icbaker self-requested a review August 12, 2024 08:15
@icbaker
Copy link
Collaborator

icbaker commented Aug 12, 2024

Thanks for sending this. I'm going to submit some of this myself directly internally:

  1. Move a couple of parsing methods from Atom to AtomParsers
  2. Move Atom to lib-container and make it public (so it can in future be used by muxer)

Once these changes are submitted I'll let you know and you can rebase this change so it only has the AtomParsers, Sniffer and TrackSampleTable changes.

@icbaker
Copy link
Collaborator

icbaker commented Aug 15, 2024

The changes have been submitted:

The final change renamed Atom to Mp4Box for two reasons:

  1. The 'box' terminology more closely matches the ISO 14496-12 spec.
  2. Other MP4-specific files in androidx.media3.container are prefixed with Mp4 instead of being in an mp4 sub-package.

I suggest we rename AtomParsers to BoxParsers and keep it in the extractor.mp4 package. We don't need to also touch every call-site, because it makes the blast radius of the change too large.

I don't mind if you do that in this PR, or we can do it directly as an internal change.

@colinkho
Copy link
Contributor Author

colinkho commented Aug 15, 2024

@icbaker Rebased to capture your commits and renamed to BoxParsers. Thank you!

edit: There was one commit from the pull that didn't pass CLA checks for some reason. Hence, I've rebased my commits to the top and force pushed for simplicity

@icbaker
Copy link
Collaborator

icbaker commented Aug 16, 2024

I'm going to send this for internal review now. You may see some more commits being added as I make changes in response to review feedback. Please refrain from pushing any more substantive changes as it will complicate the internal review - thanks!

@copybara-service copybara-service bot merged commit 643ec73 into androidx:main Aug 20, 2024
1 check passed
@androidx androidx locked and limited conversation to collaborators Oct 20, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants