Skip to content

feat: Add screen video recording support#66

Merged
mykola-mokhnach merged 16 commits intomasterfrom
video
Mar 13, 2020
Merged

feat: Add screen video recording support#66
mykola-mokhnach merged 16 commits intomasterfrom
video

Conversation

@mykola-mokhnach
Copy link
Copy Markdown

No description provided.

'-t', `${this._timeLimit}`,
'-f', 'gdigrab',
'-framerate', `${this._fps}`,
'-i', 'desktop',
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

}

async _enforceTermination () {
if (this._process && this.isRunning()) {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

No need for this._process.

log.debug(`Starting ${FFMPEG_BINARY}: ${util.quote(fullCmd)}`);
this._process.on('output', (stdout, stderr) => {
if (_.trim(stdout || stderr)) {
log.debug(`[${FFMPEG_BINARY}] ${stdout || stderr}`);
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

If the output has multiple lines, only the first will have the [${FFMPEG_BINARY}] prefix.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I would say this is fine. There is no need to see this prefix for each log line

Comment thread lib/commands/record-screen.js Outdated
forceRestart = true,
} = options;
if (this._screenRecorder) {
if (this._screenRecorder.isRunning()) {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

These two can just be

if (this._screenRecorder?.isRunning()) {
  // ...
}
this._screenRecorder = null;

The last line isn't needed, really, either, since it is set to the new ScreenRecorder() a couple of lines later.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

changed

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.

3 participants