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

Refactor/switch id3 functions with cml id3 functions #6260

Conversation

felipeYoungi
Copy link
Contributor

This PR will...

What improvements to ID3 Parsing will this change include?

  • Removed the methods for id3 parsing,decoding and getting frames that were in hls.js(See the PR for further details) and replaced them with the same methods from CML.
  • Added new unit tests in CML(these functions weren’t being tested before) for:
    • readId3Size
    • isId3Header
    • isId3Footer
    • decodeId3Frame
    • Added APIC Id3 for hls.js
    • Added Shaka’s id3 unit tests to CML.

Why is this Pull Request needed?

The id3 utilities have been manually implemented in multiple player libraries. When bugs need to be fixed, or features added, then need to be done across all of these projects. The common media library provides a single place for these utilities to live and be maintained.
Also hls.js doesn't parse id3 APIC data and with this change it is going to be able to handle that data.

What assets have these changes been tested against?

We tested this changes using 2 assets:
https://devstreaming-cdn.apple.com/videos/streaming/examples/bipbop_16x9/bipbop_16x9_variant.m3u8
https://test-streams.mux.dev/dai-discontinuity-deltatre/manifest.m3u8
https://stream-cdn-1.open.fm/OFM18/ngrp:standard/playlist.m3u8(APIC between songs).

What history and roadmap for the CML can you share(based on which player code and tested for which application use cases)?

We currently use Github milestones (same as hls.js), in combination with the project board. We only have a few features officially planned, so the roadmap isn't very big:
https://github.com/streaming-video-technology-alliance/common-media-library/milestones

Checklist

  • changes have been done against master branch, and PR does not conflict
  • new unit / functional tests have been added

robwalch
robwalch previously approved these changes Mar 4, 2024
src/utils/utf8-utils.ts Outdated Show resolved Hide resolved
package-lock.json Outdated Show resolved Hide resolved
@robwalch robwalch added this to the 1.6.0 milestone Mar 4, 2024
@robwalch robwalch self-requested a review March 4, 2024 22:26
@robwalch robwalch dismissed their stale review March 4, 2024 22:27

Would like to see de-duplication of utils and payload size before and after.

Signed-off-by: Fernando Cuadro <fernandoc@qualabs.com>
Signed-off-by: Fernando Cuadro <fernandoc@qualabs.com>
package.json Outdated Show resolved Hide resolved
src/demux/audio/aacdemuxer.ts Outdated Show resolved Hide resolved
src/utils/utf8-utils.ts Outdated Show resolved Hide resolved
Signed-off-by: hernan <hernanr@qualabs.com>
Signed-off-by: hernan <hernanr@qualabs.com>
@felipeYoungi felipeYoungi requested a review from robwalch March 14, 2024 15:16
@robwalch robwalch merged commit f54b252 into video-dev:master Apr 4, 2024
12 checks passed
@robwalch robwalch linked an issue Apr 18, 2024 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Development

Successfully merging this pull request may close these issues.

Integrate SVTA Common Media Library's ID3 utilities
4 participants