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

Linux Support (again) #32

Closed
wants to merge 6 commits into from
Closed

Linux Support (again) #32

wants to merge 6 commits into from

Conversation

fearphage
Copy link

This is the fixes (and some) of the PR #7. I couldn't figure out how to continue progress on that branch/PR.

Let me know if everything still works.

Question: Why don't we/you use ffmpeg on all platforms? It seems like it would greatly simplify the code and it would add support for MacOS older than 10.10.

@sindresorhus
Copy link
Contributor

Why don't we/you use ffmpeg on all platforms?

https://github.com/wulkano/aperture#why

@sindresorhus
Copy link
Contributor

Also note my comment in #7: #7 (comment)

index.js Outdated
if (process.platform === 'darwin') {
return execa.stdout(path.join(__dirname, 'swift/main'), ['list-audio-devices']).then(JSON.parse);
} else if (process.platform === 'linux') {
return execa.stdout('sh', ['-c', "arecord -l | awk 'match(\$0, /card ([0-9]): ([^,]+),/, result) { print result[1] \":\" result[2] }'"]);
Copy link
Contributor

Choose a reason for hiding this comment

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

We should use JS for as much as possible. No awk.

package.json Outdated
"build": "cd swift && xcodebuild -derivedDataPath $(mktemp -d) -scheme aperture",
"prepublish": "npm run build"
"build-macos": "cd swift && xcodebuild -derivedDataPath $(mktemp -d) -scheme aperture",
"postinstall": "[ \"$(uname)\" = \"Darwin\" ] && npm run build-macos",
Copy link
Contributor

Choose a reason for hiding this comment

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

This one should be removed.

index.js Outdated
@@ -9,11 +9,17 @@ const debuglog = util.debuglog('aperture');

class Aperture {
constructor() {
macosVersion.assertGreaterThanOrEqualTo('10.10');
if (process.platform === 'darwin') {
Copy link
Contributor

Choose a reason for hiding this comment

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

Create top level constants IS_MACOS and IS_LINUX to simplify all these if else statements.

@sindresorhus
Copy link
Contributor

Let me know if everything still works.

You can do that by putting the following in index.js:

module.exports().startRecording().then(console.log).catch(console.error);

@fearphage
Copy link
Author

fearphage commented Mar 17, 2017

You can do that by putting the following

I'm sure it works for me. I just have zero macs on which to confirm the functionality.

I addressed the code review concerns and added the mouse cursor toggle.

@fearphage
Copy link
Author

Not sure what changed between last night and today, but I was getting strings back from the stderr stream and now I'm getting buffers. Weird.

@fearphage
Copy link
Author

@sindresorhus Anything else?

args.push('-i', ':0');
}

args.push('-framerate', fps, '-draw_mouse', +(showCursor === true), this.tmpPath);
Copy link
Contributor

Choose a reason for hiding this comment

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

String(showCursor === true)

Copy link
Author

Choose a reason for hiding this comment

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

Did you verify this works? Based on the documentation, draw_mouse expects a binary value. That's why I'm coercing the boolean to an int.

@sindresorhus
Copy link
Contributor

sindresorhus commented May 6, 2017

Setting showCursor: false doesn't seem to work. I still get the cursor in the recording.

By default it captures only part of the screen, while it should capture the whole screen. Setting the cropArea option does nothing.

.getAudioSources() return duplicate audio sources for me:

[ '0:I82801BAICH2 [Intel 82801BA-ICH2]',
  '0:I82801BAICH2 [Intel 82801BA-ICH2]' ]

It also needs to be in the format:

[ { id: 'AppleHDAEngineInput:1B,0,1,0:1',
    name: 'Built-in Microphone' } ]

ffmpeg version 3.0.7-0ubuntu0.16.10.1

@fearphage
Copy link
Author

Setting showCursor: false doesn't seem to work

Confirmed.

By default it captures only part of the screen, while it should capture the whole screen

I'll look into it.

.getAudioSources() return duplicate audio sources for me

Cannot reproduce locally.

It also needs to be in the format

From your sample output, is that id [name]?

ffmpeg version 3.0.7-0ubuntu0.16.10.1

Same here.

if (IS_MACOS) {
return execa.stdout(path.join(__dirname, 'swift/main'), ['list-audio-devices']).then(JSON.parse);
} else if (IS_LINUX) {
return execa.stdout('arecord', ['-l']).then(
Copy link
Contributor

Choose a reason for hiding this comment

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

-l => --list-devices

@sindresorhus
Copy link
Contributor

From your sample output, is that id [name]?

$ arecord --list-devices
**** List of CAPTURE Hardware Devices ****
card 0: I82801BAICH2 [Intel 82801BA-ICH2], device 0: Intel ICH [Intel 82801BA-ICH2]
  Subdevices: 1/1
  Subdevice #0: subdevice #0
card 0: I82801BAICH2 [Intel 82801BA-ICH2], device 1: Intel ICH - MIC ADC [Intel 82801BA-ICH2 - MIC ADC]
  Subdevices: 1/1
  Subdevice #0: subdevice #0

@sindresorhus
Copy link
Contributor

It seems if you specify default to ffmpeg, it will use the default device. According to the web it's sometimes hard to determine this yourself, so maybe we should add in a Default Microphone entry, that just supplies -i default to ffmpeg?

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