On Tue, Oct 15, 2002 at 08:50:18PM +0200, Tor-Einar Jarnbjo wrote:> Hi, > > as Conrad suggested, I've made a complete list of all points in the > specification, which I beleive are errors, or where the explanation > is unclear, contains unneccessary steps and so on. > > I hope someone has time to look through the points and if and when > accepting or rejecting them be so very kind and inform me about it. > I will also once again try to work through the residue specification, > as I beleive there are more errors in that part.Ah, it's here already. I'll be handling it now then. Monty --- >8 ---- List archives: http://www.xiph.org/archives/ Ogg project homepage: http://www.xiph.org/ogg/ To unsubscribe from this list, send a message to 'vorbis-dev-request@xiph.org' containing only the word 'unsubscribe' in the body. No subject is needed. Unsubscribe messages sent to the list will be ignored/filtered.
Hi,
as Conrad suggested, I've made a complete list of all points in the
specification, which I beleive are errors, or where the explanation
is unclear, contains unneccessary steps and so on.
I hope someone has time to look through the points and if and when
accepting or rejecting them be so very kind and inform me about it.
I will also once again try to work through the residue specification,
as I beleive there are more errors in that part.
Tor
<p>###############
#2002-09-23-001
Floor 1 / curve computation / step 1:
23) vector [floor1_final_Y] element [i] =
[predicted] - ([val] - [lowroom]) - 1
hould be:
23) vector [floor1_final_Y] element [i] =
[predicted] - [val] + [hiroom] - 1
due to floor1.c, line 1019:
val = -1-(val-hiroom);
<p>###############
#2002-09-23-002
Floor 1 / curve computation / step 1:
25) vector [floor1_final_Y] element [i] =
[predicted] - (([val] - 1) divided by 2 using integer division)
hould be:
25) vector [floor1_final_Y] element [i] =
[predicted] - (([val] + 1) divided by 2 using integer division)
due to floor1.c, line 1023:
val= -((val+1)>>1);
###############
#2002-09-23-003
Floor 1 / curve computation / step 2:
Steps 11 and 12 should be performed outside and after the iteration
started in step 4. As far as I can see, performing these two steps
for each iteration makes no sense, as the values set in the vector
are overwritten by the later iterations.
11) if ( [hx] is less than [n] ) {
12) render_line( [hx], [hy], [n], [hy], [floor] )
}
It also does not make sense to use the expensive function render_line
to set all remaining elements in the vector to the same value, so
a better algorithm for this might be something like:
if ( [hx] is less than [n] ) {
iterate [i] over the range [hx] + 1 ... [n] - 1 {
set vector [floor1_final_Y]' element [i] to [hy]
}
)
<p>###############
#2002-09-23-004
Floor 1 / curve computation / step 2:
Are steps 13 and 14 necessary? It does not make sense to me for the
encoder to produce an audio packet where the condition in step 13
is met.
13) if ( [hx] is greater than [n] ) {
14) truncate vector [floor] to [n] elements
}
<p>###############
#2002-09-23-005
Floor 1 / curve computation / step 2:
It is confusing that these steps are using a different parameter
order for calling the same function:
8) render_line( [lx], [hx], [ly], [hy], [floor] )
12) render_line( [hx], [hy], [n], [hy], [floor] )
As well as the definitions of the functions render_line
and render_point:
render_line(x0, y0, x1, y1, v)
render_point(x0, x1, y0, y1, X)
<p>###############
#2002-10-15-001
Setup header decode, mappings:
When decoding the mappings in the setup header, the specification
says:
-----------------------------------------------------------------
------------
read 1 bit as a boolean flag; if set, square polar channel mapping
is in use:
[vorbis_mapping_coupling_steps]= read 8 bits as unsigned integer
and add one
for [j] each of [vorbis_mapping_coupling_steps] steps:
vector [vorbis_mapping_magnitude] element [j]=
read ilog([audio_channels] - 1) bits as unsigned integer
vector [vorbis_mapping_angle] element [j]=
read ilog([audio_channels] - 1) bits as unsigned integer
-----------------------------------------------------------------
------------
In this block, the vectors vorbis_mapping_magnitude and
vorbis_mapping_magnitude are filled, and the value of
vorbis_mapping_coupling_steps is set only if the flag read in the
first line
is true. If the flag is false, the value of vorbis_mapping_coupling_steps
and the length and content of the two vectors are left unspecified,
although
they are referenced anyway in the audio packet decode section under
"nonzero
vector propagate".
<p>###############
#2002-10-15-002
In the document "Ogg logical bitstream framing", Ogg pages are
described
as beginning with a capture of the form "OggS". I've come across
some
older Ogg files, which have other data between the valid Ogg pages,
mostly
starting with the three letters "TAG". If it is common practice to add
data between the pages, this should at least be mentioned in the framing
specification, and a suggestion should be made how to find the beginning
of the next Ogg page. Simply seeking forward until finding the OggS
pattern
seems obvious, but this could lead to some trial and errors if the
pattern
occurs in the extra data.
<p>###############
#2002-10-15-003
The function used to describe the window slope in "Ogg Vorbis I format
specification: introduction and description" is:
y=sin(2PI*sin^2(x/n))
(here, I even don't understand the notation you use)
The function described in the source code comment (window.c, line 29)
is:
y=sin(sin(x)*sin(x)*2pi)
And at last, the function actually implemented in libvorbis,
(window.c, lines 33-38) is:
y=sin(sin(x/n*PI/2)^2*PI/2)
In the implementation, this is the calcuation of the leading flank,
where x is running from 0 (y=0) to n, where n is the end of the flank
(y=1).
<p><p>==================================================================EASY
and FREE access to your email anywhere: http://Mailreader.com/
==================================================================
<p>--- >8 ----
List archives: http://www.xiph.org/archives/
Ogg project homepage: http://www.xiph.org/ogg/
To unsubscribe from this list, send a message to
'vorbis-dev-request@xiph.org'
containing only the word 'unsubscribe' in the body. No subject is
needed.
Unsubscribe messages sent to the list will be ignored/filtered.
Sorry for taking a few hours, I was running my 'what the fuck was I thinking' filter set to 'high' just to make sure... Naturally folks will want to verify the corrections. They've been propogated to the online docs as well. On Tue, Oct 15, 2002 at 08:50:18PM +0200, Tor-Einar Jarnbjo wrote:> ############### > #2002-09-23-001 > > Floor 1 / curve computation / step 1: > > 23) vector [floor1_final_Y] element [i] = > [predicted] - ([val] - [lowroom]) - 1 > > should be: > > 23) vector [floor1_final_Y] element [i] = > [predicted] - [val] + [hiroom] - 1 > > due to floor1.c, line 1019: > > val = -1-(val-hiroom);Correction committed.> ############### > #2002-09-23-002 > > Floor 1 / curve computation / step 1: > > 25) vector [floor1_final_Y] element [i] = > [predicted] - (([val] - 1) divided by 2 using integer division) > > should be: > > 25) vector [floor1_final_Y] element [i] = > [predicted] - (([val] + 1) divided by 2 using integer division) > > due to floor1.c, line 1023: > > val= -((val+1)>>1);Correction committed.> ############### > #2002-09-23-003 > > Floor 1 / curve computation / step 2: > > Steps 11 and 12 should be performed outside and after the iteration > started in step 4. As far as I can see, performing these two steps > for each iteration makes no sense, as the values set in the vector > are overwritten by the later iterations. > > 11) if ( [hx] is less than [n] ) { > 12) render_line( [hx], [hy], [n], [hy], [floor] ) > }Correct, this was a bracketing error.> It also does not make sense to use the expensive function render_line > to set all remaining elements in the vector to the same value, so > a better algorithm for this might be something like: > > if ( [hx] is less than [n] ) { > iterate [i] over the range [hx] + 1 ... [n] - 1 { > set vector [floor1_final_Y]' element [i] to [hy] > } > )In this case it's more important to be clear than fast.> ############### > #2002-09-23-004 > > Floor 1 / curve computation / step 2: > > Are steps 13 and 14 necessary? It does not make sense to me for the > encoder to produce an audio packet where the condition in step 13 > is met. > > 13) if ( [hx] is greater than [n] ) { > 14) truncate vector [floor] to [n] elements > }It does not make sense, but it's important to be well defined. An ecoder may be cofused, or the stream may be malicious and looking to create an intentional overrun.> ############### > #2002-09-23-005 > > Floor 1 / curve computation / step 2: > > It is confusing that these steps are using a different parameter > order for calling the same function: > > 8) render_line( [lx], [hx], [ly], [hy], [floor] ) > 12) render_line( [hx], [hy], [n], [hy], [floor] ) > > As well as the definitions of the functions render_line > and render_point: > > render_line(x0, y0, x1, y1, v) > render_point(x0, x1, y0, y1, X)...and by 'confusing' you meant 'utterly wrong not to mention confusing'. Fixed.> ############### > #2002-10-15-001[...]> If the flag is false, the value of vorbis_mapping_coupling_steps > and the length and content of the two vectors are left unspecified, > although they are referenced anyway in the audio packet decode section under > "nonzero vector propagate".You're correct, the default value of vorbis_mapping_coupling_steps should be zero. Fixed.> ############### > #2002-10-15-002 > > In the document "Ogg logical bitstream framing", Ogg pages are described > as beginning with a capture of the form "OggS". I've come across some > older Ogg files, which have other data between the valid Ogg pages, > mostly > starting with the three letters "TAG".This is id3v2 information, eplicitly against spec, and we've informed a number of tool authors that they're violating spec by allowing id3v2 tags in Ogg streams. Mike went into more detail.> If it is common practice to add > data between the pages, this should at least be mentioned in the framing > specification,It is not.> and a suggestion should be made how to find the beginning > of the next Ogg page. Simply seeking forward until finding the OggS > pattern > seems obvious, but this could lead to some trial and errors if the > pattern > occurs in the extra data.No, because a full capture with valid CRC, sequence, version and serial number is needed for stream capture. You'd need a perfect, accidental match of at least 128 bits.> ############### > #2002-10-15-003 > > The function used to describe the window slope in "Ogg Vorbis I format > specification: introduction and description" is: > > y=sin(2PI*sin^2(x/n)) > (here, I even don't understand the notation you use) > > The function described in the source code comment (window.c, line 29) > is: > > y=sin(sin(x)*sin(x)*2pi) > > And at last, the function actually implemented in libvorbis, > (window.c, lines 33-38) is: > > y=sin(sin(x/n*PI/2)^2*PI/2) > > In the implementation, this is the calcuation of the leading flank, > where x is running from 0 (y=0) to n, where n is the end of the flank > (y=1).Corrected. Monty --- >8 ---- List archives: http://www.xiph.org/archives/ Ogg project homepage: http://www.xiph.org/ogg/ To unsubscribe from this list, send a message to 'vorbis-dev-request@xiph.org' containing only the word 'unsubscribe' in the body. No subject is needed. Unsubscribe messages sent to the list will be ignored/filtered.
On Tue, 15 Oct 2002, Tor-Einar Jarnbjo wrote:> ############### > #2002-10-15-003 > > The function used to describe the window slope in "Ogg Vorbis I format > specification: introduction and description" is: > > y=sin(2PI*sin^2(x/n)) > (here, I even don't understand the notation you use) > > The function described in the source code comment (window.c, line 29) > is: > > y=sin(sin(x)*sin(x)*2pi) > > And at last, the function actually implemented in libvorbis, > (window.c, lines 33-38) is: > > y=sin(sin(x/n*PI/2)^2*PI/2)Even this isn't exact. It rather looks like: y=sin(sin((x+0.5)/n*PI/2)^2*PI/2) <p>Nicolas --- >8 ---- List archives: http://www.xiph.org/archives/ Ogg project homepage: http://www.xiph.org/ogg/ To unsubscribe from this list, send a message to 'vorbis-dev-request@xiph.org' containing only the word 'unsubscribe' in the body. No subject is needed. Unsubscribe messages sent to the list will be ignored/filtered.
> >############### >#2002-10-15-002 > >In the document "Ogg logical bitstream framing", Ogg pages are described >as beginning with a capture of the form "OggS". I've come across some >older Ogg files, which have other data between the valid Ogg pages, >mostly >starting with the three letters "TAG". If it is common practice to add >data between the pages, this should at least be mentioned in the framing >specification, and a suggestion should be made how to find the beginning >of the next Ogg page. Simply seeking forward until finding the OggS >pattern >seems obvious, but this could lead to some trial and errors if the >pattern >occurs in the extra data.I'll handle this one since it's not a spec bug and is fairly straightforward. There are a number of broken tools out there which add ID3V(1,2) tags to ogg vorbis files. This is not allowed. Other data between valid ogg pages is always tolerated (because of how the ogg framing/sync works), but is not common practice, and is very highly discouraged. There are no known non-bug uses of it. The ogg spec, as I recall, does specify how to find the next ogg page, and a forward seek to the next occurrance of the sync pattern 'OggS' is correct (there's no other way to do it, since we're considering cases where there's garbage data between pages). The page checksum should prevent us from incorrect sync if this string just happens to occur in the garbage. Mike --- >8 ---- List archives: http://www.xiph.org/archives/ Ogg project homepage: http://www.xiph.org/ogg/ To unsubscribe from this list, send a message to 'vorbis-dev-request@xiph.org' containing only the word 'unsubscribe' in the body. No subject is needed. Unsubscribe messages sent to the list will be ignored/filtered.
Tirsdag, 15 oktober 2002, skrev du:>Sorry for taking a few hours, I was running my 'what the fuck was I >thinking' filter set to 'high' just to make sure... Naturally folks >will want to verify the corrections. They've been propogated to the >online docs as well.I appreciate, that you spent some time to check them out.>> It also does not make sense to use the expensive function render_line >> to set all remaining elements in the vector to the same value, so >> a better algorithm for this might be something like: >> >> if ( [hx] is less than [n] ) { >> iterate [i] over the range [hx] + 1 ... [n] - 1 { >> set vector [floor1_final_Y]' element [i] to [hy] >> } >> ) > >In this case it's more important to be clear than fast.But is the current specification "draw a line from hx, hy to n-1, hy" really easier to understand and more consistent than "set the elements from hx to n-1 to the value hy"? In my suggestion, the iteration should of course start with hx and not hx+1.>> 13) if ( [hx] is greater than [n] ) { >> 14) truncate vector [floor] to [n] elements >> } > >It does not make sense, but it's important to be well defined. An >ecoder may be cofused, or the stream may be malicious and looking to >create an intentional overrun.And this reminds me of another point I forgot to mention. The specification does not at any point specify the vector length. Wouldn't it make sense to do so? I assume that any reasonable implementation needs to know the needed length before setting the values, and although the vector length in most cases is pretty obvious, there are a few points in the specification where it is not, like when reading the x-list when decoding a floor 1 entry in the header.>> ############### >> #2002-10-15-001 >[...] >> If the flag is false, the value of vorbis_mapping_coupling_steps >> and the length and content of the two vectors are left unspecified, >> although they are referenced anyway in the audio packet decodesection under>> "nonzero vector propagate". > >You're correct, the default value of vorbis_mapping_coupling_steps >should be zero. Fixed.And this is also a point where I think it would make sense to specify, that the vector length should be 0. When implementing the specification, I interpreted this part as if the vectors would not be used later when the bit is unset, and ran into accessing a null reference since no array was allocated. I don't know how the part is implemented in libvorbis, but in Java it sure makes a difference having no array at all as compared to an array with 0 elements. <p>Tor <p><p><p>==================================================================EASY and FREE access to your email anywhere: http://Mailreader.com/ ================================================================== <p>--- >8 ---- List archives: http://www.xiph.org/archives/ Ogg project homepage: http://www.xiph.org/ogg/ To unsubscribe from this list, send a message to 'vorbis-dev-request@xiph.org' containing only the word 'unsubscribe' in the body. No subject is needed. Unsubscribe messages sent to the list will be ignored/filtered.