Conversation
|
I'm confused what the "Use maximum retries" option does. I'd assume that Mixxx would always try to reconnect up to the maximum times defined above that option. What does it do when that is not checked? |
|
If the checkbox is not set Mixxx retries until eternity. |
|
I think it would be clearer to flip it around. Label the checkbox "Limit number of reconnection attempts". Gray out the maximum connections field if that box isn't checked. |
| ConfigKey(BROADCAST_PREF_KEY, "reconnect_delay")).toInt()); | ||
|
|
||
| // Maximum Retries | ||
| spinBoxMaximumReties->setValue(m_pConfig->getValueString( |
There was a problem hiding this comment.
Typos: spinBoxMaximumReties-> spinBoxMaximumRetries
| <item row="2" column="0"> | ||
| <widget class="QLabel" name="label_17"> | ||
| <property name="text"> | ||
| <string>Maximum reties</string> |
| <item row="3" column="0" colspan="2"> | ||
| <widget class="QCheckBox" name="checkBoxUseMaximumRetries"> | ||
| <property name="text"> | ||
| <string>Use maximum retires</string> |
| <widget class="QLabel" name="label_16"> | ||
| <property name="text"> | ||
| <string>Host</string> | ||
| <string>Format </string> |
There was a problem hiding this comment.
The new retries block should align with the existingcoding block in the grid layout. Currently the coding block is misaligned.
|
If you hammer the streaming server with login attempts, you risk being rate limited or banned. Increasing the delay for each reconnection attempt sounds right, but imo there should be an timeout, since we had an established connection before, and temporary network errors clear quickly. |
|
Now it looks like this: |
I have not fully understand your comment. Do you propose a change? Lets have a look at the use-cases:
The current implementation was a result of a discussion: http://www.mixxx.org/forums/viewtopic.php?f=1&t=8613 Do you have an idea to improve it? I will try to get this verified by streaming services recommending Mixxx. |
|
Okay, that makes it clearer what the new options do :) |
|
if I understand correctly, "Maximum retries" are only used when you check the box "limit number of attempts". So why not just remove the checkbox and use "0" in maximum retries to say "unlimited", adding a tooltip or some hint on that as a label in the interface ? also, at first, it is not clear if "reconnect delay" is only an int or can be a float. Can we use 0.1 as default to show that it can be a float (assuming it can be, 1s step seems too large) ? |
I thought about this, but it would be confusing. 0 maximum retries would imply it does not try to reconnect, rather than it continuously tries to reconnect. |
|
0 retries = do not reconnect.
Check box unchecked = unlimited reconnects.
It look like we need better strings or tooltips again.
The delay value is currently an integer.
The internal implementation uses a simple sleep(500) loop. If we need a
float resolution we need to refactor this.
What is a use-case for a finer setting?
I am afraid that the casual user is not able to tweak these values
reasonable.
I consider to allow him only to select between two or three reasonable
error handling strategies, that respects the need and rules of steaming
providers.
Unfortunately theses cases are not that clear to me. Hopefully we can work
them out here.
|
|
This is the answer from
Marco Pracucci:
The main problem client and server face when using the Icecast protocol is
that there's no way to know if the stream was intentionally closed by the
client or server (the server don't know if the disconnection was
intentional, the same for the client). This makes me think that it would be
nearly impossible to implement the point #2: "Mixxx should not try to
reconnect, if the stream was intentional closed by the server.".
In our Spreaker Studio app for Windows and Mac (
https://www.spreaker.com/download) we do have an auto-reconnect feature,
with an exponential backoff delay (with a reasonable min/max range) and
with a maximum number of retries (this should actually be quite large,
since the networking issues could last for some time).
|
|
I have evaluated other software and it seams that the current implementation is somehow common. How about add an additional check-box: and pick a default reconnection delay of 15 s @esbrandt: Does this fit your needs? |
Conflicts: src/engine/sidechain/enginebroadcast.cpp
|
Hm, what was wrong with the mac build that removing getValueString fixes? |
|
This is the original clang error message I am trying to fix: gcc compiles that without complaining. IMHO gcc is right, but since the getValueString with a type conversion on the users side looks ugly anyway, I have decided to remove it in favor of the type aware The pending issue now is that I have not catched all occurrences. |
|
I have also send some time in investigating the crasher. Now we have some safety checks around the possible recursion with flush(). It turns out that your crasher and the provided logs, cannot be caused by a flush() recursion, since it crashes in a straight forward EngineBroadcast::process() call: Possible crash causes due to mixxx is an invalid encoder pointer or invalid buffer provided. What else might has happen during the crash? |
rryan
left a comment
rryan
left a comment
There was a problem hiding this comment.
Looking great! Thanks for the changes. I'll give it another test.
| QString BroadcastSettings::getMetadataFormat() const { | ||
| return m_pConfig->getValue( | ||
| ConfigKey(kConfigKey, kMetadataFormat), | ||
| // No tr() here, see https://bugs.launchpad.net/mixxx/+bug/1419500 |
There was a problem hiding this comment.
Could probably remove the comment since it's also up at kDefaultMetadataFormat.
|
|
||
| bool BroadcastSettings::getOggDynamicUpdate() const { | ||
| return m_pConfig->getValue( | ||
| ConfigKey(kConfigKey, kOggDynamicUpdate), false); |
There was a problem hiding this comment.
replace false with getDefaultOggDynamicUpdate()
| } | ||
|
|
||
| int BroadcastSettings::getDefaultPort() const { | ||
| return -1; |
|
|
||
| bool BroadcastSettings::getStreamPublic() const { | ||
| return m_pConfig->getValue( | ||
| ConfigKey(kConfigKey, kStreamPublic), false); |
| spinBoxMaximumRetries->setValue(m_settings.getDefaultMaximumRetries()); | ||
| spinBoxMaximumRetries->setEnabled(true); | ||
| stream_name->setText(m_settings.getDefaultStreamName()); | ||
| stream_website->setText(m_settings.getDefaultStreamName()); |
| mountpoint->setText(m_settings.getDefaultMountpoint()); | ||
| host->setText(m_settings.getDefaultHost()); | ||
| int iPort = m_settings.getDefaultPort(); | ||
| if (iPort != 0 && iPort <= 0xffff) { |
There was a problem hiding this comment.
Should this be a DEBUG_ASSERT instead? Seems like this would only happen when a programmer makes an error changing the default.
There was a problem hiding this comment.
Yes, now we have a valid port instead of -1.
| int ConfigObject<ConfigValue>::getValue(const ConfigKey& key, | ||
| const int& default_value) const { | ||
| int ConfigObject<ConfigValue>::getValue( | ||
| const ConfigKey& key, const int& default_value) const { |
There was a problem hiding this comment.
I'm guessing this was some auto-reformat of the file?
Hanging indent of function arguments is how our style guide specifies:
https://google.github.io/styleguide/cppguide.html#Function_Calls
Personally, I find it much more readable since my eyes don't have to leave the right-side of the screen.
There was a problem hiding this comment.
The QString version requires the second format option to not exceed column 80.
I have changed the others as well because I want to have all getValue functions sharing the same line brakes.
Conflicts: src/engine/cuecontrol.cpp src/preferences/configobject.cpp
|
Notes Addressed. |
|
Sorry for the conflicts -- could you please resolve them? |
|
I would also need this feature to start using Mixxx for broadcasting with Shoutcast. Otherwise it's impossible to switch between between DJ sets. This is indispensable. |
|
I merged this with master locally to test it out. I killed my SC2 server and Mixxx segfaulted. The SC2 log showed Mixxx had reconnected right before it crashed. |
|
Another, this time with LLDB backtraces and log: |
|
I'm pretty confused at where this is coming from. Building with asan to see if I can pick up any memory safety issues. |
|
On broadcast connect (don't have to kill my server) -- asan notices a heap buffer overflow in libshout. Looks like it could be shoutcast-specific ( |
|
The heap overflow happens here when And this happens when |
Conflicts: src/preferences/configobject.cpp src/preferences/dialog/dlgprefwaveform.cpp src/skin/legacyskinparser.cpp
|
I finally reproduced the same crash on reconnect in master. https://paste.debian.net/911047/ |
|
Thank you! |
6 participants
This PR adds some preference options to control the reconnect behavior in case of broadcast issues.
https://bugs.launchpad.net/mixxx/+bug/1080981
You can now set a retry counter and define a delay between the connection retries.
This PR also fixes a deadlock issue tracked here:
https://bugs.launchpad.net/mixxx/+bug/1532461