-
-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Videos greater than 25MB uploaded as m.file rather than m.video (clone of 1752 from riot-ios) #6426
Comments
Any updates on this bug? |
Thanks Michael. Will try to get this across to the Chrome devs and see if there's anything they can do to rectify. |
code section where error is visible; https://github.com/matrix-org/matrix-react-sdk/blob/a1581ad2dc4cbb196867a1fd2c66f60e04d2bef6/src/ContentMessages.js#L165-L167 |
Tested this in Firefox on client hosted at riot.im today and was able to reproduce the issue. Does not seem to be limited to Chrome. Don't see the same ffmpeg error in the console though. |
I also confirmed that even on Firefox 60.0.1 (64-bit), in a non-E2E room, this problem occurs. |
The code path doesn't log to console, you need to breakpoint on it |
I have a breakpoint on lines 165 through 167 in ContentMessages.js as you indicated above, but I don't receive an error in the console on upload completion even when the upload is incorrectly categorized as m.file. |
Starting to wonder if this might be related to #4264, except it doesn't quite go as far as crashing the app. Chrome/Electron doesn't seem to like large files sent as file objects. |
I think this one is much more closely related to #7069 given I had a 4.77kb image end up being an m.file. |
This got fixed |
Copy of 1752 from riot-ios.
On all versions of the Riot app (web, Android, iOS), any video file over 25MB is uploaded as an m.file rather than m.video, and is not presented in an embedded HTML5 player as a result. Would be great to be able to specify where this cutoff is in case we want to use this for sending larger screeners.
It looks like @manuroe already has this in the pipeline on the iOS side, per @turt2live posting here to make sure there's visibility on the desktop app side as well.
Thanks!
The text was updated successfully, but these errors were encountered: