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

Multi-broadcasting #1300

Merged
merged 187 commits into from
Nov 20, 2017
Merged

Multi-broadcasting #1300

merged 187 commits into from
Nov 20, 2017

Conversation

Palakis
Copy link
Contributor

@Palakis Palakis commented Jul 3, 2017

These contributions add the ability to do Live Broadcasting to several servers simultaneously from Mixxx.
This is phase 2 and 3 of my GSoC project for Mixxx and is based on the work done on XML live broadcasting profiles during phase 1 of GSoC.

Tasks:

  • Implement ShoutConnection
    • Set broadcasting profile
    • Start output
    • Stop output
    • Push audio samples to encoder then output
  • Refactor streaming engine
    • Internal output instances list
    • Push received samples to outputs
    • Connection list synchronization with BroadcastSettings' profile list
    • Proper buffer sync management in EngineNetworkStream
  • Live Broadcastings preferences UI refactor
    • List/table view of all outputs/connections
      • Correct column sizing
      • Show connection state
      • Colorful connection states
    • "Create connection" button
    • Enable/disable a connection
    • Changes saved only on Apply
    • Edit settings for a connection
    • Rename a connection
    • Remove a connection
      • Better removal UX

Palakis added 30 commits June 2, 2017 14:45
I initially forgot to add broadcastprofile.cpp into the list
of source files in build/depends.py, so I didn't noticed all the
errors and mistakes I've made while writing this earlier.
This commit aims to fix these errors and have the BroadcastProfile
class to build.
No idea if this works. I just want to make sure it is saved
somewhere out of my computer.
I may squash this with other commits later.
Both ways didn't handled the document's root tag.
Now, profile values are saved properly.
This isn't needed in the current state of the code.
These additions automatically fetches existing
Live Broadcasting preferences from mixxx.cfg, puts
them in the Default Profile and removes the now
useless keys from mixxx.cfg
Add more tests in the process, because the regex changed.
@Palakis
Copy link
Contributor Author

Palakis commented Nov 18, 2017

@daschuer

Do you plan to ad the "disconnect all" Button to the preferences? I think it would be nice, but no blocking feature.

Implemented!

@Palakis
Copy link
Contributor Author

Palakis commented Nov 18, 2017

@ronso0 @Be-ing Fixed the merge conflicts with Tango. Did additional changes, tested all 5 Live Broadcasting states show properly in Tango's UI.

@Be-ing
Copy link
Contributor

Be-ing commented Nov 18, 2017

Great! @daschuer is this ready for merge? If there are minor issues that remain, I think we should merge it already and file bugs for the remaining issues.

@Palakis
Copy link
Contributor Author

Palakis commented Nov 18, 2017

@Be-ing Thanks! Understood.

@Palakis
Copy link
Contributor Author

Palakis commented Nov 18, 2017

Once this PR gets merged, it will be possible to open PRs for the new Opus and HE-AAC encoders, as their branches are based on this one.

@daschuer
Copy link
Member

Thank you, I have tested it once again, and it works good.
Some minor issues:

  • I think the connection checkboxes should remain unchanged when disconnecting all.
  • Do we really need the warning dialogue? I am unsure, it was surprising for me.
  • The tango icon changes to "off air" during mouse hoover

@JosepMaJAZ
Copy link
Contributor

@JosepMaJAZ That's a weird issue... What platform are you running this build of Mixxx on? When this error happened, were you running the latest version of this branch?

@Palakis That was on Windows, and using the build generated by AppVeyor for this PR.
Oh, and I used the Menu entry to enable broadcasting.

@Palakis
Copy link
Contributor Author

Palakis commented Nov 19, 2017

@daschuer

Do we really need the warning dialogue? I am unsure, it was surprising for me.

It's better to ask the user for confirmation.

The tango icon changes to "off air" during mouse hoover

This is already the case in upstream. I just updated it to support the new button states.

@Palakis
Copy link
Contributor Author

Palakis commented Nov 19, 2017

@daschuer

I think the connection checkboxes should remain unchanged when disconnecting all.

Fixed. Actually turning broadcasting off now instead of disabling every connection.

@Palakis
Copy link
Contributor Author

Palakis commented Nov 20, 2017

Merge conflicts originating from the merge of networkclockref into Mixxx's master: fixed!

@daschuer
Copy link
Member

LGTM! Thank you very much! This is really a nice feature for Mixxx that really matters for the broadcasting users.
I am looking forward to the next broadcasting related PRs ;-)

@daschuer
Copy link
Member

Now we have failing tests. It looks like a kind of merge conflict ... The similar is reported for #1377 (comment)

@daschuer
Copy link
Member

Master is Failing as well. So we can merge this.

@JosepMaJAZ
Copy link
Contributor

I know this has already been merged, but I have discovered a bug in it.
If you have multiple connections and go to preferences, initially there is no connection selected, but the settings show data of one of the connections. (in my case, it was the second of two connections, which was the last that I had been editing)

If the users modifies any data, this data is not saved, so the changes are lost.
Either, when loading the dialog, select the first and load its data, or disable editing of the data until a selection has been made (probably, it's easier to do the first).

Side note: The whole preference page talks about connections, but the correct term is sources (or source connection if you preffer). That's what I've been using in the translations I've done.

@JosepMaJAZ
Copy link
Contributor

Another bug reported by an user on the boards. The setting for "Dynamically Update Ogg Vorbis Metadata" is not being saved to the config:
https://mixxx.org/forums/viewtopic.php?f=3&t=9496&view=unread#unread

@Palakis
Copy link
Contributor Author

Palakis commented Dec 8, 2017

Thanks for the report. Gonna investigate these and open a PR with fixes for these issues.

@Palakis
Copy link
Contributor Author

Palakis commented Dec 16, 2017

Issues fixed, see PR #1418.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants