Stale comment referring to nonexisting WriteMetadata() function #475

Closed
opened 2026-01-29 20:44:33 +00:00 by claunia · 6 comments
Owner

Originally created by @juj on GitHub (Aug 16, 2023).

9ff341daaf/c/enc/encode.c (L932) references a function WriteMetadata(), but no such function seems to exist in the repository.

Maybe it is intended to refer to the function WriteMetadataHeader, or maybe it is referring to an earlier now deleted function BrotliEncoderWriteMetadata?

Originally created by @juj on GitHub (Aug 16, 2023). https://github.com/google/brotli/blob/9ff341daaf2419e7985c89ec09964c81e4e2f3da/c/enc/encode.c#L932 references a function `WriteMetadata()`, but no such function seems to exist in the repository. Maybe it is intended to refer to the function [WriteMetadataHeader]( https://github.com/google/brotli/blob/9ff341daaf2419e7985c89ec09964c81e4e2f3da/c/enc/encode.c#L1176), or maybe it is referring to an earlier now deleted function `BrotliEncoderWriteMetadata`?
Author
Owner

@juj commented on GitHub (Aug 16, 2023):

Btw, commit 0a63f99db9 removed the WriteMetadataHeader function, and the commit message says * added BROTLI_OPERATION_EMIT_METADATA instead. Is that something that should be available to users from the command line, or is it just an internally supported operation?

@juj commented on GitHub (Aug 16, 2023): Btw, commit 0a63f99db9eeb8a15dc3941b96802141b1993912 removed the `WriteMetadataHeader` function, and the commit message says `* added BROTLI_OPERATION_EMIT_METADATA instead`. Is that something that should be available to users from the command line, or is it just an internally supported operation?
Author
Owner

@eustas commented on GitHub (Aug 16, 2023):

This was supported by encoder back then, but not in decoder. Earlier this year I've added API to access metadata seen by decoder. Hopefully soon this will be available in command line tool for "watermarking" (see #628, #827)

@eustas commented on GitHub (Aug 16, 2023): This was supported by encoder back then, but not in decoder. Earlier this year I've added API to access metadata seen by decoder. Hopefully soon this will be available in command line tool for "watermarking" (see #628, #827)
Author
Owner

@juj commented on GitHub (Aug 16, 2023):

Thanks @eustas , that makes sense.

At Unity, we have been relying for quite some time (since 2016) on a manual fork of the encoder that added a feature to inject a custom string. This is used to identify whether a browser fed us a brotli-compressed or uncompressed data stream.

Recently we have been working on getting us to a newer version of Brotli to clear some CVEs that were raised. As part of that update, I have tried to bring back the WriteMetadata function since that is what was used at the time. I had to change the code a little bit, as last_byte and last_byte_bits had been renamed to last_bytes and last_bytes_bits. (iiuc this was a u8 to u16 expansion of some trailing bytes going into a bit writer, or similar)

The code that I brought back is this: https://github.com/Unity-Technologies/brotli/pull/7/files#diff-f7342e36363c8b43f6debffb54f2de0d6d958a48320e4b7811efe2bc4942ead3R924-R964

@eustas I wonder if you might be able to know from a glance if there is any hope from that code still being correct? Should I expect to be able to utilize that kind of metadata serialization construct in the latest codebase branch?

@juj commented on GitHub (Aug 16, 2023): Thanks @eustas , that makes sense. At Unity, we have been relying for quite some time (since 2016) on a manual fork of the encoder that added a feature [to inject a custom string](https://github.com/Unity-Technologies/brotli/pull/7/files#diff-d1e19300da1b5397621007794f5430f5a3d6c4ebabdb4f07ccfbe719d5748162R117-R123). This is used to identify whether a browser fed us a brotli-compressed or uncompressed data stream. Recently we have been working on getting us to a newer version of Brotli to clear some CVEs that were raised. As part of that update, I have tried to bring back the WriteMetadata function since that is what was used at the time. I had to change the code a little bit, as `last_byte` and `last_byte_bits` had been renamed to `last_bytes` and `last_bytes_bits`. (iiuc this was a u8 to u16 expansion of some trailing bytes going into a bit writer, or similar) The code that I brought back is this: https://github.com/Unity-Technologies/brotli/pull/7/files#diff-f7342e36363c8b43f6debffb54f2de0d6d958a48320e4b7811efe2bc4942ead3R924-R964 @eustas I wonder if you might be able to know from a glance if there is any hope from that code still being correct? Should I expect to be able to utilize that kind of metadata serialization construct in the latest codebase branch?
Author
Owner

@eustas commented on GitHub (Aug 16, 2023):

Sure, will take a look tomorrow morning.

@eustas commented on GitHub (Aug 16, 2023): Sure, will take a look tomorrow morning.
Author
Owner

@eustas commented on GitHub (Aug 18, 2023):

At glance looks ok. Perhaps stream_state_ and is_last_block_emitted_ should be checked and set accordingly (if metadata block is last).

@eustas commented on GitHub (Aug 18, 2023): At glance looks ok. Perhaps `stream_state_` and `is_last_block_emitted_` should be checked and set accordingly (if metadata block is last).
Author
Owner

@juj commented on GitHub (Aug 21, 2023):

Great, thanks for taking a peek! I'll read more into those parts, and see how to accommodate that in if needed.

@juj commented on GitHub (Aug 21, 2023): Great, thanks for taking a peek! I'll read more into those parts, and see how to accommodate that in if needed.
Sign in to join this conversation.
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: starred/brotli#475