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

Compress video and improve file too big error detection #3264

Merged
merged 32 commits into from
May 5, 2021

Conversation

bmarty
Copy link
Member

@bmarty bmarty commented Apr 30, 2021

Add support to compress video before sending.

Improve file too big detection. It's now handle by the SDK, since the file can be compressed, and so become acceptable for the server. The message bottom sheet now display the "too big file" error to the user.

image

Also the SDK checks the size after the encryption of the file (can be bigger when encrypted).

Thumbnails are now sent after the main content, to avoid sending them and then get an error when sending the main file.

Add support for preview of video files (no play preview though, just an indicator in the strip to show it's a video), to let the user selects the option to send the original uncompressed video.

image

Remaining issue (to be fixed in another PR?): if file is too large for the server when retrying, it forces to compress the file and so it has more chance to work. But it is not the original intent from the user.

This PR also:

  • fixes a regression on the fail to send indicator color.
  • avoid "soft_logout" to be serialized by making it optional

EDIT:

Now when we do multiple selection, it handle correctly the several Event type (fixes #3177)

We can now take a video (this PR replaces #2411) Design input will be updated later, for now we are using a dialog when clicking on "Camera":

image

There also a new setting "Take photo or video" to update the stored value:

image

image

EDIT 2:

Added some information about attachment in the bottom sheet: when applicable user will see: duration, dimensions, size and mime type (for files only).

Example for a video:

image


override fun onCancelled() {
Timber.d("Compressing: cancel")
job.cancel()
Copy link
Member Author

Choose a reason for hiding this comment

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

@Dominaezzz there is probably room for improvement here, on https://github.com/AbedElazizShe/LightCompressor API, but also on my workaround :)

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm slightly confused by the use of both Job and Dispatchers.IO, I think one of them has to go.
In reality this API should've exposed a flow but oh well, it's not too hard to wrap.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've added the support to track the progress, but I'm a bit confused too about how I can improve that. The Dispatchers.IO is here because we are creating a file, and the Job is here to make the call "synchronous". What do you suggest?

Copy link
Contributor

Choose a reason for hiding this comment

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

Looking at https://natario1.github.io/Transcoder/docs/events , (unfortunately) transcoding is already done on a background thread, so you don't need to use Dispatchers.IO. You could also just limit it to just the createDestinationFile().

(I can't wait until ProgressListener is replaced with Flow)

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for the feedback, I will change that. By the way the link you provide also shows me that there is a successCode in the onTranscodeCompleted fun. I did not noticed that. When the value is SUCCESS_NOT_NEEDED it appears that the target file is not written, so it explain a bug I observed today with a 0-lenght file. I will fix that too.

@DoM1niC
Copy link

DoM1niC commented May 3, 2021

Hey @bmarty :)
I hope this will fixed this issue #3177 too 👍🏻 nice work btw!

@DoM1niC
Copy link

DoM1niC commented May 3, 2021

Hey @bmarty :)
I hope this will fixed this issue #3177 too 👍🏻 nice work btw!

is fixed with this PR THX!!!!

Copy link
Member

@BillCarsonFr BillCarsonFr left a comment

Choose a reason for hiding this comment

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

LGTM, you should update open_source_licence.html with transcoder lib

@bmarty
Copy link
Member Author

bmarty commented May 5, 2021

@BillCarsonFr the file open_source_licence.html is on the application side. Another PR could add licence for eveything which is used by the MatrixSDK, IDK...

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.

[Bug] Mixed media sending will stuck
4 participants