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

Fixes issue#8 output file metadata .MOV -> .mp4 #11

Merged
merged 1 commit into from
Apr 16, 2020

Conversation

khainhero
Copy link
Contributor

I believe originally it defaulted to the input extension of the original file

now its modified to match the output file type (.mp4)

I believe originally it defaulted to the input extension of the original file

now its modified to match the output file type (.mp4)
@khainhero khainhero changed the title .MOV extension metadata change to .mp4 Fixes issue#8 output file metadata .MOV -> .mp4 Apr 15, 2020
Copy link
Contributor

@Serdnad Serdnad left a comment

Choose a reason for hiding this comment

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

I think the current version is fine, sourceVideoType is only used for constructing the source path. If you look at line 195, the output type is hardcoded as MP4.

Edit: Nvm, think you're right. Nice catch!

@Serdnad
Copy link
Contributor

Serdnad commented Apr 15, 2020

On second look through, it seems like the crux of the problem is reusing the source url as the output url. Am I misunderstanding, or does it look like the original file is deleted/overwritten regardless of deleteOrigin?

@jonataslaw
Copy link
Owner

On second look through, it seems like the crux of the problem is reusing the source url as the output url. Am I misunderstanding, or does it look like the original file is deleted/overwritten regardless of deleteOrigin?

It is excluding compressionUrl, to prevent an exception from being thrown if the file already exists or is duplicated, the original file (sourceVideoUrl) will only be deleted if deleteOrigin is true.

@jonataslaw
Copy link
Owner

I believe originally it defaulted to the input extension of the original file

now its modified to match the output file type (.mp4)

I need further testing to make sure that this will not raise an exception if a file other than mp4 is selected.
My mac is in repair, so I am temporarily unable to test whether to change the output format to mp4, the recorded videos from the camera and also the mp4 videos already stored on the device will behave properly.
If any of you test this out and it goes well, I’ll merge it.

I thank you both for the help you are giving me in this library. I will add your readme profile as contributors soon.

@Serdnad
Copy link
Contributor

Serdnad commented Apr 16, 2020

I've tested it myself on an iPhone with videos in MOV format (or potentially HVEC?), and it seemed to fix some issues I was having. Regarding the file deletion I mentioned, read through the file yet again, and I have no clue what made me think the source file was being deleted... looks good.

Glad to help, this library has already been rather useful!

@jonataslaw
Copy link
Owner

I've tested it myself on an iPhone with videos in MOV format (or potentially HVEC?), and it seemed to fix some issues I was having. Regarding the file deletion I mentioned, read through the file yet again, and I have no clue what made me think the source file was being deleted... looks good.

Glad to help, this library has already been rather useful!

Perhaps there is a better way to do this, renaming the file to NameOfFile(1).mp4 instead of trying to delete it if the file already exists.
But this is only in case you accidentally click twice to compress the same video. The destination file will be created, and when you try to create the same file again, it will give an exception and terminate the application. As I was out of time, I thought that this could be the solution, but there are better approaches, like checking if the file exists, and renaming it. I will think about doing this in the future if I run into problems, but for now, it looks good to me.
I will merge and send the update to pub.dev
Thank you!

@jonataslaw jonataslaw merged commit ccf947f into jonataslaw:master Apr 16, 2020
@TungWang TungWang mentioned this pull request Oct 29, 2020
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.

None yet

3 participants