ericblade / quagga2

An advanced barcode-scanner written in Javascript and TypeScript - Continuation from https://github.com/serratus/quaggajs
MIT License
758 stars 85 forks source link

Android 11 Hanging Chrome Fix: Call `stop()` on LiveStream <video> element before stopping video stream #266

Closed anonmos closed 3 years ago

anonmos commented 4 years ago

Description On Android 11 we started experiencing a behavior where calling quaggaInstance.stop() causes the video element to freeze Chrome rendering. Touch events are still active (tapping around on the app causes the URL to change when accidentally clicking links), but nothing on the view changes. Even elements under the video element are frozen. Testing on Android 10 did not seem to exhibit the issue. If I had to guess, there's a memory leak that happens when stopping underlying MediaChannel references without first pausing the parent <video> element itself.

Device Test Specifics

Reproduction Steps

Solution Steps We've found a workaround for now, thanks to this comment in the Quagga code base: https://github.com/ericblade/quagga2/blob/8226a048ad526845ff6b102be1a10f2ca57fbcce/src/input/camera_access.ts#L101

Through some experimentation, we came up with the following solution:

public stopScanner() {
        const htmlVideoElements: NodeListOf<HTMLVideoElement> = document.getElementsByName('video') as NodeListOf<HTMLVideoElement>
        for (let i = 0; i < htmlVideoElements.length; ++i) {
            const targetElement = htmlVideoElements[i]

            if (targetElement && targetElement.pause) {
                targetElement.pause()
            }
        }
        setTimeout(() => {
            Quagga.stop()
        },         0)
    }

We also did the reverse in a startScanner() method, calling play() on all <video> elements, setting a timeout, and calling Quagga.start(). So far it's fixed the issue for us.

The setTimeout seems to be necessary to give the target elements a chance to pause the underlying camera hardware before attempting to stop the MediaChannels. Otherwise my test devices exhibit a race condition behavior where the solution only sometimes works.

If I get the spare chance I'll open a PR for you all, but wanted to file this as an issue first in case anyone else is running into this problem.

github-actions[bot] commented 4 years ago

Thank you for filing an issue! Please be patient. :-)

ericblade commented 4 years ago

Interesting that it's more visible of a problem with Android 11 despite using the same Chrome version as what I have, on my Android 9 LG G6 .. I think.. phone is off charging so I can't check it for sure right now.

A PR would be great, because I don't have anything newer than Android 9 to check it against. It's not something that I've dug into at all, other than making that comment, because the problem that I was experiencing seems to be intermittently happening, and only in Android. I can only repro it sporadically in Android, and not at all on desktop. Basically, even after stopping everything, the notification bar on Android 8/9 reports that Chrome is still using the camera. As far as I can tell, from my application, it's receiving camera input, but since Quagga is stopped, and there's no visual elements when it's stopped, I see nothing, and it's not processing anything.

hariaw commented 3 years ago

@anonmos I have filed a bug with Android re this at https://issuetracker.google.com/u/1/issues/173142922: please add any additional information you have there.

anonmos commented 3 years ago

@hariaw very thorough. I don't see anything you've missed. Looks like you've got it covered.

Thanks for submitting. Still working on a time slot to get the patch in on quagga.

Zaphod-Beeblebrox commented 3 years ago

For what it's worth, this does seem to be either an Android 11 issue, or more specifically the Pixels. I have a PWA that not only uses this barcode scanner library, but also has normal camera functions. 3 of my users are on Pixels (this is an internal business app), and all have similar issues with Chrome freezing when the video playback is stopped. In both my camera section, and the barcode scanner.

It aggravated me so much I bought a Pixel 4a just to test this. Absolutely no errors in the console.

Following @anonmos suggestion about pausing the video elements before issuing a stop fixed this for me. In both the barcode scanner, and my other camera functions. Just to be on the safe side, I used a 10ms wait on the setTimeout, but that probably isn't necessary.

One other tidbit of information. When my Pixel 4a arrived, I immediately tested my app, without doing any OS updates. No freezing or lockups. Once I took the latest update, the freezing was consistent. Unfortunately, I didn't take note of the OS version at unboxing.

ericblade commented 3 years ago

Well, I'm definitely willing to believe this is an Android 11 issue, possibly specifically Pixel related, perhaps there's a missing instruction in the camera driver on that phone or something equally odd. I wonder if a similar problem can be hit with native code as well. I think someone had mentioned it also occurs in Firefox?

Unfortunately, I can't test this in the slightest -- just buying a Pixel is not within my capabilities right now -- so I can't really put together a fix properly. :|

I really appreciate everyone's help here, by the way! Nice to see some of the users out here :)

ericblade commented 3 years ago

Hey all, I still haven't been bit by this, but I did decide to spend some time poking at solving the issue where the camera appears to still be in use in Android despite calling Quagga.stop(), so I've attempted to implement a similar fix. I'm going to make a pull request as soon as it passes testing, but basically right at the point where I left that original comment, I'm implementing same as above code:

release(): void {
    const tracks = streamRef && streamRef.getVideoTracks();
    if (QuaggaJSCameraAccess.requestedVideoElement !== null) {
        QuaggaJSCameraAccess.requestedVideoElement.pause();
    }
    setTimeout(() => {
        if (tracks && tracks.length) {
            tracks[0].stop();
        }
        streamRef = null;
        QuaggaJSCameraAccess.requestedVideoElement = null;
    });
},

I don't really have any idea yet whether it will solve either issue, but I'm going to give it a try and see what happens.

ericblade commented 3 years ago

@anonmos @hariaw @Zaphod-Beeblebrox if any of you could pull 1.3.0 and give it a spin without your additional workarounds, i'd love to hear the results?

I think I've put the details of the described fix into the relevant location.

putimir commented 3 years ago

Hi, sorry for the newb post, but I'll just give it a shot: I'm using one of the earlier versions (specifically: https://serratus.github.io/quaggaJS/v1.0.0-beta.1/examples/js/quagga.min.js) quite happy (!) for some time now, but I also noticed this issue on my android11.

The question is, is this fix apply-able to v1.0.0-beta.1?

If I just drop in v1.3, I'm getting the "Uncaught TypeError: Quagga.decoder is not a function"

This is my setup script:

var Quagga = window.Quagga;
var App = {
    _scanner: null,
    init: function() {
        this.attachListeners();
    },
    activateScanner: function() {
        var scanner = this.configureScanner('.overlay__content'),
            onDetected = function (result) {
                var beep = document.getElementById('scanner-beep');
                beep.play();
                beep.onended = function() {
                  goto_product_stock(result.codeResult.code);
                  stop();
                };
            }.bind(this),
            stop = function() {
                scanner.stop();  // should also clear all event-listeners?
                scanner.removeEventListener('detected', onDetected);
                this.hideOverlay();
                this.attachListeners();
            }.bind(this);

        this.showOverlay(stop);
        scanner.addEventListener('detected', onDetected).start();
    },
    attachListeners: function() {
        var self = this,
            button = document.querySelector('#scan-button');

        button.addEventListener("click", function onClick(e) {
            e.preventDefault();
            button.removeEventListener("click", onClick);
            self.activateScanner();
        });
    },
    showOverlay: function(cancelCb) {
        if (!this._overlay) {
            var content = document.createElement('div'),
                closeButton = document.createElement('div');

            close_button = document.createElement('i');
            close_button.className = 'icon-close';
            closeButton.appendChild(close_button);
            content.className = 'overlay__content';
            closeButton.className = 'overlay__close';
            this._overlay = document.createElement('div');
            this._overlay.className = 'overlay';
            this._overlay.appendChild(content);
            content.appendChild(closeButton);
            closeButton.addEventListener('click', function closeClick() {
                closeButton.removeEventListener('click', closeClick);
                cancelCb();
            });
            document.body.appendChild(this._overlay);
        } else {
            var closeButton = document.querySelector('.overlay__close');
            closeButton.addEventListener('click', function closeClick() {
                closeButton.removeEventListener('click', closeClick);
                cancelCb();
            });
        }
        this._overlay.style.display = "block";
    },
    hideOverlay: function() {
        if (this._overlay) {
            this._overlay.style.display = "none";
        }
    },
    configureScanner: function(selector) {

        if (!this._scanner) {
            this._scanner = Quagga
                .decoder({
                    readers: ['code_39_reader', 'code_128_reader'],
                })
                .locator({patchSize: 'medium'})
                .fromSource({
                    target: selector,
                    constraints: {
                        width: 1024,
                        height: 768,
                        deviceId: backCamID
                    }
                  });
        }
        return this._scanner;
    }
};

var backCamID = null;
var last_camera = null;
navigator.mediaDevices.enumerateDevices()
.then(function(devices) {
  devices.forEach(function(device) {
    if( device.kind == "videoinput" && device.label.match(/back/) !== null ){
      backCamID = device.deviceId;
    }
    if( device.kind === "videoinput"){
      last_camera = device.deviceId;
    }
  });
  if( backCamID === null){
    backCamID = last_camera;
  }
  })
.catch(function(err) {});

App.init();
putimir commented 3 years ago

Hm, interestingly, it seems I found (I hope) a possible workaround (derived from the suggestion in initial post):

I changed my close function from:

.....
            stop = function() {
                scanner.stop();  // should also clear all event-listeners?
                scanner.removeEventListener('detected', onDetected);
                this.hideOverlay();
                this.attachListeners();
            }.bind(this);
....

to

....
            stop = function() {
                htmlVideoElements = document.getElementsByTagName('video');
                console.log(htmlVideoElements);
                for (i = 0; i < htmlVideoElements.length; ++i) {
                    targetElement = htmlVideoElements[i]
                    if (targetElement && targetElement.pause) {
                        targetElement.pause();
                    }
                }
                setTimeout(function() {scanner.stop() }, 50);
                scanner.removeEventListener('detected', onDetected);
                this.hideOverlay();
                this.attachListeners();
            }.bind(this);
....

I decided to use setTimeout of 50ms and from that point forward I couldn't reproduce the error anymore (cca. 100 scans in rapid succession).

Not exactly knowing what worked (the video.pause or the delay on the .close), so I also tried to use delay only and discovered, that the delay only of 50ms fixes my problem.

ericblade commented 3 years ago

I discovered that adding a 10ms timeout in my application, before removing the video element from the document, prevents the problem I had where Chrome would leave the camera running despite all of the calls to stop the media running before hand anyway. So, seems Chrome has some issues with that on it's own.

Unfortunately @putimir I'm not sure what the differences are between the "1.0.0 beta" in the original repo, and the last code that was on master, which is where I decided to start from. It does look like the interface for it was changed pretty drastically, and actually I describe making what appears to be a change with a similar idea, over in https://github.com/ericblade/quagga2/discussions/323 .. I don't think I like that idea of chaining several function calls to build the config, because there is a certain amount of stuff that is required to be able to do anything, whereas chaining implies that the intermediate steps should be functional somehow.

So, it looks like your Quagga.decoder().locator().fromSource() chain would need to become something like

Quagga.init({ decoder: { readers: [...] }, locator: { ... }, inputStream: { target, constraints, type: 'LiveStream' } })

something like that.

anonmos commented 3 years ago

@anonmos @hariaw @Zaphod-Beeblebrox if any of you could pull 1.3.0 and give it a spin without your additional workarounds, i'd love to hear the results?

I think I've put the details of the described fix into the relevant location.

Thanks for putting the time in on the fix @ericblade. I'll set some time aside later on this week to test against our app using the Pixels I have on hand.

On the surface it looks like your change is what I had in mind before I got completely distracted. I don't see any reason why it wouldn't work, but I've been bitten enough to know that my sanity checks are absolutely never a substitute for real-world testing, hah.

putimir commented 3 years ago

I discovered that adding a 10ms timeout in my application, before removing the video element from the document, prevents the problem I had where Chrome would leave the camera running despite all of the calls to stop the media running before hand anyway. So, seems Chrome has some issues with that on it's own.

Hey @ericblade, yes as I said in my previous post, I also mitigated the problem by adding a delay to my close function... I only hope it lasts a while until Android / Chrome devs do something next to break :D (so for little while I'll stick to my version as it works beautifully for my purpose but nevertheless, thank you for taking your time into this. I will, of course try your suggestion and update to quagga2 asap.

ericblade commented 3 years ago

@anonmos I'd really appreciate it, because I don't have the hardware/software combination to test, so I'll definitely be looking for feedback from people who've run into the issue before.

@putimir Like I said, I don't know exactly what the differences are between my fork, and the original 1.x, so I'm not going to guarantee you it's an upgrade.. :-D If it's just interface differences, then great, but I make no guarantees on that, since I didn't realize there was a 1.x branch on the original, until after i'd already spent quite a lot of time on the fork :| oops

ericblade commented 3 years ago

... i just realized reviewing this post, that I didnt specify a timeout amount, so hopefully that doesn't make things weird. note to self: make sure that argument is supplied

tillschander commented 3 years ago

I was running into this freeze-issue with the original quagga as well. But it was resolved once I replaced the original quagga with quagga2 v.1.3.0. in my code. I didn't have to change anything else.

anonmos commented 3 years ago

@ericblade confirming @tillschander's findings. I upgraded our package version to v1.3.1, stripped out our temporary measure I described in the description of this issue, and replaced it with an await Quagga.stop(). I can't reproduce the problem any longer on my Pixel 4 using Chrome.

I think we can stick a fork in this. Great work! Thanks for the fix.

ericblade commented 3 years ago

Awesome! Thanks to all here who helped provide info, research, etc. I've been in sort of a bad headspace for a bit, but once I cleared that up, I was able to pretty clearly understand what was going on, and apply the given workaround into the main. I think it improves the API a little bit, too.

Though hopefully Google will fix Android :-D