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

Develop #23

Merged
merged 12 commits into from Aug 1, 2019
Merged

Develop #23

merged 12 commits into from Aug 1, 2019

Conversation

blurtime
Copy link
Collaborator

@blurtime blurtime commented Jul 27, 2019

This PR contains:

  1. reset zoom after recording
  2. switch between audio sessions such that audio has normal volume when replaying in processMediaView
  3. memory leak virtually fixed

Regarding the memory leak:

  • I added a few further changes which have now reduced it even more (in VideoPlayerView.swift and ProcessView+Event.swift)
  • there are still some mallocs of a couple of hundred bytes leaking here and there but that's almost nothing

Carthage still works fine and is building.

Suggestion:
You said you wanted to add animations. Maybe one could move the self.resetZoom into the .onExit closure as originally suggested and instead of suddenly resetting it, zoom out using animations. (I thought putting it outside would fix it. But now one sees how the zoom is suddenly reset shortly before (instead of after) being presented the process media view which I apparently didn't notice yesterday.)

@eonist
Copy link
Owner

eonist commented Jul 27, 2019

Regarding zoom out glitch. I suggest we Add a small delay of say 0.2sec. To give the chance to zoom out before showing virwfinder

@eonist
Copy link
Owner

eonist commented Jul 27, 2019

Actually. What about resetting zoom when you go from viewfinder to review mode?

@blurtime
Copy link
Collaborator Author

Sure, sounds good 👍 would either do that or the animation as I mentioned (although I would prefer the former).

@eonist
Copy link
Owner

eonist commented Jul 27, 2019

The expectee behaviour is to be in zoom 1x once you restart the camera. Similar to how other cam apps work. IMO

@blurtime
Copy link
Collaborator Author

That's what I'm saying: I'd prefer immediately being zoomed in at 1x over animations, too. 😅 I usually try to make people feel it's as natural as possible...

@eonist
Copy link
Owner

eonist commented Jul 28, 2019

  • Added the doubleClick to flip cam feature. I figured out how to derive the position from apples framework, instead of storing it in CamView etc.
  • Also cleaned up bits and pieces.
  • I will test with the iPhone tomorrow

@blurtime
Copy link
Collaborator Author

Sounds awesome! 🎉 I bet this library will do a lot better now, too :) I know CocoaPods is finicky to set up but adding it at some point would definitely help this library even more (as we've seen a few days ago when I was still a carthage noob :D)

@eonist
Copy link
Owner

eonist commented Jul 29, 2019

Yeah. I totally agree. I need to start adding cocoaPods. Seems people are addicted to pods. I have to relearn how set it up tho, cuz i forgot. 🤦

@blurtime
Copy link
Collaborator Author

Fortunately, it isn't important in my case anymore since I'm now a carthage expert 😄 But many others will probably have a hard time when confronted with carthage for the first time, too...
Anyway, if you should work on this project anytime soon again, adding support for flipping the camera while recording would really seal the deal with regards to advantages over other camera libraries. If I have some time next month (around mid-August) I'll try to add this feature.
Keep up the great work! 👍

@eonist
Copy link
Owner

eonist commented Jul 29, 2019

Yeah. that's a pretty cool feature. Like your filming something and then you could show your reaction etc. Does insta and snap have this?

@blurtime
Copy link
Collaborator Author

blurtime commented Jul 29, 2019

Yeah, they both do. It was somewhat big news when Snap introduced it. Especially doing it this way sounds pretty nifty since you should be able to switch very quickly between the cameras which as far as I know still is a little problematic in Snap and Instagram because the recording usually seems to be interrupted for a small amount of time so all you see is a black screen for a moment.

EDIT: 2 more questions related to this. Unfortunately, I cannot read Obj-C but the Obj-C version seems to have worked.:
https://stackoverflow.com/questions/44577331/multiple-avcapturevideodataoutput-in-same-avcapturesession
https://stackoverflow.com/questions/47983572/switch-between-front-and-back-camera-while-recording-a-video

@eonist
Copy link
Owner

eonist commented Jul 29, 2019

Nice. You should add these resources to an issue. Maybe someone picks it up and makes a PR. My priority is adding slow motion. As I need it for another project.

@blurtime
Copy link
Collaborator Author

Did you have a chance to test it?

@eonist
Copy link
Owner

eonist commented Jul 30, 2019

I will test tonight for sure. And merge 👍

@blurtime
Copy link
Collaborator Author

Don't want to be annoying but no time at all yesterday? 😅

@eonist
Copy link
Owner

eonist commented Aug 1, 2019

Yeah. Checked. forgot to merge.

@eonist eonist merged commit 4312a7d into master Aug 1, 2019
@eonist
Copy link
Owner

eonist commented Aug 1, 2019

And great job. Thanks for the PR 👍. Please let me know if I can help with anything in the future.

@blurtime
Copy link
Collaborator Author

blurtime commented Aug 1, 2019

Thank you so much - especially for helping me out so many times already! I'll try to add the feature to switch the camera while recording asap (probably in about 2-3 weeks). Really loved working with you so far! 😊

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

2 participants