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

replace Communication\Connection::waitForDelay's usleep with select #606

Open
divinity76 opened this issue Feb 15, 2024 · 7 comments
Open

Comments

@divinity76
Copy link
Contributor

divinity76 commented Feb 15, 2024

the usleep in

private function waitForDelay(): void
{
if ($this->lastMessageSentTime) {
$currentTime = (int) (\hrtime(true) / 1000 / 1000);
// if not enough time was spent until last message was sent, wait
if ($this->lastMessageSentTime + $this->delay > $currentTime) {
$timeToWait = ($this->lastMessageSentTime + $this->delay) - $currentTime;
\usleep($timeToWait * 1000);

is problematic, it should be a socket_select() / stream_select(), and the usleep causes laggy/non-smooth video recordings with Page.startScreencast + method:Page.screencastFrame (sample implementation here )

So when I record videos, to make them smooth, I have to replace stuff like

$navigate->waitForNavigation(\HeadlessChromium\Page::NETWORK_IDLE);

with a cpu busyloop like

$target_time = microtime(true) + 5;
while (microtime(true) < $target_time) {
    // calling readData() fast causes smooth video recordings.
    // if there is too much delay in sending Page.screencastFrameAck, chromium will drop frames.
    $page->getSession()->getConnection()->readData();
}

That's a dirty quickfix, and I believe the correct solution would be to replace Communication\Connection::waitForDelay's usleep() with select() , it should be something like

             $timeToWait = ($this->lastMessageSentTime + $this->delay) - $currentTime;
             $timeToWaitSeconds = (int)$timeToWait;
             $fractionMicroseconds = ((int)(($timeToWait - $timeToWaitSeconds) * 1000000));
              $read=[$page->socket]; 
              stream_select($read,null,null,$timeToWaitSeconds, $fractionMicroseconds);

But first issue is to figure out how to get $page's socket handle into function waitForDelay 🤔 and seems the raw socket handle is wrapped inside wrench... suggestions?

divinity76 added a commit to divinity76/wrench that referenced this issue Mar 17, 2024
This method is a prerequisite to eventually resolving chrome-php/chrome#606
@divinity76
Copy link
Contributor Author

divinity76 commented Mar 17, 2024

For this to be fixed, \Wrench\Client needs a getSocketResource() method.

Made a PR: chrome-php/wrench#17

Before more progress can be made, it seems we need to wait for the method/PR to be accepted, and wait for a new chrome-php/wrench version to be released 🤔

Here is my investigation notes so far:
class \HeadlessChromium\Communication\Connection has a protected \HeadlessChromium\Communication\Socket\SocketInterface $wsClient;

\HeadlessChromium\Communication\Socket\SocketInterface has no getSocketResource().

\HeadlessChromium\Communication\Socket\Wrench must then create its own getSocketResource(),
returning the resource from protected \Wrench\Client $client;
\Wrench\Client needs a getSocketResource(), returning it's socket resource from protected \Wrench\Socket\ClientSocket $socket;

\Wrench\Socket\ClientSocket inherits $this->socket property from UriSocket...

\Wrench\Socket\UriSocket inherits $this->socket property from \Wrench\Socket\AbstractSocket...

\Wrench\Socket\AbstractSocket has a public function getResource()

that means UriSocket has it too... that means ClientSocket also has it.. \Wrench\Client needs to expose it publicly (or at least protectedly)
it needs a
public function getSocketResource(){return $this->socket->getResource();}

GrahamCampbell pushed a commit to chrome-php/wrench that referenced this issue Mar 18, 2024
* Add getSocketResource method to enhance socket management

This method is a prerequisite to eventually resolving chrome-php/chrome#606

* StyleCI

* public function waitForData

* remove getSocketResource

* StyleCI

* StyleCI

* error message

* oops use stream_select not socket_select

* error handling
@divinity76
Copy link
Contributor Author

divinity76 commented Mar 18, 2024

update: The upcoming Wrench 1.7 will have a new \Wrench\Client::waitForData method which uses select() internally, we should replace \usleep($timeToWait * 1000); with $this->wsClient->waitForData($timeToWait); once Wrench >=1.7 is available.

@GrahamCampbell
Copy link
Member

@divinity76 Are you planning on picking up the next step of this, with a PR to this repo?

@GrahamCampbell
Copy link
Member

Is this basically what you had in mind as the only change?

image

@divinity76
Copy link
Contributor Author

@GrahamCampbell no, I've since lost interest, I made some workaround for my specific problem and moved on.

But yes that's exactly what I had in mind!

@GrahamCampbell
Copy link
Member

OK, I can make the change then. I just didn't want to be treading on toes. ;)

@divinity76
Copy link
Contributor Author

@GrahamCampbell perfect, thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

2 participants