-
Notifications
You must be signed in to change notification settings - Fork 24.3k
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
BlobManager: implement Blob from ArrayBuffer #39276
BlobManager: implement Blob from ArrayBuffer #39276
Conversation
Thank you for your pull request and welcome to our community. Action RequiredIn order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you. ProcessIn order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA. Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with If you have received this in error or have any questions, please contact us at cla@meta.com. Thanks! |
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks! |
|
@dmytrorykun has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
This pull request was successfully merged by @CodyJasonBennett in a228b0f. When will my fix make it into a release? | Upcoming Releases |
Summary: As per #41079, we're outputting ASCII encoded data URIs to `FileReader.readAsDataURL` due to lack of native `ArrayBuffer` support and unclear use of encoding to align with web. I'll revisit this at a later point with a better testing strategy once we have a good idea of how this should behave internally. Aside from purely reverting #39276, I've kept the use of `ArrayBuffer.isView(part)` to the previous `part instanceof global.ArrayBufferView` since it is more correct. ## Changelog: [INTERNAL] [REMOVED] - Revert Blob from ArrayBuffer Pull Request resolved: #41170 Test Plan: Run the following at the project root to selectively test changes: `jest packages/react-native/Libraries/Blob` Reviewed By: cipolleschi Differential Revision: D50601036 Pulled By: dmytrorykun fbshipit-source-id: 0ef5c960c253db255c2f8532ea1f44111093706c
facebook#41170) Summary: As per facebook#41079, we're outputting ASCII encoded data URIs to `FileReader.readAsDataURL` due to lack of native `ArrayBuffer` support and unclear use of encoding to align with web. I'll revisit this at a later point with a better testing strategy once we have a good idea of how this should behave internally. Aside from purely reverting facebook#39276, I've kept the use of `ArrayBuffer.isView(part)` to the previous `part instanceof global.ArrayBufferView` since it is more correct. ## Changelog: [INTERNAL] [REMOVED] - Revert Blob from ArrayBuffer Pull Request resolved: facebook#41170 Test Plan: Run the following at the project root to selectively test changes: `jest packages/react-native/Libraries/Blob` Reviewed By: cipolleschi Differential Revision: D50601036 Pulled By: dmytrorykun fbshipit-source-id: 0ef5c960c253db255c2f8532ea1f44111093706c
Thank you for this important patch @CodyJasonBennett. Is there a way to track in which RN version this has landed? |
Summary:
Fixes a critical networking path used commonly in graphics code, among many other use-cases. It is commonplace to store images within a single binary such as a GLB (see KHR_binary_glTF). Base64 encoded glTF can be critically slow in older browsers with an increased size, so it's not a suitable fallback for those targeting multiple platforms. Furthermore, large base64 payloads can cause crash behavior on native when passed via JSI (e.g.
expo-gl
). For textures, this can be several megabytes of high precision data, and a uri from a blob is preferred rather than writing platform-specific workarounds via the filesystem.A user-land patch I've employed in pmndrs/react-three-fiber, as well as existing user-land patches, is to test the
Blob
constructor with an emptyArrayBuffer
.base64-js
is used here since it's already installed withreact-native
for binary utils elsewhere. This is declared as a dependency in the linked library since this is an implementation detail, but will be de-duplicated at install time.Changelog:
[INTERNAL] [FIXED] - Implement Blob from ArrayBuffer
Test Plan:
Run the following at the project root to selectively test changes:
jest packages/react-native/Libraries/Blob