Skip to content

Conversation

sihekuang
Copy link

use a single AVAssetWriter to write both audio and video so that we can capture the mic in both scenarios.

Copy link
Owner

@Mnpn Mnpn left a comment

Choose a reason for hiding this comment

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

Thanks a lot for the PR! I do like the idea of and the changes you've made regarding consolidating the code. I have a few comments though:

  • When recording "System Audio", the microphone track will be included. This doesn't make much sense in my opinion, but maybe there are scenarios where you'd want both of those? This was avoided when using a separate audio-specific file writer; I think you could probably just check if recordMic && !audioOnly in initMedia() here instead

  • I think there's a bug in here, as recording (sometimes..? I think it was working at first but now it consistently) causes a crash
    Edit: This appears to have been an issue with my code, sorry!

  • See my comment about the file format 👍


if audioOnly{
fileEnding = ud.string(forKey: "audioFormat") ?? "wat"
fileType = .m4a // it looks like file type m4a works for all tyle extensions. but maybe need to revise this
Copy link
Owner

Choose a reason for hiding this comment

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

This actually doesn't work! While the file ending appears correct, the actual format in the file will be wrong. (So, for example, when recording Opus, you'll get a file with an AAC stream in a file named .ogg)

Copy link
Author

Choose a reason for hiding this comment

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

thanks for the review! Sorry has been out for a while for the Christmas + New Year holidays. Let me get back and make this right. Thanks again and happy new year!

Copy link
Owner

Choose a reason for hiding this comment

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

No worries at all! Likewise, happy new year :)

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.

2 participants