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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Dependency Updates] Reconfigure Media-for-Mobile Dependency #17903

Merged
merged 7 commits into from
Apr 19, 2023

Conversation

ParaskP7
Copy link
Contributor

@ParaskP7 ParaskP7 commented Feb 7, 2023

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 the Mp4ComposerVideoOptimizer 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:

  1. @daniloercoli since you were the last to contribute to the media-for-mobile project (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? 馃
  2. @develric since you were the one that added this unused M4mVideoOptimizer implementation. Is that really as unused as it seems, is there a reason for that, is it safe to remove this implementation? 馃

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:

  1. See the dependency tree diff result and verify correctness.
  2. Smoke test any video optimization related functionality on both, the WordPress and Jetpack apps, and see if they both work as expected. For more info on how to test the video optimization you can follow the testing instructions on this Using stories lib in video compression scenario聽#14256 PR.

Regression Notes

  1. Potential unintended areas of impact

    • The media-for-mobile:domain transitive dependency added might be causing some kind of misbehaviour.
  2. What I did to test those areas of impact (or what existing automated tests I relied on)

    • See To test section above.
  3. What automated tests I added (or what prevented me from doing so)

    • N/A

PR submission checklist:

  • I have completed the Regression Notes.
  • I have considered adding accessibility improvements for my changes.
  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

@ParaskP7 ParaskP7 added this to the Future milestone Feb 7, 2023
@ParaskP7 ParaskP7 self-assigned this Feb 7, 2023
@ParaskP7 ParaskP7 mentioned this pull request Feb 7, 2023
50 tasks
@wpmobilebot
Copy link
Contributor

Found 1 violations:

The PR caused the following dependency changes:

+\--- com.github.indexos.media-for-mobile:domain:43a9026f0973a2f0a74fa813132f6a16f7499c3a

Please review and act accordingly

@wpmobilebot
Copy link
Contributor

wpmobilebot commented Feb 7, 2023

WordPress馃摬 You can test these changes on WordPress by downloading wordpress-installable-build-pr17903-124aead.apk
馃挕 Scan this QR code with your Android phone to download and install the APK directly on it.
AppWordPress
Build FlavorJalapeno
Build TypeDebug
Commit124aead
Note: This installable build uses the JalapenoDebug build flavor, and does not support Google Login.

@wpmobilebot
Copy link
Contributor

wpmobilebot commented Feb 7, 2023

Jetpack馃摬 You can test these changes on Jetpack by downloading jetpack-installable-build-pr17903-124aead.apk
馃挕 Scan this QR code with your Android phone to download and install the APK directly on it.
AppJetpack
Build FlavorJalapeno
Build TypeDebug
Commit124aead
Note: This installable build uses the JalapenoDebug build flavor, and does not support Google Login.

@ParaskP7 ParaskP7 requested a review from a team February 13, 2023 15:08
@develric
Copy link
Contributor

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 M4mVideoOptimizer that I think was added to uniform the approach with the Mp4ComposerVideoOptimizer but apparently is not used any more as you say 馃 . Hope this at least helps clarify the context meanwhile 馃檱

@ParaskP7
Copy link
Contributor Author

馃憢 @develric !

Hey @ParaskP7 馃憢 , you are bringing me back to a looong time ago 馃槃.

I know right! 馃槃

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 INDExOS/media-for-mobile#100 o that library never got merged by the repo owners.

Thanks so much for adding some context here, much appreciated! 馃挴

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.

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 M4mVideoOptimizer related so I think just verifying that is unused should do it.

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. 馃

Gut feeling I agree it should be possible to remove the M4mVideoOptimizer that I think was added to uniform the approach with the Mp4ComposerVideoOptimizer but apparently is not used any more as you say 馃 .

Got it! 馃憤

Hope this at least helps clarify the context meanwhile 馃檱

Definitely helps, thank you Riccardo! 馃檱

@wpmobilebot
Copy link
Contributor

wpmobilebot commented Mar 16, 2023

WordPress馃摬 You can test the changes from this Pull Request in WordPress by scanning the QR code below to install the corresponding build.
App NameWordPress WordPress
FlavorJalapeno
Build TypeDebug
Versionpr17903-da8bc30
Commitda8bc30
Direct Downloadwordpress-prototype-build-pr17903-da8bc30.apk
Note: Google Login is not supported on these builds.

@wpmobilebot
Copy link
Contributor

wpmobilebot commented Mar 16, 2023

Jetpack馃摬 You can test the changes from this Pull Request in Jetpack by scanning the QR code below to install the corresponding build.
App NameJetpack Jetpack
FlavorJalapeno
Build TypeDebug
Versionpr17903-da8bc30
Commitda8bc30
Direct Downloadjetpack-prototype-build-pr17903-da8bc30.apk
Note: Google Login is not supported on these builds.

Copy link
Contributor

@develric develric left a 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. :shipit:

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)

@ParaskP7
Copy link
Contributor Author

Hey @ParaskP7 馃憢 ! This LGTM. I tested adding videos in WP and JP with/without video compression from Media Library, and Editor. :shipit:

Thanks so much for review and testing this @develric , much appreciated! 馃檱 鉂わ笍 馃殌

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)

Great, thanks so much for the confirmation and clarification on that, bye bye M4mVideoOptimizer, see you on the other side! 馃憢 馃槄 馃憢

@ParaskP7 ParaskP7 merged commit c0bfef0 into trunk Apr 19, 2023
2 checks passed
@ParaskP7 ParaskP7 deleted the deps/reconfigure-media-for-mobile-dependency branch April 19, 2023 15:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants