Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix all the files that fail to compare correctly #26

Closed
18 tasks done
est31 opened this issue Mar 22, 2018 · 5 comments
Closed
18 tasks done

Fix all the files that fail to compare correctly #26

est31 opened this issue Mar 22, 2018 · 5 comments
Labels
bug

Comments

@est31
Copy link
Member

est31 commented Mar 22, 2018 •
edited
Loading

Commits 3224e94...6352249 have added files to the testsuite from libnogg as well as from xiphmont. Some are commented out however, as lewton shows mismatches to libvorbis.

xiph testsuite failures (27 test files in total):

libnogg testsuite failures (32 test files in total):
@est31 est31 added the bug label Mar 22, 2018
@est31
Copy link
Member Author

est31 commented Sep 23, 2018

So, as of the 0.9.1 release, only these 5 files are remaining. I ran stb_vorbis on them. For some, it sadly considered the files as invalid and returned VORBIS_invalid_setup.

  • chain-test1.ogg → here, the "first" stream in the chain has no error, and I think stb_vorbis supports no chaining. So for further investigation, I'd have to split the file up.
  • chain-test3.ogg → VORBIS_invalid_setup
  • singlemap-test.ogg → mismatch in residue
  • test-short.ogg → VORBIS_invalid_setup
  • square-interleaved.ogg → VORBIS_invalid_setup

est31 added a commit that referenced this issue Oct 7, 2018
@est31
Fix an issue in the float32_unpack routine
93c7744 
There has been a bug that led lewton to wrongly decode
singlemap-test.ogg as well as audio_simple_with_error.ogg.

I tracked it down by printing out intermediary values
for lewton as well as for stb_vorbis and comparing them.

First, I saw that there was a mismatch in the pre imdct
values. Then, I tracked it down to a mismatch in the
residue values. After that, I found out that the codebook
values were wrong. And then, I saw that the settings for
codebook_minimum_value and codebook_delta_value have been
parsed wrongly by lewton, which caused all the other
mismatches. This wrong parsing has been caused by
a bug in the behaviour of the float conversion routine
float32_unpack.

The deployed fix was to replace the overengineered
and overoptimized bit twiddling implementation
by a more straight forward, spec following one.
Performance impact wasn't measured, but the code is
super cold: It's called only during header setup
and even there only twice per codebook.
In singlemap-test.ogg, it was called
a few dozen times or such.

Fixes #24, and also addresses parts of #26.
est31 added a commit that referenced this issue Oct 7, 2018
@est31
Fix an issue in the float32_unpack routine
1073e58 
There has been a bug that led lewton to wrongly decode
singlemap-test.ogg as well as audio_simple_with_error.ogg.

I tracked it down by printing out intermediary values
for lewton as well as for stb_vorbis and comparing them.

First, I saw that there was a mismatch in the pre imdct
values. Then, I tracked it down to a mismatch in the
residue values. After that, I found out that the codebook
values were wrong. And then, I saw that the settings for
codebook_minimum_value and codebook_delta_value have been
parsed wrongly by lewton, which caused all the other
mismatches. This wrong parsing has been caused by
a bug in the behaviour of the float conversion routine
float32_unpack.

The deployed fix was to replace the overengineered
and overoptimized bit twiddling implementation
by a more straight forward, spec following one.
Performance impact wasn't measured, but the code is
super cold: It's called only during header setup
and even there only twice per codebook.
In singlemap-test.ogg, it was called
a few dozen times or such.

Fixes #24, and also addresses parts of #26.
@est31
Copy link
Member Author

est31 commented Oct 22, 2018

With 1073e58, singlemap-test.ogg has been fixed.
As of the 0.9.2 release, only 4 files are thus remaining.
I ran further tests on them.

  • chain-test1.ogg → I've split up the file via opening it with hexdump -C | less and then searching for stream starts by /-ing for vorbis, looking for the OggS preceeding it, then taking this as an offset to discard the bytes via dd. Basically dd if="chain-test1.ogg" of="chain-test-1-first-part.ogg" bs=1 count=565344 and dd if="chain-test1.ogg" of="chain-test-1-second-part.ogg" bs=1 skip=565344. The results were interesting! Apparently there is no mismatch either of the two parts alone, only in the combined version. We have a mono file followed by a stereo file, maybe this is the reason? No idea.

  • chain-test3.ogg → I've split up this file as well (at offs 29781 this time). Here we have a stereo file followed by a mono one. The comparison of the combined stream already fails at packet 1, while the first part in isolation fails to compare, and the second part in isolation compares successfully (without a difference). If I play back the first file in a player it sounds just like test-short.ogg so I suspect it's the same bug.

  • test-short.ogg → libvorbis outputs samples for the first packet, while lewton outputs only silence. Residue post inverse values look reasonable (non-zero values), but the post dot product pre imdct values have tons of NaNs, inf, 0, and values close to 0. test-short.ogg is using using floor type zero. If you print the output of floor_zero_compute_curve you can see the crazy values (very small, close to infinity, nan, etc).

  • square-interleaved.ogg → Opening the file with the format-info example of libvorbis indicates that the file is indeed interleaved. The ogg spec has some words on interleaved streams, but I still am not 100% sure what to do here. Maybe testing a bit will help.

est31 added a commit that referenced this issue Oct 27, 2018
@est31
Fix test-short.ogg and chain-test2.ogg
d58c172 
Fixes comparison to libvorbis for two sample
files by fixing two bugs in lewton:

1. In cases where the last header packet
   was directly followed by audio packets,
   without any page boundary,
   libvorbis just ignored those packets.
   lewton didn't so it treated the packets
   as normal content. This created a
   misalignment between lewton and libvorbis
   when parsing the sample files.
   Such a situation occured in test-short.ogg
   as well as chain-test2.ogg.
   The spec says that audio packets
   must start in a fresh page, so this
   means that the files are technically invalid.
   However, libvorbis parses them just fine.

2. Second, we move the last = 0 resetting into
   the outer loop. This is what the spec says,
   and it is also needed in order to fix
   the comparison to libvorbis for the two files.
   Otherwise, there'd be just silence, as the
   floor0 samples would assume crazy values.

As we rely on a new function that has been added
to the ogg crate for the very purpose of using
it in this commit, we bump the version
requirement for the ogg crate to 0.6.1.

Both files get fixed at once as
chain-test2.ogg is just a concatenation of
test-short.ogg with another file where no
mismatch to libvorbis was present.

Transparency notice: this commit marks the
first and hopefully last time I've ever looked
into the vorbis source code for my work on the
lewton crate. I think that this creates no change
in the licensing situation of the lewton crate.
The changes of this commit are so small that
they don't fall under copyright.

This commit addresses parts of #26.
est31 added a commit that referenced this issue Oct 27, 2018
@est31
Fix test-short.ogg and chain-test3.ogg
b65d3d1 
Fixes comparison to libvorbis for two sample
files by fixing two bugs in lewton:

1. In cases where the last header packet
   was directly followed by audio packets,
   without any page boundary,
   libvorbis just ignored those packets.
   lewton didn't so it treated the packets
   as normal content. This created a
   misalignment between lewton and libvorbis
   when parsing the sample files.
   Such a situation occured in test-short.ogg
   as well as chain-test2.ogg.
   The spec says that audio packets
   must start in a fresh page, so this
   means that the files are technically invalid.
   However, libvorbis parses them just fine.

2. Second, we move the last = 0 resetting into
   the outer loop. This is what the spec says,
   and it is also needed in order to fix
   the comparison to libvorbis for the two files.
   Otherwise, there'd be just silence, as the
   floor0 samples would assume crazy values.

As we rely on a new function that has been added
to the ogg crate for the very purpose of using
it in this commit, we bump the version
requirement for the ogg crate to 0.6.1.

Both files get fixed at once as
chain-test2.ogg is just a concatenation of
test-short.ogg with another file where no
mismatch to libvorbis was present.

Transparency notice: this commit marks the
first and hopefully last time I've ever looked
into the vorbis source code for my work on the
lewton crate. I think that this creates no change
in the licensing situation of the lewton crate.
The changes of this commit are so small that
they don't fall under copyright.

This commit addresses parts of #26.
est31 added a commit that referenced this issue Oct 27, 2018
@est31
Fix test-short.ogg and chain-test3.ogg
e2b38e8 
Fixes comparison to libvorbis for two sample
files by fixing two bugs in lewton:

1. In cases where the last header packet
   was directly followed by audio packets,
   without any page boundary,
   libvorbis just ignored those packets.
   lewton didn't so it treated the packets
   as normal content. This created a
   misalignment between lewton and libvorbis
   when parsing the sample files.
   Such a situation occured in test-short.ogg
   as well as chain-test3.ogg.
   The spec says that audio packets
   must start in a fresh page, so this
   means that the files are technically invalid.
   However, libvorbis parses them just fine.

2. Second, we move the last = 0 resetting into
   the outer loop. This is what the spec says,
   and it is also needed in order to fix
   the comparison to libvorbis for the two files.
   Otherwise, there'd be just silence, as the
   floor0 samples would assume crazy values.

As we rely on a new function that has been added
to the ogg crate for the very purpose of using
it in this commit, we bump the version
requirement for the ogg crate to 0.6.1.

Both files get fixed at once as
chain-test3.ogg is just a concatenation of
test-short.ogg with another file where no
mismatch to libvorbis was present.

Transparency notice: this commit marks the
first and hopefully last time I've ever looked
into the vorbis source code for my work on the
lewton crate. I think that this creates no change
in the licensing situation of the lewton crate.
The changes of this commit are so small that
they don't fall under copyright.

This commit addresses parts of #26.
@est31
Copy link
Member Author

est31 commented Oct 27, 2018

With commit e2b38e8, only chain-test1.ogg and square-interleaved.ogg remain.

est31 added a commit that referenced this issue Oct 28, 2018
@est31
Fix chain-test1.ogg decoding mismatch
dbfaae5 
Prior to this commit, the individual
streams that make up chain-test1.ogg
all had no difference to libvorbis
in isolation, but when their concatenation
was played back, a decoding mismatch
occured for every packet of the second
stream.
The issue was that the last packet of
the first stream should have been shorter
than the one lewton outputted.
E.g. if you put code like

if n == 2022 {
	dec_data.truncate(1044);
}

right before the let mut diffs = 0;
in the `cmp_output` function,
you'd get a match because that'd
have leveled out the mismatch.

The vorbis spec says that the absgp
of each page marks the last sample
inside the last packet that ends
it. It also says that if the absgp
of the last page of the entire
stream is before the actually
decoded length of the last packet,
truncation is neccessary.

This truncation was the cause for
the mismatch as vorbis implemented
and we didn't.
This commit implements the truncation
and thus fixes the mismatch for
chain-test1.ogg.

Addresses parts of #26.
@est31
Copy link
Member Author

est31 commented Oct 28, 2018

Progress on chain-test1.ogg with commit dbfaae5 . Now only one file square-interleaved.ogg remains.

@est31 est31 closed this as completed in 31f64bb Oct 28, 2018
@est31
Copy link
Member Author

est31 commented Oct 28, 2018

With commit 31f64bb , I could fix the last mismatch and thus, this bug can be closed!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
1 participant
@est31