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

Use an updated fork with Opus support #896

Merged
merged 1 commit into from
Feb 7, 2024

Conversation

drizzt
Copy link

@drizzt drizzt commented Dec 21, 2023

This fork supports reading metadata from Opus files
Opus support backported from https://github.com/Kaned1as/jaudiotagger

Fixes: #780, #781

@soncaokim
Copy link
Collaborator

Hello,

Thanks for the submission. Have you been able to test with other file types (mp3 mp4 flac aac)?

@soncaokim
Copy link
Collaborator

soncaokim commented Dec 24, 2023

Hi,

I'm a bit reluctant to merge this.

The underlying library is a hard fork since Jan 2019 of the original jaudiotagger if I understand well, and it doesnt track/incorporate changes from the latter - notably those in 2020 and 2021 (MP3 improvements, modern Java support, etc).

I'm just browsing the change logs of the two repos, not looked at the code, so please correct me if I'm wrong.

@soncaokim
Copy link
Collaborator

@AdrienPoupa what's your opinion on this?

Fyi, I've been using this PR in my integration build since few days, with songs added/removed to the library. No notable regression on MP3, though my library is quite homogeneous on the tag format (ie not testing MP3 corner cases).

I will not block this if this brings notable improvement to OPUS, that I cannot assess since dont have OPUS files nor good knowledge on the format.

@soncaokim
Copy link
Collaborator

Note: If we merge this, there must be a note on the change log that invite users to rescan the library.

@drizzt
Copy link
Author

drizzt commented Jan 2, 2024

In the meanwhile, I'm trying to push that upstream: https://bitbucket.org/ijabz/jaudiotagger/pull-requests/71
If I don't have any reply or it's denied I could consider to do use mine as an updated fork

@drizzt drizzt changed the title Use https://github.com/Kaned1as/jaudiotagger fork Use an updated fork with Opus support Jan 2, 2024
@AdrienPoupa
Copy link
Member

When possible, I think it is better to use an official library but I understand it's not always feasible, hence why we used a fork ourselves to begin with. Getting modifications in upstream is optimal.

If it's not possible, I guess our second best option is to apply the patches on top of the current version rather than a several years old outdated fork.

Unfortunately it does not look like there's a lot of activity upstream so using a fork sounds like a sensible approach. Maybe we could host it in the VinylMusicPlayer organization?

@soncaokim
Copy link
Collaborator

Let's see what's the outcome of the PR on upstream jaudiotagger.

Thanks for the effort!

@drizzt
Copy link
Author

drizzt commented Jan 2, 2024

When possible, I think it is better to use an official library but I understand it's not always feasible, hence why we used a fork ourselves to begin with. Getting modifications in upstream is optimal.

If it's not possible, I guess our second best option is to apply the patches on top of the current version rather than a several years old outdated fork.

Unfortunately it does not look like there's a lot of activity upstream so using a fork sounds like a sensible approach. Maybe we could host it in the VinylMusicPlayer organization?

As you wish...

In the meanwhile, I implemented the remaining stuff into jaudiotagger in order to get format, bitrate and sample rate correctly on opus and I moved the tag:

photo_2024-01-02_20-40-05
photo_2024-01-02_20-40-06

@drizzt drizzt closed this Jan 2, 2024
@drizzt drizzt deleted the jaudiotagger-opus branch January 2, 2024 19:52
@drizzt drizzt restored the jaudiotagger-opus branch January 2, 2024 19:53
@drizzt
Copy link
Author

drizzt commented Jan 2, 2024

mmm I just renamed the branch...

@drizzt drizzt reopened this Jan 2, 2024
@soncaokim
Copy link
Collaborator

Still no activity upstream on the proposed PR.

Since @drizzt rebased his clone of jaudiotagger on top of the latest code (is this correct @drizzt ?), if there is any change upstream it would be easier to rebase/merge.

-> I'm in favor to give it a go.

Should we host the clone in the Vinyl organization (within github)?

@drizzt
Copy link
Author

drizzt commented Jan 24, 2024

yes, it's correct.
I currently have 2 branches:
https://bitbucket.org/drizztbsd/jaudiotagger/src/opus/ that is aligned to master
and
https://bitbucket.org/drizztbsd/jaudiotagger/src/stable-opus/ that is aligned to the last released version (aka tag)

@soncaokim
Copy link
Collaborator

Hi @drizzt and @AdrienPoupa ,

I'm going to clone the https://bitbucket.org/drizztbsd/jaudiotagger/src/opus/ repo inside the VinylMusicPlayer organization in Github (i.e along the vinyl repo). This way we can do proper release with tags and still track upstream jaudiotagger.

Are you okay with that? Anything that I might have overlooked?

S

@soncaokim
Copy link
Collaborator

The repo https://github.com/VinylMusicPlayer/jaudiotagger is created. @drizzt You should have write access to this, please tell me if this is working for you or if anything is missing.

Looks like the opus branch is not mirrored there yet (only master for now), I'm looking into that.

@soncaokim
Copy link
Collaborator

I've reimported the repo with all branches and tags

@AdrienPoupa
Copy link
Member

Perfect, thanks a lot!

app/build.gradle Outdated Show resolved Hide resolved
The fork is updated to 3.0.1 + Opus support backported from
https://github.com/Kaned1as/jaudiotagger
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Songs in Opus format have "unknown" genre
3 participants