Fix WAV and add RIFF #229

generalmimon · 2019-09-22T20:35:00Z

Wave sample files: http://www-mmsp.ece.mcgill.ca/Documents/AudioFormats/WAVE/Samples.html
WAV with bext chunk: http://www.noiseofnorway.net/wp-content/uploads/2011/11/170139-Printer-Scanner-close-scanner-lid-Schoeps.wav

RIFF description can be tested with any of its subtypes: WAV, AVI, RMID, DLS, SF2, ...

common/riff.ksy

KOLANICH · 2019-09-22T21:09:52Z

Thank you for the contribution. It is a long-lasting tech debt to unify all riff-based formats under a riff.ksy spec, but it requires multiple features lacking from KSC currently: kaitai-io/kaitai_struct#81, kaitai-io/kaitai_struct#458, and maybe kaitai-io/kaitai_struct#135 and kaitai-io/kaitai_struct#196.

You may also want to add https://www.johnloomis.org/cpe102/asgn/asgn1/riff.html into doc-ref.

and Fixes: #230 into commit message. And please, don't use master branch for pull requests. Create an own branch for each PR.

KOLANICH · 2019-11-23T07:55:58Z

common/riff.ksy

+      - id: form_type
+        type: str
+        size: 4
+        pad-right: 0x20


Not sure that we need this pad-right. pad-right is the contract that all the strings have padding stripped here. But the strings are 4 bytes, 4 bytes is 1 dword, one dword is a unit for reading memory on 32-bit archs and is faster than reading 3 bytes. The strings are fixed length. There is no chance that there will be arbitrary long padding. So IMHO pad-right here doesn't make any sense and would have prevened KSC from optimizing the code if such optimizations ever were introduced.

There are two types of RIFF structs: with asserts (chunk and parent_chunk_data) and without asserts (chunk_generic and parent_chunk_data_generic). Those without asserts won't be needed when kaitai-io/kaitai_struct#435 (comment) is resolved.

generalmimon · 2019-11-24T20:38:50Z

@KOLANICH
Could you please take a look at the new approach I take? I've already explained it on #230 (comment), just split generic RIFF types to those with assertions and without assertions (sometimes you want to force chunk to be of specific chunk ID, sometimes not).

I think it's now quite readable and less error-prone. Silly mistakes like this one (the marked line also causes kaitai-io/kaitai_struct#604) should no longer happen if all RIFF-based formats use the common classes as containers for specific data. I would continue with the other RIFF formats if nobody objects.

The riff.ksy format can be used for exploring chunk structure of any RIFF-compliant binary file. It is generic, it only implements INFO list chunk for which RIFF specification says the following: "The ‘INFO’ list is a registered global chunk that can be used within any RIFF file." I don't expect that someone would use it in production, but it is quite handy for development.

KOLANICH · 2019-11-26T22:26:00Z

I wonder if we really need to assert chunk ids instead of just switching on them everywhere? I mean just read a chunk id and interpret the blob based on the read chunk id only, without any checks wheter this chunk type is allowed to be here.

generalmimon · 2019-12-07T11:34:38Z

You are right, thank you the suggestion! I haven't realized that the assertions are not vital. Is it now OK?

generalmimon · 2019-12-22T12:41:29Z

@KOLANICH, can this PR be already merged?

KOLANICH · 2019-12-22T14:22:00Z

I'm sorry, but I am not the one who merges PRs into this repo. @GreyCat is the one who decides and merges.

generalmimon · 2019-12-22T15:00:49Z

Actually, I can merge it myself, because @GreyCat trusts me that I won't do silly things 😏, but I hesitate to do so when I'm not 100% sure that it is OK. I wonder why you don't want to enter the Kaitai team, you belong there more than everyone else. I understand that you might not want to feel the pressure to somehow deserve the membership (e.g. at least one contribution per week 😂), but I think everyone understands that you're busy person and you can participate only once in a while.

KOLANICH · 2019-12-22T16:25:26Z

I wonder why you don't want to enter the Kaitai team, you belong there more than everyone else.

I cannot be sure in cybersecurity of my devices. In fact I am sure in their insecurity. I don't want other's repos being compromised because of this.
GreyCat doesn't trust me because I am a nonconformist and don't follow the style guide I personally consider defective :) It is probably the reason why my PRs are ignored for such a long time. It is completely OK ;)

at least one contribution per week

AFAIK there are no such requirements.

generalmimon · 2019-12-22T19:37:11Z

I cannot be sure in cybersecurity of my devices. (...) I don't want other's repos being compromised because of this.

Nothing is ever secure, but I can't imagine the degree of boredom of a hacker that would bother with screwing up an open-source repo like this, but thanks for the info 🙂

KOLANICH · 2019-12-22T19:49:38Z

open-source repo like this

access to an org gives access to all the repos. this repo is probably safe, ksys have no io, but if an adversary have managed to backdoor the KSC repo or builds, it would be devastating.

generalmimon · 2020-04-07T22:13:10Z

@GreyCat, can it be finally merged? It's blocking my master branch (which is of course my fault, but it quite sucks), every time I want to contribute something I must be careful because the branch master isn't master. And I actually don't see a place for improvement here.

common/riff/chunk.ksy

common/riff/parent_chunk_data.ksy

GreyCat · 2020-04-13T18:48:14Z

Apologies for not reviewing this earlier. I totally agree on the motivation and approach, but there are two things that I've flagged:

Naming/file placement - that's minor and likely easy to fix. Actually, nowadays we might even just keep these pieces as subtypes inside riff.ksy and address them using something like type: 'riff::chunk'
Using strings to work with RIFF chunk identifiers. That's probably not too major too - we can have it declared as an u4, as then in every specific format we can convert it to .wav-specific, .avi-specific, etc, map of enum values.

What do you think?

generalmimon · 2020-04-13T19:59:46Z

@GreyCat

Actually, nowadays we might even just keep these pieces as subtypes inside riff.ksy and address them using something like type: 'riff::chunk'

At first, I meant the riff spec to be primarily for debugging and reverse-engineering (and it also can be used for extracting metadata from the INFO chunk), but yeah, I agree with this approach. It makes perfect sense to include the chunk and parent_chunk_data as subtypes, we get rid of two import-only files which are not meaningful on their own. And it's better to write riff::parent_chunk_data than riff_parent_chunk_data.

Using strings to work with RIFF chunk identifiers. That's probably not too major too - we can have it declared as an u4, as then in every specific format we can convert it to .wav-specific, .avi-specific, etc, map of enum values.

At time of writing the specs, I was experimenting with the enum implementation as well. The problem was it didn't come to my mind that I can use enum key in value instances as well (the docs don't mention it at all), so I thought that the only use enums is to introduce an enum on the riff KSY level with all possible RIFF chunk IDs that can ever occur, which just isn't possible. So I chose the only viable approach besides enums with respect to people (people typically don't know that number 0x45564157 means "WAVE" in little-endian), and that are plain text strings.

But yeah, you're right, string conversion and comparison is inefficent compared to a primitive integer read, and the inconvenience of working with plain numbers can be gracefully solved with converting the integer to format-specific (wav, avi, ...) chunk ID dictionary enum just using a value instance with the enum key.

I'll go ahead and try to incorporate the suggestions.

GreyCat · 2020-05-30T13:42:22Z

All the latest changes look great, thanks!

However, there are currently 2 conflicting fragment. Can I ask you to take a look and resolve these conflicts, so we can merge this one?

generalmimon · 2020-05-30T19:20:47Z

@GreyCat

However, there are currently 2 conflicting fragment. Can I ask you to take a look and resolve these conflicts, so we can merge this one?

Yeah, I no longer hoped that this PR can be merged in a reasonable amount of time, so I've recently fixed the WAV format manually so that it at least works (it was broken due to kaitai-io/kaitai_struct#604 and merging this PR would also fix it).

Thanks for the review, please merge it if all looks good to you 🙂

GreyCat · 2020-05-30T19:34:46Z

Thanks for this great work!

generalmimon added 2 commits September 22, 2019 22:07

Update WAV format

422d035

Add RIFF

66a79dd

KOLANICH reviewed Sep 22, 2019

View reviewed changes

common/riff.ksy Outdated Show resolved Hide resolved

KOLANICH mentioned this pull request Sep 22, 2019

Resource Interchange File Format #230

Closed

Fix minor bugs in WAV and RIFF

d0918ed

generalmimon force-pushed the master branch from b496323 to d0918ed Compare November 22, 2019 22:10

KOLANICH reviewed Nov 23, 2019

View reviewed changes

generalmimon added 3 commits November 24, 2019 17:05

Add reusable generic RIFF types

4b4c7dd

There are two types of RIFF structs: with asserts (chunk and parent_chunk_data) and without asserts (chunk_generic and parent_chunk_data_generic). Those without asserts won't be needed when kaitai-io/kaitai_struct#435 (comment) is resolved.

Use RIFF types in WAV format

fb29e45

Use RIFF types in RIFF format

7d0ae01

Remove RIFF types with assertions

c03a031

generalmimon linked an issue Apr 7, 2020 that may be closed by this pull request

Resource Interchange File Format #230

Closed

GreyCat reviewed Apr 13, 2020

View reviewed changes

common/riff/chunk.ksy Outdated Show resolved Hide resolved

GreyCat reviewed Apr 13, 2020

View reviewed changes

common/riff/chunk.ksy Outdated Show resolved Hide resolved

GreyCat reviewed Apr 13, 2020

View reviewed changes

common/riff/parent_chunk_data.ksy Outdated Show resolved Hide resolved

generalmimon added 2 commits May 7, 2020 21:44

Parse RIFF FourCC identifiers as a single u4 int

ebd9871

Move generic RIFF types into riff.ksy as subtypes

6d18cea

generalmimon added 2 commits May 7, 2020 22:44

Update wav.ksy to import subtypes of riff.ksy

89cfab0

Refactor riff.ksy: throw away byte array literals

22cd882

generalmimon requested a review from GreyCat May 12, 2020 19:52

generalmimon added 2 commits May 12, 2020 22:50

Improve style of riff.ksy

ab4af4a

Add link to RFC with WAV codecs

637a6c5

GreyCat approved these changes May 30, 2020

View reviewed changes

Merge branch 'master' into master

fe98ef5

GreyCat merged commit d353072 into kaitai-io:master May 30, 2020

KOLANICH mentioned this pull request Jun 10, 2020

SoundFont 2 format #293

Draft

generalmimon mentioned this pull request Jul 17, 2021

Android sparse kaitai-io/kaitai_struct_samples#1

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix WAV and add RIFF #229

Fix WAV and add RIFF #229

generalmimon commented Sep 22, 2019

KOLANICH commented Sep 22, 2019 •

edited

Loading

KOLANICH Nov 23, 2019

generalmimon commented Nov 24, 2019

KOLANICH commented Nov 26, 2019 •

edited

Loading

generalmimon commented Dec 7, 2019

generalmimon commented Dec 22, 2019

KOLANICH commented Dec 22, 2019 •

edited

Loading

generalmimon commented Dec 22, 2019

KOLANICH commented Dec 22, 2019 •

edited

Loading

generalmimon commented Dec 22, 2019

KOLANICH commented Dec 22, 2019 •

edited

Loading

generalmimon commented Apr 7, 2020

GreyCat commented Apr 13, 2020

generalmimon commented Apr 13, 2020 •

edited

Loading

GreyCat commented May 30, 2020 •

edited

Loading

generalmimon commented May 30, 2020 •

edited

Loading

GreyCat commented May 30, 2020

Fix WAV and add RIFF #229

Fix WAV and add RIFF #229

Conversation

generalmimon commented Sep 22, 2019

KOLANICH commented Sep 22, 2019 • edited Loading

KOLANICH Nov 23, 2019

Choose a reason for hiding this comment

generalmimon commented Nov 24, 2019

KOLANICH commented Nov 26, 2019 • edited Loading

generalmimon commented Dec 7, 2019

generalmimon commented Dec 22, 2019

KOLANICH commented Dec 22, 2019 • edited Loading

generalmimon commented Dec 22, 2019

KOLANICH commented Dec 22, 2019 • edited Loading

generalmimon commented Dec 22, 2019

KOLANICH commented Dec 22, 2019 • edited Loading

generalmimon commented Apr 7, 2020

GreyCat commented Apr 13, 2020

generalmimon commented Apr 13, 2020 • edited Loading

GreyCat commented May 30, 2020 • edited Loading

generalmimon commented May 30, 2020 • edited Loading

GreyCat commented May 30, 2020

KOLANICH commented Sep 22, 2019 •

edited

Loading

KOLANICH commented Nov 26, 2019 •

edited

Loading

KOLANICH commented Dec 22, 2019 •

edited

Loading

KOLANICH commented Dec 22, 2019 •

edited

Loading

KOLANICH commented Dec 22, 2019 •

edited

Loading

generalmimon commented Apr 13, 2020 •

edited

Loading

GreyCat commented May 30, 2020 •

edited

Loading

generalmimon commented May 30, 2020 •

edited

Loading