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

Remove delay before recording starts #12

Merged
merged 4 commits into from Sep 30, 2020
Merged

Remove delay before recording starts #12

merged 4 commits into from Sep 30, 2020

Conversation

sindresorhus
Copy link
Member

@sindresorhus sindresorhus commented Jun 10, 2020

Turns out the recording actually starts right when we call recorder.start(). The .onStart event is emitted 2-3 seconds later. So the fix is to simply not wait for that event. I'm suspecting that even is when the recording starts writing to the file on disk, not when the actual recording starts.

Fixes #1
Related wulkano/Kap#850 (comment)

@sindresorhus
Copy link
Member Author

@karaggeorge Can you confirm my findings? Just put up a website that shows a millisecond clock, and the run node example.js and ensure the recording started when it says.

@sindresorhus sindresorhus marked this pull request as draft June 10, 2020 19:16
@sindresorhus
Copy link
Member Author

.startRecording() docs:

Returns a Promise for the path to the screen recording file.

This will no longer be true as the file is not yet written then. Not sure how to handle this. We'll need another promise that can be awaited if the user wants to know when the file is ready. It's a bit weird to have nested promises though:

Returns a Promise that resolves when the screen recording has started, and the resolve value is another promise that resolves to a promise the path to the screen recording file.

🤷‍♂️

@karaggeorge
Copy link
Member

I don't really see many use-cases for wanting to know when the file itself is getting written, since you probably will wait until you stopRecording to access it. If we do want to include it, we could expose another method to the recorder object that is pretty much just a promise for when the file is ready. Something like:

await recorder.startRecording()
console.log('Started recording')

await recorder.fileReady()
/* do something with the file */

And we can have it return a boolean, so if you call it when there's no recording it just resolves instantly to false (to avoid edge cases)

Testing the PR now

@karaggeorge
Copy link
Member

So, I start recording and when the promise resolves I print out the time, then I confirm with the recording to check when it started. I saw the recordings starting about ~1.5 second after the promise resolves.

For Kap specifically, that might not be a bad thing as we can make the tray animation last ~1.5 seconds, so we time it with the recording

@karaggeorge
Copy link
Member

A better test:

When the promise resolves I start printing how many seconds have passed since the promise resolved (0.1 increments). Then I check the first frame of the recording which was:

tmp

So, it looks like the recording starts 1 second after the promise resolves

@sindresorhus
Copy link
Member Author

sindresorhus commented Jun 13, 2020

Can you commit your test here so I can check too? I definitely saw less than 1 second on my computer.

@sindresorhus
Copy link
Member Author

I don't really see many use-cases for wanting to know when the file itself is getting written, since you probably will wait until you stopRecording to access it.

The user could want to start converting the file right away. I think FFMPEG supports converting an incomplete file. For example, if they want to have a GIF ready right away when the recording finishes.

await recorder.fileReady()

Good idea, but I think it should be await recorder.isFileReady.

@karaggeorge
Copy link
Member

I just pushed up the code I used. So what I do is, run a node shell, import aperture-node, and start recording with my terminal up. Then after a few seconds, I stop recording and check the file. The first frame of that recording, shows when it actually started recording, and the last number printed, should be how many seconds after the promise resolved that was.

For me it's always between 1 and 1.2 seconds (ran it about 10 times)

@sindresorhus
Copy link
Member Author

Same result for me. 1 second.

Maybe we just delay it by 1s? I think it's ok to be a few hundred ms off either way.

@karaggeorge
Copy link
Member

Delay the promise resolving aperture-node? Or try to time our animation on Kap side?

@sindresorhus
Copy link
Member Author

The former.

@karaggeorge karaggeorge marked this pull request as ready for review July 1, 2020 13:22
@karaggeorge
Copy link
Member

@sindresorhus ok, I updated the implementation based on all the comments we had here, if you can take a look

@karaggeorge
Copy link
Member

karaggeorge commented Jul 1, 2020

Thinking if there's a benefit to having a promise resolve the moment you get the R, for use-cases like Kap when we need to do prep work (tray animation/dimming the croppers) before the recording actually starts. The way this is now, any action you take after await startRecording will be in the recording itself, unless you trim it. And since we know that the R is ~1 sec before the recording starts we can time things pretty well.

Something like await recorder.willStartRecording with docs to say it's about 1 sec before the recording starts, but not guaranteed

readme.md Outdated

#### recorder.isFileReady

`Promise` that fullfills with the path to the screen recording file when it's ready.
Copy link
Member Author

Choose a reason for hiding this comment

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

Can you document that it will never reject? That's usually useful information.

@sindresorhus
Copy link
Member Author

Yeah, I think that could be useful.

@sindresorhus
Copy link
Member Author

Bump in case you forgot about this. Feel free to ignore if you're just busy.

@karaggeorge
Copy link
Member

I did completely forget about this, thanks for the reminder. I'll hopefully finish this up this week

@sindresorhus sindresorhus merged commit 10d447c into master Sep 30, 2020
@sindresorhus sindresorhus deleted the fix-delay branch September 30, 2020 13:06
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.

Recording starts before resolving promise
2 participants