1947431 - Implement zstd for DecompressionStream

Status: RESOLVED FIXED

(Core :: DOM: Streams, enhancement)

Description

[Kagami Rosylight 🏳️‍🌈🏳️‍⚧️ [:saschanaz] (they/them)]

We want this for internal usage as we already ship zstd. For that purpose having decoding-only is fine. Shipping would be blocked by standards consensus.

Comment 1

[Erik Nordin [:nordzilla]]

I think I'd like to take this, so I'm assigning myself.

NOTE: I will be out tomorrow, and Monday is a US holiday, so if someone with higher context gets to this between now and Tuesday, feel free to take over, though my preference would be to finish this out next week.

I looked at this a bit today, and pushed up some WIP patches, more or less a 1:1 mirror of the ZLib version, but using third_party/zstd/lib/zstd.h instead. It's not ready for formal review yet, which is why it is still in WIP, but it is working.

---

I do have a few questions.

1. We define CompressionFormat here. If this is only to be supported for decompression for the time being, do I need to make a DecompressionFormat equivalent? Or perhaps just add the enum variant and throw in the CompressionStream if it gets CompressionFormat::Zstd?

2. How can I ensure that this is supported only internally? Perhaps something like [ChromeOnly]?

3. Is it okay to modify the web platform tests with these updates if there isn't consensus on standardized adoption yet? If not, where would be the best place to put tests?

---

Thanks!

Comment 2

[Erik Nordin [:nordzilla]]

Attachment: Bug 1947431 - Rename DecompressionStreamAlgorithms r=saschanaz!

Comment 3

[Erik Nordin [:nordzilla]]

Attachment: Bug 1947431 - Add "zstd" as a CompressionFormat r=saschanaz!

Comment 4

[Paul Adenot (:padenot)]

(In reply to Erik Nordin [:nordzilla] from comment #1)

1. We define CompressionFormat here. If this is only to be supported for decompression for the time being, do I need to make a DecompressionFormat equivalent? Or perhaps just add the enum variant and throw in the CompressionStream if it gets CompressionFormat::Zstd?

Just add the enum in that list yes.

2. How can I ensure that this is supported only internally? Perhaps something like [ChromeOnly]?

I'm not sure that we can do [ChromeOnly] on an enum variant. We can throw in the ctor or something. Kagami will know for sure.

3. Is it okay to modify the web platform tests with these updates if there isn't consensus on standardized adoption yet? If not, where would be the best place to put tests?

https://searchfox.org/mozilla-central/source/testing/web-platform/mozilla/tests is where we put WPTs that aren't quite standard. You can make a temporary copy.

Comment 5

[Kagami Rosylight 🏳️‍🌈🏳️‍⚧️ [:saschanaz] (they/them)]

All Paul said. 👍🏻

Comment 6

[Erik Nordin [:nordzilla]]

Attachment: Bug 1947431 - Implement ZstdDecompressionAlgorithms r=jesup!,saschanaz!

Comment 7

[Erik Nordin [:nordzilla]]

Attachment: Bug 1947431 - Refactor ZLibHelper.h r=saschanaz!

Renames ZLibHelper.h to be CompressionStreamHelper.h and attempts
to make shared functionality between various implementations of
CompressionStreamAlgorithms and DecompressionStreamAlgorithms as
easily shared as possible.

Comment 8

[Erik Nordin [:nordzilla]]

Attachment: Bug 1947431 - Implement ZstdDecompressionAlgorithms r=jesup!,saschanaz!

Introduces a new static pref for the "zstd" format to be used
with CompressionStream and DecompressionStream. Rewrites our
current WPTs to utilize this pref instead of being forced to
construct the stream within a privileged context.

Comment 9

[Erik Nordin [:nordzilla]]

Attachment: Bug 1947431 - (Tentative) Remove temporary array from (De)compressionStreamAlgorithms r=jesup!,saschanaz!

Removes the use of a temporary array from the DecompressionStreamAlgorithms
implementation of DecompressAndEnqueue, instead enqueuing each chunk as they
come rather than collecting all chunks into an array prior to enqueueing.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b04b63d5c8ee
Rename DecompressionStreamAlgorithms r=saschanaz
https://hg.mozilla.org/integration/autoland/rev/43b5795653f1
Add "zstd" as a CompressionFormat r=saschanaz,webidl
https://hg.mozilla.org/integration/autoland/rev/1dcbc510621b
Implement ZstdDecompressionAlgorithms r=saschanaz,jesup
https://hg.mozilla.org/integration/autoland/rev/85df9c1b9761
Refactor ZLibHelper.h r=saschanaz

Comment 11

[Sandor Molnar[:smolnar]]

Backed out for causing build bustages

Backout link: https://hg.mozilla.org/integration/autoland/rev/499bd18c73010e6dc5379699bf030aafe8496d24

Push with failures

Failure log -> ERROR - /builds/worker/checkouts/gecko/dom/base/DecompressionStream.cpp:136:29: error: use of undeclared identifier 'ZLibFlush'

Comment 12

[Erik Nordin [:nordzilla]]

Unfortunately, there was one spot that I neglected to update within a MOZ_ASSERT(), which only runs in debug builds.

All of the local and try builds that I ran were optimized :(

Queuing for re-landing.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4d38cd80339c
Rename DecompressionStreamAlgorithms r=saschanaz
https://hg.mozilla.org/integration/autoland/rev/8e20d3170fb3
Add "zstd" as a CompressionFormat r=saschanaz,webidl
https://hg.mozilla.org/integration/autoland/rev/62564a163d7b
Implement ZstdDecompressionAlgorithms r=saschanaz,jesup
https://hg.mozilla.org/integration/autoland/rev/782b2ffd766b
Refactor ZLibHelper.h r=saschanaz

Comment 14

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/4d38cd80339c
https://hg.mozilla.org/mozilla-central/rev/8e20d3170fb3
https://hg.mozilla.org/mozilla-central/rev/62564a163d7b
https://hg.mozilla.org/mozilla-central/rev/782b2ffd766b

Comment 15

[Mayank Bansal]

This lead to a 4KB increase in installer size on Windows, which is below any threshold for alerts. Consider this comment as only a FYI.

Comment 16

[Paul Adenot (:padenot)]

Thanks Mayank, this is expected for this (important) feature.