Replace tone with ffmpeg for metadata and cover embedding

[mikiher]

This is an early review for tone -> ffmpeg replacement.

• No UI changes yet
• Only m4b embedding has been tested (mp3 embedding will likely only add a couple of ffmpeg flags)

Please let me know if you have any comments up until now, and please don't merge yet.

[mikiher]

Question: do we merge existing metadata with new one, or do we override existing metadata?
tone seems to merge.

Clarification:
"merge" means - tags that were not pupulated by abs remain unchanged.
'override" means - tags that were not pupulated by abs are removed.

[advplyr]

Can you give a specific example? I am interpreting this in a few different ways and not sure which one you are thinking of.

[mikiher]

ok, suppose the original audio file had metadata tags a=1, b=2, and c=3, and abs has tags b=4, c=5, e=6.

then after embedding:

• in "merge" the audio file has a=1, b=4, c=5, e=6
• in "override" the audio file has b=4, c=5, e=6 (and no a=1)

[mikiher]

In my second commit, I implemented "merge"

[mikiher]

Another thing I noted: tone does not explicitly write track (so if it existed, it remains the same, and if it did not exist, it is not populated). Should we fix this?

[advplyr]

ok, suppose the original audio file had metadata tags a=1, b=2, and c=3, and abs has tags b=4, c=5, e=6.

then after embedding:

• in "merge" the audio file has a=1, b=4, c=5, e=6
• in "override" the audio file has b=4, c=5, e=6 (and no a=1)

In this case I think it makes sense to leave a=1 because the user may define additional meta tags on their own separate from the ones Abs uses.

The trickier case would be if a is a meta tag Abs uses but it is empty in Abs.
For example, if the book does not have a series in Abs but the audio file does have data in the meta tag that Abs uses for series. We could remove that tag from the audio file or leave it there.

[mikiher]

The trickier case would be if a is a meta tag Abs uses but it is empty in Abs.
For example, if the book does not have a series in Abs but the audio file does have data in the meta tag that Abs uses for series. We could remove that tag from the audio file or leave it there.

I put code that removes any empty/null values from the ffmetadata object, so right now the behavior is that if the series tag is empty in abs and exists in the original file, the original will be kept.

[mikiher]

OK, I think the code is ready for review now (and thanks for implementing track embedding).

I haven't cleaned up Tone from the code yet. I'll do that once we've ran the ffmpeg-based embedding in prod for a few weeks.

[mikiher]

I also added some unit tests for the new functions in ffmpegHelpers.

[advplyr]

This was a long time coming. Thanks!

[mikiher]

Yes, third party, and I wasn't sure if the iOS and Android clients used that API endpoint. I try to be extra-cautious about breaking API - perhaps it's OK in this case.

…

On Sun, Jul 7, 2024 at 12:52 AM advplyr ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In server/routers/ApiRouter.js <#3111 (comment)> : > @@ -114,7 +114,7 @@ class ApiRouter { this.router.post('/items/:id/play/:episodeId', LibraryItemController.middleware.bind(this), LibraryItemController.startEpisodePlaybackSession.bind(this)) this.router.patch('/items/:id/tracks', LibraryItemController.middleware.bind(this), LibraryItemController.updateTracks.bind(this)) this.router.post('/items/:id/scan', LibraryItemController.middleware.bind(this), LibraryItemController.scan.bind(this)) - this.router.get('/items/:id/tone-object', LibraryItemController.middleware.bind(this), LibraryItemController.getToneMetadataObject.bind(this)) + this.router.get('/items/:id/metadata-object', LibraryItemController.middleware.bind(this), LibraryItemController.getMetadataObject.bind(this)) Right now the only place that uses that endpoint is the web client so it won't break anything for Abs. That endpoint is documented so it is possible someone built a third party tool using it. Is that what you mean? — Reply to this email directly, view it on GitHub <#3111 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AFMDFVW76GRYPR6G6GOZHF3ZLBRLPAVCNFSM6AAAAABKDK5RDOVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDCNRRGYZDOOBUGY> . You are receiving this because you authored the thread.Message ID: ***@***.***>

[advplyr]

Yeah it makes sense to be cautious, I appreciate that. There was another similar endpoint tone-scan that is still in the docs that I removed a while ago so the docs need to be updated. With the new API docs that @nichwall has been working on we can do better at managing the API in general