Add support for skippable frames #185

antoyo · 2023-01-19T18:44:58Z

Hi.
This add supports for writing and reading skippable frames as well as skipping frames.

There is no support for doing that in a streaming way in the ZSTD C library, so that's why so part of this PR parses some stuff manually.

There are a few TODOs in the code that I'd like you to tell what you think we should do with that.

Thanks for the review.

gyscos

Hi, and thanks for the PR!

Let's start with zstd-safe. I think it would be best to make these free functions, since they have nothing to do with contexts (contexts are not the only way to encode/decode zstd data).

Otherwise, with a few minor details, it looks nice!

gyscos · 2023-01-30T14:44:37Z

zstd-safe/src/lib.rs

+                    buffer,
+                    capacity,
+                    magic_variant,
+                    input.as_ptr() as *mut _,


We shouldn't turn non-mut &[u8] into *mut.

Suggested change

input.as_ptr() as *mut _,

ptr_void(input),

gyscos · 2023-01-30T14:44:58Z

zstd-safe/src/lib.rs

+    #[cfg(feature = "experimental")]
+    pub fn is_skippable_frame(input: &[u8]) -> SafeResult {
+        unsafe {
+            parse_code(zstd_sys::ZSTD_isSkippableFrame(input.as_ptr() as *mut _, input.len()) as usize)


The documentation for ZSTD_isSkippableFrame does not indicate that it can return an error, and indeed in the source code we can see it just returns either 0 or 1.

So let's just return a bool from the function.

Suggested change

parse_code(zstd_sys::ZSTD_isSkippableFrame(input.as_ptr() as *mut _, input.len()) as usize)

zstd_sys::ZSTD_isSkippableFrame(ptr_void(input), input.len() > 0

gyscos · 2023-01-30T15:00:02Z

zstd-safe/src/lib.rs

+    #[cfg(feature = "experimental")]
+    // TODO: use InBuffer?
+    pub fn write_skippable_frame<C: WriteBuf + ?Sized>(output: &mut OutBuffer<'_, C>, input: &[u8], magic_variant: u32) -> SafeResult {
+        let input_len = input.len();
+        unsafe {
+            output.dst.write_from(|buffer, capacity| {
+                parse_code(zstd_sys::ZSTD_writeSkippableFrame(
+                    buffer,
+                    capacity,
+                    input.as_ptr() as *mut _,
+                    input_len,
+                    magic_variant,
+                ))
+            })
+        }
+    }


There is no need for an InBuffer here, or even really an OutBuffer, since the zstd lib either writes everything in one go (in which case no need to maintain an input cursor) or returns an error.

Suggested change

#[cfg(feature = "experimental")]

// TODO: use InBuffer?

pub fn write_skippable_frame<C: WriteBuf + ?Sized>(output: &mut OutBuffer<'_, C>, input: &[u8], magic_variant: u32) -> SafeResult {

let input_len = input.len();

unsafe {

output.dst.write_from(|buffer, capacity| {

parse_code(zstd_sys::ZSTD_writeSkippableFrame(

buffer,

capacity,

input.as_ptr() as *mut _,

input_len,

magic_variant,

))

})

}

}

pub fn write_skippable_frame<C: WriteBuf + ?Sized>(dst: &mut C, input: &[u8], magic_variant: u32) -> SafeResult {

let input_len = input.len();

unsafe {

dst.write_from(|buffer, capacity| {

parse_code(zstd_sys::ZSTD_writeSkippableFrame(

buffer,

capacity,

ptr_void(input),

input_len,

magic_variant,

))

})

}

}

gyscos · 2023-01-30T15:00:53Z

zstd-safe/src/lib.rs

+    pub fn read_skippable_frame<C: WriteBuf + ?Sized>(output: &mut OutBuffer<'_, C>, magic_variant: &mut u32, input: &[u8]) -> SafeResult {
+        let input_len = input.len();
+        unsafe {
+            output.dst.write_from(|buffer, capacity| {


Here too no need for an OutBuffer:

Suggested change

pub fn read_skippable_frame<C: WriteBuf + ?Sized>(output: &mut OutBuffer<'_, C>, magic_variant: &mut u32, input: &[u8]) -> SafeResult {

let input_len = input.len();

unsafe {

output.dst.write_from(|buffer, capacity| {

pub fn read_skippable_frame<C: WriteBuf + ?Sized>(dst: &mut C, magic_variant: &mut u32, input: &[u8]) -> SafeResult {

let input_len = input.len();

unsafe {

dst.write_from(|buffer, capacity| {

gyscos · 2023-01-30T16:16:40Z

On the higher-level zstd-rs side, it seems that there's 2 main things:

Some wrappers for the zstd-safe functions.
Some re-implementation of read/write skippable frames in a slightly more "streamable" way - no need to buffer the entire frame twice.

The second point in particular is where most changes are required to re-implement quite a bit of logic from the zstd library. I feel like there's quite a lot of added code and complexity, maybe without being worth the added convenience.

In particular, regarding the re-implementation of read_skippable_frame:

Seeking back when an error occurs is a big source of copied code, either from the standard library or from the C zstd library.
The implementation can stream the Read source, but still buffers the entire result.

I would be more comfortable with a solution that:

Does not seek back on error. If an error happens, the stream is left in an unspecified state. Should simplify the implementation.
Does not buffer the output, but either provide a Read that will only read the skippable content (using .take(content_size) on the underlying reader) or stream the frame content to a Write given as parameter. This way, users could buffer it if needed.

antoyo · 2023-01-30T16:21:36Z

Does not seek back on error. If an error happens, the stream is left in an unspecified state. Should simplify the implementation.

If you do not seek back, how can you detect that there is a skippable frame?
is_skippable_frame() requires a &[u8], so it's not streaming.

And an attempt to call read_skippable_frame() and checking for an error could leave the stream at a wrong position without being able to continue reading normal frames.

gyscos · 2023-01-30T16:23:20Z

is_skippable_frame only needs the &[u8] to cover the header (in practice the first 4 bytes), so it's still mostly streaming.

And if something happens in read_skippable_frame, then something is wrong, and there's no guarantee the stream is valid anyway.

antoyo · 2023-01-30T16:26:15Z

is_skippable_frame only needs the &[u8] to cover the header (in practice the first 8 bytes), so it's still mostly streaming.

But how would you combine this function with reading frames in a streaming fashion without seeking back?

If you read the first 8 bytes (which could be too much bytes IIRC, e.g. I think you can have a frame of 5 bytes) to check if it's a skippable frame, how would you then read the frame (whether it is skippable or not)?

gyscos · 2023-01-30T16:46:03Z

We could rely on the BufRead own buffer to "peek" at the first 4 bytes. Though it's true there's no guarantee the BufRead will actually fill more than 1 byte. If we do want to support that, one alternative would be to store up to 3 bytes of buffered data in a new zstd::stream::zio::reader::State::Peeked, which would be read from before moving back to State::Reading - I suspect it might be simpler than the current situation.

Maybe we could go a different way and register a skippable_frame_handler: Option<Box<dyn FnMut(u32, &mut dyn Reader) -> io::Result<()>>> callback, which will be called in the regular operations if a skippable frame is found.

antoyo-light · 2023-02-10T21:19:49Z

Ok, so I just pushed with all the changes you asked.

cgbur · 2023-03-17T17:00:45Z

Closes #13 if merged

antoyo · 2023-04-04T15:58:13Z

Ping.

antoyo · 2023-05-30T19:34:24Z

Any update on this?

gyscos · 2023-06-02T18:02:15Z

Hi - sorry, have had very little time to look more into this.

antoyo · 2023-08-31T11:36:36Z

Hi.
Any update?

ariel-miculas · 2023-11-07T20:24:23Z

I'm also interested in this feature.
I wonder if seekable compression could be implemented in this crate, so I don't have to use https://crates.io/crates/zstd-seekable

ariel-miculas · 2023-11-15T16:58:36Z

zstd-safe/src/lib.rs

+#[cfg(feature = "experimental")]
+pub fn read_skippable_frame<C: WriteBuf + ?Sized>(dst: &mut C, magic_variant: &mut u32, input: &[u8]) -> SafeResult {
+    let input_len = input.len();
+    unsafe {


I suggest adding SAFETY comments for the unsafe blocks and wrapping the minimum amount of code in the unsafe block. See https://rust-lang.github.io/rust-clippy/master/#/undocumented_unsafe_blocks

ariel-miculas · 2023-11-15T17:01:18Z

src/stream/zio/reader.rs

@@ -113,12 +153,17 @@ where
        loop {
            match self.state {
                State::Reading => {
+                    let is_peeking = self.peeking;


I think you should call the peeking() function here.

ariel-miculas · 2023-11-15T17:02:24Z

src/stream/zio/reader.rs

                    let (bytes_read, bytes_written) = {
                        // Start with a fresh pool of un-processed data.
                        // This is the only line that can return an interruption error.
                        let input = if first {
                            // eprintln!("First run, no input coming.");
                            b""
+                        } else if self.peeking {


Either check the is_peeking variable or call the peeking() function.

antoyo force-pushed the feature/skippable-frames branch 2 times, most recently from 511e7a7 to b0e5811 Compare January 26, 2023 20:02

gyscos reviewed Jan 30, 2023

View reviewed changes

ariel-miculas reviewed Nov 15, 2023

View reviewed changes

antoyo added 5 commits February 6, 2024 16:50

Add support for skippable frames

d896fe4

Accept a slice instead of a Vec in read_skippable_frame

71f3a83

Address API reviews

e3e0431

Do not seek back on error

f75bcf8

Update read_skippable_frame to take a Write parameter

d26b298

antoyo force-pushed the feature/skippable-frames branch from 3dd6aad to d26b298 Compare February 6, 2024 21:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for skippable frames #185

Add support for skippable frames #185

antoyo commented Jan 19, 2023

gyscos left a comment

gyscos Jan 30, 2023

gyscos Jan 30, 2023

gyscos Jan 30, 2023

gyscos Jan 30, 2023

gyscos commented Jan 30, 2023

antoyo commented Jan 30, 2023

gyscos commented Jan 30, 2023 •

edited

Loading

antoyo commented Jan 30, 2023

gyscos commented Jan 30, 2023

antoyo-light commented Feb 10, 2023

cgbur commented Mar 17, 2023

antoyo commented Apr 4, 2023

antoyo commented May 30, 2023

gyscos commented Jun 2, 2023

antoyo commented Aug 31, 2023

ariel-miculas commented Nov 7, 2023 •

edited

Loading

ariel-miculas Nov 15, 2023

ariel-miculas Nov 15, 2023 •

edited

Loading

ariel-miculas Nov 15, 2023

	parse_code(zstd_sys::ZSTD_isSkippableFrame(input.as_ptr() as *mut _, input.len()) as usize)
	zstd_sys::ZSTD_isSkippableFrame(ptr_void(input), input.len() > 0

Add support for skippable frames #185

Are you sure you want to change the base?

Add support for skippable frames #185

Conversation

antoyo commented Jan 19, 2023

gyscos left a comment

Choose a reason for hiding this comment

gyscos Jan 30, 2023

Choose a reason for hiding this comment

gyscos Jan 30, 2023

Choose a reason for hiding this comment

gyscos Jan 30, 2023

Choose a reason for hiding this comment

gyscos Jan 30, 2023

Choose a reason for hiding this comment

gyscos commented Jan 30, 2023

antoyo commented Jan 30, 2023

gyscos commented Jan 30, 2023 • edited Loading

antoyo commented Jan 30, 2023

gyscos commented Jan 30, 2023

antoyo-light commented Feb 10, 2023

cgbur commented Mar 17, 2023

antoyo commented Apr 4, 2023

antoyo commented May 30, 2023

gyscos commented Jun 2, 2023

antoyo commented Aug 31, 2023

ariel-miculas commented Nov 7, 2023 • edited Loading

ariel-miculas Nov 15, 2023

Choose a reason for hiding this comment

ariel-miculas Nov 15, 2023 • edited Loading

Choose a reason for hiding this comment

ariel-miculas Nov 15, 2023

Choose a reason for hiding this comment

gyscos commented Jan 30, 2023 •

edited

Loading

ariel-miculas commented Nov 7, 2023 •

edited

Loading

ariel-miculas Nov 15, 2023 •

edited

Loading