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

Enhance AudioParser #414

Merged
merged 8 commits into from
May 21, 2024
Merged

Enhance AudioParser #414

merged 8 commits into from
May 21, 2024

Conversation

domkm
Copy link
Contributor

@domkm domkm commented Apr 18, 2024

This adds common metadata and audio properties to Metadata and reading order Links generated by AudioParser.

I didn't see how to add the cover image here, though. Is it possible to do so?

@domkm
Copy link
Contributor Author

domkm commented Apr 18, 2024

All tests pass locally. How do I reproduce the CI failure?

@mickael-menu
Copy link
Member

Hi @domkm, you can ignore the Carthage failure, it only runs properly on a local branch.

@domkm
Copy link
Contributor Author

domkm commented Apr 25, 2024

@mickael-menu Any thoughts on this PR? Could you share how to attach a cover image in the parser?

Copy link
Member

@mickael-menu mickael-menu left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution @domkm, it's much appreciated!

Regarding the cover, you can implement a CoverService or use one of the existing implementations (GeneratedCoverService) and pass it to the ServicesBuilder returned by the AudioParser. There's an example in the PDFParser:

cover: document.cover.map(GeneratedCoverService.makeFactory(cover:)),

Sources/Streamer/Parser/Audio/AudioParser.swift Outdated Show resolved Hide resolved
@domkm
Copy link
Contributor Author

domkm commented May 16, 2024

Hi @mickael-menu,

Sorry for the delay. I attempted to implement the solution as outlined, ran into a few stumbling blocks, and had a few realizations:

  • Knowledge of readingOrder is necessary to form publication metadata, so passing all of the links and the fetcher is insufficient.
  • Separating publication metadata retrieval from resource metadata retrieval is inefficient because the former requires the latter, so it's better for us to cache the results of the latter. Otherwise, we need to fetch metadata for each readingOrder link twice.
  • Focusing on Manifest metadata and readingOrder exclusively precludes the possibility of easily enhancing other useful properties in the future, like tableOfContents.

Given this, I took a slightly different approach, where the configuration class processes the default manifest in its entirety, instead of processing individual fields. I’m keen to hear your thoughts on this approach. Thanks.

@mickael-menu
Copy link
Member

Thanks for updating the PR @domkm. Doing the metadata computation in one go is fine by me, if that's necessary. 👌

In the meantime, the first 3.0.0-alpha.1 was released and develop now supports iOS 13+. Do you want to revisit the API using async/await before merging?

@domkm
Copy link
Contributor Author

domkm commented May 17, 2024

I believe the relevant async methods load and loadMetadata require iOS 15.

How do you determine what versions to support? My understanding is that iOS <15 represents a small single-digit percent of worldwide usage, at this point.

@mickael-menu
Copy link
Member

We ask the various stakeholders what minimum version they are supporting and unfortunately it's still iOS 13 for now.

@domkm
Copy link
Contributor Author

domkm commented May 20, 2024

I noticed an issue with bitrates so I'm converting this to a draft until it's fixed.

@domkm
Copy link
Contributor Author

domkm commented May 20, 2024

I'm not sure how to correctly extract or calculate bitrate, so I'm dropping that functionality for now.

@domkm domkm marked this pull request as ready for review May 20, 2024 01:10
Copy link
Member

@mickael-menu mickael-menu left a comment

Choose a reason for hiding this comment

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

Thank you @domkm 🙏

@mickael-menu mickael-menu merged commit 57d81ec into readium:develop May 21, 2024
5 of 6 checks passed
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.

None yet

2 participants