add mjpeg stream helper class to basedriver #230

jlipps · 2018-07-05T05:41:45Z

There is this "standard" of sorts called MJPEG, which allows for streaming of images over HTTP. Some users of Appium have MJPEG servers connected to the devices they use with Appium, which deliver relatively high-framerate images off the device. If this is the case, why not just use it for screenshots as well and bypass the sometimes time-consuming Appium-internal screenshot commands?

MJPEG is a sort-of "push" mechanism, so in this PR I just provide a class which maintains a reference to the last-pushed image, which would then be available to any driver that wants it. I'll start the driver PRs soon, but the basic idea is that this will be behind a capability, which if turned on will just send back the last-received image immediately upon screenshot request. Something like this:

// in driver session start
if (this.opts.mjpegScreenshotUrl) {
  this.mjpegStream = new MJpegStream(this.opts.mjpegScreenshotUrl);
  this.mjpegStream.start();
}

// in screenshot command implementation 
if (this.mjpegStream && this.mjpegStream.lastChunkBase64) {
  return this.mjpegStream.lastChunkBase64;
}

// now continue with regular screenshot logic as a fallback

// in driver session stop/cleanup
if (this.mjpegStream) {
  this.mjpegStream.stop();
}

mykola-mokhnach · 2018-07-05T05:45:07Z

lib/basedriver/mjpeg.js

+   * @returns {string}
+   */
+  get lastChunkBase64 () {
+    if (!this.lastChunk) {


Ternary operator might be shorter here

mykola-mokhnach · 2018-07-05T05:45:26Z

lib/basedriver/mjpeg.js

+  /**
+   * Start reading the MJpeg stream and storing the last image
+   */
+  start () {


why not async?

mykola-mokhnach · 2018-07-05T05:51:22Z

test/basedriver/fixtures/mjpeg-server.js

+    // just send the same jpeg over and over
+    for (let i = 0; i < times; i++) {
+      await B.delay(intMs);
+      mJpegReqHandler._write(jpg, null, () => {});


there is also _.noop helper

mykola-mokhnach · 2018-07-05T05:58:47Z

We probably would need some fast PNG->JPEG converter here as well

jlipps · 2018-07-05T06:28:16Z

We probably would need some fast PNG->JPEG converter here as well

For now my thought is if someone uses this, they know they will get jpeg formatted screenshots out of appium instead of png.

mykola-mokhnach · 2018-07-05T07:19:05Z

For now my thought is if someone uses this, they know they will get jpeg formatted screenshots out of appium instead of png.

Selenium's getScreenshot always returns images in PNG format. There are no other endpoints, which allow to get JPEG directly. That is why I suppose we need to do the conversion on Appium side

mykola-mokhnach · 2018-07-05T07:20:30Z

lib/basedriver/mjpeg.js

+   * @returns {string}
+   */
+  get lastChunkBase64 () {
+    return this.lastChunk ? this.lastChunk.toString('base64') : null;


_.isBuffer(lastChunk) ?

mykola-mokhnach · 2018-07-05T07:20:58Z

lib/basedriver/mjpeg.js

+  /**
+   * Start reading the MJpeg stream and storing the last image
+   */
+  async start () {


mykola-mokhnach · 2018-07-05T07:22:50Z

lib/basedriver/mjpeg.js

+   * the HTTP request itself. Then reset the state.
+   */
+  stop () {
+    if (this.consumer) {


if (!this.consumer) return

mykola-mokhnach · 2018-07-05T07:23:47Z

lib/basedriver/mjpeg.js

+      .pipe(this.consumer)  // allow chunking and transforming of jpeg data
+      .pipe(this);          // send the actual jpegs to ourself
+
+    await startPromise;


should it block till the first image is received?

there is no last image, so that's not possible.

I mean this might block forever if the server does not produce any data

oh right. that would be an error. probably we should have a timeout and if the timeout gets hit, throw an error

mykola-mokhnach · 2018-07-05T07:24:46Z

lib/basedriver/mjpeg.js

+   * Reset internal state
+   */
+  clear () {
+    this.didStart = false;


is this used?

nope! will remove

jlipps · 2018-07-05T18:15:56Z

@mykola-mokhnach @imurchie what do you think, should this be a part of appium-support instead? now that it is just a class which can be used, but not tied to the driver code itself.

mykola-mokhnach · 2018-07-05T18:21:18Z

If the class has no direct dependencies on base driver then yes, it would be better to keep it in appium-support

mykola-mokhnach · 2018-07-05T18:23:18Z

lib/basedriver/mjpeg.js

+
+    // start a timeout so that if the server does not return data, we don't
+    // block forever.
+    setTimeout(() => {


I think bluebird promises already have the standard .timeout method

mykola-mokhnach · 2018-07-05T18:24:59Z

lib/basedriver/mjpeg.js

+      this.lastChunk.toString('base64') :
+      null;
+  }
+


extra empty line here

jlipps · 2018-07-05T19:15:06Z

closing, gonna move to appium-support

jlipps requested review from imurchie and mykola-mokhnach July 5, 2018 05:41

mykola-mokhnach reviewed Jul 5, 2018

View reviewed changes

jlipps force-pushed the jlipps-mjpeg branch from 872a825 to e1cdb3d Compare July 5, 2018 06:16

jlipps mentioned this pull request Jul 5, 2018

support mjpeg stream screenshot via new mjpegScreenshotUrl cap appium/appium-uiautomator2-driver#193

Merged

mykola-mokhnach reviewed Jul 5, 2018

View reviewed changes

jlipps force-pushed the jlipps-mjpeg branch from e1cdb3d to 58d6ac1 Compare July 5, 2018 18:15

mykola-mokhnach reviewed Jul 5, 2018

View reviewed changes

appium deleted a comment from jlipps Jul 5, 2018

mykola-mokhnach reviewed Jul 5, 2018

View reviewed changes

lib/basedriver/mjpeg.js

this.lastChunk.toString('base64') :

null;

}

Copy link

Contributor

mykola-mokhnach Jul 5, 2018

Choose a reason for hiding this comment

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

extra empty line here

add mjpeg stream helper class to basedriver

e9edf3e

jlipps force-pushed the jlipps-mjpeg branch from 58d6ac1 to e9edf3e Compare July 5, 2018 18:48

jlipps closed this Jul 5, 2018

jlipps deleted the jlipps-mjpeg branch July 5, 2018 19:15

jlipps mentioned this pull request Jul 5, 2018

add mjpeg stream helper class appium/appium-support#75

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add mjpeg stream helper class to basedriver #230

add mjpeg stream helper class to basedriver #230

jlipps commented Jul 5, 2018

mykola-mokhnach Jul 5, 2018

mykola-mokhnach Jul 5, 2018

mykola-mokhnach Jul 5, 2018

mykola-mokhnach commented Jul 5, 2018

jlipps commented Jul 5, 2018

mykola-mokhnach commented Jul 5, 2018

mykola-mokhnach Jul 5, 2018

mykola-mokhnach Jul 5, 2018

mykola-mokhnach Jul 5, 2018

mykola-mokhnach Jul 5, 2018 •

edited

Loading

jlipps Jul 5, 2018

mykola-mokhnach Jul 5, 2018

jlipps Jul 5, 2018

mykola-mokhnach Jul 5, 2018

jlipps Jul 5, 2018

jlipps commented Jul 5, 2018

mykola-mokhnach commented Jul 5, 2018

mykola-mokhnach Jul 5, 2018

mykola-mokhnach Jul 5, 2018

jlipps commented Jul 5, 2018

add mjpeg stream helper class to basedriver #230

add mjpeg stream helper class to basedriver #230

Conversation

jlipps commented Jul 5, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mykola-mokhnach commented Jul 5, 2018

jlipps commented Jul 5, 2018

mykola-mokhnach commented Jul 5, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mykola-mokhnach Jul 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jlipps commented Jul 5, 2018

mykola-mokhnach commented Jul 5, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jlipps commented Jul 5, 2018

mykola-mokhnach Jul 5, 2018 •

edited

Loading