Skip to content
This repository has been archived by the owner on Oct 25, 2023. It is now read-only.

add mjpeg stream helper class #75

Merged
merged 1 commit into from
Jul 13, 2018
Merged

add mjpeg stream helper class #75

merged 1 commit into from
Jul 13, 2018

Conversation

jlipps
Copy link
Member

@jlipps jlipps commented Jul 5, 2018

the new version of appium/appium-base-driver#230

lib/mjpeg.js Outdated

// use the deferred pattern so we can wait for the start of the stream
// based on what comes in from an external pipe
let startPromise = new B((res, rej) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

const

lib/mjpeg.js Outdated
* Start reading the MJpeg stream and storing the last image
*/
async start (serverTimeout = MJPEG_SERVER_TIMEOUT_MS) {
this.consumer = new MJpegConsumer();
Copy link
Contributor

Choose a reason for hiding this comment

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

should we call stop here first ?

Copy link
Member Author

Choose a reason for hiding this comment

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

doesn't hurt to be safe

* it closes the connection
* @returns {http.Server}
*/
function initMJpegServer (port, intMs = 300, times = 20) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd rather put this helper method into tests

Copy link
Member Author

Choose a reason for hiding this comment

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

it will be used by the drivers though, so it needs to be exported from appium-support

it('should error out if the server does not send any images before a timeout', async function () {
let stream = new MJpegStream(MJPEG_SERVER_URL, _.noop);
await stream.start(0).should.eventually.be.rejectedWith(/never sent/);
stream.stop();
Copy link
Contributor

Choose a reason for hiding this comment

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

stop might not always be called if a test fails.

@jlipps
Copy link
Member Author

jlipps commented Jul 5, 2018

@mykola-mokhnach added png translation support

lib/mjpeg.js Outdated
null;
}

async lastChunkPNG () {
Copy link
Contributor

Choose a reason for hiding this comment

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

a docstring might be useful

@jlipps
Copy link
Member Author

jlipps commented Jul 5, 2018

@imurchie any thoughts or comments on this before i merge?

Copy link
Contributor

@imurchie imurchie left a comment

Choose a reason for hiding this comment

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

One concern. Otherwise looks good.

lib/mjpeg.js Outdated
import { Writable } from 'stream';
import { fs } from 'appium-support';

const IMAGE_PATH = path.resolve(__dirname, '..', '..', 'test', 'images',
Copy link
Contributor

Choose a reason for hiding this comment

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

test/ is currently not included in the npm release, so this won't exist (fine for declaration, since path.resolve doesn't do anything to the contents, but not for the usage later).

Copy link
Member Author

Choose a reason for hiding this comment

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

ah yes, good point. i'll find somewhere else for it to go.

Copy link
Contributor

Choose a reason for hiding this comment

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

you could encode the simplest one-pixel-like image into base64-string and put it directly there

@jlipps
Copy link
Member Author

jlipps commented Jul 11, 2018

ok @mykola-mokhnach @imurchie changes made, what do you think now?

@jlipps jlipps merged commit a3e9a6d into master Jul 13, 2018
@jlipps jlipps deleted the jlipps-mjpeg branch July 13, 2018 07:24
@jlipps
Copy link
Member Author

jlipps commented Jul 13, 2018

published as 2.18.0

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

Successfully merging this pull request may close these issues.

None yet

3 participants