Add support for large video file uploads (tus.io) #46
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
…support System.Threading.Tasks
- Created `VimeoUploader` class - Created `VideoChunk` class to represent chunks of binary video data - Tested the `Tus.io` api upload mechanisem (commented out in the `VimeoApi`)
…e inspector 🙃 Plus formatting the changed files
`VimepUploader.cs` should be `VimeoUploader.cs`
caseypugh
reviewed
Oct 8, 2018
caseypugh
reviewed
Oct 9, 2018
caseypugh
reviewed
Oct 9, 2018
…ad of storing a reference to it 🙌
…en created instead of at the end of an upload This resolved situation where an error in upload makes the video uploaded "Untitled" and public
- Setting up the video (privacy/name/review page) at the begining of the upload - Implementing VimeoUplader as a derived class from the VimeoApi base class - Implementing events (OnUpload...) - Fixing typos (e.g progress in VimeoUploader) - Cleaning up whitespaces and comments - Fixing misc PR comments
caseypugh
changed the title
caseypugh
suggested changes
Oct 17, 2018
There was a problem hiding this comment.
Overall it's looking great! 🎉
Since we now have a real test suite, there is still one major todo here. You wrote two new classes: VimeoUploader and VideoChunk, both of which should come with unit tests.
Also one other thing we should do is hide all the Vimeo config settings while recording/uploading since it is all set in the beginning now.
| r.SetRequestHeader("Accept", "application/vnd.vimeo.*+json;version=" + apiVersion); | ||
| } | ||
|
|
||
| public static bool IsNetworkError(UnityWebRequest req) { | ||
| protected void TriggerDerivedOnProgress(string status, float progress) |
There was a problem hiding this comment.
What does TriggerDerived mean? is that a tus term?
…on single chunk uploads
…ferent chunk sizes
…nks even after upload
caseypugh
reviewed
Oct 22, 2018
| @@ -18,6 +18,12 @@ public class VimeoRecorder : RecorderSettings | |||
| public bool isRecording = false; | |||
| public bool isUploading = false; | |||
| public float uploadProgress = 0; | |||
| private int m_customByteChunkSize = 1024 * 1024 * 128; | |||
| public int customByteChunkSize { | |||
There was a problem hiding this comment.
why not just call this byteChunkSize ?
caseypugh
reviewed
Oct 22, 2018
|
|
||
| private bool HasChunksLeftToUpload() | ||
| { | ||
| if (currentChunkIndex < m_chunks.Count) { |
There was a problem hiding this comment.
a more future proof way of checking would be to create a method called GetTotalRemainingChunks which loops through to check status. e.g.
private bool HasPendingChunksToUpload()
{
return GetTotalRemainingChunks() == 0;
}
caseypugh
reviewed
Oct 22, 2018
…ing error better
…ld be tested sepratly for large uploads
…into tus-upload
caseypugh
approved these changes
Oct 23, 2018
… in the build process on older versions of Unity (i.e Unity 2017)
…s well as the video itself inside the Tests folder
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in 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.
Why?
What was done?
VimeoUploaderandVideoChunkto support the Vimeo tus.io upload APIVimeoUploaderextendsVimeoApito avoid repetition and code duplication[Timeout(ms)]Future
VimeoPublisherand dry it up withVimeoUploadersince they are both kind of doing the same thing. Also need to giveVimeoVideomore responsibility which will help untangle code.Tested against the following versions
2018.2.13f1,2018.2.9f1,2018.2.6f1,2018.2.4f1,2018.2.1f1,2018.1.6f1,2017.1.5f1,2017.1.4f1For local development
If you are working off of
masterand not the latest release, you will need to delete theAssets/Testsfolder and assemblies before building an executable. Our releases will not ship with these files.