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

AudioLoader: Make decoding process more robust. #13725

Closed
wants to merge 1 commit into from

Conversation

Mugen87
Copy link
Collaborator

@Mugen87 Mugen87 commented Mar 29, 2018

This PR solves the problem mentioned in #13710. It also fixes #10706, a bad race condition.

FileLoader executes now a new callback function that can be used to decode the original response. This function can be set via .setDecodeCallback(). After the decode, the actual onLoad() fires.

Not sure, this is the best solution. But at least we have an approach that solves the mentioned issues.

@mrdoob
Copy link
Owner

mrdoob commented Mar 29, 2018

Hmm, not sure about adding this logic to FileLoader. It only affects AudioLoader right?

@mrdoob mrdoob added this to the r92 milestone Mar 29, 2018
@mrdoob
Copy link
Owner

mrdoob commented Mar 29, 2018

The approach in #13710, even if wasteful, seems like it creates less technical debt.

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Mar 30, 2018

It only affects AudioLoader right?

Yes 😇

The approach in #13710, even if wasteful, seems like it creates less technical debt.

Yeah, it's definitely the easier fix although it does not solve #10706. Besides, the fix does not only clone the buffer (which allocates more memory), it also produces additional decode overhead.

@mrdoob mrdoob modified the milestones: r92, r93 Apr 18, 2018
@Mugen87
Copy link
Collaborator Author

Mugen87 commented Apr 29, 2018

Closing. I'm trying to think of another approach that duplicates less code. Until then, let's focus on #13710.

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.

AudioLoader and LoadingManager onLoad Race
2 participants