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: Use fetch instead of FileLoader #14073

Closed
wants to merge 4 commits into from
Closed

Conversation

Mugen87
Copy link
Collaborator

@Mugen87 Mugen87 commented May 16, 2018

This PR uses a different approach compared to #13725. The implementation solves #13710 and #10706.

fetch should be supported in all Browsers which support Web Audio as well.

https://caniuse.com/#feat=fetch
https://caniuse.com/#feat=audio-api

@takahirox
Copy link
Collaborator

I think this impl has the same issue as FileLoader had before. That is multiple concurrent requests for the same url makes duplicate requests even if Cache is enabled. Second request can be sent before the the result is added to Cache. #10644 ImageBitmapLoader has the same issue.

@takahirox
Copy link
Collaborator

takahirox commented May 16, 2018

Plus, just in case can you confirm that there is no error with Audio + fetch on http-server + major browsers? http-server is a server which we Three.js recommend to use for local test.

https://threejs.org/docs/#manual/introduction/How-to-run-things-locally

http-server has an issue that it returns wrong mime type for binary file

jfhbrook/node-ecstatic#220 (comment) (Requesting to fix)

Because of it, FireFox createImageBitmap() fails with fetch on http-server.

@Mugen87
Copy link
Collaborator Author

Mugen87 commented May 16, 2018

I've tested with latest Chrome, FF and Safari and with a local HTTP server. The only problem was that Safari does not support the newer Promise-based syntax of decodeAudioData().

@Mugen87
Copy link
Collaborator Author

Mugen87 commented May 16, 2018

That is multiple concurrent requests for the same url makes duplicate requests even if Cache is enabled.

Yes, but this is a tradeoff I would accept for now. I'm in general not sure if the current implementation in FileLoader is the best solution for avoiding multiple concurrent requests to the same resource.

@takahirox
Copy link
Collaborator

takahirox commented May 16, 2018

Thanks for testing.

About multiple concurrent requests, agreed that we could improve the implementation but I think the inconsistency that some loaders will avoid them but others won't isn't good. And I remember that many users seemed to be confused by the concurrent requests before we updated FileLoader.

@takahirox
Copy link
Collaborator

Even if we accept, I think we need comment at least.

@Mugen87
Copy link
Collaborator Author

Mugen87 commented May 16, 2018

About multiple concurrent requests, agreed that we could improve the implementation but I think the inconsistency that some loaders will avoid them but others won't isn't good.

That's a valid point. We definitely need a solution for this.

Even if we accept, I think we need comment at least.

Good idea. I'll update the docs with a note.

@Mugen87
Copy link
Collaborator Author

Mugen87 commented May 16, 2018

One more thing: I want to highlight that the current implementation of AudioLoader leads to a runtime error when multiple request to the same URL are executed (see fiddle). The PR fixes this error and additionally a race condition (#10706). I vote to merge the PR or at least #13710, although cloning the audio buffer is not my preferred solution 😇.

https://jsfiddle.net/f2Lommf5/3935/

@mrdoob mrdoob added this to the r94 milestone May 16, 2018
@Mugen87
Copy link
Collaborator Author

Mugen87 commented May 17, 2018

We should also keep in mind that the progress of a fetch is not observable so far. AFAIK, there is no equivalent to the progress event of XMLHttpRequest (see whatwg/fetch#607). So the onProgress() callback that a user might provide will be ignored.

@takahirox
Copy link
Collaborator

@Mugen87
Copy link
Collaborator Author

Mugen87 commented May 17, 2018

I'm closing the PR for now since I'm not happy with the consequences of this change. A different approach is needed to solve the problems in context of loading audio buffer files. Cloning the audio buffer like suggested in #13710 is still better than introducing new limitations. Even #13725 is a more consistent solution.

@Mugen87 Mugen87 closed this May 17, 2018
@Mugen87 Mugen87 removed this from the r94 milestone May 17, 2018
@Mugen87
Copy link
Collaborator Author

Mugen87 commented May 17, 2018

@takahirox Thanks for your feedback in this PR 😊

@gkjohnson
Copy link
Collaborator

gkjohnson commented Nov 18, 2020

It looks like ReadableStream API can be used with fetch to achieve something akin to the onProgress callback with fetch:

https://javascript.info/fetch-progress

(and another reference)

And it seems it's been supported in all major browsers for a bit now. Maybe this can be revisited?

https://caniuse.com/?search=ReadableStream

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.

4 participants