Preserve redundant cbsize = 0 with --keep-foreign-metadata by tobinus · Pull Request #878 · xiph/flac

tobinus · 2026-02-18T15:01:01Z

A year back, I reported a limitation with the --keep-foreign-metadata functionality, where decoding fails if the fmt chunk is defined according to the WAVEFORMATEX structure (with wFormatTag = WAVE_FORMAT_PCM while including cbSize) despite not having any additional format information (so that cbSize is 0). I ran into this issue again this week, so I decided to have a go at solving it.

Thinking aloud, I see three ways of dealing with this limitation:

Accept the limitation, but maybe warn about it when encoding
Modify decoding so that the decoded fmt chunk equals the original in this case
Modify the verification of the decoding, so that it accepts the fmt chunk losing cbSize, or provide some way for the user to accept such changes from the original file. Whether cbSize is there or not in WAVE_FORMAT_PCM files makes no difference for applications consuming WAV anyway

Judging by earlier discussions on the mailing list, I'm guessing that solution 3 is undesirable. Of the two remaining solutions, I prefer solution 2 since it would let us convert the recordings we have at work to and from FLAC with --keep-foreign-metadata.

The decoding logic is already complicated, with logic regarding fmt chunk length spread across multiple locations, so I'm a bit worried about adding more to the burden. That is why I have refactored this part in this pull request. Please note that I am not fluent in C, so I may have made some trade-offs concerning performance or memory usage without knowing.

Alternatively, I have also pushed a branch without refactoring while keeping support for WAVE64, and a branch where cbSize = 0 is not supported for WAVE64, simplifying the solution a bit.

What do you think?

Closes #808

Usually, the cbSize field is left out when no WAVEFORMATEXTENSIBLE data is included. However, encoders may also choose to leave it in and set it to 0 to indicate that there is no extra format information, in accordance with the [WAVEFORMATEX] structure. Such files would cause an error when decoding the flac file with --keep-foreign-metadata. This solution handles WAV, RF64 and WAVE64 files using the WAVEFORMATEX structure with no extra format information. [WAVEFORMATEX]: http://web.archive.org/web/20250815212210/https://learn.microsoft.com/en-us/previous-versions/dd757713(v=vs.85) Closes xiph#808

tobinus · 2026-02-18T15:07:33Z

src/flac/decode.c


 	FLAC__uint64 iff_size;
 	uint32_t foreign_metadata_size = 0; /* size of all non-audio non-fmt/COMM foreign metadata chunks */
+	FLAC__bool preserve_dummy_cbsize = false;


I decided to call this variable preserve_dummy_cbsize because

preserve makes it clear that we are bringing something over from the original WAV file

dummy communicates that this only applies to cbSize when it is not strictly needed, so this has no bearing on cbSize when is_waveformatextensible is true

Some possible alternatives:

keep_dummy_cbsize using the same verb as the CLI flag, --keep-foreign-metadata

preserve_redundant_cbsize avoiding the word dummy

is_waveformatex using the same naming convention as the existing is_waveformatextensible variable (referencing WAVEFORMATEX), with the downside of being a prefix of the existing variable, so developers would likely get the wrong variable when using autocomplete

tobinus · 2026-02-18T15:10:09Z

src/flac/decode.c

 			return false;

+		if(format == FORMAT_WAVE64) {
+			unsigned padding = ((8 - ftello(f) % 8) % 8);


Is there a better way to get the number of bytes we need to pad with to align at 8 bytes? I had to use a modulo of the final result in order to get 0 instead of 8 when no padding is needed (checking for padding < 8 below seemed a bit counter-intuitive).

tobinus · 2026-02-18T15:16:11Z

src/flac/decode.c

+		if(format == FORMAT_WAVE64) {
+			unsigned padding = ((8 - ftello(f) % 8) % 8);
+			if(padding > 0) {
+				/* fmt chunk must be padded in order to align with 8-byte alignment */
+				if(flac__utils_fwrite("\x00\x00\x00\x00\x00\x00\x00", 1, padding, f) != padding)
+					return false;
+			}
+		}
+


I originally had some logic here specific to the case where format == FORMAT_WAVE64 && !is_waveformatextensible && preserve_dummy_cbsize, since this is the only case where the size of the fmt chunk isn't a multiple of 8. However, that would create a strong coupling between this conditional and the contents of the write_riff_wave_fmt_chunk_body function, spreading the knowledge across multiple locations. I therefore rewrote this part so that we are able to add padding without knowing about what causes write_riff_wave_fmt_chunk_body to write a non-aligned fmt chunk.

Is it okay to use ftello(f) like this?

tobinus · 2026-02-18T15:22:08Z

src/test_streams/main.c

+	FLAC__byte wav_cbsize0[] = {
+		0x72,0x69,0x66,0x66,0x2E,0x91,0xCF,0x11, /* RIFF GUID */
+		0xA5,0xD6,0x28,0xDB,0x04,0xC1,0x00,0x00,
+		 192,   0,   0,   0,   0,   0,   0,   0,
+		0x77,0x61,0x76,0x65,0xF3,0xAC,0xD3,0x11, /* WAVE GUID */
+		0x8C,0xD1,0x00,0xC0,0x4F,0x8E,0xDB,0x8A,
+		0x6A,0x75,0x6E,0x6B,0xF3,0xAC,0xD3,0x11, /* junk GUID */
+		0x8C,0xD1,0x00,0xC0,0x4F,0x8E,0xDB,0x8A,
+		  32,   0,   0,  0 ,   0,   0,   0,   0,
+		 'b', 'l', 'a', 'h', 'b', 'l', 'a', 'h',
+		0x66,0x6D,0x74,0x20,0xF3,0xAC,0xD3,0x11, /* fmt GUID */
+		0x8C,0xD1,0x00,0xC0,0x4F,0x8E,0xDB,0x8A,
+		  42,   0,   0,  0 ,   0,   0,   0,   0,
+		   1,   0,   1,   0,0x44,0xAC,   0,   0,
+		0x88,0x58,0x01,   0,   2,   0,  16,   0,
+		/* cbSize,   p   a   d   d   i   n   g */
+		   0,   0,   0,   0,   0,   0,   0,   0,
+		0x64,0x61,0x74,0x61,0xF3,0xAC,0xD3,0x11, /* data GUID */
+		0x8C,0xD1,0x00,0xC0,0x4F,0x8E,0xDB,0x8A,
+		  40,   0,   0,  0 ,   0,   0,   0,   0,
+		   0,   0,   1,   0,   4,   0,   9,   0,
+		  16,   0,  25,   0,  36,   0,  49,   0,
+		0x6A,0x75,0x6E,0x6B,0xF3,0xAC,0xD3,0x11, /* junk GUID */
+		0x8C,0xD1,0x00,0xC0,0x4F,0x8E,0xDB,0x8A,
+		  32,   0,   0,  0 ,   0,   0,   0,   0,
+		 'b', 'l', 'a', 'h', 'b', 'l', 'a', 'h'
+	};


I should note: I don't have a usecase with WAVE64 at work, only WAV/RF64. The reason I added support for WAVE64 is because it also has support for the fmt chunk, so it theoretically could have the same quirk. The test file wacky_cbsize0.w64 is therefore made by hand, I don't know if there actually exists an encoder that would write WAVE64 files like this.

H2Swine · 2026-02-20T18:44:37Z

Might I ask whether this has anything to do with the issue reported in #858 , or maybe it is so "close to" what you have fixed, that you might do this too?

ktmf01 · 2026-02-21T18:42:28Z

Thanks!

Please note that I am not fluent in C, so I may have made some trade-offs concerning performance or memory usage without knowing.

This code isn't performance sensitive, so that is really the least of my worries.

I'll take a look. If I can't decide on this today, it might take a while before I have time to look at it again

ktmf01 · 2026-02-21T21:02:03Z

I see your patches use a different email address than your github account has. I'd like to squash ans merge these patches, but that would mean the commit would have your github email instead of what the patches currently use.

tobinus · 2026-02-23T08:22:45Z

@ktmf01

I'll take a look. If I can't decide on this today, it might take a while before I have time to look at it again

No worries!

I see your patches use a different email address than your github account has. I'd like to squash ans merge these patches, but that would mean the commit would have your github email instead of what the patches currently use.

I have multiple email addresses associated with my GitHub account, but I guess you're not allowed to choose among them, which makes sense. It's not that important, so it's fine if the commit is associated with my GitHub email. But I've pushed a squashed commit to tobinus:808-handle-redundant-cbsize/all-formats-refactored-squashed, if it helps? (I can force-push it onto this PR's branch if desired.)

tobinus · 2026-02-23T08:53:29Z

@H2Swine

Might I ask whether this has anything to do with the issue reported in #858 , or maybe it is so "close to" what you have fixed, that you might do this too?

The way I see it, this PR fixes a subset of the cases in #858, namely the files that store zero in the cbSize field. Files that store anything else will still fail. So this PR can be viewed as a stepping stone towards fixing it, perhaps? I've followed up with a comment on that issue.

ktmf01 · 2026-02-23T12:41:23Z

It's not that important, so it's fine if the commit is associated with my GitHub email.

OK. I just ask because:

Some people like to be credited with a specific email address
Some people have enforced commit signature verification, which results in issues when I squash-and-merge through something else than Github itself

But as you don't mind, I'll use the squash-and-merge button, saves me some hassle.

tobinus · 2026-02-23T13:17:47Z

I see, thank you for checking 🙂

tobinus added 5 commits February 17, 2026 15:25

Avoid magic when making test file with cbsize = 0

cf43c55

Move fmt chunk size calculations closer to writing

247ac5c

Avoid redundant WAVE64 conditionals

d028149

Make WAVE64 fmt chunk padding independent of inner logic

0d89278

tobinus commented Feb 18, 2026

View reviewed changes

tobinus mentioned this pull request Feb 23, 2026

Files that fail --keep-foreign-metadata #858

Open

ktmf01 merged commit 9d23884 into xiph:master Feb 23, 2026
18 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Preserve redundant cbsize = 0 with --keep-foreign-metadata#878

Preserve redundant cbsize = 0 with --keep-foreign-metadata#878
ktmf01 merged 5 commits intoxiph:masterfrom
tobinus:808-handle-redundant-cbsize/all-formats-refactored

tobinus commented Feb 18, 2026 •

edited

Loading

Uh oh!

tobinus Feb 18, 2026

Uh oh!

tobinus Feb 18, 2026 •

edited

Loading

Uh oh!

tobinus Feb 18, 2026 •

edited

Loading

Uh oh!

tobinus Feb 18, 2026

Uh oh!

H2Swine commented Feb 20, 2026

Uh oh!

ktmf01 commented Feb 21, 2026

Uh oh!

ktmf01 commented Feb 21, 2026

Uh oh!

tobinus commented Feb 23, 2026

Uh oh!

tobinus commented Feb 23, 2026

Uh oh!

ktmf01 commented Feb 23, 2026

Uh oh!

Uh oh!

tobinus commented Feb 23, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

tobinus commented Feb 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tobinus Feb 18, 2026

Choose a reason for hiding this comment

Uh oh!

tobinus Feb 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tobinus Feb 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tobinus Feb 18, 2026

Choose a reason for hiding this comment

Uh oh!

H2Swine commented Feb 20, 2026

Uh oh!

ktmf01 commented Feb 21, 2026

Uh oh!

ktmf01 commented Feb 21, 2026

Uh oh!

tobinus commented Feb 23, 2026

Uh oh!

tobinus commented Feb 23, 2026

Uh oh!

ktmf01 commented Feb 23, 2026

Uh oh!

Uh oh!

tobinus commented Feb 23, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

tobinus commented Feb 18, 2026 •

edited

Loading

tobinus Feb 18, 2026 •

edited

Loading

tobinus Feb 18, 2026 •

edited

Loading