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

fixes #287 Unable to stop the camera when route changed #357

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

Patrick-Sherlund
Copy link

@Patrick-Sherlund Patrick-Sherlund commented Sep 12, 2023

There is an issue in the following scenario:

  1. QrReader Component is initially mounted on render
  2. User navigates to another component (i.e. via React Router, without reloading page)
  3. Component is unmounted, but the camera continues to run in the background, even though the component was unmounted. Decode function from @zxing runs independently of any video source and does not stop.

Now, Upon unmounting of the QrReader component, the video track will be stopped and parent decode functions will be haulted.

Upon unmounting of the QrReader component, the video track will now be stopped.
@Patrick-Sherlund
Copy link
Author

@JodusNodus @JonatanSalas Please review and let me know if there are any issues! Thank you

Patrick-Sherlund and others added 2 commits September 12, 2023 11:17
Stops the callback function passed inside parent function 'decodeFromConstraints'
@uptownben
Copy link

Any chance this can get merged in? Deal breaker for my use case.

Comment on lines +36 to +40
if (stopRequested.current) {
//Callback to parent function continuously runs after destruction of Video Element // Stream Video Tracks
//Intentionally throwing exception to step out of parent function.
throw new StopRequestedException();
}

Choose a reason for hiding this comment

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

I have found that throwing an Exception here is unnecessary and undesirable as it leads to an uncaught runtime error. We can also stop the code reader using controls:

    if (isValidType(video, 'constraints', 'object')) {
      codeReader
-       .decodeFromConstraints({video}, videoId, (result, error) => {
+       .decodeFromConstraints({video}, videoId, (result, error, controls) => {
          if (stopRequested.current) {
-           //Callback to parent function continuously runs after destruction of Video Element // Stream Video Tracks
-           //Intentionally throwing exception to step out of parent function.
-           throw new StopRequestedException();
+           controls?.stop();
          }
          if (isValidType(onResult, 'onResult', 'function')) {
            onResult?.(result, error, codeReader);
          }
        })
        .then((controls: IScannerControls) => (controlsRef.current = controls))
        .catch((error: Error) => {
          if (isValidType(onResult, 'onResult', 'function')) {
            onResult?.(null, error, codeReader);
          }
        });
    }

Comment on lines +6 to +11
export class StopRequestedException extends Exception {
constructor() {
super();
Object.setPrototypeOf(this, new.target.prototype);
}
}

Choose a reason for hiding this comment

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

We can remove this class if we use controls instead of throwing an exception

Comment on lines +32 to +42
navigator.mediaDevices
.getUserMedia({
video: true,
audio: false,
})
.then((mediaStream) =>
mediaStream.getTracks().forEach((track) => {
track.stop();
mediaStream.removeTrack(track);
})
);

Choose a reason for hiding this comment

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

This causes an error when QrReader is conditionally rendered, because when QrReader is unmounted, no media can be found. We can handle it:

        navigator.mediaDevices
          .getUserMedia({
            video: true,
            audio: false,
          })
          .then((mediaStream) =>
            mediaStream.getTracks().forEach((track) => {
              track.stop();
              mediaStream.removeTrack(track);
            })
          )
+         .catch((error) => {
+           console.warn('Error stopping video tracks:', error);
+         });

@adboio
Copy link

adboio commented Jul 31, 2024

is this going to be merged?

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

Successfully merging this pull request may close these issues.

6 participants