-
-
Notifications
You must be signed in to change notification settings - Fork 2.6k
pulseaudio package installation for Firefox containers #614
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
Conversation
|
||
# Generating a default config during build time | ||
RUN /opt/bin/generate_config > /opt/selenium/config.json | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if it is better to install the package when the user is root and avoid using sudo... If you install it when the user is root, are the results the same @pabloFuente?
If so, I think it would be better to install it with root.
Hi @pabloFuente, Thanks for submitting the PR again, also thank you for the clear explanation for it. Did you check how much does the image grow by installing the package? I guess not so much, would be nice to know that. From my point of view, it looks good. I don't see why we could not merge it. @ddavison, what do you think? |
Hello again. I am glad to answer your questions:
Thank you. |
Thanks for the changes! From my point of view it looks good. The extra 50MB is not ideal but I don't think there is a big issue about it. The Chrome container is already 848MB for example. We should look into that later. I'll just wait for @ddavison's opinion, after that, it should be ok to merge. |
i guess i'm just confused of the practicality of this change. i appreciate the deep explanation, @pabloFuente but i guess i'm still just confused of the practicality. If we installed this in the containers...
I noticed you had only changed the NodeFirefox image. Does this already "passively" exist in the NodeChrome images? If this is the case, then I have no problem with it whatsoever. If doing an "apt-get google-chrome-stable..." automatically installs pulseaudio, then we're green for this PR. |
Hi @ddavison. If I may, I would like to put in my two cents here. The whole point of this PR is to made available Firefox for testing WebRTC applications using dockerized browsers. I trace the problem in the past, as explained in the stackoverflow question previously linked by @pabloFuente. The problem started in Firefox 52. It seems the internal implementation of the WebRTC stack requires pulseaudio to work as of this version. Regarding your first and second questions, it is required by people testing WebRTC applications and using the latest versions of Firefox by means of Docker. Regarding your third question, I disagree with your comment. Selenium is perfectly capable of that. In fact, it is a must to make it in an automated fashion, since the WebRTC stack is mainly implemented in modern real browsers such as Chrome or Firefox, and therefore, to test these services properly, we need to drive programmatically browsers (i.e. using Selenium). Here, in my opinion it is very interesting to use dockerized browsers, for example to carry out compatibility tests (i.e. different version or browser type) or performance test (i.e. use a lot of browsers in a test), but at this moment it is not possible using the official Selenium Docker images, which is a pity for the open source community. Regarding your last comment, no, Chrome does not require pulseaudio. I suppose this is because the internal implementation of the WebRTC is different in Chrome. But IMHO this should not affect the final decision about merging this PR. |
your comments are very welcome, @bonigarcia! thanks 👍 thanks for also greater clarification. perhaps it's my ignorance on WebRTC.
Ok, so this is a Firefox specific thing. perfect.
Well, i didn't say it wasn't capable, i just said it probably wasn't the "best" option. again, this may stem from my lack of knowledge of WebRTC. As a summation, i've done a little more research, and these testers that need this - are testing sites that use WebRTC. This includes, perhaps sites that use the computer peripherals (camera, mic, etc). I think it's perfectly reasonable to include this. @pabloFuente , looks like your branch is out of date. I still don't trust the "Update branch" button in |
@ddavison Cool, thanks to you! |
Update branch to master
There it goes. Updated to master. |
X
in the preceding checkbox, I verify that I have signed the Contributor License AgreementHi. First of all, sorry for the previous PR. You're right: the file clearly says not to modify it.
I have already shifted the change to Dockerfile.txt on NodeFirefox, so all StandaloneFirefox, StandaloneFirefoxDebug, NodeFirefox and NodeFirefoxDebug should now include the change.
Let me dig a little deeper into this PR:
Output in the first container when checking if pulseaudio is running:
RESULT: Test fails
Output in the second container:
(pulseaudio returns exit 0 for this command when it is properly running)
RESULT: Test passes
Thanks for your time.