Enhance `AudioParser` - Githubissues

readium / swift-toolkit

A toolkit for ebooks, audiobooks and comics written in Swift

https://readium.org/mobile/

BSD 3-Clause "New" or "Revised" License

222 stars 96 forks source link

Enhance `AudioParser` #414

Closed domkm closed 1 month ago

domkm commented 2 months ago

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 commented 2 months ago

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

mickael-menu commented 2 months ago

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

domkm commented 2 months ago

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

domkm commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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

domkm commented 1 month ago

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

domkm commented 1 month ago

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