Fix/continuous scan performance regression #5
Conversation
- Add BACKUP file with original polling implementation. - Add REFACTOR-NOTES with objectives and impact analysis. - Prepare for performance optimization to restore frame callbacks.
- Implement _scanContinuousFrameCallback() using requestVideoFrameCallback. - Provides 30-60 fps scanning vs 0.4 fps polling approach. - Includes proper timeout and abort signal handling. - Adds frame count logging for debugging. - Retries on errors except AbortError.
- Implement _scanContinuousPolling() as compatibility fallback. - Uses setTimeout with 2.5s intervals for non-video sources. - Maintains existing polling behavior from original implementation. - Provides universal compatibility with any source type. - Will serve as graceful fallback when frame callbacks unavailable.
- Replace polling-only implementation with intelligent routing. - Delegate to _scanContinuousFrameCallback for HTMLVideoElement (fast). - Delegate to _scanContinuousPolling for other sources (compatible). - Add warning when non-optimal source type is used. - Reduce public method complexity from 60+ to 15 lines. - Keep original implementation as scanContinuous_BACKUP for reference. - Move backup inline instead of separate file for easier comparison. - Delete BACKUP-scanContinuous-before-refactor.js file.
- Add block comment explaining frame-accurate vs polling strategies. - Document performance characteristics and trade-offs. - Include routing logic visualization with ASCII diagram. - Explain design rationale and historical context. - Add implementation status section to REFACTOR-NOTES.md. - Document performance impact and testing requirements. - Improve maintainability for future developers.
- Add customMessage parameter to override base error message. - Add reason parameter for timeout vs user cancellation distinction. - Update _scanContinuousFrameCallback to use helper with custom messages. - Update _scanContinuousPolling to use helper with custom messages. - Ensure consistent error format: 'after N frames/attempts (reason)'. - Helper now properly preserves original descriptive error messages.
- Verify all methods exist and are properly integrated. - Confirm QR and PDF417 scanning performance improvement. - Validate MRZ and file upload paths unchanged. - Test timeout and abort error handling with context. - Verify frame-accurate scanning achieves 12-16 fps.
- Add Performance section highlighting frame-accurate scanning. - Document scanning strategies (frame-accurate vs polling fallback). - Include performance metrics table showing 30x-40x improvement. - Add architecture section explaining continuous scanning implementation. - Enhance Features list with performance and error context improvements. This documents the significant performance improvements from the frame-accurate scanning refactor.
- Add version 2.0.0 entry documenting performance improvements. - Document 30-40x performance improvement (0.4 fps to 12-16 fps). - List all new features (frame-accurate scanning, enhanced errors). - Document refactored scanContinuous method implementation. This captures the complete scope of the scanning performance optimization.
bparth24
requested review from
BigBlueHat,
dlongley,
gannan08,
jameseaster,
mandyvenables,
msporny and
tminard
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5 +/- ##
=========================================
- Coverage 10.18% 9.28% -0.90%
=========================================
Files 8 10 +2
Lines 805 883 +78
=========================================
Hits 82 82
- Misses 723 801 +78
Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
| * ==================================== | ||
| * | ||
| * ORIGINAL IMPLEMENTATION (Reference File): | ||
| * - Spread across multiple Vue component computed properties. | ||
| * - Used document.body dimensions (clientWidth/clientHeight). | ||
| * - Applied Math.floor() to regionMaskEdgeLength. | ||
| * - No bounds checking on calculated values. | ||
| * - Tightly coupled to Vue component lifecycle. | ||
| * | ||
| * CHANGES (Current Implementation): | ||
| * | ||
| * 1. DIMENSION SOURCE CHANGE: | ||
| * - BEFORE: Used document.body.clientWidth/clientHeight. | ||
| * - AFTER: Extracts dimensions from source element itself. | ||
| * - IMPACT: More accurate region calculation based on actual media dimensions. | ||
| * - CONSIDERATION: Behavior may differ if video size ≠ viewport size. | ||
| * | ||
| * 2. CONSOLIDATION: | ||
| * - BEFORE: Scattered across computed properties (regionMaskEdgeLength, | ||
| * regionLeft, regionTop, region). | ||
| * - AFTER: Single, self-contained utility function. | ||
| * - BENEFIT: Improved reusability and testability. |
There was a problem hiding this comment.
This has a very strong "generated with an LLM" smell to it -- was it generated using an LLM?
There was a problem hiding this comment.
I use Grammarly often to check tone, clarity, and correct grammar while inline documenting and or readme instructions in general.
My approach usually goes: I rough draft with bullet points and then use Grammarly to get tone, clarity, and grammar check.
CHANGELOG.md
Outdated
|
|
||
| ### Improved | ||
|
|
||
| - Continuous scanning performance: 0.4 fps > 12-16 fps (30x-40X improvement) |
There was a problem hiding this comment.
We don't have an Improved section in our changelogs, so we can fold this into Changed`:
| ### Improved | |
| - Continuous scanning performance: 0.4 fps > 12-16 fps (30x-40X improvement) | |
| - Continuous scanning performance: 0.4 fps > 12-16 fps (30x-40X improvement) |
CHANGELOG.md
Outdated
| - **Frame-accurate scanning** using `requestVideoFrameCallback()` for optimal performance | ||
| - 12-16 fps scanning rate (30x-40x faster than polling approach) | ||
| - < 0.5 second average detection time (15x faster) | ||
| - Automatic routing based on source type (HTMLVideoElement vs others) | ||
| - Polling fallback method for non-video sources | ||
| - Ensures universal compatibility with all source types | ||
| - Emits warning when using slower fallback path | ||
| - Enhanced error messages with scan context | ||
| - Includes frame count for frame-accurate scanning | ||
| - Includes attempt count for polling-based scanning | ||
| - Specifies scan method used (for debugging) | ||
| - Provides abort reason (timeout vs user cancellation) | ||
| - Comprehensive architecture documentation in code | ||
| - Detailed comments explaining continuous scanning strategies | ||
| - Performance characteristics for each approach |
There was a problem hiding this comment.
Add periods/full stops to the end of all of these to be consistent with our pattern in other libraries.
| - Overall separation of UI concerns from scanning logic for better code reusability | ||
|
|
||
| ### Changed | ||
|
|
There was a problem hiding this comment.
Should clean up the format of entries in the same ways mentioned above.
lib/opticalScanner.js
Outdated
|
|
||
| // No results yet - schedule next frame. | ||
| video.requestVideoFrameCallback(scanFrame); | ||
|
|
lib/opticalScanner.js
Outdated
|
|
||
| // Create timeout-aware abort signal. | ||
| const controller = this._createTimeoutController(signal, timeoutMs); | ||
| const effectiveSignal = controller?.signal || signal; |
There was a problem hiding this comment.
It seems like this could be reworked to just produce a variable named signal ... instead of calling the one that is used everywhere effective. A quick scan doesn't show controller being used anywhere other than for its signal ... and it looks like _createTimeoutController is meant to be some kind of combiner on signals. It was hard to follow what it's doing, but could it be simplified by using the built-in signal combiner?
https://developer.mozilla.org/en-US/docs/Web/API/AbortSignal/any_static
Perhaps AbortSignal.timeout() might also be of some use here?
There was a problem hiding this comment.
I understand. I will simplify and improve the code structure/flow.
Context: Related to signal and timeout. I was trying to achieve two things in one go —
- User cancellation signal (passed from Vue)
- Automatic timeout signal (based on timeout param)
Additionally, for timeout values, based on testing results, I have set default values here:
https://github.com/digitalbazaar/bedrock-web-optical-scanner/blob/fix/continuous-scan-performance-regression/lib/cameraScanner.js#L261C3-L295
Note: Thinking out loud here, and have not tested the code below. Here is what I'm thinking of implementing instead of _createTimeoutController(). Before I implement it, I would like to hear your feedback.
_combineSignals(userSignal, timeoutMs) {
// No signal or timeout needed
if (!userSignal && timeoutMs <= 0) {
return undefined;
}
// Collect signals to combine
const signals = [];
if (userSignal) signals.push(userSignal);
if (timeoutMs > 0) signals.push(AbortSignal.timeout(timeoutMs));
// Return combined signal (or single signal if only one)
return signals.length === 1 ? signals[0] : AbortSignal.any(signals);
}
There was a problem hiding this comment.
+1 Yeah, that looks cleaner (modulo style adjustments of course).
There was a problem hiding this comment.
@dlongley, appreciate the quick feedback.
There was a problem hiding this comment.
It seems like this could be reworked to just produce a variable named
signal... instead of calling the one that is used everywhereeffective. A quick scan doesn't showcontrollerbeing used anywhere other than for its signal ... and it looks like_createTimeoutControlleris meant to be some kind of combiner on signals. It was hard to follow what it's doing, but could it be simplified by using the built-in signal combiner?https://developer.mozilla.org/en-US/docs/Web/API/AbortSignal/any_static
Perhaps
AbortSignal.timeout()might also be of some use here?
lib/opticalScanner.js
Outdated
| return new Promise((resolve, reject) => { | ||
| // Performance tracking | ||
| const scanStartTime = Date.now(); | ||
| // frame timing inside scanFrame: | ||
| let lastFrameTime = Date.now(); | ||
| let frameCount = 0; | ||
|
|
||
| const scanFrame = async () => { |
There was a problem hiding this comment.
I suspect this could be reworked to just call an async function instead -- which will improve maintenance. This scheduling function seems to be doing a lot (maybe it's mostly metric calculations though?) and a signal to me that it could be refactored to be simpler is the fact that video.requestVideoFrameCallback(scanFrame); is called in three independent places. My intuition is that those could be coerced into a single code path, resulting in something that's simpler to follow and maintain.
There was a problem hiding this comment.
I suspect this could be reworked to just call an async function instead -- which will improve maintenance. This scheduling function seems to be doing a lot (maybe it's mostly metric calculations though?) and a signal to me that it could be refactored to be simpler is the fact that
video.requestVideoFrameCallback(scanFrame);is called in three independent places. My intuition is that those could be coerced into a single code path, resulting in something that's simpler to follow and maintain.
The attempt to address the above concerns in this commit - 2e61ace.
There was a problem hiding this comment.
Additionally, I will improve the existing coding pattern to use async functions instead of new Promise(...).
…rate) line comments.
lib/plugins/enhancedPdf417Plugin.js
Outdated
| * @param {object} options - Plugin-specific options. | ||
| * @param {AbortSignal} options.signal - Abort signal. | ||
| * @param {string} options.license - Dynamsoft license key. | ||
| * @param {boolean} options.useDynamsoft - Use Dynamsoft engine. |
There was a problem hiding this comment.
Use markup for all the optional params here:
| * @param {boolean} options.useDynamsoft - Use Dynamsoft engine. | |
| * @param {boolean} [options.useDynamsoft] - Use Dynamsoft engine. |
There was a problem hiding this comment.
Noted. I will use [] if it is an optional parameter whenever it is applicable.
| ## 1.1.0 - 2025-mm-dd | ||
|
|
||
| ### Added | ||
|
|
There was a problem hiding this comment.
Normal style is no blank line after these sub headers.
- Replaces custom _createTimeoutController() with _combineSignals(). - Uses AbortSignal.any() and AbortSignal.timeout(). - Eliminates manual cleanup. - Renames effectiveSignal to signal for clarity.
- Checks for Dynamsoft license key in CameraScanner.scan(). - Throws clear error.
Frame Loop Refactoring: - Extract _runFrameLoop() for separation of concerns. - Replace _createTimeoutController() with _combineSignals(). - Use standard AbortSignal.any() and AbortSignal.timeout() APIs. - Extract metrics and logging helpers for maintainability. Documentation Improvements: - Reduce inline documentation verbosity in opticalScanner.js. - Fix performance metric inconsistencies (62-83ms throughout). - Add device specifications to README (MacBook Pro M1, Chrome). - Remove duplicate architecture details from method comments. Addresses code review feedback on maintainability and clarity.
5 participants
Restore frame-accurate scanning performance
Summary
Restores 12-16 fps frame-accurate scanning using
requestVideoFrameCallback()that was lost during the Vue component architecture refactor. Improves performance by 30x-40x over the polling approach.For details, refer to the README.md and CHANGELOG.md files.
@dlongley cc: @mandyvenables @jameseaster @tminard @msporny @BigBlueHat