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.

Sign up for GitHub

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

Jump to bottom

Add support for detecting and turning on/off flash light #241

Closed
wants to merge 1 commit into from
Closed

Add support for detecting and turning on/off flash light #241

wants to merge 1 commit into from

Conversation

lafriks
Copy link

@lafriks lafriks commented Jun 21, 2021 •
edited by mebjas
Loading

Add support for flash light in Html5QrcodeScanner and Html5Qrcode #129

lafriks
lafriks commented Jun 21, 2021
View reviewed changes
tsconfig.json
@@ -5,7 +5,7 @@
"sourceMap": true,
"moduleResolution": "classic",
"lib": [
"es7",
"es2018",
Copy link
Author

Choose a reason for hiding this comment

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

this was already needed for:

html5-qrcode/src/html5-qrcode.ts

Line 1033 in a14ca5e

.finally(() => {

@mebjas
Copy link
Owner

mebjas commented Jun 21, 2021

Thanks @lafriks for this, super helpful!

Would you mind sending this PR to https://github.com/mebjas/html5-qrcode/tree/flashlight instead, so I can tweak things to taste a bit after trying out and merge to master later & publish?

@mebjas
Copy link
Owner

mebjas commented Jun 21, 2021

I'll review that PR directly.

@lafriks lafriks changed the base branch from master to flashlight June 21, 2021 13:41
@lafriks
Copy link
Author

lafriks commented Jun 21, 2021

Done

mebjas
mebjas reviewed Jun 21, 2021
View reviewed changes
Copy link
Owner

@mebjas mebjas left a comment

Choose a reason for hiding this comment

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

Thanks for the pull request, it already looks good. Added some minor comments.

src/html5-qrcode.ts
@@ -237,6 +237,7 @@ export class Html5Qrcode {
private qrRegion: QrcodeRegionBounds | null = null;
private context: CanvasRenderingContext2D | null = null;
private lastScanImageFile: string | null = null;
private flashOn: boolean;
Copy link
Owner

Choose a reason for hiding this comment

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

nit, rename to isFlashOn for easier reading.

src/html5-qrcode.ts
return Promise.resolve(false);
}

const track = this.videoElement?.srcObject ? (<MediaStream>this.videoElement.srcObject).getVideoTracks()[0] : null;
Copy link
Owner

Choose a reason for hiding this comment

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

nit, split into two lines?

Copy link
Owner

Choose a reason for hiding this comment

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

Comment 2: could this lead to null ptr exception if ...getVideoTracks() returns undefined or empty array? we should do a length check first.

src/html5-qrcode.ts
*
* @returns Current flash state.
*/
public isFlashOn(): boolean {
Copy link
Owner

Choose a reason for hiding this comment

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

why expose this state via a public method?

src/html5-qrcode.ts
* @param on if true flash will be turned on otherwise turned off.
* @returns Promise that flash state has changed.
*/
public setFlash(on: boolean): Promise<void> {
Copy link
Owner

Choose a reason for hiding this comment

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

optional, rename to more readable shouldSetFlashOn or something more relevant.

src/html5-qrcode.ts
@@ -767,6 +833,7 @@ export class Html5Qrcode {
const tracks = stream.getVideoTracks();
for (const track of tracks) {
track.enabled = false;
// This will also stop flash light
Copy link
Owner

Choose a reason for hiding this comment

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

super nit, add a "." after the comment

Copy link
Owner

Choose a reason for hiding this comment

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

(ditto elsewhere)

@rodneystover
Copy link

Not sure if I'm posting this in the correct place. I am using the Html5Qrcode source successfully and was wondering how I would insert code to add a toggle to access on/off for the flashlight in my code. I read above you have to call it after the camera is accessed?

const config = { fps: 60,
				 qrbox: 275,
				 aspectRatio: 1.0,
			     experimentalFeatures: {
				     useBarCodeDetectorIfSupported: true
				 }
			   };	

html5QrCode.start({ facingMode: { exact: "environment"} }, config, qrCodeSuccessCallback);

@ROBERT-MCDOWELL
Copy link

@rodneystover
please open a new issue describing exactvly your case, here is a PR section

@ROBERT-MCDOWELL
Copy link

@mebjas
Any news about this PR?

@mebjas
Copy link
Owner

mebjas commented Oct 18, 2021

Some comments need to be addressed here. I can alternatively add this feature in a separate PR.

@mebjas mebjas deleted the branch mebjas:flashlight October 22, 2021 04:59
@mebjas mebjas closed this Oct 22, 2021
@lafriks lafriks deleted the feat/flashlight branch October 22, 2021 06:35
@mageshk98
Copy link

@mebjas Any update on flashlight support?

@mebjas
Copy link
Owner

mebjas commented Oct 29, 2022 •
edited
Loading

I am working on this now! - follow #129 for progress.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mebjas mebjas mebjas left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Feature request] Add support for flash Light
5 participants
@lafriks @mebjas @rodneystover @ROBERT-MCDOWELL @mageshk98