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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use a fallback default video resolution when it can't be obtained from mediaMetadataExrtactor #621

Merged
merged 1 commit into from
Dec 4, 2020

Conversation

mzorz
Copy link
Contributor

@mzorz mzorz commented Dec 3, 2020

Fixes #620

Issue
It was observed When using a .mov video located on the server (https://..., with a .mov video that was previously uploaded to a site's Media section), the player and the composer would behave randomly. As per my tests, it definitely does work right after uploading it to the Media section, but ceases to work after a few minutes of having it uploaded, and then it only works again if, after selecting the video and leaving the player playing in a loop, you then proceed to try saving the Story. By "work" I mean, the media metadata extractor is able to obtain the video resolution and other metadata without a problem, while it returns null values for the same keys in other occasions on the same video / same url.

Weird as it sounds, I couldn't find the correlation yet but I believe there might be the Android 10 permissions handling coming into play, since the targetSdkVersion was changed on WPAndroid to API level 29. I still have to dig more into it, and I think one potential safer approach would be to first download the video to a local copy, before attempting to extract and make the composition by reading the remote video file into a buffer on the fly.

For now, this PR only takes care for the exact crash described above.

Fix
This PR catches the possible exceptions when trying to figure out the original video resolution.

However, when the mediaExtractor can't obtain the video height and width, it will likely also fail just a few steps later when trying to obtain any other relevant information for the composer to work.

In the tests I've made, it will fail in obtaining the default track to read, for example here:

2020-12-03 18:34:31.854 22891-23445/org.wordpress.android E/PhotoEditor: onFailed()
    java.lang.IllegalArgumentException
        at android.media.MediaExtractor.getTrackFormatNative(Native Method)
        at android.media.MediaExtractor.getTrackFormat(MediaExtractor.java:590)
        at com.daasuu.mp4compose.composer.Mp4ComposerEngine.compose(Mp4ComposerEngine.kt:159)
        at com.daasuu.mp4compose.composer.Mp4ComposerEngine.composeFromVideoSource(Mp4ComposerEngine.kt:72)
        at com.daasuu.mp4compose.composer.Mp4Composer$start$1.run(Mp4Composer.kt:199)
        at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1167)
        at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:641)
        at java.lang.Thread.run(Thread.java:919)
2020-12-03 18:34:31.951 22891-23114/org.wordpress.android I/WordPress-STATS: 🔵 Tracked: notification_shown, Properties: {"notification_type":"story_save_error"}

The app won't crash and will show an error notification, but from what I could see it is likely not a retriable error. You will be offered the option to retry as per the error handling mechanism in place, but chances are it's easier to discard the Story and start all over.

At least, with this patch won't crash the app, but this is certainly a thing we need to continue looking into further.

To test:

CASE A: (recent added video doesn't crash)

  1. upload a .mov video to the Media section of your test site
  2. create a story and when the media picker appears, choose the W icon, then choose the video you just uploaded
  3. add a text
  4. publish the story
  5. observe it saves it and publishes correctly

CASE B: (after some minutes)

  1. after doing test case A, now wait some minutes or restart the app
  2. try to create the story and observe the video now shows an error when added to the Story as a slide (it cannot be played!)
  3. try to save it and observe it doesn't crash anymore, although an error notification will appear.

I'll cotinue working on this, but for now this patch should at least save the crash.

@@ -349,6 +349,15 @@ class Mp4Composer {
val height = Integer.valueOf(retriever.extractMetadata(MediaMetadataRetriever.METADATA_KEY_VIDEO_HEIGHT))

return Size(width, height)
} catch (e: IllegalArgumentException) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see this same catch block set in getVideoRotation()

@mzorz mzorz requested a review from aforcier December 3, 2020 21:58
@aforcier aforcier self-assigned this Dec 4, 2020
@aforcier
Copy link
Collaborator

aforcier commented Dec 4, 2020

Tested and confirmed it seems to avert a crash 👍

@aforcier aforcier merged commit 8ae4d19 into develop Dec 4, 2020
@aforcier aforcier deleted the issue/crash-number-format-exception branch December 4, 2020 08:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Crash: NumberFormatException in Mp4Composer
2 participants