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.

Sign up for GitHub

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

Player volume fix #303

Merged
merged 2 commits into from
Nov 22, 2020
Merged

Player volume fix #303

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
05aaf43
Player volume fix for issue #276
mostafa-nabil Jul 6, 2020
3a83555
Merge branch 'master' into player_volume_fix
mostafa-nabil Jul 6, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 42 additions & 1 deletion source/audio/midiplayer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -185,11 +185,29 @@ void MidiPlayer::run()
if (!(event->isNoteOnOff() &&
event->getChannel() == METRONOME_CHANNEL &&
!myMetronomeEnabled) &&
!event->isTempoChange() && !event->isTrackEnd())
!event->isTempoChange() &&
!event->isTrackEnd() &&
!event->isVolumeChange())
{
device.sendMessage(event->getData());
}

// get the channel and the correspondin player
int channel = event->getChannel();
int player = getPlayerFromChannel(channel);
// if the channel corresponds to a valid player, set its maximum volume
if(player != -1)
{
device.setChannelMaxVolume(channel, myScore.getPlayers()[player].getMaxVolume());
}
// handle volume change events
// using device.setVolume() ensures that the maximum volume threshold
// is taken into consideration
if(event -> isVolumeChange())
Copy link
Member

Choose a reason for hiding this comment

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

The formatting is a bit inconsistent here - should be if (event->isVolumeChange())

{
device.setVolume(channel, event->getData()[2]);
Copy link
Member

Choose a reason for hiding this comment

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

Could you wrap the event->getData()[2] in a function similar to MidiEvent::getTempo()? It's nice to encapsulate the MIDI details in that class.

}

// Notify listeners of the current playback position.
if (event->getLocation() != current_location)
{
Expand Down Expand Up @@ -279,3 +297,26 @@ bool MidiPlayer::isPlaying() const
{
return myIsPlaying;
}

int MidiPlayer::getPlayerFromChannel(const int channel)
Copy link
Member

Choose a reason for hiding this comment

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

An alternative to this function would be to record the player number on the MidiEvent, which avoids having to duplicate the logic for how players are assigned to channels. (There's actually an unused myPlayer member variable in MidiEvent that was intended for this.purpose, but the myInstrument member should probably be removed).

What are your thoughts on that?

If we stick with the current approach, those unused members should be removed, and there's also a compiler warning (below) where the compiler incorrectly thinks the result could be uninitialized:

../source/audio/midiplayer.cpp:314:14: warning: variable 'player' is used uninitialized whenever 'if' condition is false [-Wsometimes-uninitialized]
    else if (channel > METRONOME_CHANNEL)
             ^~~~~~~~~~~~~~~~~~~~~~~~~~~
../source/audio/midiplayer.cpp:319:12: note: uninitialized use occurs here
    return player;
           ^~~~~~
../source/audio/midiplayer.cpp:314:10: note: remove the 'if' if its condition is always true
    else if (channel > METRONOME_CHANNEL)
         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../source/audio/midiplayer.cpp:303:15: note: initialize the variable 'player' to silence this warning
    int player;
              ^
               = 0
1 warning generated.

{
int player;

//check for invalid channel
if (METRONOME_CHANNEL == channel || 15 == channel)
{
player = -1;
}
else if (channel < METRONOME_CHANNEL)
{
player = channel;
}
else if (channel > METRONOME_CHANNEL)
{
player = channel - 1;
}

return player;

}

3 changes: 3 additions & 0 deletions source/audio/midiplayer.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,9 @@ class MidiPlayer : public QThread
void setIsPlaying(bool set);
bool isPlaying() const;

// gets the player index for a specific channel
static int getPlayerFromChannel(const int channel);

SettingsManager &mySettingsManager;
const Score &myScore;
ScoreLocation myStartLocation;
Expand Down
5 changes: 5 additions & 0 deletions source/midi/midievent.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,11 @@ bool MidiEvent::isTempoChange() const
myData[1] == MetaType::SetTempo;
}

bool MidiEvent::isVolumeChange() const
{
return myData[1] == Controller::ChannelVolume;
}

bool MidiEvent::isTrackEnd() const
{
return getStatusByte() == StatusByte::MetaMessage &&
Expand Down
1 change: 1 addition & 0 deletions source/midi/midievent.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@ class MidiEvent
const SystemLocation &getLocation() const { return myLocation; }

bool isTempoChange() const;
bool isVolumeChange() const;
bool isTrackEnd() const;
Midi::Tempo getTempo() const;
bool isProgramChange() const;
Expand Down