Fix chunk request byte header math #45
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
I believe this is the crash I ran into -- does the For context I was using the simulator, which doesn't have any videos built into the camera roll, I downloaded a video from a web page inside the simulator. That might have led to a a funky video file vs. something that was recorded on device. |
daytime-em
reviewed
Jun 29, 2023
andrewjl-mux
commented
Jun 29, 2023
| @@ -170,7 +170,15 @@ fileprivate actor ChunkActor { | |||
| let urlSession: URLSession | |||
|
|
|||
| func upload() async throws -> URLResponse { | |||
| let contentRangeValue = "bytes \(chunk.startByte)-\(chunk.endByte - 1)/\(chunk.totalFileSize)" | |||
| let startByte = "\(chunk.startByte)" | |||
There was a problem hiding this comment.
Using the subtraction method that reports overflow might be a good alternative here too.
Yes I suspect there is either an issue with the file or with the handle we get to the file via PhotosKit
I tried downloading an mp4 file to the camera roll on the Simulator and I don't see the file inside the example app. Did you have to do something special for it to appear? 
|
|
I guess if the final SDK behavior makes sense (with the crash fix) then I'm willing to come back later for input validation. Up to you guys, but I'll give a ✅ |
daytime-em
approved these changes
Jun 29, 2023
|
I used @dylanjha example app and I have a set of consistent repro steps for this crash on the main branch. Also confirmed that this branch does not crash, though it still results in an upload error. Will dig in separately. Repro Steps
@daytime-em should call out that it does not happen if following the above steps on the simulator and uploading a file that was dragged into the camera roll directly as you mentioned before. (Something different about the resulting file in this case. Worth digging into for sure.) |
andrewjl-mux
added a commit
that referenced
this pull request
Jul 19, 2023
cjpillsbury
pushed a commit
that referenced
this pull request
Jul 20, 2023
andrewjl-mux
added a commit
that referenced
this pull request
Jul 20, 2023
Merged
andrewjl-mux
added a commit
that referenced
this pull request
Jul 20, 2023
andrewjl-mux
added a commit
that referenced
this pull request
Jul 21, 2023
* Project Input standardization (#17) (#41) (#46) (#48) (#57) (#77) Add AVFoundation and PhotosKit initializers Add internal and external state mapping Remove duplicate status enum and add inline docs to external status Add inline API docs to PHAsset-based MuxUpload constructor Consolidate all `MuxUpload` options into a single struct `UploadOptions` Declare asynchronous MuxUpload constructor in PHAsset extension Place extension methods into dedicated directories Polish inline API documentation Add new API documentation and note the placeholder implementation Add option variants as static members: defaults, disabled inputStandardization Deprecate existing initializer, normally this API should be removed prior to GA, but since it was the only initializer exposed up to this point removing it would break everybody. Instead deprecate and remove at a later date. Store all MuxUpload-related options in UploadInfo Use correct starting byte parameter when restarting upload If input standardized, standardized input URL is passed to UploadInfo instead of the original input URL used for initializer Note: SDK probably needs to re-export a high quality asset anyway so possibly need a bridging status Add dedicated internal initializer for MuxUpload error with unknown error code Request local and remote assets Standardize via AVFoundation asset export session Expose hook for client to cancel upload if standardization failed Call cancellation hook if inspection fails. We're not sure if the input is standard or not so better to be safe and confirm Export based on maximum resolution set by client Cleaner non standard input handler invocation Add CustomStringConvertible conformance to maximum resolution (#56) Only mark upload as started if its ready Safe storage for MuxUpload (#71) Intended to prevent a crash if MuxUpload is extended by the SDK client to conform to Equatable or Hashable protocols Switch order of operations to avoid long pause on fetching duration AVAsset sometimes hangs when asked to asynchronously fetch duration when there aren't audio or video tracks present. To avoid this after starting the upload, the inspection step will get the video tracks first and get the duration afterwards. --------- Co-authored-by: Emily Dixon <edixon@mux.com> * Minor example app renaming (#29) * Use a UUID string as MuxUpload internal identifier (#30) * Display a more specific error message when the direct upload POST request fails (#32) * Use MuxUpload id instead if the input URL when looking up or writing state in the SDK (#33) * Change upload creation example app method to use discardableResult (#34) * Add dedicated internal initializer for MuxUpload error with unknown error code (#35) * Rename enum and adjust to camel casing (#36) * Adhere to Swift formatting guidelines, remove snake casing (#37) * Fix potential crash in ChunkedFile (#38) * Include Cloud shared asset sources when requesting assets (#40) * Prevent arithmentic overflow when setting chunk content range value (#45) * Remove force unwrap that can cause a crash (#47) * Make internal class methods internal (#51)
tomkordic
pushed a commit
that referenced
this pull request
Mar 1, 2024
* Project Input standardization (#17) (#41) (#46) (#48) (#57) (#77) Add AVFoundation and PhotosKit initializers Add internal and external state mapping Remove duplicate status enum and add inline docs to external status Add inline API docs to PHAsset-based MuxUpload constructor Consolidate all `MuxUpload` options into a single struct `UploadOptions` Declare asynchronous MuxUpload constructor in PHAsset extension Place extension methods into dedicated directories Polish inline API documentation Add new API documentation and note the placeholder implementation Add option variants as static members: defaults, disabled inputStandardization Deprecate existing initializer, normally this API should be removed prior to GA, but since it was the only initializer exposed up to this point removing it would break everybody. Instead deprecate and remove at a later date. Store all MuxUpload-related options in UploadInfo Use correct starting byte parameter when restarting upload If input standardized, standardized input URL is passed to UploadInfo instead of the original input URL used for initializer Note: SDK probably needs to re-export a high quality asset anyway so possibly need a bridging status Add dedicated internal initializer for MuxUpload error with unknown error code Request local and remote assets Standardize via AVFoundation asset export session Expose hook for client to cancel upload if standardization failed Call cancellation hook if inspection fails. We're not sure if the input is standard or not so better to be safe and confirm Export based on maximum resolution set by client Cleaner non standard input handler invocation Add CustomStringConvertible conformance to maximum resolution (#56) Only mark upload as started if its ready Safe storage for MuxUpload (#71) Intended to prevent a crash if MuxUpload is extended by the SDK client to conform to Equatable or Hashable protocols Switch order of operations to avoid long pause on fetching duration AVAsset sometimes hangs when asked to asynchronously fetch duration when there aren't audio or video tracks present. To avoid this after starting the upload, the inspection step will get the video tracks first and get the duration afterwards. --------- Co-authored-by: Emily Dixon <edixon@mux.com> * Minor example app renaming (#29) * Use a UUID string as MuxUpload internal identifier (#30) * Display a more specific error message when the direct upload POST request fails (#32) * Use MuxUpload id instead if the input URL when looking up or writing state in the SDK (#33) * Change upload creation example app method to use discardableResult (#34) * Add dedicated internal initializer for MuxUpload error with unknown error code (#35) * Rename enum and adjust to camel casing (#36) * Adhere to Swift formatting guidelines, remove snake casing (#37) * Fix potential crash in ChunkedFile (#38) * Include Cloud shared asset sources when requesting assets (#40) * Prevent arithmentic overflow when setting chunk content range value (#45) * Remove force unwrap that can cause a crash (#47) * Make internal class methods internal (#51)
# for free
to join this conversation on GitHub.
Already have an account?
# to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
A crash from an arithmetic overflow happens when subtracting from
UIntthat is equal to zero.This is a quick fix for the crash, there's still a remaining issue of how to handle an
endByte.This is likely indicating there's some sort of issue reading the file and the chunk request associated with this content range will most likely fail. That will treated by the SDK as an error more gracefully than just crashing.