Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[compression-dictionary] Moved the dictionary hash into the body #2784

Merged
merged 4 commits into from
May 20, 2024
Merged

[compression-dictionary] Moved the dictionary hash into the body #2784

merged 4 commits into from
May 20, 2024

Conversation

pmeenan
Copy link
Contributor

@pmeenan pmeenan commented May 9, 2024 •
edited

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:

  • Dictionary-Compressed Brotli: \xFF\x44\x43\x42 (0xFF followed by DCB)
  • Dictionary-Compressed Zstandard: \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 and dcz).

From the cli, the stream is equivalent to:

echo -en '\xffDCB' > data.txt.dcb && \
openssl dgst -sha256 -binary dictionary.txt >> data.txt.dcb && \
brotli --stdout -D dictionary.txt data.txt >> data.txt.dcb

or

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

Fix #2770

@pmeenan pmeenan self-assigned this May 9, 2024
martinthomson
martinthomson approved these changes May 9, 2024
View reviewed changes
Copy link
Contributor

@martinthomson martinthomson left a 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.

draft-ietf-httpbis-compression-dictionary.md Outdated Show resolved Hide resolved
draft-ietf-httpbis-compression-dictionary.md Outdated Show resolved Hide resolved
draft-ietf-httpbis-compression-dictionary.md Outdated Show resolved Hide resolved
draft-ietf-httpbis-compression-dictionary.md Outdated Show resolved Hide resolved
draft-ietf-httpbis-compression-dictionary.md Outdated Show resolved Hide resolved
@pmeenan
Copy link
Contributor Author

pmeenan commented May 10, 2024

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.

@pmeenan pmeenan mentioned this pull request May 10, 2024
Closed
yoavweiss
yoavweiss approved these changes May 13, 2024
View reviewed changes
Copy link
Contributor

@yoavweiss yoavweiss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@yoavweiss
Copy link
Contributor

Still LGTM

@pmeenan
Copy link
Contributor Author

pmeenan commented May 16, 2024

@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

felixhandte
felixhandte approved these changes May 17, 2024
View reviewed changes
Copy link

@felixhandte felixhandte left a 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.

draft-ietf-httpbis-compression-dictionary.md Show resolved Hide resolved
@pmeenan
Copy link
Contributor Author

pmeenan commented May 17, 2024

Looks like you just need to update the PR summary to reflect this change.

Thanks. Updated and added an example with the zstd cli.

@pmeenan pmeenan merged commit cf299e2 into httpwg:main May 20, 2024
1 check passed
@pmeenan pmeenan deleted the hash branch May 20, 2024 14:28
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request May 22, 2024
@horo-t @chromium-wpt-export-bot
Use the new "dcb" and "dcz" content encodings of Compressed Dictionary
51126b9 
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
@chromium-wpt-export-bot chromium-wpt-export-bot mentioned this pull request May 22, 2024
Merged
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request May 22, 2024
@horo-t @chromium-wpt-export-bot
Use the new "dcb" and "dcz" content encodings of Compressed Dictionary
6968838 
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
aarongable pushed a commit to chromium/chromium that referenced this pull request May 23, 2024
@horo-t
Use the new "dcb" and "dcz" content encodings of Compressed Dictionary
d934bf3 
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 <bashi@chromium.org>
Commit-Queue: Tsuyoshi Horo <horo@chromium.org>
Reviewed-by: Patrick Meenan <pmeenan@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1304826}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request May 23, 2024
@horo-t @chromium-wpt-export-bot
Use the new "dcb" and "dcz" content encodings of Compressed Dictionary
f9e5dc4 
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 <bashi@chromium.org>
Commit-Queue: Tsuyoshi Horo <horo@chromium.org>
Reviewed-by: Patrick Meenan <pmeenan@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1304826}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request May 23, 2024
@horo-t @chromium-wpt-export-bot
Use the new "dcb" and "dcz" content encodings of Compressed Dictionary
0af64c1 
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 <bashi@chromium.org>
Commit-Queue: Tsuyoshi Horo <horo@chromium.org>
Reviewed-by: Patrick Meenan <pmeenan@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1304826}
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request May 30, 2024
@horo-t @moz-wptsync-bot
Bug 1898228 [wpt PR 46436] - Use the new "dcb" and "dcz" content enco…
d4d9530 
…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 <bashi@chromium.org>
Commit-Queue: Tsuyoshi Horo <horo@chromium.org>
Reviewed-by: Patrick Meenan <pmeenan@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1304826}

--

wpt-commits: 0af64c1077da2f0e27b98cd05e41dc81f28dca3a
wpt-pr: 46436
jamienicol pushed a commit to jamienicol/gecko that referenced this pull request May 31, 2024
@horo-t @moz-wptsync-bot
Bug 1898228 [wpt PR 46436] - Use the new "dcb" and "dcz" content enco…
b7213c4 
…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 <bashi@chromium.org>
Commit-Queue: Tsuyoshi Horo <horo@chromium.org>
Reviewed-by: Patrick Meenan <pmeenan@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1304826}

--

wpt-commits: 0af64c1077da2f0e27b98cd05e41dc81f28dca3a
wpt-pr: 46436
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@martinthomson martinthomson martinthomson approved these changes

@yoavweiss yoavweiss yoavweiss approved these changes

@felixhandte felixhandte felixhandte approved these changes

@MNocenyelli01 MNocenyelli01 MNocenyelli01 approved these changes

Assignees

@pmeenan pmeenan

Labels
compression-dictionary
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Essential content coding metadata: header or body?
5 participants
@pmeenan @yoavweiss @martinthomson @felixhandte @MNocenyelli01