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

FEAT(client, ui): Client-side JACK transport addon to multichannel recording feature #6043

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

IsaMorphic
Copy link

Solves #4112. I have chosen client-side support for the feature over server-side support due to the concerns over privacy and E2E encryption. See #3530 for relevant discussion.

Checks

@davidebeatrici
Copy link
Member

I didn't take a good look at the code yet, but it looks solid to me!

Keep in mind that we could implement the same mechanism for PipeWire as well.

src/mumble/JackAudio.cpp Fixed Show resolved Hide resolved
@IsaMorphic
Copy link
Author

@davidebeatrici sounds good! It should be easy to implement a similar override for prepareOutputBuffers like I did for JACK. Please keep in mind that the PR is not quite complete, I still need to fix a couple of bugs and hook everything up to the recorder interface.

Thanks for the feedback!

@IsaMorphic
Copy link
Author

IsaMorphic commented Jan 26, 2023

There we go! Builds on all platforms and is pretty much polished; ready for review 😃
Edit: had to amend the last commit message to conform to the guidelines.

@IsaMorphic IsaMorphic marked this pull request as ready for review January 26, 2023 00:48
IsaMorphic added a commit to IsaMorphic/mumble-morphic that referenced this pull request Jan 26, 2023
@Hartmnt
Copy link
Member

Hartmnt commented Jan 26, 2023

had to amend the last commit message to conform to the guidelines

Since all the commits currently in your branch are really all implementing the same feature, you should squash them all down into a single commit. In the end, when your feature is reviewed, you should then add a translation commit by running the translation script in the Mumble repo.

@IsaMorphic
Copy link
Author

@Hartmnt can do. I'm busy the rest of the day so I'll have to look into this tomorrow.

@Hartmnt
Copy link
Member

Hartmnt commented Jan 26, 2023

@Hartmnt can do. I'm busy the rest of the day so I'll have to look into this tomorrow.

No hurry, I imagine there will be a few rounds of reviewing and updating going on here :3

@IsaMorphic IsaMorphic requested review from Hartmnt and removed request for davidebeatrici January 27, 2023 19:15
@IsaMorphic IsaMorphic changed the title FEAT(client): Client-side JACK transport addon to multichannel recording feature FEAT(client, ui): Client-side JACK transport addon to multichannel recording feature Jan 27, 2023
@IsaMorphic
Copy link
Author

I did re-request a review but I wanted to bump this just in case!

@Krzmbrzl
Copy link
Member

Krzmbrzl commented Feb 2, 2023

This is still on my TODO list but I'm still busy with other stuff. Haven't forgotten about this though :)

src/mumble/AudioOutput.cpp Outdated Show resolved Hide resolved
src/mumble/AudioOutput.cpp Outdated Show resolved Hide resolved
src/mumble/AudioOutput.cpp Show resolved Hide resolved
src/mumble/EnumStringConversions.cpp Outdated Show resolved Hide resolved
src/mumble/JackAudio.cpp Show resolved Hide resolved
src/mumble/JackAudio.h Outdated Show resolved Hide resolved
src/mumble/JackAudio.h Show resolved Hide resolved
src/mumble/JackAudio.h Outdated Show resolved Hide resolved
src/mumble/Settings.h Show resolved Hide resolved
src/mumble/VoiceRecorderDialog.cpp Show resolved Hide resolved
@Krzmbrzl Krzmbrzl added client recording feature-request This issue or PR deals with a new feature labels Feb 3, 2023
@IsaMorphic
Copy link
Author

Howdy! Just wanted to give a status update, I'm gonna try and finish this up this week or the week after. Excited to get this merged!! :D

@Krzmbrzl
Copy link
Member

Thanks for the update 👍
You're in no hurry though - even if you finish it within this week, it'll probably take me a while until I can re-review this again 🙈

@Krzmbrzl
Copy link
Member

Krzmbrzl commented Apr 8, 2023

@IsaMorphic just checking in here to check in on whether you still intend to finish this PR at some point. No need to hurry - a simple "yes" or "no" is enough for now.
For the time being, I'll mark this PR as a draft.

@Krzmbrzl Krzmbrzl marked this pull request as draft April 8, 2023 15:24
@IsaMorphic
Copy link
Author

Yes! Eventually I'll get to it. Thanks!

@IsaMorphic
Copy link
Author

Hi @Krzmbrzl !!
I'm finally getting around to this. Expect to see this finished in about a week! Thanks for continuing to maintain this repo.

@IsaMorphic
Copy link
Author

@IsaMorphic Not commenting on any actual changes right now, but you should make sure not to include a merge commit in this pull request

Whoopsie! Fixing this right away :)

@IsaMorphic
Copy link
Author

OnO just realized I forgot about all my auto declarations I need to fix xD
I'll get to it asap

@IsaMorphic
Copy link
Author

OnO just realized I forgot about all my auto declarations I need to fix xD I'll get to it asap

Alr that's done, I also did some other style fixes according to the guidelines I just realized exist 😅

src/mumble/JackAudio.cpp Fixed Show resolved Hide resolved
@IsaMorphic
Copy link
Author

@Krzmbrzl bump! No rush, but check this out when you can :)

Copy link
Member

@Krzmbrzl Krzmbrzl left a comment

Choose a reason for hiding this comment

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

I will likely not be able to test this myself (I'm lacking the time needed to get into Jack and get things setup correctly) so I'll rely on you having done thorough testing yourself.

Code-wise this mostly LGTM.

src/mumble/AudioOutput.cpp Outdated Show resolved Hide resolved
src/mumble/AudioOutput.cpp Show resolved Hide resolved
src/mumble/JackAudio.cpp Outdated Show resolved Hide resolved
src/mumble/JackAudio.cpp Outdated Show resolved Hide resolved
src/mumble/JackAudio.cpp Outdated Show resolved Hide resolved
src/mumble/JackAudio.cpp Show resolved Hide resolved
src/mumble/JackAudio.h Outdated Show resolved Hide resolved
src/mumble/JackAudio.cpp Outdated Show resolved Hide resolved
src/mumble/VoiceRecorder.cpp Outdated Show resolved Hide resolved
src/mumble/VoiceRecorder.h Outdated Show resolved Hide resolved
@IsaMorphic
Copy link
Author

This has indeed survived a 1 hour stress test where I successfully recorded a podcast segment without issues or crashes. UI additions have also been tested for all possible edge cases.

I'll implement the last of these changes tonight.

Tysm <3

@Krzmbrzl
Copy link
Member

This has indeed survived a 1 hour stress test where I successfully recorded a podcast segment without issues or crashes. UI additions have also been tested for all possible edge cases.

Perfect 👍

Copy link
Member

@Krzmbrzl Krzmbrzl left a comment

Choose a reason for hiding this comment

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

Had more time today and I found some more stuff to nit-pick about :D

src/mumble/JackAudio.cpp Outdated Show resolved Hide resolved
src/mumble/JackAudio.cpp Outdated Show resolved Hide resolved
src/mumble/JackAudio.cpp Outdated Show resolved Hide resolved
src/mumble/JackAudio.cpp Outdated Show resolved Hide resolved
src/mumble/JackAudio.cpp Outdated Show resolved Hide resolved
src/mumble/JackAudio.cpp Outdated Show resolved Hide resolved
src/mumble/JackAudio.cpp Outdated Show resolved Hide resolved
src/mumble/VoiceRecorder.cpp Show resolved Hide resolved
@IsaMorphic
Copy link
Author

Hopefully that's everything lol! You have an eye for detail :3

Copy link
Member

@Krzmbrzl Krzmbrzl left a comment

Choose a reason for hiding this comment

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

Please rebase your PR against current master. That should finally make the FreeBSD CI succeed.

git pull --rebase https://github.com/mumble-voip/mumble.git master

src/mumble/JackAudio.cpp Outdated Show resolved Hide resolved
src/mumble/JackAudio.cpp Outdated Show resolved Hide resolved
@Krzmbrzl
Copy link
Member

Before this is merged your commits will have to be squashed together into the first FEAT commit (except for the TRANSLATION commit - that one will remain separate and after the FEAT commit)

src/mumble/JackAudio.cpp Fixed Show fixed Hide fixed
@IsaMorphic
Copy link
Author

Sounds good. I'll be right on it. I'll be sure to fix that overflow bug too beforehand.

@IsaMorphic
Copy link
Author

Meow! :3 the deed is done.

@IsaMorphic
Copy link
Author

@Krzmbrzl not trying to be pushy, but any idea on when this might get merged? I'm so excited to have this contribution done and dusted! :D

@IsaMorphic IsaMorphic requested a review from Krzmbrzl March 4, 2024 14:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
client feature-request This issue or PR deals with a new feature recording
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants