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

FileUploader: add case for video upload size #465

Closed
wants to merge 2 commits into from

Conversation

pthieu
Copy link

@pthieu pthieu commented Sep 9, 2018

Twitter allows video to be 512MB, so limiting to 15MB is severely
limiting a lot of functionality. This will keep all formats that are not video to
keep the same beahviour while changing the allowed total file byte size and max
chunk byte size to allow for correct limits on videos

Related to: #461

@pthieu
Copy link
Author

pthieu commented Sep 10, 2018

This should actually rebase off of #462 but there isn't a branch for that.

@Arahain
Copy link

Arahain commented Oct 10, 2018

This worked for me, thank you!

I did however remove the lines that change the max Chunk size, because twitter allows chunks to be max 5 MB in size and not 15 MB like in your change: https://developer.twitter.com/en/docs/media/upload-media/api-reference/post-media-upload-append.

@pthieu
Copy link
Author

pthieu commented Oct 10, 2018

@Arahain: it's not explicitly clear but you can do 15mb chunks for video. The 15 mb (sync) in this link: https://developer.twitter.com/en/docs/media/upload-media/uploading-media/media-best-practices.html alludes to this

@Arahain
Copy link

Arahain commented Oct 10, 2018

@pthieu You are refering to the line "File size should not exceed 15 mb (sync) / 512 mb (async)" in the Video specifications and recommendations sections? Because I think this refers to the total file size and not the chunk size. I did test it with 15MB chunk size while printing the respons I get from twitter after appending each chunk (so after line 57 in your file_uploader.js) and this is the response:
statusCode : 413, statusMessage : "Request Entity Too Large"

The chunk size is only references in the link in my previous post: "The raw binary file content being uploaded. It must be <= 5 MB, and cannot be used with media_data."

@pthieu
Copy link
Author

pthieu commented Oct 10, 2018

@Arahain: interesting. I am using this in production right now and it sends 15mb chunks. Maybe something else is different in our use-cases. I'm using .mp4 files. I'll do another test when I get a chance.

@Arahain
Copy link

Arahain commented Oct 10, 2018

@pthieu Oh right. In your code, the MAX_VIDEO_CHUNK_BYTES variable gets defined (line 9) and later ond line 134 you also defined a local variable MAX_CHUNK_BYTES but neither of these are actually used. I did only get the error mentioned above after I changed the code to actually use these variables, sorry for not stating this before.

So your code works find as it is but it defines a few unused variables that led me to believe I should use them which led to the error mentioned above.

@pthieu
Copy link
Author

pthieu commented Oct 10, 2018

@Arahain interesting, I might have rebased, created the PR, then added more commits but didn't push. I'll check when I get in my laptop

@pthieu pthieu force-pushed the video/upload-chunk-size branch from 0ecedee to 2e0dcb2 Compare November 9, 2018 05:12
@pthieu
Copy link
Author

pthieu commented Nov 10, 2018

@Arahain: I've taken a look at my code and I was indeed missing some lines that might have gotten missed in my rebase. MAX_VIDEO_CHUNK_BYTES now works. It looks like my production copy was using my latest code, but for some reason it didn't make it into the PR.

@pthieu pthieu force-pushed the video/upload-chunk-size branch from 2e0dcb2 to b4b9cde Compare November 12, 2018 05:50
Twitter allows video to be 512MB, so limiting to 15MB is severely
limiting a lot of functionality. This will keep all formats not video to
remain the same while changing the allowed total file byte size and max
chunk byte size to allow for correct limits
@pthieu pthieu force-pushed the video/upload-chunk-size branch from b4b9cde to ad9376a Compare November 12, 2018 05:55
TomFromThePool pushed a commit to TomFromThePool/twit that referenced this pull request Oct 13, 2019
@pthieu pthieu closed this Mar 21, 2024
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.

2 participants