Conversation
…esn't respect this layout. Stereo elements should default be rendered binaurally anyways?
BrandenAvalonCx
left a comment
BrandenAvalonCx
left a comment
There was a problem hiding this comment.
I think overall this looks good, seems like this addresses the issues. I think my only question is about if we're storing the timecode information in a sensible format since we seem to be dividing the number every time we fetch it.
| // Calculate the current time with the existing number of samples that have | ||
| // been processed | ||
| long currentTime = sampleTally_ / sampleRate_; | ||
| const double currentTime = static_cast<double>(sampleTally_) / sampleRate_; |
There was a problem hiding this comment.
Rather then casting here, should we just make sampleTally_ a double?
In general, this math seems a bit confused since sampleRate_ is already a long cast from a double. So feels like they could both become doubles here and it will be more straightforward?
| return false; | ||
| } | ||
| // Skip if buffer starts before the requested start time | ||
| if (startTime_ > 0 && currentTime < startTime_) { |
There was a problem hiding this comment.
Do we need to check is startTime > 0 or endTime is > 0. Looks like above we are returning if startTime or endTime are <= 0 already?
There was a problem hiding this comment.
If you make the suggested change, I would combine these into one statement
| startTime_ = configParams.getStartTime(); | ||
| endTime_ = configParams.getEndTime(); | ||
| startTime_ = configParams.getStartTime() / 1000.0; | ||
| endTime_ = configParams.getEndTime() / 1000.0; |
There was a problem hiding this comment.
Seems like every time we fetch these values we're dividing them by 1000.
Does it make more sense to divide them when we store them instead, since that will be one place instead of on each fetch? I'm genuinely asking, there might be a good reason to divide the value this way instead.
3 participants
Description
Fixes from #105
Validation and Acceptance Criteria