Repository navigation
Inflate map sightline data with the browser's gzip on the web #255
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
Merged
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| import 'dart:typed_data'; | ||
|
|
||
| import 'vision_world_gzip.dart'; | ||
|
|
||
| /// Desktop's zlib is native and fast; it runs in a background isolate. | ||
| Future<Uint8List> inflateWorldGzip(Uint8List compressed) async => | ||
| decodeWorldGzip(compressed); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,47 @@ | ||
| import 'dart:js_interop'; | ||
| import 'dart:js_interop_unsafe'; | ||
| import 'dart:typed_data'; | ||
|
|
||
| import 'vision_world_gzip.dart'; | ||
|
|
||
| @JS('DecompressionStream') | ||
| extension type _DecompressionStream._(JSObject _) implements JSObject { | ||
| external factory _DecompressionStream(String format); | ||
| } | ||
|
|
||
| extension type _ReadableStream._(JSObject _) implements JSObject { | ||
| external _ReadableStream pipeThrough(JSObject transform); | ||
| } | ||
|
|
||
| @JS('Response') | ||
| extension type _Response._(JSObject _) implements JSObject { | ||
| external factory _Response(JSAny body); | ||
| external _ReadableStream get body; | ||
| external JSPromise<JSArrayBuffer> arrayBuffer(); | ||
| } | ||
|
|
||
| /// The browser's gzip checks what [decodeWorldGzip] checks: it rejects a | ||
| /// wrong CRC-32 or length, a stream that ends before its footer, and bytes | ||
| /// after it. The length is compared again here so a short read cannot pass. | ||
| Future<Uint8List> inflateWorldGzip(Uint8List compressed) async { | ||
| // Firefox before 113 and Safari before 16.4 lack it; they keep the slow path. | ||
| if (!globalContext.has('DecompressionStream')) { | ||
| return decodeWorldGzip(compressed); | ||
| } | ||
| checkWorldGzipHeader(compressed); | ||
| final Uint8List decoded; | ||
| try { | ||
| final inflated = _Response(compressed.toJS) | ||
| .body | ||
| .pipeThrough(_DecompressionStream('gzip')); | ||
| decoded = | ||
| (await _Response(inflated).arrayBuffer().toDart).toDart.asUint8List(); | ||
| } on Object { | ||
| throw const FormatException('Invalid world gzip checksum or length.'); | ||
| } | ||
| final footer = ByteData.sublistView(compressed, compressed.length - 8); | ||
| if (footer.getUint32(4, Endian.little) != decoded.length) { | ||
| throw const FormatException('Invalid world gzip checksum or length.'); | ||
| } | ||
| return decoded; | ||
| } | ||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new browser decompression path and its older-browser fallback have no committed browser test. Both paths decoded valid gzip and rejected damaged input in Chrome, but that check is not part of the test suite. A later regression in either path could therefore go unnoticed. Please commit browser tests for valid and damaged input through
inflateWorldGzip, including the fallback. This is a non-blocking coverage concern.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Artifacts
Authored Chrome gzip probe source
Authored Chrome probe command
Committed test coverage inspection
Chrome result with the existing Dart decoder
Chrome result with the new stream and simulated older-browser fallback
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed it's a gap, but a committed browser test wouldn't close it today. CI's
webjob only builds the app (flutter build web), and nothing in CI runs@TestOn('browser')tests: the existing one,test/web/web_codecs_video_encoder_test.dart, is run by hand. On this machineflutter test --platform chromedoesn't load at all. So a new browser test would sit unexecuted. Instead, the browser path was checked in Chromium:FormatException.DecompressionStreamremoved.Running browser tests in CI is worth doing, but as its own change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That’s fair. A committed
@TestOn('browser')test would not provide regression coverage while CI neither runs browser tests nor can reliably launch them, so it wouldn’t close the practical gap today. The Chromium validation is substantially broader than the proposed test—55 fixtures across dart2js and dart2wasm, all bundled assets checked byte-for-byte, and the fallback exercised—so I’m withdrawing this finding for PR #255. Running browser tests in CI is worthwhile, but belongs in a separate change.