Fix: Match Node's image processing order in browser

[ericblade]

Refactored frame_grabber_browser.js to match Node's processing order:

1. Draw image at original size
2. Convert RGB to grayscale
3. Scale grayscale data using bilinear interpolation
4. Crop to target region

Resolves #582

[rollingversions]

Change Log for @ericblade/quagga2 (1.10.2 → 1.11.0)

Refactors

• Refactor frame_grabber_browser to more accurately mirror how it's done in node, improving scanning accuracy in code_128 and code_32 readers in test suite

Edit changelog

[Copilot]

Pull request overview

This PR refactors browser-side image processing to match Node.js processing order, addressing issue #582 to ensure consistent barcode detection results across environments.

Key Changes:

• Implements a new 4-step processing pipeline: draw at original size → grayscale conversion → bilinear interpolation scaling → crop to target
• Adds bilinear interpolation function for grayscale scaling to match Node.js behavior
• Creates test external reader (TestExternalCode128Reader) to properly demonstrate the plugin API
• Updates Cypress from 13.1.0 to 13.17.0

Reviewed changes

Copilot reviewed 4 out of 5 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
src/input/frame_grabber_browser.js	Core refactor implementing new 4-step processing order with bilinear interpolation for grayscale scaling
src/reader/test_external_code_128_reader.ts	New test reader demonstrating external reader plugin API, replacing direct import of Code128Reader
test/integration/integration.spec.ts	Updates test to use TestExternalCode128Reader and adds allowFailInBrowser flags for problematic test cases
package.json	Upgrades Cypress to 13.17.0 and removes unnecessary empty dependencies object
package-lock.json	Dependency tree updates from Cypress upgrade (ci-info, tree-kill added; is-ci removed)

[Copilot]

The non-half-sampling path now performs redundant work: it creates a temporary canvas (tempCanvas), draws the image, converts to grayscale, but then in the half-sampling branch (lines 147-160) it redraws the same image again to _canvas. Consider whether the grayscale conversion at line 145 should be conditional on !doHalfSample to avoid unnecessary work when half-sampling is enabled, since the half-sampling path doesn't use the grayData computed at line 145.

[Copilot]

The half-sampling path also creates the same tempCanvas and performs image drawing (lines 127-140) that is then not used, since the half-sampling path redraws the image to _canvas at lines 150-158. This results in duplicate work. Consider restructuring the code so that the temporary canvas and initial drawing only happen when not half-sampling.

[ericblade]

@copilot create an issue or PR to investigate that issue -- we may need to add test-cases to validate the half-sampling path before performing work in that area, I do not know if the half-sample path is well tested in the test suite. Analyze the half-sample test coverage.

[Copilot]

@ericblade I've opened a new pull request, #586, to work on those changes. Once the pull request is ready, I'll request review from you.

[Copilot]

@ericblade I've opened a new pull request, #587, to work on those changes. Once the pull request is ready, I'll request review from you.

[Copilot]

Pull request overview

Copilot reviewed 4 out of 5 changed files in this pull request and generated 2 comments.

[Copilot]

[nitpick] The rotation transformation is not being reset after drawing to tempCanvas. When drawAngle !== 0, the context remains in a transformed state. This differs from the halfSample path (lines 154-155) where the rotation is explicitly undone with _ctx.rotate(-drawAngle) and _ctx.translate(...).

However, since tempCanvas is only used once and then discarded, this may not cause issues. But for consistency and clarity, consider either:

1. Resetting the transformation after drawing (matching the halfSample pattern)
2. Adding a comment explaining why the reset is unnecessary here

This would make the code's intent clearer and prevent potential confusion.

Suggested change

	tempCtx.drawImage(drawable, -_videoSize.y / 2, -_videoSize.x / 2, _videoSize.y, _videoSize.x);
	tempCtx.drawImage(drawable, -_videoSize.y / 2, -_videoSize.x / 2, _videoSize.y, _videoSize.x);
	tempCtx.rotate(-drawAngle);
	tempCtx.translate(-_videoSize.x / 2, -_videoSize.y / 2);

[Copilot]

A new temporary canvas is created on every call to grab(). For performance-sensitive real-time video processing, creating and destroying DOM elements frequently can impact performance due to:

1. DOM element creation/garbage collection overhead
2. Canvas context creation overhead

Consider caching the temporary canvas as a property of the FrameGrabber instance (similar to _canvas and _ctx), initializing it once during creation. This would avoid repeated allocations during video processing.

Example:

// In FrameGrabber.create
const _tempCanvas = document.createElement('canvas');
_tempCanvas.width = _videoSize.x;
_tempCanvas.height = _videoSize.y;
const _tempCtx = _tempCanvas.getContext('2d');

// In _that.grab, use _tempCanvas and _tempCtx directly

This optimization is especially important since grab() is called for every frame during live video scanning.