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

merged file writing to use single avssetwriter for both audio and video #5

Open
wants to merge 1 commit into
base: mic-track
Choose a base branch
from

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