-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
[Dependency Updates] Reconfigure Media-for-Mobile Dependency #17903
Conversation
It is generally recommended that transitively used dependencies should be declared directly.
Found 1 violations: The PR caused the following dependency changes:+\--- com.github.indexos.media-for-mobile:domain:43a9026f0973a2f0a74fa813132f6a16f7499c3a
Please review and act accordingly
|
📲 You can test these changes on WordPress by downloading wordpress-installable-build-pr17903-124aead.apk
|
📲 You can test these changes on Jetpack by downloading jetpack-installable-build-pr17903-124aead.apk
|
Hey @ParaskP7 👋 , you are bringing me back to a looong time ago 😄. This internal post should link a bit the dots (p2-pbArwn-2gH). The TL;DR being that I think the reason for starting using the compression embedded in the stories lib was because of a crash issue with the m4m library that is not maintained actively and the original contribution fix o that library never got merged by the repo owners. I'm not sure I can deeper help on this soon-ish (not this week at least) but I see you are targeting the "Future" milestone so hopefully this is not super urgent and I can join the party next sprint. Gut feeling I agree it should be possible to remove the |
👋 @develric !
I know right! 😄
Thanks so much for adding some context here, much appreciated! 💯
Definitely, reviewing and testing this PR is not urgent, so please take your time with it, next week or otherwise. No matter, thanks so much for the heads-up on your availability. 👍 FYI: The changes in this PR is mainly PS: We might also take the opportunity to see how to move forward with media-for-mobile as it seems that at some point we might be forced, at least maintainability-wise. 🤔
Got it! 👍
Definitely helps, thank you Riccardo! 🙇 |
📲 You can test the changes from this Pull Request in WordPress by scanning the QR code below to install the corresponding build.
|
📲 You can test the changes from this Pull Request in Jetpack by scanning the QR code below to install the corresponding build.
|
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.
Hey @ParaskP7 👋 ! This LGTM. I tested adding videos in WP and JP with/without video compression from Media Library, and Editor.
PS: about the M4mVideoOptimizer
class, I think I confirm my gut feeling. I guess the common base part was extracted from VideoOptimizer
into VideoOptimizerBase
and aiming to in principle support different video compressors the specifics were enforced into Mp4ComposerVideoOptimizer
and (for analogy) M4mVideoOptimizer
. Since this last is not currently used, I think makes sense to remove it as you did (we still have the Mp4ComposerVideoOptimizer
that can serve as an implementation reference if ever needed)
Thanks so much for review and testing this @develric , much appreciated! 🙇 ❤️ 🚀
Great, thanks so much for the confirmation and clarification on that, bye bye |
This PR is mainly about reconfiguring the media-for-mobile dependency, in the sense of extracting its version (5cfcfd1), adding its transitive dependency directly (233ccde) and removing the unused
M4mVideoOptimizer
class (d40542d).FYI: This media-for-mobile dependency was added as part of this PR (see 4a7c9c9 commit) for video optimization purposes (Cc @daniloercoli). Then, as part of this PR (see 61ebbbc commit), the unused
M4mVideoOptimizer
implementation was added as well, that is, along side theMp4ComposerVideoOptimizer
implementation.This change will help with maintaining and updating this media-for-mobile dependency. Also, opening this PR gives me the opportunity to ask a couple of questions about this library, its maintenance and implementation overall:
Jun 15, 2017
), what's the state of this project, should it be considered outdated, should we start maintaining it more actively. Also, this project is under the INDExOS umbrella, I am not sure what that means, can you shed some light on that too? 🤔PS: @develric I added you as the main reviewer, that is, in addition to the @wordpress-mobile/apps-infrastructure team itself, but not entirely randomly as I am seeing you having some experience on working with video optimization, and want someone from the WordPress team to be aware of and sign-off on that change for WPAndroid.
To test:
Regression Notes
Potential unintended areas of impact
media-for-mobile:domain
transitive dependency added might be causing some kind of misbehaviour.What I did to test those areas of impact (or what existing automated tests I relied on)
To test
section above.What automated tests I added (or what prevented me from doing so)
PR submission checklist:
RELEASE-NOTES.txt
if necessary.