fix(extract_audio) correct extraction to WAV#101
Conversation
hm21
left a comment
There was a problem hiding this comment.
Thanks for working on the PR. I left a few review comments, and I noticed that some integration tests in audio_extract_test.dart are failing on android. Once those are fixed, we can merge it.
Let me know if you need any help.
android/src/main/kotlin/ch/waio/pro_video_editor/src/features/audio/WavFileWriter.kt
Outdated
Show resolved
Hide resolved
android/src/main/kotlin/ch/waio/pro_video_editor/src/features/audio/WavFileWriter.kt
Show resolved
Hide resolved
android/src/main/kotlin/ch/waio/pro_video_editor/src/features/audio/WavFileWriter.kt
Show resolved
Hide resolved
android/src/main/kotlin/ch/waio/pro_video_editor/src/features/audio/ExtractAudio.kt
Show resolved
Hide resolved
android/src/main/kotlin/ch/waio/pro_video_editor/src/features/audio/WavFileWriter.kt
Show resolved
Hide resolved
So you aren't against this approach then. I'll work through your comments. While having export to wav is useful, most audio processing tools need raw pcm data downsampled to 16-bit and certain sample rate to make it work with. I ended up using the Adding support for the |
|
I also having some odd issue (at least on macos, have to verify other platforms), and unsure what is the root cause is... Download some video from instagram, load it into Audacity audio editor and view waveform and spectrogram. Then extract audio to m4a (I think to wav does the same) and load extracted audio into Audacity. Note some time marker in the original audio and find it in the extracted audio - the marker times don't match, by 0.3..0.7 seconds (different between different videos). 🤷🏻 Extracting wav audio with ffmpeg producing matching time markers. Do you have any thoughts on this? Or should I create an issue for the backlog? Could it be related to some sample rate rounding/miscalculation or something like that? |
Yes, I like it, and wav is definitely a useful format.
Yeah, definitely helpful, and I agree that doing it in a separate PR would be great.
That’s honestly a very interesting issue. I also think it could be a rounding or miscalculation, like you mentioned, but I’d be really interested to know if you’re seeing the same problem on Android. |
# Conflicts: # example/integration_test/audio_extract_test.dart
It is also failing for me on macos: Probably due to mime lookup assertion I've added to make sure returned format is correct. Could use some help to align the correct mime types in the Another misalignment is that Flutter's mime package reports wav file mime types as BTW, is the |
…o pr/ekuleshov/101
…raction cancellation
Sure, I just fixed all the review comments and fixed all the failing tests, so it's ready to merge :)
yeah when building a package, the main goal is usually to keep it as compatible as possible with older versions. But enhanced enums have already been around for quite a while, probably 3 or 4 years. So I’m not planning to make the editor more backward compatible and will go with dart 3.4, which means we can go ahead and update it. |
| case "mp3": return "com.apple.m4a-audio" // MP3 in M4A container | ||
| case "aac": return "com.apple.m4a-audio" // AAC in M4A container | ||
| case "m4a": return "com.apple.m4a-audio" // M4A container | ||
| default: return "com.apple.m4a-audio" |
There was a problem hiding this comment.
should probably add com.microsoft.waveform-audio for wav
| case "mp3": return "com.apple.m4a-audio" // MP3 in M4A container | ||
| case "aac": return "com.apple.m4a-audio" // AAC in M4A container | ||
| case "m4a": return "com.apple.m4a-audio" // M4A container | ||
| default: return "com.apple.m4a-audio" |
There was a problem hiding this comment.
probably should add com.microsoft.waveform-audio for wav
|
@hm21 btw, I pushed some audio alignment fixes yesterday to make sure start times declared on the audio tracks are used. Also, for the |
|
@hm21 anyhow, please merge it when you see fit and thank you for resolving these issues. |
|
@ekuleshov yeah I think the things you mentioned above would be great to implement, so feel free to add them. After that, we can merge it |
I've pushed those changes in. Please take a look. BTW, are there instructions somewhere how to run integration tests from |
|
Great, thanks for updating it. I’ll merge it now.
Yes i recommend you the flutter documentation here |
Description
Related Issue: follow up fixes for #65 to correctly handle extraction to the wav format
ios and macos implementations returning m4a instead of wav, the code was also crashing on some missing config keys in native code
android implementation also returned some non-wav format. Unfortunately I couldn't think of an easy way to encode wav on android other than creating a wav file writer to save converted PCM audio
added mime lookup into the example app to verify what audio format is returned
On a side note - the video used in the example has a 6-track audio. It is probably not the most representative example.
Type of Change