-
Notifications
You must be signed in to change notification settings - Fork 226
When transcoding a video fails, send it as a file #4257
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
When transcoding a video fails, send it as a file #4257
Conversation
📱 Scan the QR code below to install the build (arm64 only) for this PR. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #4257 +/- ##
===========================================
- Coverage 79.99% 79.96% -0.04%
===========================================
Files 2107 2107
Lines 55847 55895 +48
Branches 6973 6992 +19
===========================================
+ Hits 44676 44696 +20
- Misses 8774 8798 +24
- Partials 2397 2401 +4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a first comment, I do not see any MonotonicTimelineInterpolator
in the changes. What am I missing?
The title is a bit outdated, this PR was just added so we could test solutions to fix an issue when trying to transcode videos before sending them (the linked one above) in some Android versions (MIUI for Android 12?). The original error said:
So I tried to fix that by using an interpolator that ensures the video timestamps always go forward, to see if that could solve the issue (it doesn't). This is either a problem with the codecs in the OS shipped by problematic vendors or an issue with the lib we're using to simplify the transcoding process. So I guess our options are either allowing videos to be sent as is if transcoding fails (either as a file or as the actual media) or using a different lib or transcoder for this. I'm waiting for a decision from @mxandreas , but the latest commit tries to implement the first solution. |
If "as is" means that the video size/quality will not be affected, but we can still remove the sensitive pieces (like geolocation tags), then I think that's fine. However, I would send them as "file" to have some indication that the transcoding was not applied. |
67839c4
to
98e1ef0
Compare
MonotonicTimelineInterpolator
in video transcoderAllow uploading the original file if it can't be transcoded.
Also, try to match bitrate and framerate.
98e1ef0
to
b7c7dbe
Compare
@jmartinesp what are we doing with that? |
…octet-stream` mime type
It's now ready for review (I just remembered we had to add a mime-type change). If the video can't be sent as a video it'll be sent as a file, as the description says. I think it matches what Andreas said above. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, maybe we should add some test on the VideoCompressor
class?
Actually, that's going to be difficult since that's where we're actually performing the video transcoding. Maybe I can extract the transcoding configuration logic somewhere else and test that instead. |
|
Content
Changes:
While reviewing you might want to skip/not thoroughly review commit 64ba34c since it's reworked shortly after.
Motivation and context
Fixes #4243, or at least adds a workaround.
Tests
I was able to make the transcoding fail with this video in a Xiaomi Mi 9T with Android 14.
m.video
message instead of am.file
one, but it seems like this might need to be changed in the SDK.Tested devices
Checklist