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

Support Opus codec #67

Merged
merged 1 commit into from
Feb 28, 2024
Merged

Support Opus codec #67

merged 1 commit into from
Feb 28, 2024

Conversation

rubdos
Copy link
Contributor

@rubdos rubdos commented Feb 24, 2024

(untested)

Compiles, seems to index the files, will test a bit more extensively on the road!

It plays! I'll have to test whether gapless works too.

@Olf0 Olf0 changed the title Try and implement Opus Implement Opus codec Feb 24, 2024
@rubdos
Copy link
Contributor Author

rubdos commented Feb 24, 2024

It plays! I'll have to test whether gapless works too.

Looks like this works quite well, have been listening to Tool and Apocalyptica for a few hours now.

@rubdos
Copy link
Contributor Author

rubdos commented Feb 24, 2024

Looks like this works quite well, have been listening to Tool and Apocalyptica for a few hours now.

It skipped from 'Parabol' to 'Ticks & Leeches', that was very weird. Not sure whether that's only Opus, or whether that'd happen with other formats too. Any reports of that happening before in gapless mode?

I am so looking forward to finally have gapless :D

@rubdos
Copy link
Contributor Author

rubdos commented Feb 25, 2024

I'm starting to think that the "about-to-finish" signal gets fired once too often.

g_signal_connect(pipeline, "about-to-finish",
        G_CALLBACK(prepare_next_stream), this);

The bug shows up to me when I'm playing Opus files from start to end; when I just scroll to the end of the song, the gapless transition is perfect, but when I listen to the whole song, it skips the next one.

@Olf0
Copy link
Contributor

Olf0 commented Feb 26, 2024

when I listen to the whole [Opus encoded] song, it skips the next one.

Any reports of that happening before in gapless mode?

"Not that I am aware of", but this statement is mostly equivalent to "I have no idea". Thus, please try other formats, e.g. classic .mp3.

@rubdos
Copy link
Contributor Author

rubdos commented Feb 26, 2024

Thus, please try other formats, e.g. classic .mp3.

Uuuh, then I need to get my hands dirty and get some of those albums in MP3 for testing purposes. Will do.

@Olf0
Copy link
Contributor

Olf0 commented Feb 26, 2024

Uuuh, then I need to get my hands dirty …

Sorry, but occasionally this is a natural consequence of being a purist. 😉

@rubdos
Copy link
Contributor Author

rubdos commented Feb 26, 2024

Good that I kept the source FLAC files :-)

@rubdos
Copy link
Contributor Author

rubdos commented Feb 26, 2024

Ugh I can't distinguish which ones are Opus and which ones are FLAC 🤣

Test in progress, feedback in 6 minutes. Those Tool songs are looooong.

@rubdos
Copy link
Contributor Author

rubdos commented Feb 26, 2024

Test in progress, feedback in 6 minutes. Those Tool songs are looooong.

FLAC worked :-/

@Olf0
Copy link
Contributor

Olf0 commented Feb 26, 2024

FLAC worked :-/

I was suggesting to also test .mp3, if you have access to some album encoded as .mp3, because that is what most people are still using.
(Superfluous side note: Fraunhofer's patents expired long ago, the drawback which remains is that it is > 30 years old technology. Hence IMO it is not that bad, only a bit space consuming for decent quality relative to modern codecs.)

If it really looks like only OPUS is affected by the double "about-to-finish" signal, this is unfortunate for you personally, but would not affect many. Consequently I would gladly review and merge this, because it makes OPUS encoded audio playable by FlowPlayer for the first time.

Then you can take your time to file a bug report to whatever component you deem to be responsible for this mishap.

P.S.: And whatever causes the double "about-to-finish" signal, it is completely unrelated to this changeset, any way.

@rubdos
Copy link
Contributor Author

rubdos commented Feb 27, 2024

I was suggesting to also test .mp3, if you have access to some album encoded as .mp3, because that is what most people are still using.

I can definitely still do that, if you think it's still relevant!

(Superfluous side note: Fraunhofer's patents expired long ago, the drawback which remains is that it is > 30 years old technology. Hence IMO it is not that bad, only a bit space consuming for decent quality relative to modern codecs.)

Another problem would be that I have to re-transcode about 1TB of FLAC files into MP3, which would more than double the occupied size on my laptop and SD-card, giving in to a slight quality loss (arguably imperceptible to me with modern Lame). This kind of reasoning is kinda chicken-and-egg: if players don't support Opus, there will never be an incentive to move on. But you're definitely right in principle!

Don't take that as a personal attack though. Just stating my situation, and my opinion on the matter.

If it really looks like only OPUS is affected by the double "about-to-finish" signal, this is unfortunate for you personally, but would not affect many. Consequently I would gladly review and merge this, because it makes OPUS encoded audio playable by FlowPlayer for the first time.

Yeh, I think that's fine by me!

Then you can take your time to file a bug report to whatever component you deem to be responsible for this mishap.

Do you have any idea what it could be? The "about-to-finish" signal seems to be rather the default for implementing gapless through gstreamer, so I imagine this issue exists elsewhere too.

P.S.: And whatever causes the double "about-to-finish" signal, it is completely unrelated to this changeset, any way.

I'd think so too...

@Olf0
Copy link
Contributor

Olf0 commented Feb 27, 2024

I was suggesting to also test .mp3, if you have access to some album encoded as .mp3, because that is what most people are still using.

I can definitely still do that, if you think it's still relevant!

I would appreciate that, because I think it is potentially relevant for most FlowPlayer users, but unfortunately many do not mind or are used to playing albums with gaps between songs (one cannot miss something which one is not aware of). Note that I do not use FlowPlayer (I do not listen to music when on the move), I only inherited it from CepiPerez when I offered to maintain FileCase; later I decided to apply the infrastructure changes I made for FileCase also to FlowPlayer, because I thought people may be interested in it: It turned out to cause greater resonance than FileCase to my surprise.

Another problem would be that I have to re-transcode about 1TB of FLAC files into MP3, …

No, IMO you only have to transcode two consecutive songs or a single album, and only temporarily, as you can delete the transcoded MP3-files after the test. 😜
I am quite sure that is not what you meant to address, but it was easy to misread it in this way.

Then you can take your time to file a bug report to whatever component you deem to be responsible for this mishap.

Do you have any idea what it could be?

No, "audio on Linux" always has been and appears to continue to be a hell: I stopped to care about the technical details after ~ 10 years, i.e. before pulseaudio was implemented. Since then, either my Linux distributor manages to make audio work or some things simply don't: I care about too many things, in some areas I adopted the "some update may fix it (or not)"-strategy of proprietary software users.

The "about-to-finish" signal seems to be rather the default for implementing gapless through gstreamer, so I imagine this issue exists elsewhere too.

Likely: Most problems occur for many and often somebody has a solution. The issue is to discover it, if it was publicly documented at all.

P.S.: Is this PR ready to be reviewed and merged from your point of view, or do you intend to keep working on it or review it by yourself, first?

@rubdos
Copy link
Contributor Author

rubdos commented Feb 28, 2024

No, IMO you only have to transcode two consecutive songs or a single album, and only temporarily, as you can delete the transcoded MP3-files after the test. 😜 I am quite sure that is not what you meant to address, but it was easy to misread it in this way.

lol. I'll have a listen to an MP3 V0 transcode of 10000 days, and make a bug report after that :-)

I now also wonder whether the Opus playback skips with gapless disabled. Never tested that, actually. Maybe I'm blaming the wrong part of the code.

No, "audio on Linux" always has been and appears to continue to be a hell: I stopped to care about the technical details after ~ 10 years, i.e. before pulseaudio was implemented. Since then, either my Linux distributor manages to make audio work or some things simply don't: I care about too many things, in some areas I adopted the "some update may fix it (or not)"-strategy of proprietary software users.

Completely understand that :)

P.S.: Is this PR ready to be reviewed and merged from your point of view, or do you intend to keep working on it or review it by yourself, first?

No I think it can be merged, at the mercy of your review now!

@rubdos
Copy link
Contributor Author

rubdos commented Feb 28, 2024

lol. I'll have a listen to an MP3 V0 transcode of 10000 days, and make a bug report after that :-)

MP3 played flawlessly :)

@Olf0
Copy link
Contributor

Olf0 commented Feb 28, 2024

I now also wonder whether the Opus playback skips with gapless disabled. Never tested that, actually. Maybe I'm blaming the wrong part of the code.

Well, does it?

MP3 played flawlessly :)

Thank you! I was afraid something was broken by the recent (2023 / 2024) changes.

No I think it can be merged, at the mercy of your review now!

It is always "fun" for me to review code in languages of which I just know the basics, hence usually failing to get the "big picture". But often (e.g. here) I understand the changes made and they are looking good. And I am quite sure that no extant functionality may become broken.

Copy link
Contributor

@Olf0 Olf0 left a comment

Choose a reason for hiding this comment

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

LGTM

@Olf0 Olf0 merged commit 60f4446 into sailfishos-applications:devel Feb 28, 2024
1 check passed
@Olf0 Olf0 added the enhancement New feature or request label Feb 28, 2024
@Olf0 Olf0 changed the title Implement Opus codec Support Opus codec Feb 28, 2024
@rubdos rubdos deleted the opus branch March 3, 2024 19:26
@Olf0
Copy link
Contributor

Olf0 commented Mar 4, 2024

I now also wonder whether the Opus playback skips with gapless disabled. Never tested that, actually. Maybe I'm blaming the wrong part of the code.

Well, does it?

Still curious … if you find the time.

@rubdos
Copy link
Contributor Author

rubdos commented Mar 4, 2024

I now also wonder whether the Opus playback skips with gapless disabled. Never tested that, actually. Maybe I'm blaming the wrong part of the code.

Well, does it?

Still curious … if you find the time.

I'm not closing this tab before I figure it out. Latest finding was that gapless-disabled didn't play the next song at all in my car... but I hadn't used Bluetooth in my car with flowplayer yet at all!

@rubdos
Copy link
Contributor Author

rubdos commented Mar 5, 2024

I hadn't used Bluetooth in my car with flowplayer yet at all!

Turns out I just had some spurious unrelated process playing with MPRIS, so playing gapless-disabled in the car was fine on the last trip!

@Olf0
Copy link
Contributor

Olf0 commented Mar 5, 2024

Thank you. From the pieces of information you evaluated, I created issue #74 "FlowPlayer skips Opus-encoded audio tracks when gap-less playback is enabled", so users can find this issue documented when observing it.

Hence I only captured the user-facing aspects: If I captured any aspect incorrectly or missed an aspect, please denote that in a comment there.

Olf0 added a commit that referenced this pull request Mar 14, 2024
* Post-release version increase (#71)

* [flowplayer.spec] Post-release version increase

* [flowplayer.changes] Add stub for v0.3.4

* [flowplayer.changes] Add closure of issue #63 by #65

* Try and implement Opus (#67)

* Add Ruben de Smet (rubdos) as contributor (#73)

* [flowplayer.spec] Add Ruben de Smet (rubdos)

* [README.md] Add Ruben de Smet (rubdos)

* [AboutPage.qml] Add Ruben de Smet (rubdos)

* [LICENSE.txt] Add Ruben de Smet (rubdos)

* Add Mark Washeim (poetaster) to contributors (#81)

* [README.md] Add Mark Washeim (poetaster) to contributors

* [AboutPage.qml] Add Mark Washeim (poetaster) to contributors

* [LICENSE.txt] Add Mark Washeim (poetaster) to contributors

* [flowplayer.spec] Add poetaster to developers list

* [flowplayer.changes] Update for release of v0.3.4

* [Feature] Add local cover art when importing tracks (#75)

* [Feature] add initial addition for cover art if a jpg is found in the directory as tracks are added to the db. also, correct old Generic paths, and test with sailjail.

* PR: feedback integrated: 1. alternate logic for applying folder/cover image copy, basename check. 2. remove media indexing sailjail perms, 3. move cache dir creation from migrate to main

* [FlowPlayer.cpp] Improve style as suggested by @dcaliste

* [datareader.cpp] Improve style as suggested by @dcaliste

* [flowplayer.desktop] Omit SailJail sandboxing configuration for now

* [datareader.cpp] Improve style as suggested by @dcaliste

* Review: remove png processing

* Update datareader.cpp
  remove unused QImage and png matching.

* Fix:logic of the || on png

* [datareader.cpp] Indention, no `jpg` & `png` for now, iterator reuse
  Addresses comments dispersed over PR #75's lengthy discussion.

* [datareader.cpp] Rectify comment

* [datareader.cpp] Improve comments & break long code line in two

* [datareader.cpp] Break two more long lines in two

* [datareader.cpp] Remove superfluous backslashes "\"

* [datareader.cpp] Omit superfluous space character " "

* [datareader.cpp] Extend comment

* Don't recurse in subdirs when looking for covers.

* [datareader.cpp] Insert comment and align with current TS files

---------

Co-authored-by: olf <[email protected]>
Co-authored-by: Damien Caliste <[email protected]>

* Translate translations/flowplayer.ts in de (#82)

100% translated source file: 'translations/flowplayer.ts'
on 'de'.

Co-authored-by: transifex-integration[bot] <43880903+transifex-integration[bot]@users.noreply.github.com>

* Translate translations/flowplayer.ts in sv (#83)

100% translated source file: 'translations/flowplayer.ts'
on 'sv'.

Co-authored-by: transifex-integration[bot] <43880903+transifex-integration[bot]@users.noreply.github.com>

---------

Co-authored-by: Ruben De Smet <[email protected]>
Co-authored-by: Mark Washeim <[email protected]>
Co-authored-by: Damien Caliste <[email protected]>
Co-authored-by: transifex-integration[bot] <43880903+transifex-integration[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants