elgalu / docker-selenium

[NOT MAINTAINED] Please use <https://github.com/SeleniumHQ/docker-selenium>
https://github.com/SeleniumHQ/docker-selenium
Other
1.42k stars 334 forks source link

Add Audio Support with AUDIO env variable as option #372

Closed v0idp closed 4 years ago

v0idp commented 4 years ago

I added audio recording support into the video recording functionality. It can be activated by setting the env AUDIO to true. It will only work if VIDEO is set to true too.

Should solve #147

CLAassistant commented 4 years ago

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

:white_check_mark: v0idp
:x: Koray Daldal


Koray Daldal seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

v0idp commented 4 years ago

looks like some travis-ci checks don't pass. I need to re-test my commits and see what's going on.

@elgalu you have an idea where I failed ?

EDIT: Looking at the travis ci test I found this error: Unrecognized option 'vcodec libx264 -preset ultrafast'.

Not sure why this is happening tho. any idea ?

elgalu commented 4 years ago

Follow the logs

It says:

Unrecognized option 'vcodec libx264 -preset ultrafast'. Error splitting the argument list: ----------------------------------------

v0idp commented 4 years ago

@elgalu Came back from a 3 week sickness. Finally fixed the bug and run local scenario tests today. Should be fixed now.

elgalu commented 4 years ago

Thanks a lot!

I've just realized that renaming FFMPEG_CODEC_ARGS is non backward compatible and will in fact break existing users relying on overwriting this variable name, I found for instance:

https://github.com/x0rzkov/dockerfiles-search/blob/5bfbc210aa66764048341185c9ac0b2121dc405c/hub.docker.com/lucidworks/docker-selenium/Dockerfile.meta.yaml#L547-L548 https://github.com/whren/docker-devops-stack/blob/d7190e9c85cb98d1cdb19bc6212fd737000cb294/jenkins/jobs/3-maven-tests-on-selenium-grid-dind-job.xml#L73

Could avoid renaming and only add the new one? thanks

v0idp commented 4 years ago

Oh yes you're right. will change that

v0idp commented 4 years ago

accidently pushed with different idendity. this account doesn't exist here. any way you can skip it ?

elgalu commented 4 years ago

Hehe now you need to update EXPECTED_CHROME_VERSION again If you want you can also upgrade Firefox version to latest one and chromedriver if necessary.

v0idp commented 4 years ago

Hehe now you need to update EXPECTED_CHROME_VERSION again If you want you can also upgrade Firefox version to latest one and chromedriver if necessary.

chrome is already on latest stable version and docker image build works fine with that. upgraded firefox to version 75 tho. geckodriver is on latest too

elgalu commented 4 years ago

Will incorporate via #377