Various tweaks #15
Various tweaks #15
Conversation
|
woooow, great @vitch . I'll take a look in a little while 😄 |
|
|
||
| if (this.tmrCapture) { | ||
| clearTimeout(this.tmrCapture); | ||
| } | ||
|
|
||
| var gCtx = this.canvasElem.getContext("2d"); | ||
| gCtx.clearRect(0, 0, cWidth, cHeight); | ||
| if (!this.videoDimensions && this.videoElem.videoWidth && this.videoElem.videoHeight) { |
There was a problem hiding this comment.
whenever possible, try to not have a line with size > 79 columns, this facilitates reading
|
There are some problems regarding selecting the media stream (see #8), feel free to participate on that discussion About the PR, i think this could be split in 2, one regarding the mediastream selection and another regarding the refactor. I also think that before going a lot further on some features we should prepare some tests for it so that we would not fall into some regression bugs and feel more confident. What do you think? Great job @vitch 👍 |
|
I've added some more commits which address most of the points above, please check it out. |
|
Greeeat @vitch 😄 Just reviewed the code, seems reeeally good. Just some code formatting to be done but nothing big. I added a comment about the The examples are working well - with the Firefox limitation - on Opera, Firefox and Chrome (all latest linux dev builds) 👍 Anyway, great job @vitch ! If you could also squash the commits and then provide a reasonable description (maybe containing the center idea of the previous commits, just something very simple) would be nice to keep easy to read |
* Implemented the ability to select from multiple cameras if your browser supports it (currently only Chrome). Fail gracefully on other browsers. Introduced an example showing how this functionality can be used. * Setting the canvas to the same dimensions as the video being captured from. This is essential because otherwise the QR recognition code operates on a squashed version of the image and cannot recognise the patterns. * Replaced `releaseVideo` with `stop` which releases the video stream and also stops the timeout which is capturing to canvas. * Fixed some bugs in existing code. Added a `stop` method. The previously added `releaseVideo` stopped the video stream but didn't clear up the timeout which loops the `_captureToCanvas` method. Removed unnecessary code. The outside catch does the same as the inside one so we don't need nested try/catch blocks No need to set a timeout if we can't get a video stream! Adding the ability to select a source to capture from. Use `qr.getVideoSources` to get a list of video sources and pass the id of the source you are interested in to `prepareVideo`. Conflicts: build/qcode-decoder.js build/qcode-decoder.min.js src/qcode-decoder.js Updated the example so that you can pick from multiple cameras if available on your device. Also cleaning up a previously running videoCapture if you try to start another... Setting the canvas to the videos dimensions. If you manually set the canvas dimension then there is an issue when you switch to a camera which has a different aspect ratio (e.g. on my Nexus 7 both cameras are portrait rather than landscape). If you don't set the width and height that you are pulling from the video then only a subsection of the video feed is scanned for QR codes (see 39e7e0a). This seems to me to be the only way to deal with it although I now wonder if you need to call prepareCanvas manually at all... No need to have a public `releaseVideo` function. The `stop` function should be suitable for all use-cases we can currently conceive... Don't die if a browser doesn't support MediaStreamTrack.getSources It looks like currently Chrome is the only browser to support this API. Unfortunately client code will need to check the length of the returned array and behave accordingly... MediaStreamTrack.getVideoSources is not crossbrowser We now deal with this and the API is a bit cleaner (not expecting a `sourceId` when you `prepareVideo` since you can't provide this on most clients) The default example doesn't let you choose cameras and introducing a new example which lets you select camera... Removed unnecessary debug code
|
Re. squashing the commits, I've done it. To be honest I generally prefer to keep history to help with Re. Firefox, where there are multiple camera available on the system Firefox has it's own built in UI to allow the user to choose a camera (it is part of the "accept access to the camera" flow). Maybe you could mention this in the readme for people who don't have a multi-camera device with firefox to test on. |
|
@vitch that's true about Firefox, i'll open an issue to remember. That's true about blame and bisect. I've actually never used |
multiple cameras, canvas dimensions, stop method, example for multiple cameras
Sorry for a pull request with a bunch of unrelated stuff in it!
Each commit has a message which should describe the thoughts behind it. The final commit (0447b18) is the one I'm least sure and would be interested to discuss if you think it's a good idea or not.
I'm just leaving the office now so sorry I haven't given all of the information here. I will try to add to it tomorrow if there is anything that needs clarification!