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

scanner/ffmpeg: change to use ffprobe #1326

Closed
wants to merge 2 commits into from

Conversation

vs49688
Copy link
Contributor

@vs49688 vs49688 commented Sep 5, 2021

ffprobe can output JSON and should be shipped anywhere ffmpeg is.

Handling JSON is much nicer than unstructured text.

Note that I removed some tests that no longer make sense after the change, e.g. the TBPM, FBPM, and multi-line comment tests.

@deluan
Copy link
Member

deluan commented Sep 6, 2021

Thanks for this, I do agree that ffprobe is a better metadata extractor, and actually I originally implemented it with ffprobe (that's why the config option is called ProbeCommand) but unfortunately you missed one important feature of ffmpeg vs ffprobe: Batching! Please read this: #1294 (comment)

@vs49688
Copy link
Contributor Author

vs49688 commented Sep 6, 2021

Ah, I completely missed that thread. How about linking with libav{format,codec} and pulling out the data directly?

Could also get rid of the need for an external compressor that way too...

@deluan
Copy link
Member

deluan commented Sep 6, 2021

Ah, I completely missed that thread. How about linking with libav{format,codec} and pulling out the data directly?

Linking to C libraries make the project very complex to maintain (see explanation here: #1124 (comment)). SQLite3 and (specially) TagLib already give me enough headaches... Check the docker image I have to maintain to be able to build Navidrome statically

Could also get rid of the need for an external compressor that way too...

This goes against the architecture decision of allowing external transcoders to be used to support formats unknown to Navidrome (see #351 (comment))

Can I ask you what is your motivation to get rid of ffmpeg? Is it causing any issues?

@vs49688
Copy link
Contributor Author

vs49688 commented Sep 6, 2021

Ah, I completely missed that thread. How about linking with libav{format,codec} and pulling out the data directly?

Linking to C libraries make the project very complex to maintain (see explanation here: #1124 (comment)). SQLite3 and (specially) TagLib already give me enough headaches... Check the docker image I have to maintain to be able to build Navidrome statically

Could also get rid of the need for an external compressor that way too...

This goes against the architecture decision of allowing external transcoders to be used to support formats unknown to Navidrome (see #351 (comment))

Fair enough, I wasn't aware that was intended.

Can I ask you what is your motivation to get rid of ffmpeg? Is it causing any issues?

No, not at all. I wanted to start off with something relatively simple to get familiar with the codebase, before I tackle #489.

@deluan
Copy link
Member

deluan commented Sep 7, 2021

No, not at all. I wanted to start off with something relatively simple to get familiar with the codebase, before I tackle #489.

Ah, I see. Your code looks good to me but unfortunately, for the reasons above, I'll not be able to merge it, ok? Closing this now, but feel free to reach out (preferable in Discord) to discuss #489

@deluan deluan closed this Sep 7, 2021
@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 13, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants