Skip to content

Refactor/switch id3 functions with cml id3 functions#1

Open
fernandocQualabs wants to merge 50 commits into
masterfrom
refactor/switch-id3-functions-with-cml-id3-functions
Open

Refactor/switch id3 functions with cml id3 functions#1
fernandocQualabs wants to merge 50 commits into
masterfrom
refactor/switch-id3-functions-with-cml-id3-functions

Conversation

@fernandocQualabs

@fernandocQualabs fernandocQualabs commented Feb 1, 2024

Copy link
Copy Markdown

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

@fernandocQualabs
fernandocQualabs marked this pull request as draft February 1, 2024 17:43
Comment thread package.json Outdated
Comment thread src/demux/id3.ts Outdated
Comment thread tests/unit/demuxer/id3.ts
Signed-off-by: coatesjuan <juanco@qualabs.com>
@coatesjuan
coatesjuan self-requested a review February 27, 2024 19:35
Comment thread tests/unit/demuxer/id3.ts Outdated
Comment thread tests/unit/demuxer/id3.ts

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I wonder if we shouldn't just remove this file

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Agreed. Verify that utf8ArrayToStr isn't being used anywhere else in the code base, and if not, remove this file.

@fpyoung fpyoung Feb 29, 2024

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Okay turns out this function is being used in:

  • parseWebVTT function in webvtt-parser.ts
  • parseSEIMessageFromNALu function in mp4-tools.ts
  • parseIMSC1 function in imsc1-ttml-parser.ts

Signed-off-by: Felipe Young <felipey@qualabs.com>
@fpyoung
fpyoung marked this pull request as ready for review February 29, 2024 19:42
Comment thread src/demux/id3.ts Outdated

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Remove this file

Comment thread tests/unit/demuxer/id3.ts

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Agreed. Verify that utf8ArrayToStr isn't being used anywhere else in the code base, and if not, remove this file.

Felipe Young and others added 2 commits February 29, 2024 17:00
Signed-off-by: Felipe Young <felipey@qualabs.com>
Signed-off-by: hernan <hernanr@qualabs.com>
@fpyoung
fpyoung marked this pull request as draft March 1, 2024 20:13
Signed-off-by: Felipe Young <felipey@qualabs.com>
@fpyoung
fpyoung marked this pull request as ready for review March 4, 2024 20:19
fpyoung and others added 5 commits March 4, 2024 17:31
Signed-off-by: Fernando Cuadro <fernandoc@qualabs.com>
Signed-off-by: Fernando Cuadro <fernandoc@qualabs.com>
Signed-off-by: hernan <hernanr@qualabs.com>
Signed-off-by: hernan <hernanr@qualabs.com>
renovate Bot and others added 30 commits March 18, 2024 19:01
…r-122.x

chore(deps): update dependency chromedriver to v122.0.5
…r-122.x

chore(deps): update dependency chromedriver to v122.0.6
fix after rebase
…pi-documenter-7.x

chore(deps): update dependency @microsoft/api-documenter to v7.23.38
…pi-documenter-7.x

chore(deps): update dependency @microsoft/api-documenter to v7.24.1
…eslint-monorepo

chore(deps): update typescript-eslint monorepo to v7.3.0
…eslint-monorepo

chore(deps): update typescript-eslint monorepo to v7.3.1
chore(deps): update babel monorepo to v7.24.1
chore(deps): update babel monorepo to v7.24.3
chore(deps): update dependency @types/chai to v4.3.13
chore(deps): update dependency @types/chai to v4.3.14
…pi-extractor-7.x

chore(deps): update dependency @microsoft/api-extractor to v7.43.0
chore(deps): update dependency wrangler to v3.35.0
chore(deps): update dependency wrangler to v3.38.0
chore(deps): update dependency typescript to v5.4.3
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.

7 participants