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

Add size optimization for SCJingleConverter. #18

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add size optimization for SCJingleConverter. #18

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
206 changes: 158 additions & 48 deletions Jingle/SCJingleConverter/composition.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -283,6 +283,8 @@ Composition::ErrorCode Composition::parse() {
}
}

optimizeNotes();

return NO_ERROR;
}

Expand All @@ -299,6 +301,7 @@ Composition::ErrorCode Composition::parseXmlNote(QXmlStreamReader& xml, const QS
float length = 0.f;
float frequency = 0.f;
bool isChord = false;
bool isTied = false;
QString voice_id = " Voice ?"; // Used for second part of key to access voices

// Check that we are actually at the beginning of an XML note token
Expand Down Expand Up @@ -336,6 +339,12 @@ Composition::ErrorCode Composition::parseXmlNote(QXmlStreamReader& xml, const QS
}
} else if (xml.name() == QLatin1String("chord")) {
isChord = true;
} else if (xml.name() == QLatin1String("tie")) {
if (xml.tokenType() == QXmlStreamReader::StartElement) {
if (xml.attributes().value(QLatin1String("type")) == QLatin1String("start")) {
isTied = true;
}
}
}
}

Expand Down Expand Up @@ -368,6 +377,10 @@ Composition::ErrorCode Composition::parseXmlNote(QXmlStreamReader& xml, const QS
Note note;
note.frequencies.push_back(frequency);
note.length = length;
note.tied = isTied;
if (frequency == 0.f) {
note.tied = true; // Auto-tie all rests.
}

meas.notes.push_back(note);
}
Expand Down Expand Up @@ -477,9 +490,6 @@ Composition::ErrorCode Composition::download(SCSerial& serial, uint32_t jingleId
return BAD_IDX;
}

const uint32_t meas_start_idx = getMeasStartIdx();
const uint32_t meas_end_idx = getMeasEndIdx();

const uint32_t num_notes_l = getNumNotes(LEFT);
const uint32_t num_notes_r = getNumNotes(RIGHT);

Expand All @@ -503,41 +513,35 @@ Composition::ErrorCode Composition::download(SCSerial& serial, uint32_t jingleId

// Add Notes to Left Channel
if (voices.count(voiceStrL)) {
for (uint32_t meas_idx = meas_start_idx; meas_idx <= meas_end_idx; meas_idx++) {
Measure& meas = voices[voiceStrL].measures[meas_idx];
for (uint32_t notes_idx = 0; notes_idx < meas.notes.size(); notes_idx++) {
Note& note = meas.notes[notes_idx];

cmd = noteToCmd(note, LEFT, jingleIdx, note_cnt, chordIdxL);
resp = cmd + "\rNote updated successfully.\n\r";
serial_err_code = serial.send(cmd, resp);
if (serial_err_code != SCSerial::NO_ERROR) {
qDebug() << "serial.send() Error String: " << SCSerial::getErrorString(serial_err_code);
return CMD_ERR;
}
note_cnt++;
for (uint32_t notes_idx = 0; notes_idx < optimizedNotesL.size(); notes_idx++) {
Note& note = optimizedNotesL[notes_idx];

cmd = noteToCmd(note, LEFT, jingleIdx, note_cnt, chordIdxL);
resp = cmd + "\rNote updated successfully.\n\r";
serial_err_code = serial.send(cmd, resp);
if (serial_err_code != SCSerial::NO_ERROR) {
qDebug() << "serial.send() Error String: " << SCSerial::getErrorString(serial_err_code);
return CMD_ERR;
}
note_cnt++;
}
}

// Add Notes to Right Channel
if (voices.count(voiceStrR)) {
note_cnt = 0;

for (uint32_t meas_idx = meas_start_idx; meas_idx <= meas_end_idx; meas_idx++) {
Measure& meas = voices[voiceStrR].measures[meas_idx];
for (uint32_t notes_idx = 0; notes_idx < meas.notes.size(); notes_idx++) {
Note& note = meas.notes[notes_idx];

cmd = noteToCmd(note, RIGHT, jingleIdx, note_cnt, chordIdxR);
resp = cmd + "\rNote updated successfully.\n\r";
serial_err_code = serial.send(cmd, resp);
if (serial_err_code != SCSerial::NO_ERROR) {
qDebug() << "serial.send() Error String: " << SCSerial::getErrorString(serial_err_code);
return CMD_ERR;
}
note_cnt++;
for (uint32_t notes_idx = 0; notes_idx < optimizedNotesR.size(); notes_idx++) {
Note& note = optimizedNotesR[notes_idx];

cmd = noteToCmd(note, RIGHT, jingleIdx, note_cnt, chordIdxR);
resp = cmd + "\rNote updated successfully.\n\r";
serial_err_code = serial.send(cmd, resp);
if (serial_err_code != SCSerial::NO_ERROR) {
qDebug() << "serial.send() Error String: " << SCSerial::getErrorString(serial_err_code);
return CMD_ERR;
}
note_cnt++;
}
}

Expand Down Expand Up @@ -630,6 +634,8 @@ Composition::ErrorCode Composition::setMeasStartIdx(uint32_t measStartIdx) {

this->measStartIdx = measStartIdx;

optimizeNotes();

return NO_ERROR;
}

Expand Down Expand Up @@ -663,6 +669,8 @@ Composition::ErrorCode Composition::setMeasEndIdx(uint32_t measEndIdx) {

this->measEndIdx = measEndIdx;

optimizeNotes();

return NO_ERROR;
}

Expand Down Expand Up @@ -701,6 +709,8 @@ Composition::ErrorCode Composition::setVoice(Channel chan, QString voiceStr) {
break;
}

optimizeNotes();

return NO_ERROR;
}

Expand Down Expand Up @@ -849,37 +859,137 @@ uint32_t Composition::getMemUsage() {
* configured Measure Start and Stop Indices.
*/
uint32_t Composition::getNumNotes(Channel chan) {
if (chan == RIGHT) {
return optimizedNotesR.size();
} else {
return optimizedNotesL.size();
}
}

/**
* @brief Composition::optimizeNotes Condenses and trims notes to take up
* less space when downloaded to the Steam Controller.
*/
void Composition::optimizeNotes() {
const uint32_t meas_start_idx = getMeasStartIdx();
const uint32_t meas_end_idx = getMeasEndIdx();

uint32_t num_notes = 0;
optimizedNotesL.clear();
optimizedNotesR.clear();

Voice* voice = nullptr;
switch (chan) {
case RIGHT:
if (voices.count(voiceStrR)) {
voice = &voices[voiceStrR];
uint32_t removed_notes = 0;

// Optimize Notes in Left Channel
if (voices.count(voiceStrL)) {
// Collect notes into single vector
for (uint32_t meas_idx = meas_start_idx; meas_idx <= meas_end_idx; meas_idx++) {
Measure& meas = voices[voiceStrL].measures[meas_idx];
for (uint32_t notes_idx = 0; notes_idx < meas.notes.size(); notes_idx++) {
optimizedNotesL.push_back(meas.notes[notes_idx]);
}
Copy link

Choose a reason for hiding this comment

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

Assuming meas.notes is a vector, you can replace this loop with:

optimizedNotesL = meas.notes

Copy link
Author

Choose a reason for hiding this comment

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

It is, but I don't think we can. We're appending multiple measures to optimizedNotesL.

Copy link
Author

Choose a reason for hiding this comment

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

Can't reply to the comment on composistion.cpp for some reason but that sounds good to me. For some reason thought we were limited to uint32_t or maybe just unsigned ints. In hindsight that would have been a pretty easy thing to change and test lol.

Copy link

Choose a reason for hiding this comment

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

Oops, sorry, missed the context! In that case, does std::vector::insert help?

optimizedNotesL.insert(optimizedNotesL.end(), meas.notes.begin(), meas.notes.end());

Copy link
Author

Choose a reason for hiding this comment

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

Looks pretty great to me, yeah!

}
break;

case LEFT:
if (voices.count(voiceStrL)) {
voice = &voices[voiceStrL];
// Combine ties
for (uint32_t notes_idx = optimizedNotesL.size()-1; notes_idx-- > 0; ) {
Note& note = optimizedNotesL[notes_idx];

if (note.tied && notes_idx < optimizedNotesL.size()-1) {
Note& tie = optimizedNotesL[notes_idx+1];

float note_frequency = note.frequencies.back();
float tie_frequency = tie.frequencies.back();
if (chordIdxL < note.frequencies.size()) {
note_frequency = note.frequencies[chordIdxL];
}
if (chordIdxL < tie.frequencies.size()) {
tie_frequency = tie.frequencies[chordIdxL];
}

if (note_frequency == tie_frequency) {
note.length += tie.length;
optimizedNotesL.erase(optimizedNotesL.begin()+notes_idx+1);
removed_notes++;
}
}
}
break;
}

if (voice) {
if (meas_start_idx >= voice->measures.size() || meas_end_idx >= voice->measures.size()) {
qDebug() << "Start or End Measure Index out of bounds for Channel in getNumNotes()";
return 0;
// Remove trailing the trailing rest (should only be one max)
if (optimizedNotesL[optimizedNotesL.size()-1].frequencies[chordIdxL] == 0.f) {
optimizedNotesL.erase(optimizedNotesL.begin()+optimizedNotesL.size()-1);
}
}

// Optimize Notes in Right Channel
if (voices.count(voiceStrR)) {
Copy link
Owner

Choose a reason for hiding this comment

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

This is similar enough to "// Optimize Notes in Left Channel" block above and code is complex enough that this should be a function (e.g. optimizeNotes(Channel chan)).

// Collect notes into single vector
for (uint32_t meas_idx = meas_start_idx; meas_idx <= meas_end_idx; meas_idx++) {
Measure& meas = voice->measures[meas_idx];
num_notes += meas.notes.size();
Measure& meas = voices[voiceStrR].measures[meas_idx];
for (uint32_t notes_idx = 0; notes_idx < meas.notes.size(); notes_idx++) {
optimizedNotesR.push_back(meas.notes[notes_idx]);
}
}

// Combine ties
for (uint32_t notes_idx = optimizedNotesR.size()-1; notes_idx-- > 0; ) {
Copy link
Owner

Choose a reason for hiding this comment

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

notes_idx should not be decremented in comparison part of for loop

Copy link
Author

Choose a reason for hiding this comment

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

Which would fit better?

for (uint32_t notes_idx = optimizedNotes.size()-1; notes_idx < optimizedNotes.size(); notes_idx--) {
    ...

Still kinda ugly, now it'll wrap around, but it still works.

for (uint32_t notes_idx = optimizedNotes.size()-1; notes_idx > 0; ) {
    notes_idx--;
    ....

Or this only pulls the notes_idx-- down out of the comparison.

Currently leaning towards the second.

Copy link
Owner

Choose a reason for hiding this comment

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

First, the logic in your first code block doesn't match the others. Was it a mistake that the condition check part of the for loop is notes_idx < optimizedNotes.size() and not notes_idx > 0 or am I missing something?

Second, I'm less worried with text wrap around (assuming I understand your "ugly" comment) and more about code readability in terms functionality. The ++/-- features in C++ is great, but when it gets put into parts of a statement where people might not usually look for it, my opinion is that that code is hard to read and error prone if anyone were to change it. So I would think something like the following would be acceptable:

for (uint32_t notes_idx = optimizedNotes.size()-1; notes_idx > 0; notes_idx--) {

Saying all that though, I also have an addendum. Is it possible we can get to this code and optimizedNotesR.size() will be zero? In that case, since notes_idx is unsigned, we will get wrap around and out of bounds issues. Initial thought is to change the for loop to count up and use that count value to compute the access index. Thoughts?

Copy link
Author

Choose a reason for hiding this comment

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

First, the logic in your first code block doesn't match the others. Was it a mistake that the condition check part of the for loop is notes_idx < optimizedNotes.size() and not notes_idx > 0 or am I missing something?

Sorry, wrapping around is referring to integer underflow. If I use > 0, the first two notes will never be optimized since the loop never runs when notes_idx == 0. Then the compiler immediately yelled at me for trying notes_idx <= 0 on an unsigned int, which led me to this. In this code, notes_idx < optimizedNotes.size() will evaluate false when notes_idx underflows and becomes greater than optimizedNotes.size(), which is kinda ugly.

Totally get what you're saying about putting things in unexpected places.

for (uint32_t notes_idx = optimizedNotes.size()-1; notes_idx > 0; notes_idx--) {

Has the problem I touched on earlier, the loop will never run when notes_idx == 0 so the first note (first two, sorta) get ignored and never optimized. Moving the notes_idx-- to be the first statement in the loop is functionally identical to the weird notes_idx-- > 0 without being as bad for readability. It allows the rest of the loop to run while notes_idx == 0, without having to deal with that in the conditional, either by decrementing it in an unnatural place or unintuitively dealing with underflow.

Saying all that though, I also have an addendum. Is it possible we can get to this code and optimizedNotesR.size() will be zero? In that case, since notes_idx is unsigned, we will get wrap around and out of bounds issues.

Can't quite tell. It checks to see if the current active voice exists, then collects all notes from all measures in that voice into optimizedNotes. Are voices guaranteed to have at least some notes in them, even rests? As long as it has something, even if it's 100% rests we're good to go.

Initial thought is to change the for loop to count up and use that count value to compute the access index. Thoughts?

You say using the count value to compute the access index... are you saying the loop would count up while the actual index accessed would still be counting backwards or? I'm having a hard time wrapping my head around that lol.

Copy link
Owner

Choose a reason for hiding this comment

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

I was thinking something like the following maybe:

for (uint32_t notes_cnt = 1; notes_cnt < optimizedNotesR.size(); notes_cnt++) {
    uint32_t notes_idx = optimizedNotesR.size() - notes_cnt;
    ...

In this way we don't have to worry about whether optimizedNotesR.size() can ever be zero, because if it is we will never enter the loop to begin with.

Thoughts?

Copy link
Author

Choose a reason for hiding this comment

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

I haven't tested it yet but my initial thought is that something like that would be immediately thrown off once we start pulling notes off the end as they're optimized out, unless we add something to adjust the count every time that happens. If the goal's just to be sure that it's not empty, seems like making a separate, explicit check for that would be clearer.

Copy link

Choose a reason for hiding this comment

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

If the goal is just to make it run at == 0, then use a signed integer as the index var. As long as the index isn't larger than INT_MAX, there won't be any weird wrapping, and you can add asserts to that effect if you are worried about it. Also since you intentionally don't want to process the very last node first (because you are comparing with the next node), you can start at - 2 and remove the < size - 1 check below.

for (int notes_idx = optimizedNotes.size() - 2; notes_idx >= 0; notes_idx--) {

Note& note = optimizedNotesR[notes_idx];

if (note.tied && notes_idx < optimizedNotesR.size()-1) {
Note& tie = optimizedNotesR[notes_idx+1];

float note_frequency = note.frequencies.back();
float tie_frequency = tie.frequencies.back();
if (chordIdxR < note.frequencies.size()) {
note_frequency = note.frequencies[chordIdxR];
}
if (chordIdxR < tie.frequencies.size()) {
tie_frequency = tie.frequencies[chordIdxR];
}

if (note_frequency == tie_frequency) {
note.length += tie.length;
optimizedNotesR.erase(optimizedNotesR.begin()+notes_idx+1);
removed_notes++;
}
}
}

// Remove trailing the trailing rest (should only be one max)
if (optimizedNotesR[optimizedNotesR.size()-1].frequencies[chordIdxR] == 0.f) {
optimizedNotesR.erase(optimizedNotesR.begin()+optimizedNotesR.size()-1);
removed_notes++;
}
}

// Remove leading silence
Copy link
Owner

Choose a reason for hiding this comment

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

Add a GUI control to make this optional. I'm all for changing the internal structure to save space, but don't want to force any timing changes upon a user (maybe some people want preceding silence?).

if (optimizedNotesL.size() && optimizedNotesR.size()) {
Note& leftFirst = optimizedNotesL[0];
Note& rightFirst = optimizedNotesR[0];
if (leftFirst.frequencies[chordIdxL] == 0.f && rightFirst.frequencies[chordIdxR] == 0.f) {
if (leftFirst.length == rightFirst.length) {
optimizedNotesL.erase(optimizedNotesL.begin());
optimizedNotesR.erase(optimizedNotesR.begin());
removed_notes += 2;
} else if (leftFirst.length > rightFirst.length) {
leftFirst.length -= rightFirst.length;
optimizedNotesR.erase(optimizedNotesR.begin());
removed_notes++;
} else {
rightFirst.length -= leftFirst.length;
optimizedNotesL.erase(optimizedNotesL.begin());
removed_notes++;
}
}
} else if (optimizedNotesL.size()) {
if (optimizedNotesL[0].frequencies[chordIdxL] == 0.f) {
optimizedNotesL.erase(optimizedNotesL.begin());
removed_notes++;
}
} else if (optimizedNotesR.size()) {
if (optimizedNotesR[0].frequencies[chordIdxR] == 0.f) {
optimizedNotesR.erase(optimizedNotesR.begin());
removed_notes++;
}
}

return num_notes;
qDebug() << "Saved " << (6*removed_notes) << " bytes.";
}
6 changes: 6 additions & 0 deletions Jingle/SCJingleConverter/composition.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -207,6 +207,8 @@ class Composition

uint32_t getMemUsage();

void optimizeNotes();

private:
/**
* @brief The Note struct stores data relating to frequency and duration
Expand All @@ -216,11 +218,13 @@ class Composition
Note()
: frequencies(0)
, length(0.f)
, tied(false)
{}
std::vector<float> frequencies; // Frequency of note in Hz. Could be multiple
// frequencies in case of chord. Frequencies to be sorted into descending
// order at end of parse.
float length; // Number of beats that Note/Chord lasts for
bool tied; // Whether or not this note tries to be tied to the next.
Copy link
Owner

Choose a reason for hiding this comment

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

Change description to something like "True if note is tied to next (i.e. single unbroken note)"

};

/**
Expand Down Expand Up @@ -265,6 +269,8 @@ class Composition
// The QString Key is of the Form "Part {n} Voice {m}" where n is the Part {n} is the
// part id (with {n} changing for different parts) and Voice {m} designates
// which voice within the part we are referring to.
std::vector<Note> optimizedNotesL;
Copy link
Owner

Choose a reason for hiding this comment

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

Add descriptions for these. Why do they exist and how do they relate to voices.

std::vector<Note> optimizedNotesR;

uint32_t currDivisions; // Current conversion factor for Note duration to Number of beats.
uint32_t bpm; // Beats per minute. Tempo for playback of compostion.
Expand Down