-
Notifications
You must be signed in to change notification settings - Fork 143
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
[compression-dictionary] Moved the dictionary hash into the body #2784
Conversation
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.
Overall, I like this. I want to flag for others that it doesn't use the skippable frame concept.
FWIW, it doesn't look like it would be particularly difficult to use a skippable frame instead of a custom header in the Zstandard case if that's the way we want to go. It's 4-bytes bigger (for the frame size) so it will essentially have an 8-byte "magic number" (since the size will always be the same). It won't help with the creation of the files but if there's benefit to have the existing tooling be able to decode them (without verifying the hash because it will just skip it) we can go that route. On the client side, it feels like it will be cleaner on the implementation to use the same flow for verifying the dictionary hash and magic number for both encodings without having to deal with different sized magic numbers (and always buffer and remove the first 36 bytes) but it's not a significant difference. |
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.
LGTM
Still LGTM |
@felixhandte could you please take a look and make sure I got the byte sequence for the skippable frame correct? I tested with the zstd cli and it was able to decode the file with the header without issue when created like this: echo -en '\x5e\x2a\x4d\x18\x20\x00\x00\x00' > data.txt.dcz && \
openssl dgst -sha256 -binary dictionary.txt >> data.txt.dcz && \
zstd -D dictionary.txt -f -o tmp.zstd data.txt && \
cat tmp.zstd >> data.txt.dcz |
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.
Looks like you just need to update the PR summary to reflect this change.
Thanks. Updated and added an example with the zstd cli. |
The spec of Compression Dictionary Transport has been changed to use new `dcb` and `dcz` content encodings. httpwg/http-extensions#2784 To follow the spec change, this CL changes the Chromium implementation and tests as follows: - Use "dcb" content encoding name instead of "br-d". - Use "dcz" content encoding name instead of "zstd-d". - Remove the "Content-Dictionary" response header. - Check the magic number and the sha256 hash in the head of the Dictionary-Compressed streams using SharedDictionaryHeaderCheckerSourceStream. - Re-generate test files using the new content encodings. Bug: 1413922 Change-Id: I4f2f40c1f0c3666b4f0b54e34ad966ffccadd96b
The spec of Compression Dictionary Transport has been changed to use new `dcb` and `dcz` content encodings. httpwg/http-extensions#2784 To follow the spec change, this CL changes the Chromium implementation and tests as follows: - Use "dcb" content encoding name instead of "br-d". - Use "dcz" content encoding name instead of "zstd-d". - Remove the "Content-Dictionary" response header. - Check the magic number and the sha256 hash in the head of the Dictionary-Compressed streams using SharedDictionaryHeaderCheckerSourceStream. - Re-generate test files using the new content encodings. Bug: 1413922 Change-Id: I4f2f40c1f0c3666b4f0b54e34ad966ffccadd96b
The spec of Compression Dictionary Transport has been changed to use new `dcb` and `dcz` content encodings. httpwg/http-extensions#2784 To follow the spec change, this CL changes the Chromium implementation and tests as follows: - Use "dcb" content encoding name instead of "br-d". - Use "dcz" content encoding name instead of "zstd-d". - Remove the "Content-Dictionary" response header. - Check the magic number and the sha256 hash in the head of the Dictionary-Compressed streams using SharedDictionaryHeaderCheckerSourceStream. - Re-generate test files using the new content encodings. Bug: 1413922 Change-Id: I4f2f40c1f0c3666b4f0b54e34ad966ffccadd96b Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5552563 Reviewed-by: Kenichi Ishibashi <[email protected]> Commit-Queue: Tsuyoshi Horo <[email protected]> Reviewed-by: Patrick Meenan <[email protected]> Cr-Commit-Position: refs/heads/main@{#1304826}
The spec of Compression Dictionary Transport has been changed to use new `dcb` and `dcz` content encodings. httpwg/http-extensions#2784 To follow the spec change, this CL changes the Chromium implementation and tests as follows: - Use "dcb" content encoding name instead of "br-d". - Use "dcz" content encoding name instead of "zstd-d". - Remove the "Content-Dictionary" response header. - Check the magic number and the sha256 hash in the head of the Dictionary-Compressed streams using SharedDictionaryHeaderCheckerSourceStream. - Re-generate test files using the new content encodings. Bug: 1413922 Change-Id: I4f2f40c1f0c3666b4f0b54e34ad966ffccadd96b Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5552563 Reviewed-by: Kenichi Ishibashi <[email protected]> Commit-Queue: Tsuyoshi Horo <[email protected]> Reviewed-by: Patrick Meenan <[email protected]> Cr-Commit-Position: refs/heads/main@{#1304826}
The spec of Compression Dictionary Transport has been changed to use new `dcb` and `dcz` content encodings. httpwg/http-extensions#2784 To follow the spec change, this CL changes the Chromium implementation and tests as follows: - Use "dcb" content encoding name instead of "br-d". - Use "dcz" content encoding name instead of "zstd-d". - Remove the "Content-Dictionary" response header. - Check the magic number and the sha256 hash in the head of the Dictionary-Compressed streams using SharedDictionaryHeaderCheckerSourceStream. - Re-generate test files using the new content encodings. Bug: 1413922 Change-Id: I4f2f40c1f0c3666b4f0b54e34ad966ffccadd96b Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5552563 Reviewed-by: Kenichi Ishibashi <[email protected]> Commit-Queue: Tsuyoshi Horo <[email protected]> Reviewed-by: Patrick Meenan <[email protected]> Cr-Commit-Position: refs/heads/main@{#1304826}
…dings of Compressed Dictionary, a=testonly Automatic update from web-platform-tests Use the new "dcb" and "dcz" content encodings of Compressed Dictionary The spec of Compression Dictionary Transport has been changed to use new `dcb` and `dcz` content encodings. httpwg/http-extensions#2784 To follow the spec change, this CL changes the Chromium implementation and tests as follows: - Use "dcb" content encoding name instead of "br-d". - Use "dcz" content encoding name instead of "zstd-d". - Remove the "Content-Dictionary" response header. - Check the magic number and the sha256 hash in the head of the Dictionary-Compressed streams using SharedDictionaryHeaderCheckerSourceStream. - Re-generate test files using the new content encodings. Bug: 1413922 Change-Id: I4f2f40c1f0c3666b4f0b54e34ad966ffccadd96b Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5552563 Reviewed-by: Kenichi Ishibashi <[email protected]> Commit-Queue: Tsuyoshi Horo <[email protected]> Reviewed-by: Patrick Meenan <[email protected]> Cr-Commit-Position: refs/heads/main@{#1304826} -- wpt-commits: 0af64c1077da2f0e27b98cd05e41dc81f28dca3a wpt-pr: 46436
…dings of Compressed Dictionary, a=testonly Automatic update from web-platform-tests Use the new "dcb" and "dcz" content encodings of Compressed Dictionary The spec of Compression Dictionary Transport has been changed to use new `dcb` and `dcz` content encodings. httpwg/http-extensions#2784 To follow the spec change, this CL changes the Chromium implementation and tests as follows: - Use "dcb" content encoding name instead of "br-d". - Use "dcz" content encoding name instead of "zstd-d". - Remove the "Content-Dictionary" response header. - Check the magic number and the sha256 hash in the head of the Dictionary-Compressed streams using SharedDictionaryHeaderCheckerSourceStream. - Re-generate test files using the new content encodings. Bug: 1413922 Change-Id: I4f2f40c1f0c3666b4f0b54e34ad966ffccadd96b Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5552563 Reviewed-by: Kenichi Ishibashi <[email protected]> Commit-Queue: Tsuyoshi Horo <[email protected]> Reviewed-by: Patrick Meenan <[email protected]> Cr-Commit-Position: refs/heads/main@{#1304826} -- wpt-commits: 0af64c1077da2f0e27b98cd05e41dc81f28dca3a wpt-pr: 46436
This eliminates the
Content-Dictionary
response header and moves the hash of the dictionary into the payload of the response.Specifically, it adds 36-byte header in front of the compressed stream which is a 4 or 8-byte signature followed by the 32-byte SHA-256 hash digest.
The signature is different for the two different compressions:
\xFF\x44\x43\x42
(0xFF followed byDCB
)\x5e\x2a\x4d\x18\x20\x00\x00\x00
(32-byte skippable Zstandard frame)This also changes the content-encoding names for the different schemes to make it clear that they are a new format and not a raw brotli or Zstandard stream (
dcb
anddcz
).From the cli, the stream is equivalent to:
or
Fix #2770