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

Adding a Copy Constructor to AudioStreamBuilder #528

Merged
merged 8 commits into from
Jun 3, 2019
Merged

Adding a Copy Constructor to AudioStreamBuilder #528

merged 8 commits into from
Jun 3, 2019

Conversation

atneya
Copy link
Collaborator

@atneya atneya commented May 30, 2019

See issue #527 for more details.

@atneya atneya requested a review from philburk May 30, 2019 21:41
Copy link
Collaborator

@philburk philburk left a comment

Choose a reason for hiding this comment

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

Looking good. Much cleaner.

if (error == oboe::Result::ErrorDisconnected) restartStream();
if (error == oboe::Result::ErrorDisconnected) {
oboe::AudioStreamBuilder builder_ = oboe::AudioStreamBuilder(*oboeStream);
restartStream(&builder_);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we leak the old AudioStream pointer?
If you delete it then you may hit this issue!
#519

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Restarting will use the same pointer, mPlayStream, but create a new object. The old object is closed but not deleted, however, I think this issue should be addressed by using a unique pointer, which involves issue #529

* Moved duplex stream to live effect

* LiveEffect refactor with full duplex stream completely functional. Fixes #60

* Preserving comments and naming conventions. Also making duplex passthrough more resilient.
* Audio Engine needs to be reinitialized on app start, using pointer instead of object

* shared Mixer refactored to use raw pointers instead of shared pointers. This refactor impacted MegaDrone and RhythmGame.

This prevents ambiguous ownership of audio components, leaving the owner of the Mixer as the sole owner of the inputs to the Mixer.
This allows for objects with the Mixer as a member along with its inputs to control their duration automatically, in aggregate.
This fixes crashes on destruction of objects composing the Mixer and various inputs. (E.g. Fixes #521).

* Refactoring use of unique ptr in RhythmGame and const ptr in MegaDrone

* Documenting mixer input ownership

* Removing const pointer since it requires a messy ternary
Copy link
Collaborator

@philburk philburk left a comment

Choose a reason for hiding this comment

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

This PR contains a lot of changes that have already been merged. There seems to be a rebase problem. Rebasing is problematic with GitHub. You may need to start a new pull request,

void *audioData,
int numFrames) override;

int32_t getMNumInputBurstsCushion() const;
Copy link
Collaborator

Choose a reason for hiding this comment

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

remove 'M' from getMNum and setMNum...

*
* @param mNumInputBurstsCushion
*/
void setMNumInputBurstsCushion(int32_t mNumInputBurstsCushion);
Copy link
Collaborator

Choose a reason for hiding this comment

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

parameter should starts with "num"

static constexpr int32_t kNumCallbacksToDiscard = 30;

// let input fill back up, usually 0 or 1
int32_t mNumInputBurstsCushion = 1;
Copy link
Collaborator

Choose a reason for hiding this comment

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

align with other m variables


/**
* Called when data is available on both streams.
* Caller should override this method.
Copy link
Collaborator

Choose a reason for hiding this comment

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

App should override


/**
* Called by Oboe when the stream is ready to process audio.
*/
Copy link
Collaborator

Choose a reason for hiding this comment

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

Add "This implements the stream synchronization. App should NOT override this method."

@atneya
Copy link
Collaborator Author

atneya commented Jun 3, 2019

Do I need to roll back the rebase?

Copy link
Collaborator

@philburk philburk left a comment

Choose a reason for hiding this comment

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

If it will merge cleanly then go ahead and merge.

@atneya atneya merged commit b36c4af into master Jun 3, 2019
@atneya atneya deleted the issue-527 branch June 3, 2019 18:59
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.

2 participants