-
Notifications
You must be signed in to change notification settings - Fork 6
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
Using lib in WPAndroid video compression scenario #652
Conversation
…nd Mp4ComposerEngine
…e case requested.
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.
HEAD so placing a note for the reviewer to consider if that's correct or we need to merge develop in
yes we should update it 😅 it's been a while since the PR was up, let me know if you want to to do that or I can start another branch off your working branch to make he merge on it otherwise 👍, and you could review, whatever works for you 👍
Meanwhile I suppressed MemberName, MethodName and similar with SuppressWarnings and fixed formatting issues like spacing and blank lines. This seems to have worked for avoiding linter issues in WPAndroid PR but not here.
That's ok I think - if there's a ton of warnings product of including the new files from the original library we could also potentially update the linter baseline file, given we have no plans on fixing all of them.
NOTE 2: all over the places timeScale became int instead of float, should be ok with the current use cases, but wondering if it could make any sense to return it to float. Wdyt?
It makes sense to convert them all to float, yes.
I gave it a first pass and tested it on a device, I really liked how you approached it with keeping most of the originals while still adding just enough flexibility to parameterize it and expose the needed functionality through the ComposerInterface
interface.
I think this change is in the right path to make use of one solution for both stories and WPAndroid, so feel free to update the branches with latest develop
on both codebases and let's do another round of testing. I'm approving it
Thanks a ton for this change @develric ! and thanks for the patience as well 🙏 😄
Hey @mzorz 👋 .
As we discussed given that parameter is not currently used in our scenarios and it would require decoupling/changes in the I merged from develop and this should be ready for another pass 🙇 |
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.
I tested this with several videos, works very well 👍 nice job @develric summarizing here, going to merge this one and then update the WPAndroid branch with latest commit hash:
Pixel 4a:
- tried with videos on device of a size smaller than the optimize target, verified the original file is returned
- tried with videos on device of a size larger than the optimize target, verified the conversion happens
- tried with videos on google drive, also worked fine for both cases above
- Also tested the optimizer gets called as well for videos being added to Stories (kind of schroedinger cat), and verified it plays along well with the web player (i.e. when you upload a video and add text to a video story slide, the mp4composer will be used once to process the new video, then called another time for optimizing)
- Also tested turning optimization off, verified it doesn't go through the mp4composer optimization process.
Samsung J2:
- tried with videos on device of a size smaller than the optimize target, verified the original file is returned
- tried with videos on device of a size larger than the optimize target, verified the conversion happens
- Also tested turning optimization off, verified it doesn't go through the mp4composer optimization process.
This PR tries to restore some of the files needed to use the library for video compression in WPAndroid. I tried to keep the files from this lib as unchanged as possible so to not affect the current behaviour in the stories scenarios.
NOTE 1: the linked commit in WPAndroid is deaf18d so I branched from there for this changes. That commit is not develop HEAD so placing a note for the reviewer to consider if that's correct or we need to merge develop in (and I suspect at least some of the errors in lint and installable build are actually resolved in develop, right? 😄 ). Thanks for advice 🙇 . Meanwhile I suppressed
MemberName
,MethodName
and similar withSuppressWarnings
and fixed formatting issues like spacing and blank lines. This seems to have worked for avoiding linter issues in WPAndroid PR but not here.NOTE 2: all over the places
timeScale
became int instead of float, should be ok with the current use cases, but wondering if it could make any sense to return it to float. Wdyt?I introduced a ComposerProvider to get the correct composer based on the use case requested.
The following files were reintroduced without any modification from the originals (a part minors needed to fix or skip linters issues):
The following files were reintroduced with few changes from the originals:
PS: as said tried to minimize the changes to the original flows so to not affect the stories scenarios, but happy to collaborate to identify and run relevant stories tests to avoid any regression 😄